feat(secrets): stop retaining Secret values (roadmap Track A, steps 1-3)#208
Open
sunib wants to merge 2 commits into
Open
feat(secrets): stop retaining Secret values (roadmap Track A, steps 1-3)#208sunib wants to merge 2 commits into
sunib wants to merge 2 commits into
Conversation
…cret watches Implements Track A, steps 1-3 of the secret-handling roadmap (docs/future/secret-handling-roadmap.md) - the immediate process- and crypto-level security wins, ahead of the separate RBAC-narrowing track. Step 1 - stop caching Secret values, drop control-plane Secret watches: - Remove the GitTarget control-plane Secret watch (and its now-orphaned encryptionSecretToGitTargets map func). A full-object Secret watch made the controller-runtime cache retain every Secret value in every namespace. - Disable the typed-Secret cache on the manager (Client.Cache.DisableFor corev1.Secret), so typed Secret reads go straight to the API server and stay fresh. GitProvider stays pull-based (no auth-Secret watch, by design). Step 2 - remove private age identities from the SOPS encrypt path: - The write path only encrypts, so it needs public age recipients only. - Drop AgeIdentities/Environment from ResolvedEncryptionConfig; delete the age identity temp-file path / SOPS_AGE_KEY_FILE; stop passing blanket Secret data into the sops process env. Recipients still reach sops via the target .sops.yaml, so encryption output is unchanged. A BYO age-key Secret is still read to derive its public recipient; the private identity is never returned, written to disk, or handed to sops. Step 3 - unify the control-plane reconcile fallback at 5 minutes: - Collapse the short (2m) / medium (5m) / long (10m) requeue intervals into a single RequeueSteadyInterval (5m) for GitProvider, GitTarget, WatchRule, and ClusterWatchRule. Genuine spec changes still trigger immediately via watches; this is only the periodic fallback that picks up out-of-band credential and age-key changes now that Secrets are not watched. Docs: security-model.md and design/reconcile-triggering.md updated to the direct-read model (behavior change, not yet an RBAC change - the default install still grants secrets get;list;watch until the RBAC-narrowing track lands); the gitpathaccepted-projection-race design doc gets a dated note for the removed Secret watch and unified interval. Adds the roadmap and two plan docs under docs/future/. RBAC narrowing (Track B) is intentionally out of scope. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR unifies controller requeue intervals into a single ChangesRequeue interval unification and Secret retention removal
Estimated code review effort: 4 (Complex) | ~60 minutes Design and roadmap documentation
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)Not applicable — the changes are primarily interval/config adjustments, watch removal, and documentation, without new multi-component interaction flows warranting a sequence diagram. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/future/secret-value-retention-plan.md`:
- Around line 29-31: Update the “Current problem” wording to match the new
Secret cache exception introduced by Client.Cache.DisableFor for corev1.Secret.
In the secret-value-retention-plan document, revise the manager/cache baseline
text so it no longer says the manager is simply created without cache options;
make it clear that the shared cache still applies generally, but Secrets are now
explicitly excluded, and keep the distinction between the existing state and
what Track A changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 14b58067-3923-4abf-8f21-bbee92c19a97
📒 Files selected for processing (17)
cmd/main.godocs/design/manifest/gitpathaccepted-projection-race-and-external-drift.mddocs/design/reconcile-triggering.mddocs/future/scoped-rbac-least-privilege-plan.mddocs/future/secret-handling-roadmap.mddocs/future/secret-value-retention-plan.mddocs/security-model.mdinternal/controller/clusterwatchrule_controller.gointernal/controller/constants.gointernal/controller/dependency_watches_test.gointernal/controller/gitprovider_controller.gointernal/controller/gitprovider_controller_test.gointernal/controller/gittarget_controller.gointernal/controller/watchrule_controller.gointernal/git/encryption.gointernal/git/encryption_test.gointernal/watch/manager.go
Comment on lines
+29
to
+31
| The manager is created without cache options: | ||
| [cmd/main.go](../../cmd/main.go#L841). Any informer started by the shared cache is | ||
| cluster-wide unless configured otherwise. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the current-state wording to reflect the new Secret cache exception.
The "Current problem" section says the manager is created "without cache options," but this PR now adds Client.Cache.DisableFor for corev1.Secret. As written, the doc's baseline is no longer accurate and blurs what Track A is changing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/future/secret-value-retention-plan.md` around lines 29 - 31, Update the
“Current problem” wording to match the new Secret cache exception introduced by
Client.Cache.DisableFor for corev1.Secret. In the secret-value-retention-plan
document, revise the manager/cache baseline text so it no longer says the
manager is simply created without cache options; make it clear that the shared
cache still applies generally, but Secrets are now explicitly excluded, and keep
the distinction between the existing state and what Track A changes.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…ommit Follow-up to the secret-value-retention change (plan step 5). With the Secret cache disabled, every typed Secret read is a real API GET, so redundant reads now cost etcd round-trips. commitPendingWrites resolved the credentials Secret unconditionally but only used it on the first commit of a push cycle (the branch that runs PrepareBranch to fetch the remote tip). Every subsequent commit in a burst read the credentials Secret - plus its known_hosts lookups - and discarded it. Resolve auth lazily, only inside the !hasPendingCommits branch that actually touches the remote, mirroring ensureRepositoryInitialized. No memo/cache is introduced (the plan cautions against that - it would reintroduce stale-credential behavior): pushPendingCommits and the first commit still resolve directly, once per operation, reflecting current credentials. This only removes the reads that were never used. Adds a unit test that counts credentials-Secret GETs across a two-commit cycle: 1 on the first commit, 0 on the second. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements Track A, steps 1–3 of the secret-handling roadmap
(
docs/future/secret-handling-roadmap.md) —the immediate process- and crypto-level security wins, ahead of the separate
RBAC-narrowing track (Track B). Detailed plan:
docs/future/secret-value-retention-plan.md.The controller no longer keeps every cluster Secret value in memory, no longer
writes private age keys to disk, and reacts to out-of-band credential/age-key
changes on one steady 5-minute reconcile instead of a Secret informer.
What changed
Step 1 — stop caching Secret values, drop control-plane Secret watches
GitTargetcontrol-plane Secret watch (and its now-orphanedencryptionSecretToGitTargetsmap function). A full-object Secret watch madethe controller-runtime cache retain every Secret value in every namespace.
(
Client.Cache.DisableFor: corev1.Secret), so typed Secret reads go straightto the API server and stay fresh (no stale-cache reads).
GitProviderstays pull-based — no auth-Secret watch, by design.Step 2 — remove private age identities from the SOPS encrypt path
AgeIdentitiesandEnvironmentfromResolvedEncryptionConfig; deletethe age identity temp-file path /
SOPS_AGE_KEY_FILE; stop passing blanketSecret data into the
sopsprocess environment. Recipients still reachsopsthrough the target's committed
.sops.yaml, so encryption is unchanged.private identity is never returned, written to disk, or handed to
sops.Step 3 — unify the control-plane reconcile fallback at 5 minutes
into a single
RequeueSteadyInterval(5 min) forGitProvider,GitTarget,WatchRule, andClusterWatchRule. Genuine spec changes still triggerimmediately via existing watches; this is only the periodic fallback cadence.
Step 5 — resolve Git credentials once per push cycle (data-plane read reduction)
commitPendingWritesresolved the credentials Secret on every commit butonly used it on the first commit of a cycle (the one that fetches the remote
tip via
PrepareBranch). Later commits in a burst read it and threw it away.!hasPendingCommitsbranch — mirroringensureRepositoryInitialized. No memo/cache is introduced (the plan warnsthat would reintroduce stale-credential behavior); each operation still resolves
directly, once, reflecting current credentials. A unit test counts credential
GETs across a two-commit cycle: 1 on the first, 0 on the second.
Docs
docs/security-model.md: the control-plane runtime now reads input Secretsdirectly by name (
get), no longerlist/watches them, and retains no Secretvalues — explicitly called out as a behavior change, not (yet) an RBAC change
(the default install still grants
secrets get;list;watchplus thedynamic-watch wildcard until the RBAC-narrowing track lands).
docs/design/reconcile-triggering.md: updated interval table + dependencytable; the "watch the auth Secret" recommendation is now marked superseded.
docs/design/manifest/gitpathaccepted-projection-race-and-external-drift.md:dated note flagging the removed Secret watch and unified 5-minute interval.
docs/future/.Out of scope
RBAC narrowing (Track B). The default install keeps
secrets get;list;watchuntil the RBAC generator lands; the runtime no longer needs
list/watchoncontroller-owned Secret inputs, so packaging can drop them later.
Validation
task lint✅ ·task test✅ (coverage ratchet holds) ·task test-e2e✅(
TestConfigureSecretEncryptionWriter_NoPrivateAgeMaterial), recipientderivation returns recipient only, steady interval locked at 5 min. E2e SOPS/age
encryption specs exercise the recipients-from-
.sops.yamlpath end-to-end.