Skip to content

fix: push secret delete was removing the wrong key#5799

Merged
Skarlso merged 2 commits intoexternal-secrets:mainfrom
Skarlso:fix-deleting-empty-secret
Jan 9, 2026
Merged

fix: push secret delete was removing the wrong key#5799
Skarlso merged 2 commits intoexternal-secrets:mainfrom
Skarlso:fix-deleting-empty-secret

Conversation

@Skarlso
Copy link
Copy Markdown
Contributor

@Skarlso Skarlso commented Jan 6, 2026 •
edited by coderabbitai bot
Loading

Problem Statement

What is the problem you're trying to solve?

Related Issue

Fixes #5753

Proposed Changes

How do you like to solve the issue and why?

Format

Please ensure that your PR follows the following format for the title:

feat(scope): add new feature
fix(scope): fix bug
docs(scope): update documentation
chore(scope): update build tool or dependencies
ref(scope): refactor code
clean(scope): provider cleanup
test(scope): add tests
perf(scope): improve performance
desig(scope): improve design

Where scope is optionally one of:

  • charts
  • release
  • testing
  • security
  • templating

Checklist

  • I have read the contribution guidelines
  • All commits are signed with git commit --signoff
  • My changes have reasonable test coverage
  • All tests pass with make test
  • I ensured my PR is ready for review with make reviewable

Summary

Fixes a bug where deleting a PushSecret with multiple fields would remove the wrong key from the internal tracking map, causing deletion to hang and leave orphaned 1Password items.

Changes

pkg/controllers/pushsecret/pushsecret_controller.go

  • Modified DeleteSecretFromProviders to use oldEntry instead of oldRef.Match.RemoteRef.RemoteKey when removing secrets from the SyncedPushSecretsMap, ensuring the correct key is removed during deletion.

providers/v1/onepassword/onepassword.go

  • Updated DeleteSecret to treat ErrKeyNotFound as a no-op (returning nil) instead of propagating the error, allowing graceful handling when a key is already missing.

pkg/controllers/pushsecret/pushsecret_controller_test.go

  • Added syncAndDeleteWithProperties test scenario to verify correct cleanup of the SyncedPushSecrets status map when a PushSecret with multiple properties is deleted.
  • Included the test in both the main reconciliation and un/managed stores test suites.

@github-actions github-actions bot added the kind/bug Categorizes issue or PR as related to a bug. label Jan 6, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Jan 6, 2026 •
edited
Loading

Walkthrough

Changes update the PushSecret controller's secret deletion to use a different map key when removing entries, add test coverage for multi-property deletion cleanup, and modify the OnePassword provider to handle missing secrets as no-ops.

Changes

Cohort / File(s) Summary
PushSecret Controller
pkg/controllers/pushsecret/pushsecret_controller.go, pkg/controllers/pushsecret/pushsecret_controller_test.go		 Modified DeleteSecretFromProviders to remove map entries using oldEntry instead of oldRef.Match.RemoteRef.RemoteKey. Added test scenario syncAndDeleteWithProperties to verify status SyncedPushSecretsMap cleanup when deleting PushSecrets with multiple properties, with test entries added to both primary and secondary test suites.
OnePassword Provider
providers/v1/onepassword/onepassword.go		 Modified DeleteSecret to treat ErrKeyNotFound as a no-op by returning nil instead of propagating the error.

Pre-merge checks

✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR addresses issue #5753 by fixing the key removal logic in DeleteSecretFromProviders and handling ErrKeyNotFound appropriately in 1Password provider, enabling proper deletion of PushSecrets with multiple fields.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing PushSecret deletion: key removal logic, 1Password error handling, and comprehensive test coverage for multi-field deletion scenarios.
📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e62f972 and 1750760.

📒 Files selected for processing (3)
  • pkg/controllers/pushsecret/pushsecret_controller.go
  • pkg/controllers/pushsecret/pushsecret_controller_test.go
  • providers/v1/onepassword/onepassword.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • providers/v1/onepassword/onepassword.go
  • pkg/controllers/pushsecret/pushsecret_controller.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/controllers/pushsecret/pushsecret_controller_test.go (1)
apis/externalsecrets/v1alpha1/pushsecret_types.go (7)
  • PushSecret (261-267)
  • PushSecretSpec (89-115)
  • PushSecretStoreRef (36-53)
  • PushSecretSelector (135-143)
  • PushSecretData (175-186)
  • PushSecretMatch (166-172)
  • PushSecretRemoteRef (146-153)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0 GOEXPERIMENT=boringcrypto, amd64 ppc64le, linux/... / Build and Publish
  • GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0, amd64 arm64 ppc64le, linux/amd64,linux/arm64,li... / Build and Publish
  • GitHub Check: publish-artifacts (Dockerfile, CGO_ENABLED=0, amd64 arm64 s390x ppc64le, linux/amd64,linux/arm64,... / Build and Publish
  • GitHub Check: unit-tests
  • GitHub Check: check-diff
  • GitHub Check: Analyze project (go, autobuild)
🔇 Additional comments (2)
pkg/controllers/pushsecret/pushsecret_controller_test.go (2)

580-673: Excellent test coverage for the property deletion bug fix!

This test properly validates the fix for issue #5753 by creating a PushSecret with multiple properties on the same remote key, verifying initial sync, removing one property, and confirming the status map is cleaned up correctly. The test logic comprehensively covers the scenario where property-level deletions were previously causing finalizer issues.

1357-1357: LGTM!

The table entry properly invokes the new test scenario with a clear, descriptive message.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions bot added the size/m label Jan 6, 2026
@Skarlso
Copy link
Copy Markdown
Contributor Author

Skarlso commented Jan 6, 2026

/ok-to-test sha=e62f9722f8153c5cd5da5e60a63cc2c3a53f352e

@eso-service-account-app
Copy link
Copy Markdown
Contributor

eso-service-account-app bot commented Jan 6, 2026

[Bot] - ✅ e2e for e62f9722f8153c5cd5da5e60a63cc2c3a53f352e passed

@Skarlso
fix: push secret delete was removing the wrong key
1750760 
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
@Skarlso Skarlso force-pushed the fix-deleting-empty-secret branch from e62f972 to 1750760 Compare January 6, 2026 08:46
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Jan 9, 2026

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@Skarlso Skarlso merged commit a1c0b36 into external-secrets:main Jan 9, 2026
29 checks passed
@Skarlso Skarlso deleted the fix-deleting-empty-secret branch January 9, 2026 19:31
This was referenced Feb 2, 2026
Open
Merged
dsp0x4 pushed a commit to dsp0x4/external-secrets that referenced this pull request Mar 22, 2026
@Skarlso @dsp0x4
fix: push secret delete was removing the wrong key (external-secrets#…
4759142 
…5799)
@coderabbitai coderabbitai bot mentioned this pull request Mar 25, 2026
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gusfcarvalho gusfcarvalho gusfcarvalho approved these changes

Assignees

No one assigned

Labels

kind/bug Categorizes issue or PR as related to a bug. size/m

Projects

External Secrets
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1Password Connect Server: PushSecret will not delete item with multiple fields

2 participants

@Skarlso @gusfcarvalho