Skip to content

test(controller): stop caching Secrets in the envtest manager #317

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sunib merged 2 commits into from
Aug 28, 2026
Merged

test(controller): stop caching Secrets in the envtest manager #317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
acea892
test(controller): stop caching Secrets in the envtest manager
sunib Aug 28, 2026
a8dbe9f
docs: drop the encryption-secret flake from the open-work inventories
sunib Aug 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 0 additions & 31 deletions docs/TODO.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,37 +133,6 @@ This file is meant to track the smaller current backlog, not historical notes.
else re-measures it. CI is slower than a dev machine, which would widen that window and explain
why it has never reproduced locally.

- [ ] Fix the encryption-secret recreation flake, and settle what its contract actually is.
`internal/controller/gittarget_controller_test.go` "Should recreate encryption secret when it
is deleted while GitTarget still exists" fails roughly **1 run in 11, on branches and on main**,
with `secrets "recreated-sops-age-key" not found` after its 45s budget. It has been
misattributed to at least two innocent branches, so treat a red build on this spec as ambient
until proven otherwise.

**Do not fix it with a Secret watch.** Any watch needs `list`+`watch` on Secrets, which this
project deliberately does not grant: `cmd/main.go` excludes Secrets from the manager cache,
[gittarget_controller.go](../internal/controller/gittarget_controller.go) records that a
full-object Secret watch was tried and removed because it retained every Secret value in
memory, and [rbac.md](rbac.md) advertises "cannot enumerate Secrets" as a security guarantee.
Recovery must stay polled; only the cadence is in question. Nor is a third widening of the
budget the answer — it has already been raised twice, months apart, and is capped deliberately
as the spec's implicit SLO.

Ambient-flake context, and the traps in reading CI for it, are inventoried in
[watch-plane-status-convergence-failures.md](design/watch-plane-status-convergence-failures.md)
§5.

**Unconfirmed root cause, to verify first:** recreation rides the periodic requeue, and
`gitTargetRequeue` returns `RequeueStreamSettleInterval` (10s) only for a NON-converged target
— 45s is 4.5 ticks of that. Several early-return gate paths return `RequeueSteadyInterval`
(**5 minutes**) regardless of convergence: the `Validated` gate and both `EncryptionConfigured`
gates. A reconcile landing on one of those after the deletion puts the next one five minutes
out, which no 45s budget survives. Note the tension that exposes: the code comment beside the
no-Secret-watch decision says recovery is picked up by "the periodic reconcile
(`RequeueSteadyInterval`)" — so the documented contract may be five minutes while the spec
asserts forty-five seconds, in which case the spec is asserting something the design never
promised. Settle which is wrong before changing either.

- [ ] Re-enable the `goconst` linter with a path-scoped exclusion instead of the current repo-wide
disable in [.golangci.yml](../.golangci.yml). Exempting `test/` and `internal/git/commit.go`
would silence the existing noise (~45 findings, mostly test fixtures) while still catching
Expand Down
15 changes: 2 additions & 13 deletions docs/design/watch-plane-status-convergence-failures.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,27 +448,16 @@ assertion now prints both, so never infer the target's state from the rule's mes

## 5. Flake inventory — what is ambient, and must not be misattributed

Three separate branches have now been blamed for failures that reproduce on `main`. Check this
table before bisecting.
Branches have been blamed for failures that reproduce on `main`. Check this table before
bisecting.

### 5.1 Ambient, confirmed

| Flake | Signature | Rate | Notes |
| --- | --- | --- | --- |
| **Encryption-secret recreation** | `secrets "recreated-sops-age-key" not found`, 45s, `gittarget_controller_test.go:1140` | ~1 in 11 historically; **3 of 4 CI runs on 2026-08-27** | Unit tests. See [`TODO.md`](../TODO.md). Passes on a re-run of identical code. |
| **Refused-GitTarget recovery** | `GitPathAccepted` projection racy both ways; next requeue up to 10 min | Reproduces locally and deterministically in the `unsupported-folder` refusal spec | Do not chase when the diff is test/docs-only. |
| **`target_watch` forbidden race** | `TestTargetWatchReplayAndStream_FallsBackWhenReplayWatchIsForbidden` | Only under `-race`; CI does not use it | Pre-existing shutdown race. |

**The encryption-secret flake's rate is much worse than recorded.** On 2026-08-27 it failed the
Unit job in runs `33116777679`, `33119960052` and `33120703394`, passing only `33118310453` — three
in four, against a documented ~1 in 11. It is not caused by this branch: the failing path touches
neither the watch plane nor anything this branch changes, `task test` passes locally on the same
commits, and the controller log shows the secret being generated (`Generated missing encryption
secret with age key … default/recreated-sops-age-key`) while the test's `Get` never observes it —
the create-then-read race exactly as described. But at this rate **it alone will keep CI from going
green**, so it needs its own fix before this branch can merge on a green run rather than on a
re-run.

### 5.2 Environmental, not code

| Symptom | Cause | Fix |
Expand Down
15 changes: 4 additions & 11 deletions internal/controller/gittarget_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1117,18 +1117,11 @@ var _ = Describe("GitTarget Controller Security", func() {
// Recreation rides the PERIODIC requeue, not a watch: this controller deliberately runs
// no control-plane Secret watch (SetupWithManager explains why), so nothing enqueues the
// GitTarget when its age-key Secret disappears. The wait must therefore exceed a full
// RequeueStreamSettleInterval — with the shared 10s `timeout` it equalled one, so a
// deletion landing just after a reconcile lost the race by milliseconds.
// RequeueStreamSettleInterval, which the shared 10s `timeout` alone does not.
//
// Three and a half intervals, not two: the two-interval budget was still exactly 30s and a
// CI runner busy enough to drop a tick failed the spec on timing alone — twice, months
// apart. Eventually returns as soon as the Secret is back, so the extra room is free on
// every run that was going to pass anyway; it is only ever spent by a run that was going
// to fail, and 15 seconds is a cheap price for not re-litigating a red build.
//
// It is deliberately still a BOUND rather than a generous number. The budget is this
// spec's implicit SLO — recreation must happen within about four ticks of the deletion —
// so a regression that made the requeue path slow rather than broken still fails here.
// The budget is deliberately a BOUND rather than a generous number: it is this spec's
// implicit SLO — recreation must happen within about four ticks of the deletion — so a
// regression that made the requeue path slow rather than broken still fails here.
Eventually(func(g Gomega) {
var recreated corev1.Secret
err := k8sClient.Get(ctx, secretKey, &recreated)
Expand Down
18 changes: 18 additions & 0 deletions internal/controller/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"

corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/rest"
Expand Down Expand Up @@ -71,8 +72,25 @@ var _ = BeforeSuite(func() {
Expect(err).NotTo(HaveOccurred())
Expect(k8sClient).NotTo(BeNil())

// Mirror the production client wiring (cmd/main.go): Secrets are NEVER cached. The control
// plane runs no Secret watch, so a cached read is served by a lazily started cluster-wide
// Secret informer whose view lags every write the suite makes — and a stale answer here is not
// a slow test, it is a wrong one. The GitTarget encryption gate reads its age-key Secret by
// name, and any error or stale hit there parks the target on RequeueSteadyInterval (5 minutes),
// far past what a spec waits for. That is what made "Should recreate encryption secret when it
// is deleted while GitTarget still exists" fail roughly one run in ten: the reconcile that
// should have seen the deletion was served the deleted Secret from cache, or read back a
// just-created one that the informer had not observed yet.
//
// Keep this in step with setupManager in cmd/main.go: a suite whose client is wired
// differently from the binary tests a controller that does not ship.
mgr, err = manager.New(cfg, manager.Options{
Scheme: scheme.Scheme,
Client: client.Options{
Cache: &client.CacheOptions{
DisableFor: []client.Object{&corev1.Secret{}},
},
},
})
Expect(err).NotTo(HaveOccurred())

Expand Down