diff --git a/docs/TODO.md b/docs/TODO.md index f0929ddf..62cd455e 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -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 diff --git a/docs/design/watch-plane-status-convergence-failures.md b/docs/design/watch-plane-status-convergence-failures.md index df46ee8e..e06c7197 100644 --- a/docs/design/watch-plane-status-convergence-failures.md +++ b/docs/design/watch-plane-status-convergence-failures.md @@ -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 | diff --git a/internal/controller/gittarget_controller_test.go b/internal/controller/gittarget_controller_test.go index 30a9d53b..fab4edc9 100644 --- a/internal/controller/gittarget_controller_test.go +++ b/internal/controller/gittarget_controller_test.go @@ -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) diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index afcf2762..7c3ec1de 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -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" @@ -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())