From 3b4c9ce25c5a81e5f6aff231ff7155d08624a05d Mon Sep 17 00:00:00 2001 From: Staffan Olsson Date: Fri, 4 Sep 2026 07:19:53 +0200 Subject: [PATCH 01/21] gcs: a key missing from keys.list is not evidence to re-mint it First provisioning of a serviceAccount-enabled access minted two keys about a second apart and revoked the first, stranding any consumer that had read the Secret in between (11 of 19 provisionings in one project; a gateway pod served a lapsed token for hours with the access reporting Ready=True). The reconciler's Secret write and status patch both re-enqueue the access, so a second reconcile always follows the first mint within a second. ensureAccessKey then verified the Secret's key against keys.list, and the listing - eventually consistent - had not caught up yet. The code read that as "revoked out of band", minted a replacement, and the reconciler dutifully revoked the key it had just handed out. A revoke is irreversible, so it must rest on positive evidence. The Secret's key is now reused unless it does not parse, names another identity, or the listing shows it expired. Absence from the listing no longer triggers anything. The consequence is that a key deleted server-side is no longer replaced automatically; the rotation runbook becomes "delete the Secret", which mints a fresh key and, through the existing replaced-principal path, revokes the previous one. SPEC, the driver docs and the example README are updated accordingly. The fake IAM gains an "unlisted" set so the lifecycle test covers the lag; it fails on the previous code with the second key minted. Release pin bumped. Co-Authored-By: Claude Fable 5.1 --- SPEC.md | 22 +++-- deploy/kustomize/release/kustomization.yaml | 4 +- examples/gcs/service-account/README.md | 26 ++++-- pkg/drivers/gcs/driver.go | 12 +-- pkg/drivers/gcs/serviceaccount.go | 48 +++++++---- pkg/drivers/gcs/serviceaccount_test.go | 92 +++++++++++++++------ pkg/drivers/registry/registry.go | 8 +- 7 files changed, 148 insertions(+), 64 deletions(-) diff --git a/SPEC.md b/SPEC.md index 8980e95..f9c4f29 100644 --- a/SPEC.md +++ b/SPEC.md @@ -816,12 +816,18 @@ one user-managed key for it — native GCS auth with bucket-scoped blast radius, alongside the (still backend-wide) HMAC pair. The create-only-retrievable-key problem that deferred per-access minting is solved by `GrantRequest.ExistingSecretData`: GrantAccess -returns the Secret's current key unchanged while it still verifies -against `keys.list`, and mints only when the key is absent, revoked -out of band, or expired — which makes server-side key deletion the -manual rotation runbook. `status.principal` is the key's full -resource name and `RevokeAccess` deletes it, so BucketyAccess -deletion performs real revocation for these Secrets. +returns the Secret's current key unchanged, and mints only when +the Secret has none or the key is positively unusable (expired +per `keys.list`, or not this SA's). Absence from `keys.list` is +deliberately NOT a mint trigger: the listing is eventually +consistent and misses a key minted a second earlier, which is +when the follow-up reconcile runs - and a mint revokes the +replaced key, so it must rest on evidence rather than on a +missing entry. The manual rotation runbook is therefore to delete +the Secret: the next reconcile mints a fresh key and revokes the +previous one. `status.principal` is the key's full resource name +and `RevokeAccess` deletes it, so BucketyAccess deletion performs +real revocation for these Secrets. The `serviceAccounts.project` SHOULD be a dedicated identity project, separate from the bucket project: key creation equals @@ -1186,8 +1192,8 @@ and the corresponding GHA secret. machinery is already rotation-shaped, and scheduled key rotation (mint new, overlap one period, garbage-collect the previous key) is the planned follow-up; until then rotation is - operator-driven - delete the key server-side and the next - reconcile re-mints (`gcloud iam service-accounts keys delete`). + operator-driven - delete the access Secret and the next + reconcile mints a fresh key and revokes the previous one. - Multi-cluster federation. - Admission webhook for cross-resource invariants. Per-resource parameter validation (against per-driver schemas) and diff --git a/deploy/kustomize/release/kustomization.yaml b/deploy/kustomize/release/kustomization.yaml index e643889..9352352 100644 --- a/deploy/kustomize/release/kustomization.yaml +++ b/deploy/kustomize/release/kustomization.yaml @@ -7,7 +7,7 @@ kind: Kustomization resources: - ../base images: -- digest: sha256:0b083f26c3472d42e444a83fdaaf34948f123bec39c8c4271b4caf1275dd17ef +- digest: sha256:b59c48e1f02bda32f76fe49791363f260e8464cdd4ccf1b8d8f47006e1f533cf name: ghcr.io/yolean/buckety-controller newName: ghcr.io/yolean/buckety-controller - newTag: 20260729T151707Z + newTag: 20260904T051923Z diff --git a/examples/gcs/service-account/README.md b/examples/gcs/service-account/README.md index 846f185..fe32ece 100644 --- a/examples/gcs/service-account/README.md +++ b/examples/gcs/service-account/README.md @@ -66,13 +66,23 @@ block this feature by design; the grant fails with an actionable ## Rotation (manual, until scheduled rotation lands) ``` -gcloud iam service-accounts keys delete \ - --iam-account= +kubectl -n delete secret ``` -The next reconcile (within the requeue cadence) detects the -revoked key via `keys.list` and mints a fresh one into the Secret -in place. Deleting the `BucketyAccess` revokes its key -(`status.principal` is the key's resource name); deleting the -`Buckety` with `retentionPolicy=Delete` removes the service -account with the bucket. +The controller reconciles the `BucketyAccess` on the Secret's +deletion: it mints a fresh key into a new Secret and then revokes +the previous key. Pods that +loaded the key once (a mounted file read at boot) keep using the +revoked one until restarted, so restart them after the new Secret +appears. + +Deleting the key server-side instead (`gcloud iam service-accounts +keys delete`) does NOT trigger a re-mint: the controller cannot +tell a deleted key from one that `keys.list` has not caught up +with yet, and it never replaces a key on missing evidence. Delete +the Secret afterwards to re-mint. + +Deleting the `BucketyAccess` revokes its key (`status.principal` +is the key's resource name); deleting the `Buckety` with +`retentionPolicy=Delete` removes the service account with the +bucket. diff --git a/pkg/drivers/gcs/driver.go b/pkg/drivers/gcs/driver.go index c80b4cd..694f579 100644 --- a/pkg/drivers/gcs/driver.go +++ b/pkg/drivers/gcs/driver.go @@ -36,11 +36,13 @@ // serviceAccountKeyId alongside the static HMAC pair. Keys are // only retrievable at creation, so GrantAccess - which runs on // every reconcile and rewrites the Secret from its result - reuses -// the key found in GrantRequest.ExistingSecretData while it still -// verifies against keys.list, and mints only when absent, revoked -// out of band, or expired. Scheduled key rotation is a roadmap -// item (SPEC §Roadmap); until then rotation is operator-driven: -// delete the key server-side and the next reconcile re-mints. +// the key found in GrantRequest.ExistingSecretData, and mints only +// when the Secret has none or the key is positively unusable +// (expired per keys.list, or not this SA's); see ensureAccessKey +// for why absence from keys.list does not count. Scheduled key +// rotation is a roadmap item (SPEC §Roadmap); until then rotation +// is operator-driven: delete the Secret and the next reconcile +// mints a fresh key and revokes the previous one. // // The serviceAccounts config names the GCP project the SAs live // in. A DEDICATED identity project (separate from the bucket diff --git a/pkg/drivers/gcs/serviceaccount.go b/pkg/drivers/gcs/serviceaccount.go index 99c375d..85323ae 100644 --- a/pkg/drivers/gcs/serviceaccount.go +++ b/pkg/drivers/gcs/serviceaccount.go @@ -165,14 +165,27 @@ func isMemberNotPropagated(err error) bool { return gapiCode(err, 400) && strings.Contains(err.Error(), "does not exist") } -// ensureAccessKey returns the access's SA key JSON and key id, -// reusing the key carried in the existing Secret while it still -// verifies against the backend. The private material is only -// retrievable at creation, so reuse is what makes GrantAccess -// idempotent; the keys.list check is what makes it self-healing -// (out-of-band revocation -> fresh key on the next reconcile, -// which is also the documented manual rotation runbook until -// scheduled rotation lands). +// ensureAccessKey returns the access's SA key JSON and key id. +// The private material is only retrievable at creation, so the +// key carried in the existing Secret is reused, and a re-mint +// happens only on POSITIVE evidence that it is unusable: the JSON +// does not parse, it names another identity, or keys.list shows +// it expired (constraints/iam.serviceAccountKeyExpiryHours). +// +// Absence from keys.list is NOT such evidence. The listing is +// eventually consistent, and a key created a second earlier is +// routinely missing from it - which is exactly when the second +// reconcile runs, since the Secret write and the status patch of +// the first one both re-enqueue the access. Reading absence as +// "revoked out of band" made that second reconcile mint a +// replacement and revoke the key a consumer had just read from +// the Secret (11 of 19 first provisionings in one project; +// ISSUE_first_provisioning_mints_two_keys_and_revokes_the_one_a_consumer_read.md). +// A revoke is irreversible, so it needs evidence, not the lack of +// it. The cost is that a key deleted server-side is no longer +// replaced on its own; rotation is triggered by deleting the +// Secret instead, which mints a fresh key and (in the reconciler) +// revokes the previous one. func (d *Driver) ensureAccessKey(ctx context.Context, email string, existing map[string][]byte) ([]byte, string, error) { resource := d.saResource(email) if raw, ok := existing["serviceAccountKey"]; ok { @@ -185,17 +198,22 @@ func (d *Driver) ensureAccessKey(ctx context.Context, email string, existing map if err != nil { return nil, "", fmt.Errorf("gcs: list keys of %q: %w", email, err) } + expired := false for _, key := range resp.Keys { - if strings.HasSuffix(key.Name, "/keys/"+k.PrivateKeyID) && !keyExpired(key) { - return raw, k.PrivateKeyID, nil + if strings.HasSuffix(key.Name, "/keys/"+k.PrivateKeyID) { + expired = keyExpired(key) + break } } + if !expired { + return raw, k.PrivateKeyID, nil + } } - // Mismatched email, revoked out of band, or expired - // (constraints/iam.serviceAccountKeyExpiryHours): mint - // fresh. The one key each access holds means nothing else - // needs garbage collection; GCP caps user-managed keys at - // 10 per SA, bounding accesses per bucket accordingly. + // Unparseable, mismatched email, or expired: mint fresh. + // The reconciler revokes the replaced key once the Secret + // carries its successor, so each access holds one key; GCP + // caps user-managed keys at 10 per SA, bounding accesses + // per bucket accordingly. } key, err := d.iamsvc.Projects.ServiceAccounts.Keys.Create(resource, &iam.CreateServiceAccountKeyRequest{}).Context(ctx).Do() if err != nil { diff --git a/pkg/drivers/gcs/serviceaccount_test.go b/pkg/drivers/gcs/serviceaccount_test.go index c4bddf8..273c532 100644 --- a/pkg/drivers/gcs/serviceaccount_test.go +++ b/pkg/drivers/gcs/serviceaccount_test.go @@ -38,6 +38,10 @@ type fakeGCP struct { // bindForbidden with a 403. bindRefusals int bindForbidden int + // unlisted key ids exist (Delete finds them) but are missing + // from keys.list: the eventually consistent listing lagging + // behind a create. + unlisted map[string]bool } type policyBinding struct { @@ -53,6 +57,7 @@ func newFakeGCP(t *testing.T, project string) (*fakeGCP, *httptest.Server) { keys: map[string]map[string]*iam.ServiceAccountKey{}, policies: map[string][]*policyBinding{}, tombstoned: map[string]bool{}, + unlisted: map[string]bool{}, } mux := http.NewServeMux() @@ -123,7 +128,10 @@ func newFakeGCP(t *testing.T, project string) (*fakeGCP, *httptest.Server) { return } resp := iam.ListServiceAccountKeysResponse{} - for _, k := range f.keys[email] { + for id, k := range f.keys[email] { + if f.unlisted[id] { + continue + } resp.Keys = append(resp.Keys, k) } writeJSON(w, resp) @@ -464,24 +472,60 @@ func TestGrantAccessKeyLifecycle(t *testing.T) { t.Fatalf("keys after steady-state grant: %d", n) } - // Out-of-band revocation (also the manual rotation runbook): - // next grant self-heals with a fresh key. + // keys.list lagging behind the create (the second reconcile + // after a first mint runs within a second of it): absence from + // the listing is not evidence of revocation, so the key is + // reused and nothing is minted or replaced. This is the + // regression test for the first-provisioning double mint + // (ISSUE_first_provisioning_mints_two_keys_and_revokes_the_one_a_consumer_read.md). f.mu.Lock() - delete(f.keys[email], keyID) + f.unlisted[keyID] = true f.mu.Unlock() res3, err := d.GrantAccess(ctx, req) if err != nil { - t.Fatalf("post-revocation grant: %v", err) + t.Fatalf("grant during listing lag: %v", err) + } + if res3.Principal != res1.Principal { + t.Errorf("listing lag re-minted: principal %q, want %q", res3.Principal, res1.Principal) + } + if n := len(f.keyIDs(email)); n != 1 { + t.Fatalf("keys after listing-lag grant: %d", n) + } + f.mu.Lock() + delete(f.unlisted, keyID) + f.mu.Unlock() + + // Deleted server-side: indistinguishable from the lag above, + // so the Secret keeps the key. Rotation is a Secret deletion + // (next block), not a key deletion. + f.mu.Lock() + delete(f.keys[email], keyID) + f.mu.Unlock() + res3, err = d.GrantAccess(ctx, req) + if err != nil { + t.Fatalf("grant after server-side deletion: %v", err) + } + if res3.Principal != res1.Principal || len(f.keyIDs(email)) != 0 { + t.Errorf("server-side deletion re-minted: principal %q, keys %v", res3.Principal, f.keyIDs(email)) + } + + // Secret deleted (the rotation runbook): fresh key. The + // reconciler revokes the replaced principal once the new + // Secret is written (TestReplacedPrincipalRevoked). + req.ExistingSecretData = nil + res3, err = d.GrantAccess(ctx, req) + if err != nil { + t.Fatalf("grant after Secret deletion: %v", err) } - if string(res3.SecretData["serviceAccountKeyId"]) == keyID { - t.Error("revoked key reused") + if res3.Principal == res1.Principal { + t.Error("Secret deletion did not mint a fresh key") } if n := len(f.keyIDs(email)); n != 1 { - t.Fatalf("keys after re-mint: %d", n) + t.Fatalf("keys after rotation: %d", n) } // Expired key (org-policy iam.serviceAccountKeyExpiryHours): - // treated as invalid, re-minted. + // positive evidence from the listing, re-minted. req.ExistingSecretData = res3.SecretData f.mu.Lock() f.keys[email][string(res3.SecretData["serviceAccountKeyId"])].ValidBeforeTime = time.Now().Add(-time.Hour).UTC().Format(time.RFC3339) @@ -495,20 +539,22 @@ func TestGrantAccessKeyLifecycle(t *testing.T) { } // A key file for some OTHER identity in the Secret (email - // mismatch) is never trusted. - req.ExistingSecretData = map[string][]byte{ - "serviceAccountKey": []byte(`{"type":"service_account","client_email":"other@id-proj.iam.gserviceaccount.com","private_key_id":"keyX"}`), - } - res5, err := d.GrantAccess(ctx, req) - if err != nil { - t.Fatalf("mismatched-email grant: %v", err) - } - var got struct { - ClientEmail string `json:"client_email"` - } - _ = json.Unmarshal(res5.SecretData["serviceAccountKey"], &got) - if got.ClientEmail != email { - t.Errorf("mismatched-email grant kept foreign key: %q", got.ClientEmail) + // mismatch), or one that does not parse, is never trusted. + for name, raw := range map[string]string{ + "foreign email": `{"type":"service_account","client_email":"other@id-proj.iam.gserviceaccount.com","private_key_id":"keyX"}`, + "garbage": `not json`, + } { + req.ExistingSecretData = map[string][]byte{"serviceAccountKey": []byte(raw)} + res5, err := d.GrantAccess(ctx, req) + if err != nil { + t.Fatalf("%s grant: %v", name, err) + } + var got struct { + ClientEmail string `json:"client_email"` + } + if json.Unmarshal(res5.SecretData["serviceAccountKey"], &got) != nil || got.ClientEmail != email { + t.Errorf("%s grant kept the Secret's key: %s", name, res5.SecretData["serviceAccountKey"]) + } } } diff --git a/pkg/drivers/registry/registry.go b/pkg/drivers/registry/registry.go index b112134..d2e0987 100644 --- a/pkg/drivers/registry/registry.go +++ b/pkg/drivers/registry/registry.go @@ -155,9 +155,11 @@ type GrantRequest struct { // retrievable at creation (GCS SA keys, HMAC secrets): // GrantAccess runs on every reconcile and its result rewrites // the Secret, so such a driver MUST return the existing data - // unchanged while it still verifies against the backend, and - // mint only when it is absent or invalid (which doubles as - // self-healing after out-of-band revocation). + // unchanged and mint only when it is absent or positively + // invalid. A changed Principal makes the reconciler revoke the + // previous one, which is irreversible - so "not found" from an + // eventually consistent backend listing is not grounds to + // mint: it is what a just-minted credential looks like. ExistingSecretData map[string][]byte } From a94170c5553866ec5b2eed502f96220b6354ff24 Mon Sep 17 00:00:00 2001 From: Staffan Olsson Date: Fri, 4 Sep 2026 07:20:46 +0200 Subject: [PATCH 02/21] bucketyaccess: announce a replaced principal with an event When GrantAccess returns a new principal the reconciler revokes the previous one, which is the right thing for a lost or rotated Secret - and invisible: the access stays Ready=True, the Secret holds a valid key, and nothing in the controller log names the namespace. The stranded gateway pod in ISSUE_first_provisioning_mints_two_keys_and_revokes_the_one_a_consumer_read.md was only found through the IAM audit log. A Normal PrincipalReplaced event now names the revoked and the current principal and tells the operator to restart consumers that loaded the Secret once. Steady-state reconciles emit nothing; the test covers both. Release pin bumped. Co-Authored-By: Claude Fable 5.1 --- deploy/kustomize/release/kustomization.yaml | 4 +-- examples/gcs/service-account/README.md | 2 +- pkg/controller/bucketyaccess/reconciler.go | 9 +++++ .../bucketyaccess/reconciler_test.go | 36 +++++++++++++++++-- 4 files changed, 45 insertions(+), 6 deletions(-) diff --git a/deploy/kustomize/release/kustomization.yaml b/deploy/kustomize/release/kustomization.yaml index 9352352..6c6dce0 100644 --- a/deploy/kustomize/release/kustomization.yaml +++ b/deploy/kustomize/release/kustomization.yaml @@ -7,7 +7,7 @@ kind: Kustomization resources: - ../base images: -- digest: sha256:b59c48e1f02bda32f76fe49791363f260e8464cdd4ccf1b8d8f47006e1f533cf +- digest: sha256:6cc01b8163ff6dee01768b5b2f02d5af9fffb4156e2f621ed362c86c16e5edab name: ghcr.io/yolean/buckety-controller newName: ghcr.io/yolean/buckety-controller - newTag: 20260904T051923Z + newTag: 20260904T052038Z diff --git a/examples/gcs/service-account/README.md b/examples/gcs/service-account/README.md index fe32ece..25978ce 100644 --- a/examples/gcs/service-account/README.md +++ b/examples/gcs/service-account/README.md @@ -71,7 +71,7 @@ kubectl -n delete secret The controller reconciles the `BucketyAccess` on the Secret's deletion: it mints a fresh key into a new Secret and then revokes -the previous key. Pods that +the previous key, recording a `PrincipalReplaced` event. Pods that loaded the key once (a mounted file read at boot) keep using the revoked one until restarted, so restart them after the new Secret appears. diff --git a/pkg/controller/bucketyaccess/reconciler.go b/pkg/controller/bucketyaccess/reconciler.go index 190097d..fba0325 100644 --- a/pkg/controller/bucketyaccess/reconciler.go +++ b/pkg/controller/bucketyaccess/reconciler.go @@ -361,6 +361,15 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu _ = r.patchStatus(ctx, &access, baseAccess) return ctrl.Result{}, rerr } + // A replaced credential is invisible from the resource + // otherwise: Ready stays True and the Secret looks fine, + // while a consumer that loaded the old key once is now + // broken. The event is what tells an operator to restart + // such consumers, or that an unrequested rotation happened. + if r.Recorder != nil { + r.Recorder.Event(&access, corev1.EventTypeNormal, "PrincipalReplaced", + fmt.Sprintf("credential re-minted: %q revoked, secret %q now carries %q; restart consumers that loaded the secret once", old, access.Spec.CredentialsSecretName, res.Principal)) + } } access.Status.Principal = res.Principal access.Status.PrincipalRevocable = res.Revocable diff --git a/pkg/controller/bucketyaccess/reconciler_test.go b/pkg/controller/bucketyaccess/reconciler_test.go index 940364d..f372e43 100644 --- a/pkg/controller/bucketyaccess/reconciler_test.go +++ b/pkg/controller/bucketyaccess/reconciler_test.go @@ -2,6 +2,7 @@ package bucketyaccess import ( "context" + "strings" "testing" corev1 "k8s.io/api/core/v1" @@ -351,6 +352,7 @@ func TestReplacedPrincipalRevoked(t *testing.T) { } return bky, access } + events := record.NewFakeRecorder(10) run := func(t *testing.T, rec *grantRecorder) (client.Client, *bucketyv1.BucketyAccess, error) { t.Helper() bky, access := newObjs() @@ -358,7 +360,7 @@ func TestReplacedPrincipalRevoked(t *testing.T) { WithObjects(bky, access). WithStatusSubresource(&bucketyv1.Buckety{}, &bucketyv1.BucketyAccess{}). Build() - r := &Reconciler{Client: cl, Scheme: scheme, Config: &config.Loaded{ + r := &Reconciler{Client: cl, Scheme: scheme, Recorder: events, Config: &config.Loaded{ Backends: map[string]config.Backend{"be": {Name: "be", Driver: rec}}, }} _, err := r.Reconcile(context.Background(), reconcilerRequest("t1", "reader")) @@ -368,8 +370,30 @@ func TestReplacedPrincipalRevoked(t *testing.T) { } return cl, &got, err } + drainEvents := func() []string { + var out []string + for { + select { + case e := <-events.Events: + out = append(out, e) + default: + return out + } + } + } + hasEvent := func(reason string) bool { + for _, e := range drainEvents() { + if strings.Contains(e, " "+reason+" ") { + return true + } + } + return false + } - // Principal change: old revoked, status advances. + // Principal change: old revoked, status advances, and the + // replacement is announced - it is otherwise invisible on a + // resource that stays Ready=True while consumers holding the + // old key are broken. rec := &grantRecorder{ data: map[string][]byte{"bucket": []byte("t1-orders")}, principal: "projects/p/serviceAccounts/x/keys/new", @@ -384,8 +408,11 @@ func TestReplacedPrincipalRevoked(t *testing.T) { if got.Status.Principal != "projects/p/serviceAccounts/x/keys/new" { t.Errorf("principal: %q", got.Status.Principal) } + if !hasEvent("PrincipalReplaced") { + t.Error("principal replacement emitted no PrincipalReplaced event") + } - // Unchanged principal: no revocation. + // Unchanged principal: no revocation, no announcement. rec = &grantRecorder{ data: map[string][]byte{"bucket": []byte("t1-orders")}, principal: "projects/p/serviceAccounts/x/keys/old", @@ -396,6 +423,9 @@ func TestReplacedPrincipalRevoked(t *testing.T) { if len(rec.revoked) != 0 { t.Errorf("steady state revoked: %v", rec.revoked) } + if hasEvent("PrincipalReplaced") { + t.Error("steady state emitted PrincipalReplaced") + } // Revocation failure: reconcile errors and status.principal // still names the old key so the retry revokes it again. From 95aca9712bf75900dddcb1eaf7e6745e8d716381 Mon Sep 17 00:00:00 2001 From: Staffan Olsson Date: Fri, 4 Sep 2026 07:22:49 +0200 Subject: [PATCH 03/21] one build recipe: scripts/build-image.sh The go build + contain invocation was copied five times: the e2e job, the driver-version rotation loop, the publish job's rebuild, scripts/bump-release.sh, and the local-flow comment in contain.yaml (which had already drifted - it lacked -buildvcs=false, so it could not reproduce a pin). CI asserts that a rebuild reproduces the pinned digest, and the digest is a function of exactly these flags, so every copy had to stay byte-identical for the assertion to mean anything. The recipe now lives in one script that prints the digest; the release script and the three CI steps call it, and the two build-then-assert step pairs collapse into one step each. Verified by rebuilding the pinned tag through the script and getting the pinned digest back. Co-Authored-By: Claude Fable 5.1 --- .github/workflows/e2e.yaml | 49 +++++++------------------------------- contain.yaml | 13 ++++------ scripts/build-image.sh | 39 ++++++++++++++++++++++++++++++ scripts/bump-release.sh | 17 +------------ 4 files changed, 52 insertions(+), 66 deletions(-) create mode 100755 scripts/build-image.sh diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml index bae5190..64806b7 100644 --- a/.github/workflows/e2e.yaml +++ b/.github/workflows/e2e.yaml @@ -101,26 +101,13 @@ jobs: - name: Set up contain uses: solsson/setup-contain@49cc4cce1498df8a59e88fff52c1a9b747f11f08 # v1 - - name: Build controller image (contain) with pinned tag + - name: Build controller image with pinned tag, assert the pinned digest env: T: ${{ steps.release.outputs.tag }} - run: | - set -euxo pipefail - CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build \ - -trimpath \ - -buildvcs=false \ - -ldflags "-s -w -X main.version=${T}" \ - -o target/linux/amd64/buckety \ - ./cmd/buckety - IMAGE="ghcr.io/yolean/buckety-controller:${T}" \ - contain build --output ./oci --push=false - - - name: Assert produced digest matches pinned - env: PINNED: ${{ steps.release.outputs.digest }} run: | set -euxo pipefail - BUILT=$(jq -r '.manifests[0].digest' oci/index.json) + BUILT=$(scripts/build-image.sh "$T" ./oci) if [[ "$BUILT" != "$PINNED" ]]; then echo "Pinned digest in deploy/kustomize/release/kustomization.yaml" >&2 echo " ${PINNED}" >&2 @@ -140,17 +127,10 @@ jobs: run: | set -euxo pipefail for v in 0.1.1 1.0.0; do - CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build \ - -trimpath \ - -buildvcs=false \ - -ldflags "-s -w -X main.version=dv-${v} \ - -X github.com/Yolean/buckety-controller/pkg/drivers/kadm.version=${v} \ - -X github.com/Yolean/buckety-controller/pkg/drivers/s3.version=${v} \ - -X github.com/Yolean/buckety-controller/pkg/drivers/gcs.version=${v}" \ - -o target/linux/amd64/buckety \ - ./cmd/buckety - IMAGE="ghcr.io/yolean/buckety-controller:dv-${v}" \ - contain build --output "./oci-dv-${v}" --push=false + scripts/build-image.sh "dv-${v}" "./oci-dv-${v}" \ + -X github.com/Yolean/buckety-controller/pkg/drivers/kadm.version=${v} \ + -X github.com/Yolean/buckety-controller/pkg/drivers/s3.version=${v} \ + -X github.com/Yolean/buckety-controller/pkg/drivers/gcs.version=${v} crane push --insecure "./oci-dv-${v}" \ "localhost:5000/yolean/buckety-controller:dv-${v}" done @@ -214,26 +194,13 @@ jobs: - name: Set up contain uses: solsson/setup-contain@49cc4cce1498df8a59e88fff52c1a9b747f11f08 # v1 - - name: Rebuild controller image with pinned tag + - name: Rebuild controller image with pinned tag, assert the e2e-tested digest env: T: ${{ needs.e2e.outputs.tag }} - run: | - set -euxo pipefail - CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build \ - -trimpath \ - -buildvcs=false \ - -ldflags "-s -w -X main.version=${T}" \ - -o target/linux/amd64/buckety \ - ./cmd/buckety - IMAGE="ghcr.io/yolean/buckety-controller:${T}" \ - contain build --output ./oci --push=false - - - name: Assert publish-time digest matches the e2e-tested digest - env: PINNED: ${{ needs.e2e.outputs.digest }} run: | set -euxo pipefail - BUILT=$(jq -r '.manifests[0].digest' oci/index.json) + BUILT=$(scripts/build-image.sh "$T" ./oci) if [[ "$BUILT" != "$PINNED" ]]; then echo "publish rebuild produced ${BUILT}, e2e verified ${PINNED}" >&2 exit 1 diff --git a/contain.yaml b/contain.yaml index 8e81598..9535699 100644 --- a/contain.yaml +++ b/contain.yaml @@ -1,15 +1,10 @@ # yaml-language-server: $schema=https://github.com/turbokube/contain/raw/refs/heads/main/jsonschema/config.json # -# OCI image spec for ghcr.io/yolean/buckety-controller. Built (and -# eventually pushed) by CI; the local PoC flow is: +# OCI image spec for ghcr.io/yolean/buckety-controller. Built by +# scripts/build-image.sh (the one copy of the build recipe, shared +# by CI and scripts/bump-release.sh); the local flow is: # -# CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build \ -# -trimpath \ -# -ldflags "-s -w -X main.version=$(git describe --always --dirty)" \ -# -o target/linux/amd64/buckety \ -# ./cmd/buckety -# IMAGE=ghcr.io/yolean/buckety-controller:dev \ -# contain build --output ./oci --push=false +# scripts/build-image.sh dev ./oci # y-cluster images load ./oci --context=local base: gcr.io/distroless/static:nonroot@sha256:e3f945647ffb95b5839c07038d64f9811adf17308b9121d8a2b87b6a22a80a39 platforms: diff --git a/scripts/build-image.sh b/scripts/build-image.sh new file mode 100755 index 0000000..41ce4f7 --- /dev/null +++ b/scripts/build-image.sh @@ -0,0 +1,39 @@ +#!/usr/bin/env bash +# +# Build the controller binary and its OCI layout, reproducibly. +# +# scripts/build-image.sh [out-dir] [extra -ldflags...] +# +# The ONE place the build recipe lives. CI's e2e job, the publish +# job and scripts/bump-release.sh all go through here, so the +# digest CI asserts against the release pin is a function of this +# file plus the source and the go.mod toolchain - never of which +# copy of the flags a step happened to carry. +# +# -buildvcs=false is the reproducibility lever: with it on (the +# default) Go embeds the commit SHA and a "modified" flag, which +# makes the digest depend on whether the tree had uncommitted +# files at build time. Off, the binary is a pure function of +# source + flags + Go version. + +set -euo pipefail + +TAG="${1:?usage: build-image.sh [out-dir] [extra -ldflags...]}" +OUT="${2:-./oci}" +shift $(( $# >= 2 ? 2 : $# )) + +here() { cd "$(dirname "${BASH_SOURCE[0]}")" && pwd; } +cd "$(here)/.." + +rm -rf target/linux/amd64 "$OUT" +CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build \ + -trimpath \ + -buildvcs=false \ + -ldflags "-s -w -X main.version=${TAG} $*" \ + -o target/linux/amd64/buckety \ + ./cmd/buckety + +IMAGE="ghcr.io/yolean/buckety-controller:${TAG}" \ + contain build --output "$OUT" --push=false >/dev/null + +jq -r '.manifests[0].digest' "$OUT/index.json" diff --git a/scripts/bump-release.sh b/scripts/bump-release.sh index 99da020..c9abdf4 100755 --- a/scripts/bump-release.sh +++ b/scripts/bump-release.sh @@ -35,22 +35,7 @@ fi TAG="$(date -u +%Y%m%dT%H%M%SZ)" -rm -rf target/linux/amd64 oci -# -buildvcs=false is the reproducibility lever: with it on (the default), -# Go embeds the commit SHA and a "modified" flag, which makes the digest -# depend on whether the tree had uncommitted files at build time. Off, -# the binary is a pure function of source + flags + Go version. -CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build \ - -trimpath \ - -buildvcs=false \ - -ldflags "-s -w -X main.version=${TAG}" \ - -o target/linux/amd64/buckety \ - ./cmd/buckety - -IMAGE="ghcr.io/yolean/buckety-controller:${TAG}" \ - contain build --output ./oci --push=false >/dev/null - -DIGEST="$(jq -r '.manifests[0].digest' oci/index.json)" +DIGEST="$(scripts/build-image.sh "$TAG" ./oci)" [[ "$DIGEST" == sha256:* ]] || { echo "unexpected digest: $DIGEST" >&2; exit 1; } ( cd deploy/kustomize/release \ From ca5616411b62b202ad9a8977b58c1898eee225a1 Mon Sep 17 00:00:00 2001 From: Staffan Olsson Date: Fri, 4 Sep 2026 07:29:39 +0200 Subject: [PATCH 04/21] buckety: a backend re-pointed at another driver is unavailable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SPEC §Buckety shape makes the (backend, driver, driverMajor) triple sticky and says a resource pauses with BackendUnavailable when any element no longer matches. The code compared only the major. Two drivers at major 0 - every driver shipped so far - therefore passed the check, and a backend name re-pointed from kadm to s3 would have reconciled a topic's resource name as a bucket, and under retentionPolicy=Delete deleted it as one. status.driver is now compared to the running driver's name. A mismatch surfaces BackendUnavailable with reason DriverChanged and pauses, and blocks a Delete teardown exactly as a missing backend does. The unavailability reason and message are computed once and threaded to both the reconcile and the delete path, so the two cannot drift. Co-Authored-By: Claude Fable 5.1 --- pkg/controller/buckety/reconciler.go | 52 ++++++++----- pkg/controller/buckety/reconciler_test.go | 90 ++++++++++++++++++++++- 2 files changed, 120 insertions(+), 22 deletions(-) diff --git a/pkg/controller/buckety/reconciler.go b/pkg/controller/buckety/reconciler.go index 78e8ad3..9342186 100644 --- a/pkg/controller/buckety/reconciler.go +++ b/pkg/controller/buckety/reconciler.go @@ -102,11 +102,26 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu } // Resolve the backend up front; nearly every branch needs it. + // The driver behind a backend name is part of the sticky + // (backend, driver, driverMajor) triple (SPEC §Buckety shape), + // so a backend re-pointed at another driver is as unavailable + // as a missing one: reconciling a topic's resource as a bucket, + // or deleting it as one under retentionPolicy=Delete, is worse + // than pausing. backend, backendOK := r.Config.Lookup(bky.Spec.Backend) + unavailableReason, unavailableMsg := "", "" + switch { + case !backendOK: + unavailableReason = "NotInConfig" + unavailableMsg = fmt.Sprintf("backend %q is not registered in buckety-controller.yaml", bky.Spec.Backend) + case bky.Status.Driver != "" && bky.Status.Driver != backend.Driver.Name(): + unavailableReason = "DriverChanged" + unavailableMsg = fmt.Sprintf("backend %q now runs driver %q but this resource was provisioned by driver %q; restore the backend's driver or migrate the resource", bky.Spec.Backend, backend.Driver.Name(), bky.Status.Driver) + } // Deletion path: handle finalizer before anything else. if !bky.DeletionTimestamp.IsZero() { - return r.reconcileDelete(ctx, &bky, backend, backendOK) + return r.reconcileDelete(ctx, &bky, backend, unavailableReason, unavailableMsg) } // Ensure finalizer. Patch (no optimistic lock) instead of @@ -130,8 +145,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu // .Status().Update() would conflict on. baseBky := bky.DeepCopy() - if !backendOK { - return r.surfaceBackendUnavailable(ctx, &bky, baseBky) + if unavailableReason != "" { + return r.surfaceBackendUnavailable(ctx, &bky, baseBky, unavailableReason, unavailableMsg) } // First reconcile: stamp sticky fields. @@ -307,7 +322,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return ctrl.Result{}, nil } -func (r *Reconciler) reconcileDelete(ctx context.Context, bky *bucketyv1.Buckety, backend config.Backend, backendOK bool) (ctrl.Result, error) { +// reconcileDelete runs the finalizer. A non-empty unavailableReason +// means backend cannot be used for this resource (missing from +// config, or now behind a different driver) and blocks a +// retentionPolicy=Delete teardown. +func (r *Reconciler) reconcileDelete(ctx context.Context, bky *bucketyv1.Buckety, backend config.Backend, unavailableReason, unavailableMsg string) (ctrl.Result, error) { base := bky.DeepCopy() // Block on explicit BucketyAccess children before we let the // resource go. @@ -397,14 +416,12 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, bky *bucketyv1.Buckety // until the maintainer restores the backend or the user flips // retentionPolicy to Retain (mutable). if bky.Spec.RetentionPolicy == bucketyv1.RetentionDelete && bky.Status.BackendResourceName != "" { - if !backendOK { - r.eventIfTransition(bky, base.Status.Conditions, "BackendUnavailable", metav1.ConditionTrue, "NotInConfig", - corev1.EventTypeWarning, "DeletionBlocked", - fmt.Sprintf("retentionPolicy=Delete needs backend %q to remove %q", bky.Spec.Backend, bky.Status.BackendResourceName)) - setCond(&bky.Status.Conditions, "BackendUnavailable", metav1.ConditionTrue, "NotInConfig", - fmt.Sprintf("retentionPolicy=Delete needs backend %q to remove %q; restore the backend in buckety-controller.yaml or set retentionPolicy=Retain", - bky.Spec.Backend, bky.Status.BackendResourceName), - bky.Generation) + if unavailableReason != "" { + msg := fmt.Sprintf("retentionPolicy=Delete needs backend %q to remove %q, but %s; restore it or set retentionPolicy=Retain", + bky.Spec.Backend, bky.Status.BackendResourceName, unavailableMsg) + r.eventIfTransition(bky, base.Status.Conditions, "BackendUnavailable", metav1.ConditionTrue, unavailableReason, + corev1.EventTypeWarning, "DeletionBlocked", msg) + setCond(&bky.Status.Conditions, "BackendUnavailable", metav1.ConditionTrue, unavailableReason, msg, bky.Generation) setCond(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "BackendUnavailable", "deletion blocked", bky.Generation) if err := r.patchStatus(ctx, bky, base); err != nil { return ctrl.Result{}, err @@ -529,13 +546,10 @@ func (r *Reconciler) reconcileImplicitAccess(ctx context.Context, bky *bucketyv1 return nil } -func (r *Reconciler) surfaceBackendUnavailable(ctx context.Context, bky, base *bucketyv1.Buckety) (ctrl.Result, error) { - r.eventIfTransition(bky, base.Status.Conditions, "BackendUnavailable", metav1.ConditionTrue, "NotInConfig", - corev1.EventTypeWarning, "BackendUnavailable", - fmt.Sprintf("backend %q is not registered in buckety-controller.yaml", bky.Spec.Backend)) - setCond(&bky.Status.Conditions, "BackendUnavailable", metav1.ConditionTrue, "NotInConfig", - fmt.Sprintf("backend %q is not registered in buckety-controller.yaml", bky.Spec.Backend), - bky.Generation) +func (r *Reconciler) surfaceBackendUnavailable(ctx context.Context, bky, base *bucketyv1.Buckety, reason, msg string) (ctrl.Result, error) { + r.eventIfTransition(bky, base.Status.Conditions, "BackendUnavailable", metav1.ConditionTrue, reason, + corev1.EventTypeWarning, "BackendUnavailable", msg) + setCond(&bky.Status.Conditions, "BackendUnavailable", metav1.ConditionTrue, reason, msg, bky.Generation) setCond(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "BackendUnavailable", "reconcile paused", bky.Generation) return ctrl.Result{}, r.patchStatus(ctx, bky, base) } diff --git a/pkg/controller/buckety/reconciler_test.go b/pkg/controller/buckety/reconciler_test.go index 33ac86d..c5a16b1 100644 --- a/pkg/controller/buckety/reconciler_test.go +++ b/pkg/controller/buckety/reconciler_test.go @@ -246,8 +246,12 @@ func TestPatchStatusIgnoresMessageOnlyChanges(t *testing.T) { } // provisioningDriver stubs registry.Driver with EnsureBuckety -// answering ErrProvisioningInProgress until released. -type provisioningDriver struct{ inProgress bool } +// answering ErrProvisioningInProgress until released, counting +// the calls that reach the backend. +type provisioningDriver struct { + inProgress bool + ensures, deletes int +} func (p *provisioningDriver) Name() string { return "prov" } func (p *provisioningDriver) Version() string { return "0.0.1" } @@ -255,12 +259,16 @@ func (p *provisioningDriver) InspectBuckety(context.Context, string) (registry.I return registry.Inspection{}, nil } func (p *provisioningDriver) EnsureBuckety(context.Context, registry.EnsureRequest) error { + p.ensures++ if p.inProgress { return ®istry.ErrProvisioningInProgress{Progress: "service account x is not yet bindable"} } return nil } -func (p *provisioningDriver) DeleteBuckety(context.Context, registry.DeleteRequest) error { return nil } +func (p *provisioningDriver) DeleteBuckety(context.Context, registry.DeleteRequest) error { + p.deletes++ + return nil +} func (p *provisioningDriver) GrantAccess(context.Context, registry.GrantRequest) (registry.GrantResult, error) { return registry.GrantResult{}, nil } @@ -340,3 +348,79 @@ func TestProvisioningInProgressRequeuesWithoutError(t *testing.T) { t.Errorf("not Ready after provisioning completed: %+v", got.Status.Conditions) } } + +// SPEC §Buckety shape: the (backend, driver, driverMajor) triple +// is sticky. A backend name re-pointed at another driver (both at +// major 0, so the major check alone passes) must pause the +// resource instead of reconciling a topic's name as a bucket, and +// must block a retentionPolicy=Delete teardown that would delete +// on the wrong backend type. +func TestDriverChangeIsBackendUnavailable(t *testing.T) { + scheme := runtime.NewScheme() + if err := clientgoscheme.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + if err := bucketyv1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + ctx := context.Background() + bky := &bucketyv1.Buckety{ + ObjectMeta: metav1.ObjectMeta{ + Name: "orders", Namespace: "t1", + Finalizers: []string{bucketyv1.FinalizerCleanup}, + }, + Spec: bucketyv1.BucketySpec{Backend: "be", RetentionPolicy: bucketyv1.RetentionDelete}, + Status: bucketyv1.BucketyStatus{ + Backend: "be", Driver: "kadm", BackendResourceName: "t1-orders", + Provenance: bucketyv1.ProvenanceCreated, + }, + } + cl := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(bky). + WithStatusSubresource(&bucketyv1.Buckety{}, &bucketyv1.BucketyAccess{}). + Build() + drv := &provisioningDriver{} // Name() is "prov", not "kadm" + r := &Reconciler{Client: cl, Scheme: scheme, Config: &config.Loaded{ + Backends: map[string]config.Backend{"be": {Name: "be", Driver: drv}}, + }} + key := types.NamespacedName{Namespace: "t1", Name: "orders"} + req := reconcile.Request{NamespacedName: key} + + if _, err := r.Reconcile(ctx, req); err != nil { + t.Fatalf("reconcile: %v", err) + } + if drv.ensures != 0 { + t.Errorf("EnsureBuckety reached the wrong driver %d times", drv.ensures) + } + var got bucketyv1.Buckety + if err := cl.Get(ctx, key, &got); err != nil { + t.Fatal(err) + } + unavailable, ready := "", "" + for _, c := range got.Status.Conditions { + switch c.Type { + case "BackendUnavailable": + unavailable = c.Reason + case "Ready": + ready = string(c.Status) + } + } + if unavailable != "DriverChanged" || ready != "False" { + t.Errorf("conditions after driver change: %+v", got.Status.Conditions) + } + + // Deletion under retentionPolicy=Delete blocks the same way a + // missing backend does; the finalizer stays. + if err := cl.Delete(ctx, &got); err != nil { + t.Fatal(err) + } + if _, err := r.Reconcile(ctx, req); err != nil { + t.Fatalf("delete reconcile: %v", err) + } + if drv.deletes != 0 { + t.Errorf("DeleteBuckety reached the wrong driver %d times", drv.deletes) + } + if err := cl.Get(ctx, key, &got); err != nil { + t.Fatalf("resource released to the wrong driver's teardown: %v", err) + } +} From 03fbea0d88a52bebe9e4a9843f9d02f22ddf1c49 Mon Sep 17 00:00:00 2001 From: Staffan Olsson Date: Fri, 4 Sep 2026 07:31:01 +0200 Subject: [PATCH 05/21] buckety: do not trust the informer cache right after writing to it Three places assumed the cache had caught up with this reconciler's own writes, which the reconcile triggered by the status patch a moment later does not guarantee: - reconcileImplicitAccess re-created an access the previous pass had created (AlreadyExists, returned as a hard error with backoff) and re-deleted one it had deleted (NotFound). Both are now the no-ops they mean. - reconcileDelete listed accesses from the cache to decide whether DeleteBuckety may run. That decision is irreversible, so the list now goes through the API reader, as the access reconciler's Secret gate already does. - Both reconcilers returned Requeue: true after adding their finalizer. The patch's own watch event already re-enqueues the object, so this was a guaranteed second reconcile - the shape of the double-reconcile behind the two-keys issue - and the field is deprecated in controller-runtime. The test drives a client whose cached List never shows accesses while Get/Create/Delete see the store. Co-Authored-By: Claude Fable 5.1 --- cmd/buckety/main.go | 1 + pkg/controller/buckety/reconciler.go | 46 +++++++--- pkg/controller/buckety/reconciler_test.go | 102 +++++++++++++++++++++ pkg/controller/bucketyaccess/reconciler.go | 9 +- 4 files changed, 138 insertions(+), 20 deletions(-) diff --git a/cmd/buckety/main.go b/cmd/buckety/main.go index 956aa75..475bda7 100644 --- a/cmd/buckety/main.go +++ b/cmd/buckety/main.go @@ -142,6 +142,7 @@ func main() { Config: loaded, RequeueAfter: requeue, Recorder: mgr.GetEventRecorderFor("buckety-controller"), + Live: mgr.GetAPIReader(), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "buckety controller setup failed") os.Exit(1) diff --git a/pkg/controller/buckety/reconciler.go b/pkg/controller/buckety/reconciler.go index 9342186..69bfdea 100644 --- a/pkg/controller/buckety/reconciler.go +++ b/pkg/controller/buckety/reconciler.go @@ -46,6 +46,22 @@ type Reconciler struct { // Recorder emits Events alongside condition changes so // `kubectl describe` tells the story; nil disables (tests). Recorder record.EventRecorder + // Live reads straight from the apiserver, bypassing the + // manager cache, for the one decision here that is + // irreversible: whether a retentionPolicy=Delete teardown may + // run, which depends on no explicit BucketyAccess existing. + // Wired from mgr.GetAPIReader(); nil falls back to the cached + // client (tests). + Live client.Reader +} + +// liveReader returns the uncached reader, or the cached client +// when tests have not wired one. +func (r *Reconciler) liveReader() client.Reader { + if r.Live != nil { + return r.Live + } + return r.Client } // eventIfTransition emits an Event only when the condition's @@ -128,15 +144,12 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu // Update so we don't conflict with whatever just modified // the resource (e.g. our own controller's previous reconcile // pass, the access reconciler reading the Buckety, etc.). - // Return immediately so the next reconcile sees the - // finalizer in place. + // Return immediately; the patch's own watch event brings the + // next reconcile with the finalizer in place. if !controllerutil.ContainsFinalizer(&bky, bucketyv1.FinalizerCleanup) { patch := client.MergeFrom(bky.DeepCopy()) controllerutil.AddFinalizer(&bky, bucketyv1.FinalizerCleanup) - if err := r.Patch(ctx, &bky, patch); err != nil { - return ctrl.Result{}, err - } - return ctrl.Result{Requeue: true}, nil + return ctrl.Result{}, r.Patch(ctx, &bky, patch) } // Snapshot for status patching; tolerates concurrent RV bumps @@ -329,9 +342,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu func (r *Reconciler) reconcileDelete(ctx context.Context, bky *bucketyv1.Buckety, backend config.Backend, unavailableReason, unavailableMsg string) (ctrl.Result, error) { base := bky.DeepCopy() // Block on explicit BucketyAccess children before we let the - // resource go. + // resource go. Live read: this list gates DeleteBuckety, which + // is irreversible, and an access created moments ago can be + // missing from the informer cache. accesses := &bucketyv1.BucketyAccessList{} - if err := r.List(ctx, accesses, client.InNamespace(bky.Namespace)); err != nil { + if err := r.liveReader().List(ctx, accesses, client.InNamespace(bky.Namespace)); err != nil { return ctrl.Result{}, err } var blocking []string @@ -480,6 +495,12 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, bky *bucketyv1.Buckety // reconcileImplicitAccess materialises a BucketyAccess from // spec.defaultAccess and reclaims it when an explicit access // exists or defaultAccess is removed. See SPEC §Implicit access. +// +// It reads the cache, and the reconcile that follows its own +// create or delete (the status patch re-enqueues this Buckety) +// can run before the informer has caught up, so create tolerates +// AlreadyExists and delete tolerates NotFound: both mean the +// previous pass already did the work. func (r *Reconciler) reconcileImplicitAccess(ctx context.Context, bky *bucketyv1.Buckety) error { accesses := &bucketyv1.BucketyAccessList{} if err := r.List(ctx, accesses, client.InNamespace(bky.Namespace)); err != nil { @@ -520,19 +541,16 @@ func (r *Reconciler) reconcileImplicitAccess(ctx context.Context, bky *bucketyv1 if err := controllerutil.SetControllerReference(bky, newAccess, r.Scheme); err != nil { return err } - return r.Create(ctx, newAccess) + return client.IgnoreAlreadyExists(r.Create(ctx, newAccess)) case implicit != nil && !wantImplicit: // Reclaim. Owner-ref GC will sweep the Secret. - return r.Delete(ctx, implicit) + return client.IgnoreNotFound(r.Delete(ctx, implicit)) case implicit != nil && wantImplicit: // Field drift: update name/role if the user changed // defaultAccess. CredentialsSecretName on the access is // immutable, so we delete-and-recreate if it changed. if implicit.Spec.CredentialsSecretName != bky.Spec.DefaultAccess.CredentialsSecretName { - if err := r.Delete(ctx, implicit); err != nil { - return err - } - return nil // next reconcile will recreate + return client.IgnoreNotFound(r.Delete(ctx, implicit)) // next reconcile recreates } desiredRole := bky.Spec.DefaultAccess.Role if desiredRole == "" { diff --git a/pkg/controller/buckety/reconciler_test.go b/pkg/controller/buckety/reconciler_test.go index c5a16b1..72f408b 100644 --- a/pkg/controller/buckety/reconciler_test.go +++ b/pkg/controller/buckety/reconciler_test.go @@ -9,6 +9,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -424,3 +425,104 @@ func TestDriverChangeIsBackendUnavailable(t *testing.T) { t.Fatalf("resource released to the wrong driver's teardown: %v", err) } } + +// staleLister is a client whose cached List never sees +// BucketyAccess objects, the informer lag after the controller's +// own write. Get/Create/Delete go to the real store. +type staleLister struct{ client.Client } + +func (s staleLister) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + if _, ok := list.(*bucketyv1.BucketyAccessList); ok { + return nil + } + return s.Client.List(ctx, list, opts...) +} + +// The reconcile that follows the implicit access's creation is +// triggered by this reconciler's own status patch and can run +// before the informer lists the new access; a second Create must +// be a no-op, not an AlreadyExists error with workqueue backoff. +// Deletion of the Buckety, which gates the irreversible +// DeleteBuckety on "no explicit access exists", must not trust +// that lagging cache at all. +func TestStaleAccessCacheIsHarmless(t *testing.T) { + scheme := runtime.NewScheme() + if err := clientgoscheme.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + if err := bucketyv1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + ctx := context.Background() + bky := &bucketyv1.Buckety{ + ObjectMeta: metav1.ObjectMeta{ + Name: "orders", Namespace: "t1", UID: "uid-b", + Finalizers: []string{bucketyv1.FinalizerCleanup}, + }, + Spec: bucketyv1.BucketySpec{ + Backend: "be", + RetentionPolicy: bucketyv1.RetentionDelete, + DefaultAccess: &bucketyv1.DefaultAccess{CredentialsSecretName: "orders-creds"}, + }, + } + store := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(bky). + WithStatusSubresource(&bucketyv1.Buckety{}, &bucketyv1.BucketyAccess{}). + Build() + drv := &provisioningDriver{} + r := &Reconciler{Client: staleLister{store}, Live: store, Scheme: scheme, Config: &config.Loaded{ + Backends: map[string]config.Backend{"be": {Name: "be", Driver: drv}}, + }} + key := types.NamespacedName{Namespace: "t1", Name: "orders"} + req := reconcile.Request{NamespacedName: key} + + // Two passes with the cache never showing the implicit access + // the first one created: the second must not error. + for i := 0; i < 2; i++ { + if _, err := r.Reconcile(ctx, req); err != nil { + t.Fatalf("pass %d: %v", i+1, err) + } + } + var implicit bucketyv1.BucketyAccess + if err := store.Get(ctx, key, &implicit); err != nil { + t.Fatalf("implicit access not created: %v", err) + } + + // An explicit access exists (live) while the cache says none: + // deletion must block instead of running DeleteBuckety. + explicit := &bucketyv1.BucketyAccess{ + ObjectMeta: metav1.ObjectMeta{Name: "reader", Namespace: "t1"}, + Spec: bucketyv1.BucketyAccessSpec{ + BucketyRef: bucketyv1.BucketyRef{Name: "orders"}, + CredentialsSecretName: "reader-creds", + }, + } + if err := store.Create(ctx, explicit); err != nil { + t.Fatal(err) + } + var cur bucketyv1.Buckety + if err := store.Get(ctx, key, &cur); err != nil { + t.Fatal(err) + } + if err := store.Delete(ctx, &cur); err != nil { + t.Fatal(err) + } + if _, err := r.Reconcile(ctx, req); err != nil { + t.Fatalf("delete pass: %v", err) + } + if drv.deletes != 0 { + t.Errorf("DeleteBuckety ran %d times with a live explicit access present", drv.deletes) + } + if err := store.Get(ctx, key, &cur); err != nil { + t.Fatalf("buckety released past a live explicit access: %v", err) + } + blocked := false + for _, c := range cur.Status.Conditions { + if c.Type == "BlockedByAccesses" && c.Status == metav1.ConditionTrue { + blocked = true + } + } + if !blocked { + t.Errorf("BlockedByAccesses missing: %+v", cur.Status.Conditions) + } +} diff --git a/pkg/controller/bucketyaccess/reconciler.go b/pkg/controller/bucketyaccess/reconciler.go index fba0325..f2ce8ea 100644 --- a/pkg/controller/bucketyaccess/reconciler.go +++ b/pkg/controller/bucketyaccess/reconciler.go @@ -190,15 +190,12 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu // concurrent modification of the resource between our Get and // our write - common when a Buckety just created this implicit // access and several reconcile events fire in quick succession. - // Return immediately so the next reconcile sees the finalizer - // in place. + // Return immediately; the patch's own watch event brings the + // next reconcile with the finalizer in place. if !controllerutil.ContainsFinalizer(&access, bucketyv1.FinalizerCleanup) { patch := client.MergeFrom(access.DeepCopy()) controllerutil.AddFinalizer(&access, bucketyv1.FinalizerCleanup) - if err := r.Patch(ctx, &access, patch); err != nil { - return ctrl.Result{}, err - } - return ctrl.Result{Requeue: true}, nil + return ctrl.Result{}, r.Patch(ctx, &access, patch) } // Snapshot for status patching; tolerates concurrent RV bumps From 3daf0824ab6f636174fe47bed4bb377dc64a35d8 Mon Sep 17 00:00:00 2001 From: Staffan Olsson Date: Fri, 4 Sep 2026 07:33:16 +0200 Subject: [PATCH 06/21] gcs: verify the owner marker before minting; revoke a mint whose Secret write fails Two gaps on the mint path, the one step where a mistake hands out an identity. SPEC and the package comment say every mutating path verifies the service account's ownership marker; mint did not. It relied on the access reconciler waiting for the Buckety to be Ready, which in turn had required ensureServiceAccount to accept the SA - a cross-reconciler ordering, not a check. With the webhook disabled nothing enforces parameters.serviceAccount immutability, and an access reading a still-Ready cached status can reach GrantAccess with a foreign SA name. Minting now does its own Get and marker check (one IAM read per mint; the reuse path is unchanged). GrantResult gains Minted. A key created in a pass whose Secret write then fails was held by nobody and recorded nowhere: status.principal still named its predecessor, so a retry reused the Secret's old key and deletion revoked the recorded one - the fresh key stayed on the SA forever, one of its ten slots gone. The namespace-terminating branch made this systematic, since it returns without retrying and the access is deleted next. The reconciler now revokes a minted principal in the same pass its write fails. Co-Authored-By: Claude Fable 5.1 --- pkg/controller/bucketyaccess/reconciler.go | 12 +++ .../bucketyaccess/reconciler_test.go | 84 ++++++++++++++++++- pkg/drivers/gcs/driver.go | 7 +- pkg/drivers/gcs/serviceaccount.go | 31 +++++-- pkg/drivers/gcs/serviceaccount_test.go | 49 +++++++++++ pkg/drivers/registry/registry.go | 7 ++ 6 files changed, 178 insertions(+), 12 deletions(-) diff --git a/pkg/controller/bucketyaccess/reconciler.go b/pkg/controller/bucketyaccess/reconciler.go index f2ce8ea..5d9fc5c 100644 --- a/pkg/controller/bucketyaccess/reconciler.go +++ b/pkg/controller/bucketyaccess/reconciler.go @@ -326,6 +326,18 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu // pre-label Secrets into the scoped cache. err = r.writeSecret(ctx, &access, &existing, apierrors.IsNotFound(getErr), res.SecretData) if err != nil { + // A credential minted in this pass and not written anywhere + // is held by nobody and recorded nowhere: status.principal + // still names its predecessor, so neither a retry (which + // reuses the Secret's key) nor deletion (which revokes the + // recorded principal) would ever reach it. Revoke it now; + // the retry mints again. Best effort - a failure here is + // logged, and the write error is what the status carries. + if res.Minted && res.Revocable { + if rerr := backend.Driver.RevokeAccess(ctx, res.Principal); rerr != nil { + log.Error(rerr, "revoking the credential minted for a failed secret write", "principal", res.Principal) + } + } if apierrors.HasStatusCause(err, corev1.NamespaceTerminatingCause) { // The namespace is going away and will take this // BucketyAccess with it; retrying with backoff only diff --git a/pkg/controller/bucketyaccess/reconciler_test.go b/pkg/controller/bucketyaccess/reconciler_test.go index f372e43..7839bf7 100644 --- a/pkg/controller/bucketyaccess/reconciler_test.go +++ b/pkg/controller/bucketyaccess/reconciler_test.go @@ -161,6 +161,7 @@ type grantRecorder struct { req *registry.GrantRequest data map[string][]byte principal string + minted bool revoked []string revokeErr error } @@ -178,7 +179,7 @@ func (g *grantRecorder) GrantAccess(_ context.Context, req registry.GrantRequest if p == "" { p = "rec-principal" } - return registry.GrantResult{SecretData: g.data, Principal: p, Scoped: true}, nil + return registry.GrantResult{SecretData: g.data, Principal: p, Scoped: true, Revocable: g.minted, Minted: g.minted}, nil } func (g *grantRecorder) RevokeAccess(_ context.Context, principal string) error { g.revoked = append(g.revoked, principal) @@ -523,3 +524,84 @@ func TestDeletionBlocksWithoutBackend(t *testing.T) { } } } + +// secretWriteFails is a client whose Secret creates fail with the +// given error; everything else reaches the store. +type secretWriteFails struct { + client.Client + err error +} + +func (s secretWriteFails) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + if _, ok := obj.(*corev1.Secret); ok { + return s.err + } + return s.Client.Create(ctx, obj, opts...) +} + +// A credential minted in a pass whose Secret write fails is held +// by nobody and recorded nowhere (status.principal still names +// the predecessor), so it must be revoked in that same pass - +// including when the namespace is terminating, where no retry +// follows and the access finalizer would revoke only the +// recorded principal. +func TestMintedCredentialRevokedWhenSecretWriteFails(t *testing.T) { + scheme := runtime.NewScheme() + if err := clientgoscheme.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + if err := bucketyv1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + now := metav1.Now() + terminating := &apierrors.StatusError{ErrStatus: metav1.Status{ + Status: metav1.StatusFailure, Code: 403, Reason: metav1.StatusReasonForbidden, + Details: &metav1.StatusDetails{Causes: []metav1.StatusCause{{Type: corev1.NamespaceTerminatingCause}}}, + }} + for name, werr := range map[string]error{ + "transient": context.DeadlineExceeded, + "terminating": terminating, + } { + bky := &bucketyv1.Buckety{ + ObjectMeta: metav1.ObjectMeta{Name: "orders", Namespace: "t1"}, + Spec: bucketyv1.BucketySpec{Backend: "be"}, + Status: bucketyv1.BucketyStatus{ + Backend: "be", BackendResourceName: "t1-orders", + Conditions: []metav1.Condition{{Type: "Ready", Status: metav1.ConditionTrue, Reason: "EnsuredOnBackend", LastTransitionTime: now}}, + }, + } + access := &bucketyv1.BucketyAccess{ + ObjectMeta: metav1.ObjectMeta{ + Name: "reader", Namespace: "t1", UID: "uid-a", + Finalizers: []string{bucketyv1.FinalizerCleanup}, + }, + Spec: bucketyv1.BucketyAccessSpec{ + BucketyRef: bucketyv1.BucketyRef{Name: "orders"}, + CredentialsSecretName: "reader-creds", + }, + } + store := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(bky, access). + WithStatusSubresource(&bucketyv1.Buckety{}, &bucketyv1.BucketyAccess{}). + Build() + rec := &grantRecorder{ + data: map[string][]byte{"bucket": []byte("t1-orders")}, + principal: "projects/p/serviceAccounts/x/keys/fresh", + minted: true, + } + r := &Reconciler{Client: secretWriteFails{store, werr}, Scheme: scheme, Config: &config.Loaded{ + Backends: map[string]config.Backend{"be": {Name: "be", Driver: rec}}, + }} + _, _ = r.Reconcile(context.Background(), reconcilerRequest("t1", "reader")) + if len(rec.revoked) != 1 || rec.revoked[0] != "projects/p/serviceAccounts/x/keys/fresh" { + t.Errorf("%s write failure: revoked %v, want the fresh principal", name, rec.revoked) + } + var got bucketyv1.BucketyAccess + if err := store.Get(context.Background(), types.NamespacedName{Namespace: "t1", Name: "reader"}, &got); err != nil { + t.Fatal(err) + } + if got.Status.Principal != "" { + t.Errorf("%s write failure: status recorded the unwritten principal %q", name, got.Status.Principal) + } + } +} diff --git a/pkg/drivers/gcs/driver.go b/pkg/drivers/gcs/driver.go index 694f579..b14b63c 100644 --- a/pkg/drivers/gcs/driver.go +++ b/pkg/drivers/gcs/driver.go @@ -528,7 +528,7 @@ func (d *Driver) GrantAccess(ctx context.Context, req registry.GrantRequest) (re if region != "" { data["region"] = []byte(region) } - principal, revocable := "gcs-static", false + principal, revocable, minted := "gcs-static", false, false if sa := req.BucketyParameters["serviceAccount"]; sa != "" { if d.iamsvc == nil { // Validation rejects the parameter on non-enabled @@ -538,7 +538,7 @@ func (d *Driver) GrantAccess(ctx context.Context, req registry.GrantRequest) (re return registry.GrantResult{}, fmt.Errorf("gcs: bucket %q declares parameters.serviceAccount but this backend has serviceAccounts disabled", req.BucketyName) } email := d.saEmail(sa) - keyJSON, keyID, err := d.ensureAccessKey(ctx, email, req.ExistingSecretData) + keyJSON, keyID, fresh, err := d.ensureAccessKey(ctx, email, req.BucketyName, req.ExistingSecretData) if err != nil { return registry.GrantResult{}, err } @@ -546,13 +546,14 @@ func (d *Driver) GrantAccess(ctx context.Context, req registry.GrantRequest) (re data["serviceAccountEmail"] = []byte(email) data["serviceAccountKeyId"] = []byte(keyID) principal = d.saResource(email) + "/keys/" + keyID - revocable = true + revocable, minted = true, fresh } return registry.GrantResult{ SecretData: data, Principal: principal, Scoped: false, Revocable: revocable, + Minted: minted, }, nil } diff --git a/pkg/drivers/gcs/serviceaccount.go b/pkg/drivers/gcs/serviceaccount.go index 85323ae..46342ad 100644 --- a/pkg/drivers/gcs/serviceaccount.go +++ b/pkg/drivers/gcs/serviceaccount.go @@ -186,7 +186,16 @@ func isMemberNotPropagated(err error) bool { // replaced on its own; rotation is triggered by deleting the // Secret instead, which mints a fresh key and (in the reconciler) // revokes the previous one. -func (d *Driver) ensureAccessKey(ctx context.Context, email string, existing map[string][]byte) ([]byte, string, error) { +// +// Minting verifies the ownership marker first: a key is +// impersonation, and the reconciler-side ordering that normally +// keeps a foreign SA name from reaching here (the Buckety must be +// Ready, which required ensureServiceAccount to accept it) is not +// a check this function can rely on - with the webhook disabled +// nothing stops parameters.serviceAccount from changing under an +// access that reads the still-Ready cached status. fresh reports +// that a key was created. +func (d *Driver) ensureAccessKey(ctx context.Context, email, bucket string, existing map[string][]byte) (keyJSON []byte, keyID string, fresh bool, err error) { resource := d.saResource(email) if raw, ok := existing["serviceAccountKey"]; ok { var k struct { @@ -196,7 +205,7 @@ func (d *Driver) ensureAccessKey(ctx context.Context, email string, existing map if json.Unmarshal(raw, &k) == nil && k.ClientEmail == email && k.PrivateKeyID != "" { resp, err := d.iamsvc.Projects.ServiceAccounts.Keys.List(resource).KeyTypes("USER_MANAGED").Context(ctx).Do() if err != nil { - return nil, "", fmt.Errorf("gcs: list keys of %q: %w", email, err) + return nil, "", false, fmt.Errorf("gcs: list keys of %q: %w", email, err) } expired := false for _, key := range resp.Keys { @@ -206,7 +215,7 @@ func (d *Driver) ensureAccessKey(ctx context.Context, email string, existing map } } if !expired { - return raw, k.PrivateKeyID, nil + return raw, k.PrivateKeyID, false, nil } } // Unparseable, mismatched email, or expired: mint fresh. @@ -215,16 +224,22 @@ func (d *Driver) ensureAccessKey(ctx context.Context, email string, existing map // caps user-managed keys at 10 per SA, bounding accesses // per bucket accordingly. } + sa, err := d.iamsvc.Projects.ServiceAccounts.Get(resource).Context(ctx).Do() + if err != nil { + return nil, "", false, fmt.Errorf("gcs: get service account %q before minting a key: %w", email, err) + } + if err := verifySAMarker(sa, bucket); err != nil { + return nil, "", false, err + } key, err := d.iamsvc.Projects.ServiceAccounts.Keys.Create(resource, &iam.CreateServiceAccountKeyRequest{}).Context(ctx).Do() if err != nil { - return nil, "", fmt.Errorf("gcs: create key for %q (needs roles/iam.serviceAccountKeyAdmin; the org policy constraints/iam.disableServiceAccountKeyCreation blocks user-managed keys entirely): %w", email, err) + return nil, "", false, fmt.Errorf("gcs: create key for %q (needs roles/iam.serviceAccountKeyAdmin; the org policy constraints/iam.disableServiceAccountKeyCreation blocks user-managed keys entirely): %w", email, err) } - keyJSON, err := base64.StdEncoding.DecodeString(key.PrivateKeyData) + keyJSON, err = base64.StdEncoding.DecodeString(key.PrivateKeyData) if err != nil { - return nil, "", fmt.Errorf("gcs: decode created key for %q: %w", email, err) + return nil, "", false, fmt.Errorf("gcs: decode created key for %q: %w", email, err) } - id := key.Name[strings.LastIndex(key.Name, "/")+1:] - return keyJSON, id, nil + return keyJSON, key.Name[strings.LastIndex(key.Name, "/")+1:], true, nil } // keyExpired reports whether the key's validity window has diff --git a/pkg/drivers/gcs/serviceaccount_test.go b/pkg/drivers/gcs/serviceaccount_test.go index 273c532..c8abaf1 100644 --- a/pkg/drivers/gcs/serviceaccount_test.go +++ b/pkg/drivers/gcs/serviceaccount_test.go @@ -808,3 +808,52 @@ func TestBindPropagationWindow(t *testing.T) { t.Errorf("403 handling: %v", err) } } + +// Minting is the impersonation-shaped step, so it checks the +// ownership marker itself rather than trusting that the Buckety +// reconciler accepted the SA earlier (a stale Ready status plus a +// changed parameters.serviceAccount reaches here with the webhook +// disabled). The result also reports whether a key was created. +func TestGrantAccessRefusesForeignServiceAccount(t *testing.T) { + f, srv := newFakeGCP(t, "id-proj") + d := saDriver(t, srv) + ctx := context.Background() + f.mu.Lock() + f.sas["victim@id-proj.iam.gserviceaccount.com"] = &iam.ServiceAccount{ + Email: "victim@id-proj.iam.gserviceaccount.com", Description: "hand-made", + } + f.mu.Unlock() + if err := d.ensureServiceAccount(ctx, "orders-t1", "bucket-x"); err != nil { + t.Fatal(err) + } + + for _, c := range []struct{ sa, bucket string }{ + {"victim", "bucket-x"}, // no marker at all + {"orders-t1", "bucket-y"}, // marker for another bucket + } { + _, err := d.GrantAccess(ctx, registry.GrantRequest{ + BucketyName: c.bucket, + BucketyParameters: map[string]string{"serviceAccount": c.sa}, + }) + if err == nil || !strings.Contains(err.Error(), "refusing") { + t.Errorf("grant for %s on %s: %v", c.sa, c.bucket, err) + } + if n := len(f.keyIDs(d.saEmail(c.sa))); n != 0 { + t.Errorf("grant for %s on %s minted %d keys", c.sa, c.bucket, n) + } + } + + // The owned SA mints, and says so; the reuse path does not. + req := registry.GrantRequest{BucketyName: "bucket-x", BucketyParameters: map[string]string{"serviceAccount": "orders-t1"}} + res, err := d.GrantAccess(ctx, req) + if err != nil { + t.Fatal(err) + } + if !res.Minted { + t.Error("first grant did not report Minted") + } + req.ExistingSecretData = res.SecretData + if res, err = d.GrantAccess(ctx, req); err != nil || res.Minted { + t.Errorf("reuse reported Minted=%v, err=%v", res.Minted, err) + } +} diff --git a/pkg/drivers/registry/registry.go b/pkg/drivers/registry/registry.go index d2e0987..648380a 100644 --- a/pkg/drivers/registry/registry.go +++ b/pkg/drivers/registry/registry.go @@ -188,6 +188,13 @@ type GrantResult struct { // static-credential accesses keep v1alpha1's release // semantics. Revocable bool + // Minted reports that this call created Principal on the + // backend, as opposed to returning the one already carried by + // ExistingSecretData. The reconciler revokes a minted + // principal whose Secret write then fails: nothing holds it, + // nothing records it, and left alone it would count against + // backend key quotas forever. + Minted bool } // Factory builds a Driver from its raw `config:` block as it From fe5c5edf7aa83e10aede9b47d467ed607e2e1594 Mon Sep 17 00:00:00 2001 From: Staffan Olsson Date: Fri, 4 Sep 2026 07:34:57 +0200 Subject: [PATCH 07/21] buckety: an in-progress answer is trusted for two minutes, not forever ErrProvisioningInProgress became a Normal event and a 2s requeue with no bound. IAM propagation takes seconds, but an SA that never becomes bindable (org policy, disabled account, missing permission) kept the resource at Ready=False/Provisioning indefinitely - a full Ensure against the backend every 2s, reported as progress, with nothing that looks like a failure. Ready's Provisioning reason now carries its own deadline via LastTransitionTime: past provisioningPatience the same driver answer takes the EnsureFailed path (Warning event, workqueue backoff), which is what a stuck resource should look like. Co-Authored-By: Claude Fable 5.1 --- pkg/controller/buckety/reconciler.go | 22 +++++++- pkg/controller/buckety/reconciler_test.go | 65 +++++++++++++++++++++++ 2 files changed, 85 insertions(+), 2 deletions(-) diff --git a/pkg/controller/buckety/reconciler.go b/pkg/controller/buckety/reconciler.go index 69bfdea..8ae9c2b 100644 --- a/pkg/controller/buckety/reconciler.go +++ b/pkg/controller/buckety/reconciler.go @@ -290,12 +290,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu setCond(&bky.Status.Conditions, "Reconciling", metav1.ConditionFalse, "Paused", "drift requires human resolution", bky.Generation) return ctrl.Result{}, r.patchStatus(ctx, &bky, baseBky) } - if registry.IsProvisioningInProgress(err) { + if registry.IsProvisioningInProgress(err) && !provisioningOverdue(baseBky.Status.Conditions) { // The backend needs a moment, not a fix (freshly // created SA awaiting IAM propagation): Normal event, // prompt requeue - the DeletingContents posture, not // the EnsureFailed one, whose Warning + backoff would - // misreport a merely-young resource as broken. + // misreport a merely-young resource as broken. Bounded + // by provisioningPatience: past it, the same answer IS + // a failure and gets the Warning + backoff below. r.eventIfTransition(&bky, baseBky.Status.Conditions, "Ready", metav1.ConditionFalse, "Provisioning", corev1.EventTypeNormal, "Provisioning", err.Error()) setCond(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "Provisioning", err.Error(), bky.Generation) @@ -581,6 +583,22 @@ func (r *Reconciler) surfaceCondition(ctx context.Context, bky, base *bucketyv1. return ctrl.Result{}, r.patchStatus(ctx, bky, base) } +// provisioningPatience bounds how long an ErrProvisioningInProgress +// answer is taken at its word. IAM propagation takes seconds; a +// resource still "provisioning" minutes later is stuck (an org +// policy, a disabled account, a permission), and a Normal event +// with a 2s requeue would hide that indefinitely while hammering +// the backend. +const provisioningPatience = 2 * time.Minute + +// provisioningOverdue reports whether Ready has carried the +// Provisioning reason for longer than provisioningPatience. +func provisioningOverdue(conds []metav1.Condition) bool { + c := meta.FindStatusCondition(conds, "Ready") + return c != nil && c.Status == metav1.ConditionFalse && c.Reason == "Provisioning" && + time.Since(c.LastTransitionTime.Time) > provisioningPatience +} + // decideProvenance is the adoption gate's pure core (SPEC // §Adoption): the provenance to freeze at first reconcile, or "" // when adoption must be refused (exists with content and no diff --git a/pkg/controller/buckety/reconciler_test.go b/pkg/controller/buckety/reconciler_test.go index 72f408b..d899c65 100644 --- a/pkg/controller/buckety/reconciler_test.go +++ b/pkg/controller/buckety/reconciler_test.go @@ -3,8 +3,10 @@ package buckety import ( "context" "testing" + "time" apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -526,3 +528,66 @@ func TestStaleAccessCacheIsHarmless(t *testing.T) { t.Errorf("BlockedByAccesses missing: %+v", cur.Status.Conditions) } } + +// An in-progress answer is trusted for provisioningPatience and +// no longer: a resource still "provisioning" minutes later is +// stuck, and must get the failure posture (error with backoff, +// Warning event) instead of a Normal event every 2s forever. +func TestProvisioningOverdueBecomesFailure(t *testing.T) { + scheme := runtime.NewScheme() + if err := clientgoscheme.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + if err := bucketyv1.AddToScheme(scheme); err != nil { + t.Fatal(err) + } + ctx := context.Background() + for _, c := range []struct { + name string + since time.Duration + fail bool + }{ + {"fresh", 10 * time.Second, false}, + {"overdue", provisioningPatience + time.Minute, true}, + } { + bky := &bucketyv1.Buckety{ + ObjectMeta: metav1.ObjectMeta{ + Name: "orders", Namespace: "t1", + Finalizers: []string{bucketyv1.FinalizerCleanup}, + }, + Spec: bucketyv1.BucketySpec{Backend: "be"}, + Status: bucketyv1.BucketyStatus{ + Backend: "be", Driver: "prov", BackendResourceName: "orders", + Conditions: []metav1.Condition{{ + Type: "Ready", Status: metav1.ConditionFalse, Reason: "Provisioning", + LastTransitionTime: metav1.NewTime(time.Now().Add(-c.since)), + }}, + }, + } + cl := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(bky). + WithStatusSubresource(&bucketyv1.Buckety{}, &bucketyv1.BucketyAccess{}). + Build() + r := &Reconciler{Client: cl, Scheme: scheme, Config: &config.Loaded{ + Backends: map[string]config.Backend{"be": {Name: "be", Driver: &provisioningDriver{inProgress: true}}}, + }} + res, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Namespace: "t1", Name: "orders"}}) + if c.fail && err == nil { + t.Errorf("%s: overdue provisioning did not surface as an error", c.name) + } + if !c.fail && (err != nil || res.RequeueAfter <= 0) { + t.Errorf("%s: err=%v res=%+v, want prompt requeue without error", c.name, err, res) + } + var got bucketyv1.Buckety + if err := cl.Get(ctx, types.NamespacedName{Namespace: "t1", Name: "orders"}, &got); err != nil { + t.Fatal(err) + } + want := "Provisioning" + if c.fail { + want = "EnsureFailed" + } + if r := meta.FindStatusCondition(got.Status.Conditions, "Ready"); r == nil || r.Reason != want { + t.Errorf("%s: Ready condition %+v, want reason %s", c.name, r, want) + } + } +} From a2539a7e1113ebee8475c15c2580980964d8591d Mon Sep 17 00:00:00 2001 From: Staffan Olsson Date: Fri, 4 Sep 2026 07:34:57 +0200 Subject: [PATCH 08/21] kadm: existence for the adoption gate bypasses the metadata cache; config errors surface InspectBuckety answered through kadm's ListTopics, which serves metadata up to MetadataMinAge (5s) old from the client's cache. That answer freezes into status.provenance, and Created is what later licenses DeleteBuckety to remove the topic and its records - an irreversible decision resting on a possibly stale "does not exist". The probe now issues its own metadata request to the brokers; reconciliation of a known topic keeps the cached path. describeTopic skipped per-resource DescribeConfigs errors, which left the config view empty, so an authorization failure made the align step re-apply every config.* key on every reconcile with the cause never surfacing. The error is returned. kmsg becomes a direct dependency. Co-Authored-By: Claude Fable 5.1 --- go.mod | 2 +- pkg/drivers/kadm/driver.go | 44 +++++++++++++++++++++++++++++++++++--- 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/go.mod b/go.mod index 1b17303..f5876d8 100644 --- a/go.mod +++ b/go.mod @@ -13,6 +13,7 @@ require ( github.com/aws/smithy-go v1.27.2 github.com/twmb/franz-go v1.21.2 github.com/twmb/franz-go/pkg/kadm v1.18.0 + github.com/twmb/franz-go/pkg/kmsg v1.13.1 go.uber.org/zap v1.27.1 golang.org/x/sync v0.21.0 google.golang.org/api v0.287.1 @@ -81,7 +82,6 @@ require ( github.com/prometheus/procfs v0.19.2 // indirect github.com/spf13/pflag v1.0.10 // indirect github.com/spiffe/go-spiffe/v2 v2.6.0 // indirect - github.com/twmb/franz-go/pkg/kmsg v1.13.1 // indirect github.com/x448/float16 v0.8.4 // indirect go.opentelemetry.io/auto/sdk v1.2.1 // indirect go.opentelemetry.io/contrib/detectors/gcp v1.43.0 // indirect diff --git a/pkg/drivers/kadm/driver.go b/pkg/drivers/kadm/driver.go index 6c93811..cac1619 100644 --- a/pkg/drivers/kadm/driver.go +++ b/pkg/drivers/kadm/driver.go @@ -20,6 +20,7 @@ import ( "github.com/twmb/franz-go/pkg/kadm" "github.com/twmb/franz-go/pkg/kerr" "github.com/twmb/franz-go/pkg/kgo" + "github.com/twmb/franz-go/pkg/kmsg" ) // DriverName is the name backends reference under @@ -83,11 +84,11 @@ func (d *Driver) Bootstrap() string { // expired records count as none, matching what a consumer could // actually read. func (d *Driver) InspectBuckety(ctx context.Context, name string) (registry.Inspection, error) { - existing, err := d.describeTopic(ctx, name) + exists, err := d.topicExists(ctx, name) if err != nil { return registry.Inspection{}, err } - if existing == nil { + if !exists { return registry.Inspection{}, nil } starts, err := d.aclient.ListStartOffsets(ctx, name) @@ -255,6 +256,40 @@ type topicView struct { configs map[string]string } +// topicExists asks the brokers directly. kadm's ListTopics (which +// describeTopic uses) serves metadata up to MetadataMinAge old +// from the client's cache, which is fine for reconciling a topic +// this client already knows - but the answer here freezes into +// status.provenance, and Created is what later licenses +// DeleteBuckety to remove the topic and its records. A cached +// "does not exist" for a topic that does must not be possible on +// that path. +func (d *Driver) topicExists(ctx context.Context, name string) (bool, error) { + req := kmsg.NewPtrMetadataRequest() + rt := kmsg.NewMetadataRequestTopic() + rt.Topic = kmsg.StringPtr(name) + req.Topics = append(req.Topics, rt) + resp, err := req.RequestWith(ctx, d.kclient) + if err != nil { + return false, fmt.Errorf("kadm: metadata for topic %q: %w", name, err) + } + for _, t := range resp.Topics { + if t.Topic == nil || *t.Topic != name { + continue + } + err := kerr.ErrorForCode(t.ErrorCode) + switch { + case err == nil: + return true, nil + case errors.Is(err, kerr.UnknownTopicOrPartition): + return false, nil + default: + return false, fmt.Errorf("kadm: metadata for topic %q: %w", name, err) + } + } + return false, nil +} + func (d *Driver) describeTopic(ctx context.Context, name string) (*topicView, error) { td, err := d.aclient.ListTopics(ctx, name) if err != nil { @@ -285,7 +320,10 @@ func (d *Driver) describeTopic(ctx context.Context, name string) (*topicView, er view.configs = make(map[string]string) for _, rc := range cfgs { if rc.Err != nil { - continue + // Skipping would leave the config view empty, and the + // align step would then re-apply every config.* key on + // every reconcile without the cause ever surfacing. + return nil, fmt.Errorf("kadm: describe configs %q: %w", name, rc.Err) } for _, c := range rc.Configs { if c.Value != nil { From 2aaf03f01851b249ca08cd0d1996042c8759faf1 Mon Sep 17 00:00:00 2001 From: Staffan Olsson Date: Fri, 4 Sep 2026 07:34:57 +0200 Subject: [PATCH 09/21] s3: only a 403 means "taken by another account" Every non-404 HeadBucket error - timeouts, DNS failures, 500s from the gateway - was reported as "bucket exists but is not accessible with this backend's credentials (name likely taken by another account)", sending the operator to audit credentials and name clashes for what is an outage. The hint is now attached only to a 403 (a synthesized Forbidden/AccessDenied code or a bare status, since HeadBucket answers without a body); everything else is wrapped plainly, as the gcs driver already did. Co-Authored-By: Claude Fable 5.1 --- pkg/drivers/s3/driver.go | 30 +++++++++++++++++++++++++++--- pkg/drivers/s3/driver_test.go | 11 +++++++++++ 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/pkg/drivers/s3/driver.go b/pkg/drivers/s3/driver.go index b15457b..144049e 100644 --- a/pkg/drivers/s3/driver.go +++ b/pkg/drivers/s3/driver.go @@ -124,10 +124,13 @@ func (d *Driver) Version() string { return version } // content. func (d *Driver) InspectBuckety(ctx context.Context, name string) (registry.Inspection, error) { if _, err := d.client.HeadBucket(ctx, &awss3.HeadBucketInput{Bucket: aws.String(name)}); err != nil { - if isNotFound(err) { + switch { + case isNotFound(err): return registry.Inspection{}, nil + case isForbidden(err): + return registry.Inspection{}, fmt.Errorf("s3: bucket %q exists but is not accessible with this backend's credentials (name likely taken by another account): %w", name, err) } - return registry.Inspection{}, fmt.Errorf("s3: bucket %q exists but is not accessible with this backend's credentials (name likely taken by another account): %w", name, err) + return registry.Inspection{}, fmt.Errorf("s3: head bucket %q: %w", name, err) } objs, err := d.client.ListObjectsV2(ctx, &awss3.ListObjectsV2Input{ Bucket: aws.String(name), MaxKeys: aws.Int32(1), @@ -182,7 +185,10 @@ func (d *Driver) EnsureBuckety(ctx context.Context, req registry.EnsureRequest) // ours case isAlreadyExists(err): if _, herr := d.client.HeadBucket(ctx, &awss3.HeadBucketInput{Bucket: aws.String(req.Name)}); herr != nil { - return fmt.Errorf("s3: bucket %q exists but is not accessible with this backend's credentials (name likely taken by another account): %w", req.Name, herr) + if isForbidden(herr) { + return fmt.Errorf("s3: bucket %q exists but is not accessible with this backend's credentials (name likely taken by another account): %w", req.Name, herr) + } + return fmt.Errorf("s3: head bucket %q after create conflict: %w", req.Name, herr) } default: return fmt.Errorf("s3: create bucket %q: %w", req.Name, err) @@ -471,6 +477,24 @@ func isAccessDenied(err error) bool { return errors.As(err, &api) && api.ErrorCode() == "AccessDenied" } +// isForbidden reports a 403 on HeadBucket, the one signal that a +// bucket exists under someone else's account (names are global). +// HeadBucket answers with no body, so the SDK synthesizes the +// code "Forbidden" or leaves only the status; a timeout, DNS +// failure or 500 is none of these and must not be reported as +// "taken by another account". +func isForbidden(err error) bool { + var api smithy.APIError + if errors.As(err, &api) { + switch api.ErrorCode() { + case "Forbidden", "AccessDenied": + return true + } + } + var httpErr interface{ HTTPStatusCode() int } + return errors.As(err, &httpErr) && httpErr.HTTPStatusCode() == http.StatusForbidden +} + // isNotFound reports whether err is the S3 service signalling a // missing bucket. NoSuchBucket is the documented code; some // implementations (VersityGW, MinIO older releases) return diff --git a/pkg/drivers/s3/driver_test.go b/pkg/drivers/s3/driver_test.go index 7276d9c..d491a86 100644 --- a/pkg/drivers/s3/driver_test.go +++ b/pkg/drivers/s3/driver_test.go @@ -143,6 +143,17 @@ func TestErrorClassification(t *testing.T) { if isNotFound(other) { t.Fatal("isNotFound over-matches") } + // HeadBucket 403s arrive as a synthesized "Forbidden" code or + // as a bare status; anything else (timeouts, 500s) must not be + // classified as "taken by another account". + bare403 := &awshttp.ResponseError{ResponseError: &smithyhttp.ResponseError{Response: &smithyhttp.Response{Response: &http.Response{StatusCode: http.StatusForbidden}}}} + if !isForbidden(&smithy.GenericAPIError{Code: "Forbidden"}) || !isForbidden(other) || !isForbidden(bare403) { + t.Fatal("403 not classified as forbidden") + } + bare500 := &awshttp.ResponseError{ResponseError: &smithyhttp.ResponseError{Response: &smithyhttp.Response{Response: &http.Response{StatusCode: http.StatusInternalServerError}}}} + if isForbidden(nsb) || isForbidden(bare500) || isForbidden(fmt.Errorf("dial tcp: i/o timeout")) { + t.Fatal("isForbidden over-matches") + } } // The object-store family parameters on the s3 driver: portable From 1f6427115575d9d728a464eef73b1031388ec0eb Mon Sep 17 00:00:00 2001 From: Staffan Olsson Date: Fri, 4 Sep 2026 07:39:45 +0200 Subject: [PATCH 10/21] controllers: one status/event helper set, one name resolver, a plain recheck duration The two reconcilers each carried byte-identical copies of the condition setter, the transition-gated event emitter, and the "changed beyond messages" status comparison, plus their own copy of the name-template resolver that the webhook had a third copy of. Copies drift: the reconcile-loop fix (a88198d) had to be applied twice, and the resolver comment in config already said "reconcilers and the webhook must not hand-roll the resolution". pkg/controller/status now holds Set, Event and a generic ChangedBeyondMessages; each reconciler keeps a four-line surface() that sets a condition and emits the transition event under the same reason, which is what nearly every call site did in two statements. Backend.ResolveName is the single resolver. RequeueAfter was a func() ctrl.Result injected from main and nil-checked at eight return sites; it is a time.Duration now, zero meaning no periodic requeue, which is what the tests want. ControllerName was never read. Also: the access reconciler reported a parameter-template resolution failure as GrantFailed; it is ParameterTemplate, as on the Buckety. Net: -211 lines, no behaviour change beyond that reason string. Co-Authored-By: Claude Fable 5.1 --- cmd/buckety/main.go | 26 ++- pkg/config/config.go | 16 ++ pkg/controller/buckety/reconciler.go | 201 +++++------------- pkg/controller/buckety/reconciler_test.go | 10 +- pkg/controller/bucketyaccess/reconciler.go | 143 ++++--------- .../bucketyaccess/reconciler_test.go | 62 +----- pkg/controller/status/status.go | 77 +++++++ pkg/controller/status/status_test.go | 82 +++++++ pkg/webhook/webhook.go | 15 +- 9 files changed, 290 insertions(+), 342 deletions(-) create mode 100644 pkg/controller/status/status.go create mode 100644 pkg/controller/status/status_test.go diff --git a/cmd/buckety/main.go b/cmd/buckety/main.go index 475bda7..d408ef8 100644 --- a/cmd/buckety/main.go +++ b/cmd/buckety/main.go @@ -134,26 +134,24 @@ func main() { os.Exit(1) } - requeue := func() ctrl.Result { return ctrl.Result{RequeueAfter: periodicRecheck} } - if err := (&bucketyctrl.Reconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - Config: loaded, - RequeueAfter: requeue, - Recorder: mgr.GetEventRecorderFor("buckety-controller"), - Live: mgr.GetAPIReader(), + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Config: loaded, + Recheck: periodicRecheck, + Recorder: mgr.GetEventRecorderFor("buckety-controller"), + Live: mgr.GetAPIReader(), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "buckety controller setup failed") os.Exit(1) } if err := (&accessctrl.Reconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - Config: loaded, - RequeueAfter: requeue, - Recorder: mgr.GetEventRecorderFor("buckety-controller"), - Live: mgr.GetAPIReader(), + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Config: loaded, + Recheck: periodicRecheck, + Recorder: mgr.GetEventRecorderFor("buckety-controller"), + Live: mgr.GetAPIReader(), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "bucketyaccess controller setup failed") os.Exit(1) diff --git a/pkg/config/config.go b/pkg/config/config.go index fcf3135..3df29b3 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -78,6 +78,22 @@ func (b Backend) EffectiveParameters(crParams map[string]string) map[string]stri return out } +// ResolveName resolves a Buckety's spec.name template (empty +// means metadata.name) against its metadata and this backend's +// defaults. Admission and the reconciler both call this, so they +// cannot disagree on what a template resolves to. +func (b Backend) ResolveName(specName, name, namespace string, labels map[string]string) (string, error) { + if specName == "" { + return name, nil + } + return template.Resolve(specName, template.Inputs{ + Name: name, + Namespace: namespace, + Labels: labels, + BackendDefaults: b.Defaults, + }) +} + // ResolvedParameters is EffectiveParameters followed by template // resolution of the keys the driver declares templated, against // the Buckety's metadata.name/namespace and this backend's diff --git a/pkg/controller/buckety/reconciler.go b/pkg/controller/buckety/reconciler.go index 8ae9c2b..7725693 100644 --- a/pkg/controller/buckety/reconciler.go +++ b/pkg/controller/buckety/reconciler.go @@ -12,7 +12,6 @@ import ( "strings" "time" - "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -30,8 +29,8 @@ import ( bucketyv1 "github.com/Yolean/buckety-controller/pkg/api/v1alpha1" "github.com/Yolean/buckety-controller/pkg/config" + "github.com/Yolean/buckety-controller/pkg/controller/status" "github.com/Yolean/buckety-controller/pkg/drivers/registry" - "github.com/Yolean/buckety-controller/pkg/template" ) // Reconciler reconciles Buckety resources. Backends are resolved @@ -39,10 +38,11 @@ import ( // requires re-rolling the Pod per SPEC §controller config file). type Reconciler struct { client.Client - Scheme *runtime.Scheme - Config *config.Loaded - RequeueAfter func() ctrl.Result // periodic re-check cadence; injected so tests can stub - ControllerName string // for ownership references on implicit BucketyAccess + Scheme *runtime.Scheme + Config *config.Loaded + // Recheck is the periodic re-check cadence returned from every + // settled reconcile; zero disables it (tests). + Recheck time.Duration // Recorder emits Events alongside condition changes so // `kubectl describe` tells the story; nil disables (tests). Recorder record.EventRecorder @@ -64,20 +64,11 @@ func (r *Reconciler) liveReader() client.Reader { return r.Client } -// eventIfTransition emits an Event only when the condition's -// (status, reason) pair differs from the pre-reconcile state, so -// steady-state requeues do not spam the event stream. Reason is -// part of the comparison: Ready staying False while its reason -// moves (e.g. WaitingForBuckety to SecretConflict) is a -// transition users need to see. -func (r *Reconciler) eventIfTransition(obj runtime.Object, baseConds []metav1.Condition, condType string, status metav1.ConditionStatus, condReason, eventType, eventReason, message string) { - if r.Recorder == nil { - return - } - if c := meta.FindStatusCondition(baseConds, condType); c != nil && c.Status == status && c.Reason == condReason { - return - } - r.Recorder.Event(obj, eventType, eventReason, message) +// surface sets a condition and, when that is a transition from +// base, emits an Event under the same reason. +func (r *Reconciler) surface(bky, base *bucketyv1.Buckety, typ string, st metav1.ConditionStatus, reason, msg, eventType string) { + status.Event(r.Recorder, bky, base.Status.Conditions, typ, st, reason, eventType, reason, msg) + status.Set(&bky.Status.Conditions, typ, st, reason, msg, bky.Generation) } // SetupWithManager registers the controller with the supplied @@ -164,7 +155,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu // First reconcile: stamp sticky fields. if bky.Status.Backend == "" { - resolved, err := resolveName(&bky, backend) + resolved, err := backend.ResolveName(bky.Spec.Name, bky.Name, bky.Namespace, bky.Labels) if err != nil { return r.surfaceCondition(ctx, &bky, baseBky, "Ready", metav1.ConditionFalse, "NameTemplate", err.Error()) } @@ -182,9 +173,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu // until the spec or the backend changes. inspection, err := backend.Driver.InspectBuckety(ctx, resolved) if err != nil { - r.eventIfTransition(&bky, baseBky.Status.Conditions, "Ready", metav1.ConditionFalse, "InspectFailed", - corev1.EventTypeWarning, "InspectFailed", err.Error()) - setCond(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "InspectFailed", err.Error(), bky.Generation) + r.surface(&bky, baseBky, "Ready", metav1.ConditionFalse, "InspectFailed", err.Error(), corev1.EventTypeWarning) _ = r.patchStatus(ctx, &bky, baseBky) return ctrl.Result{}, err } @@ -193,20 +182,15 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu msg := fmt.Sprintf( "backend resource %q already exists on backend %q and holds content; refusing to adopt it. Set spec.adoption=Adopt to claim it (adopted resources are never deleted from the backend), or change spec.name.", resolved, backend.Name) - r.eventIfTransition(&bky, baseBky.Status.Conditions, "Ready", metav1.ConditionFalse, "BackendResourceExists", - corev1.EventTypeWarning, "BackendResourceExists", msg) - setCond(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "BackendResourceExists", msg, bky.Generation) - setCond(&bky.Status.Conditions, "Reconciling", metav1.ConditionFalse, "BackendResourceExists", "adoption refused", bky.Generation) + r.surface(&bky, baseBky, "Ready", metav1.ConditionFalse, "BackendResourceExists", msg, corev1.EventTypeWarning) + status.Set(&bky.Status.Conditions, "Reconciling", metav1.ConditionFalse, "BackendResourceExists", "adoption refused", bky.Generation) if err := r.patchStatus(ctx, &bky, baseBky); err != nil { return ctrl.Result{}, err } // Spec edits re-enqueue via the watch; the periodic // cadence catches the backend resource being emptied // or removed out-of-band. - if r.RequeueAfter != nil { - return r.RequeueAfter(), nil - } - return ctrl.Result{}, nil + return ctrl.Result{RequeueAfter: r.Recheck}, nil } if provenance == bucketyv1.ProvenanceAdopted && r.Recorder != nil { r.Recorder.Event(&bky, corev1.EventTypeNormal, "Adopted", @@ -234,14 +218,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu // exempt every pre-1.0 resource from the compatibility check. runningMajor, _ := majorOf(backend.Driver.Version()) if bky.Status.Backend != "" && runningMajor != bky.Status.DriverMajor { - r.eventIfTransition(&bky, baseBky.Status.Conditions, "DriverVersionIncompatible", metav1.ConditionTrue, "DriverMajorBump", + status.Event(r.Recorder, &bky, baseBky.Status.Conditions, "DriverVersionIncompatible", metav1.ConditionTrue, "DriverMajorBump", corev1.EventTypeWarning, "DriverVersionIncompatible", fmt.Sprintf("stamped major=%d, running=%d; reconcile paused", bky.Status.DriverMajor, runningMajor)) - setCond(&bky.Status.Conditions, "DriverVersionIncompatible", metav1.ConditionTrue, + status.Set(&bky.Status.Conditions, "DriverVersionIncompatible", metav1.ConditionTrue, "DriverMajorBump", fmt.Sprintf("stamped major=%d, running=%d; pin a compatible binary or migrate the resource", bky.Status.DriverMajor, runningMajor), bky.Generation) - setCond(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "DriverVersionIncompatible", "reconcile paused", bky.Generation) + status.Set(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "DriverVersionIncompatible", "reconcile paused", bky.Generation) _ = r.patchStatus(ctx, &bky, baseBky) return ctrl.Result{}, nil } @@ -262,32 +246,28 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu // config.Backend.ResolvedParameters). effective, perr := backend.ResolvedParameters(bky.Name, bky.Namespace, bky.Spec.Parameters) if perr != nil { - r.eventIfTransition(&bky, baseBky.Status.Conditions, "Ready", metav1.ConditionFalse, "ParameterTemplate", - corev1.EventTypeWarning, "ParameterTemplate", perr.Error()) - setCond(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "ParameterTemplate", perr.Error(), bky.Generation) - setCond(&bky.Status.Conditions, "Reconciling", metav1.ConditionFalse, "ParameterTemplate", "spec change required", bky.Generation) + r.surface(&bky, baseBky, "Ready", metav1.ConditionFalse, "ParameterTemplate", perr.Error(), corev1.EventTypeWarning) + status.Set(&bky.Status.Conditions, "Reconciling", metav1.ConditionFalse, "ParameterTemplate", "spec change required", bky.Generation) return ctrl.Result{}, r.patchStatus(ctx, &bky, baseBky) } if err := backend.Driver.ValidateParameters(effective); err != nil { - r.eventIfTransition(&bky, baseBky.Status.Conditions, "Ready", metav1.ConditionFalse, "InvalidParameters", - corev1.EventTypeWarning, "InvalidParameters", err.Error()) - setCond(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "InvalidParameters", err.Error(), bky.Generation) - setCond(&bky.Status.Conditions, "Reconciling", metav1.ConditionFalse, "InvalidParameters", "spec change required", bky.Generation) + r.surface(&bky, baseBky, "Ready", metav1.ConditionFalse, "InvalidParameters", err.Error(), corev1.EventTypeWarning) + status.Set(&bky.Status.Conditions, "Reconciling", metav1.ConditionFalse, "InvalidParameters", "spec change required", bky.Generation) return ctrl.Result{}, r.patchStatus(ctx, &bky, baseBky) } // Reconcile the backend resource itself. - setCond(&bky.Status.Conditions, "Reconciling", metav1.ConditionTrue, "Ensuring", "calling driver.EnsureBuckety", bky.Generation) + status.Set(&bky.Status.Conditions, "Reconciling", metav1.ConditionTrue, "Ensuring", "calling driver.EnsureBuckety", bky.Generation) if err := backend.Driver.EnsureBuckety(ctx, registry.EnsureRequest{ Name: bky.Status.BackendResourceName, Parameters: effective, }); err != nil { if registry.IsParameterDrift(err) { - r.eventIfTransition(&bky, baseBky.Status.Conditions, "ParameterDrift", metav1.ConditionTrue, "Unreconcilable", + status.Event(r.Recorder, &bky, baseBky.Status.Conditions, "ParameterDrift", metav1.ConditionTrue, "Unreconcilable", corev1.EventTypeWarning, "ParameterDrift", err.Error()) - setCond(&bky.Status.Conditions, "ParameterDrift", metav1.ConditionTrue, "Unreconcilable", err.Error(), bky.Generation) - setCond(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "ParameterDrift", err.Error(), bky.Generation) - setCond(&bky.Status.Conditions, "Reconciling", metav1.ConditionFalse, "Paused", "drift requires human resolution", bky.Generation) + status.Set(&bky.Status.Conditions, "ParameterDrift", metav1.ConditionTrue, "Unreconcilable", err.Error(), bky.Generation) + status.Set(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "ParameterDrift", err.Error(), bky.Generation) + status.Set(&bky.Status.Conditions, "Reconciling", metav1.ConditionFalse, "Paused", "drift requires human resolution", bky.Generation) return ctrl.Result{}, r.patchStatus(ctx, &bky, baseBky) } if registry.IsProvisioningInProgress(err) && !provisioningOverdue(baseBky.Status.Conditions) { @@ -298,18 +278,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu // misreport a merely-young resource as broken. Bounded // by provisioningPatience: past it, the same answer IS // a failure and gets the Warning + backoff below. - r.eventIfTransition(&bky, baseBky.Status.Conditions, "Ready", metav1.ConditionFalse, "Provisioning", - corev1.EventTypeNormal, "Provisioning", err.Error()) - setCond(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "Provisioning", err.Error(), bky.Generation) + r.surface(&bky, baseBky, "Ready", metav1.ConditionFalse, "Provisioning", err.Error(), corev1.EventTypeNormal) if perr := r.patchStatus(ctx, &bky, baseBky); perr != nil { return ctrl.Result{}, perr } return ctrl.Result{RequeueAfter: 2 * time.Second}, nil } log.Error(err, "driver.EnsureBuckety failed") - r.eventIfTransition(&bky, baseBky.Status.Conditions, "Ready", metav1.ConditionFalse, "EnsureFailed", - corev1.EventTypeWarning, "EnsureFailed", err.Error()) - setCond(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "EnsureFailed", err.Error(), bky.Generation) + r.surface(&bky, baseBky, "Ready", metav1.ConditionFalse, "EnsureFailed", err.Error(), corev1.EventTypeWarning) _ = r.patchStatus(ctx, &bky, baseBky) return ctrl.Result{}, err } @@ -322,19 +298,16 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu } // All done. - r.eventIfTransition(&bky, baseBky.Status.Conditions, "Ready", metav1.ConditionTrue, "EnsuredOnBackend", + status.Event(r.Recorder, &bky, baseBky.Status.Conditions, "Ready", metav1.ConditionTrue, "EnsuredOnBackend", corev1.EventTypeNormal, "Provisioned", fmt.Sprintf("backend resource %q ensured on backend %q", bky.Status.BackendResourceName, bky.Status.Backend)) - setCond(&bky.Status.Conditions, "Reconciling", metav1.ConditionFalse, "Idle", "", bky.Generation) - setCond(&bky.Status.Conditions, "Ready", metav1.ConditionTrue, "EnsuredOnBackend", "", bky.Generation) + status.Set(&bky.Status.Conditions, "Reconciling", metav1.ConditionFalse, "Idle", "", bky.Generation) + status.Set(&bky.Status.Conditions, "Ready", metav1.ConditionTrue, "EnsuredOnBackend", "", bky.Generation) bky.Status.ObservedGeneration = bky.Generation if err := r.patchStatus(ctx, &bky, baseBky); err != nil { return ctrl.Result{}, err } - if r.RequeueAfter != nil { - return r.RequeueAfter(), nil - } - return ctrl.Result{}, nil + return ctrl.Result{RequeueAfter: r.Recheck}, nil } // reconcileDelete runs the finalizer. A non-empty unavailableReason @@ -363,10 +336,10 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, bky *bucketyv1.Buckety blocking = append(blocking, a.Name) } if len(blocking) > 0 { - r.eventIfTransition(bky, base.Status.Conditions, "BlockedByAccesses", metav1.ConditionTrue, "Pending", + status.Event(r.Recorder, bky, base.Status.Conditions, "BlockedByAccesses", metav1.ConditionTrue, "Pending", corev1.EventTypeWarning, "BlockedByAccesses", fmt.Sprintf("deletion waits on BucketyAccess: %s", strings.Join(blocking, ", "))) - setCond(&bky.Status.Conditions, "BlockedByAccesses", metav1.ConditionTrue, "Pending", + status.Set(&bky.Status.Conditions, "BlockedByAccesses", metav1.ConditionTrue, "Pending", fmt.Sprintf("waiting on BucketyAccess: %s", strings.Join(blocking, ", ")), bky.Generation) if err := r.patchStatus(ctx, bky, base); err != nil { @@ -374,10 +347,7 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, bky *bucketyv1.Buckety } // Requeue periodically; the BucketyAccess deletions also // re-enqueue this Buckety via the access watch. - if r.RequeueAfter != nil { - return r.RequeueAfter(), nil - } - return ctrl.Result{}, nil + return ctrl.Result{RequeueAfter: r.Recheck}, nil } // The implicit access is not a deletion blocker, but it must @@ -403,7 +373,7 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, bky *bucketyv1.Buckety // Normally one pass - but a revocable principal with its // backend missing blocks the access (with its own // condition), so say what is being waited on. - setCond(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "RevokingAccesses", + status.Set(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "RevokingAccesses", fmt.Sprintf("waiting for implicit BucketyAccess %q to revoke before teardown", a.Name), bky.Generation) if err := r.patchStatus(ctx, bky, base); err != nil { @@ -436,17 +406,14 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, bky *bucketyv1.Buckety if unavailableReason != "" { msg := fmt.Sprintf("retentionPolicy=Delete needs backend %q to remove %q, but %s; restore it or set retentionPolicy=Retain", bky.Spec.Backend, bky.Status.BackendResourceName, unavailableMsg) - r.eventIfTransition(bky, base.Status.Conditions, "BackendUnavailable", metav1.ConditionTrue, unavailableReason, + status.Event(r.Recorder, bky, base.Status.Conditions, "BackendUnavailable", metav1.ConditionTrue, unavailableReason, corev1.EventTypeWarning, "DeletionBlocked", msg) - setCond(&bky.Status.Conditions, "BackendUnavailable", metav1.ConditionTrue, unavailableReason, msg, bky.Generation) - setCond(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "BackendUnavailable", "deletion blocked", bky.Generation) + status.Set(&bky.Status.Conditions, "BackendUnavailable", metav1.ConditionTrue, unavailableReason, msg, bky.Generation) + status.Set(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "BackendUnavailable", "deletion blocked", bky.Generation) if err := r.patchStatus(ctx, bky, base); err != nil { return ctrl.Result{}, err } - if r.RequeueAfter != nil { - return r.RequeueAfter(), nil - } - return ctrl.Result{}, nil + return ctrl.Result{RequeueAfter: r.Recheck}, nil } // The same resolved parameter view Ensure operated on, so // the driver can find per-resource principals (gcs @@ -456,9 +423,9 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, bky *bucketyv1.Buckety // deletion blocks rather than orphaning the principal. effective, perr := backend.ResolvedParameters(bky.Name, bky.Namespace, bky.Spec.Parameters) if perr != nil { - r.eventIfTransition(bky, base.Status.Conditions, "Ready", metav1.ConditionFalse, "ParameterTemplate", + status.Event(r.Recorder, bky, base.Status.Conditions, "Ready", metav1.ConditionFalse, "ParameterTemplate", corev1.EventTypeWarning, "DeleteFailed", perr.Error()) - setCond(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "ParameterTemplate", perr.Error(), bky.Generation) + status.Set(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "ParameterTemplate", perr.Error(), bky.Generation) _ = r.patchStatus(ctx, bky, base) return ctrl.Result{}, perr } @@ -471,17 +438,13 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, bky *bucketyv1.Buckety // slices; this is progress, not failure. The // (status, reason) event gate keeps the stream // quiet while the message advances. - r.eventIfTransition(bky, base.Status.Conditions, "Ready", metav1.ConditionFalse, "DeletingContents", - corev1.EventTypeNormal, "DeletingContents", err.Error()) - setCond(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "DeletingContents", err.Error(), bky.Generation) + r.surface(bky, base, "Ready", metav1.ConditionFalse, "DeletingContents", err.Error(), corev1.EventTypeNormal) if perr := r.patchStatus(ctx, bky, base); perr != nil { return ctrl.Result{}, perr } return ctrl.Result{RequeueAfter: 2 * time.Second}, nil } - r.eventIfTransition(bky, base.Status.Conditions, "Ready", metav1.ConditionFalse, "DeleteFailed", - corev1.EventTypeWarning, "DeleteFailed", err.Error()) - setCond(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "DeleteFailed", err.Error(), bky.Generation) + r.surface(bky, base, "Ready", metav1.ConditionFalse, "DeleteFailed", err.Error(), corev1.EventTypeWarning) _ = r.patchStatus(ctx, bky, base) return ctrl.Result{}, err } @@ -567,19 +530,17 @@ func (r *Reconciler) reconcileImplicitAccess(ctx context.Context, bky *bucketyv1 } func (r *Reconciler) surfaceBackendUnavailable(ctx context.Context, bky, base *bucketyv1.Buckety, reason, msg string) (ctrl.Result, error) { - r.eventIfTransition(bky, base.Status.Conditions, "BackendUnavailable", metav1.ConditionTrue, reason, + status.Event(r.Recorder, bky, base.Status.Conditions, "BackendUnavailable", metav1.ConditionTrue, reason, corev1.EventTypeWarning, "BackendUnavailable", msg) - setCond(&bky.Status.Conditions, "BackendUnavailable", metav1.ConditionTrue, reason, msg, bky.Generation) - setCond(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "BackendUnavailable", "reconcile paused", bky.Generation) + status.Set(&bky.Status.Conditions, "BackendUnavailable", metav1.ConditionTrue, reason, msg, bky.Generation) + status.Set(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "BackendUnavailable", "reconcile paused", bky.Generation) return ctrl.Result{}, r.patchStatus(ctx, bky, base) } -func (r *Reconciler) surfaceCondition(ctx context.Context, bky, base *bucketyv1.Buckety, condType string, status metav1.ConditionStatus, reason, message string) (ctrl.Result, error) { - // Only failure surfaces route through here; the happy path - // records its own Provisioned event. - r.eventIfTransition(bky, base.Status.Conditions, condType, status, reason, - corev1.EventTypeWarning, reason, message) - setCond(&bky.Status.Conditions, condType, status, reason, message, bky.Generation) +// surfaceCondition is surface for failures that end the reconcile: +// Warning event, then the status patch. +func (r *Reconciler) surfaceCondition(ctx context.Context, bky, base *bucketyv1.Buckety, typ string, st metav1.ConditionStatus, reason, msg string) (ctrl.Result, error) { + r.surface(bky, base, typ, st, reason, msg, corev1.EventTypeWarning) return ctrl.Result{}, r.patchStatus(ctx, bky, base) } @@ -614,19 +575,6 @@ func decideProvenance(insp registry.Inspection, policy bucketyv1.AdoptionPolicy) } } -// resolveName runs the name template against the Buckety + backend. -func resolveName(bky *bucketyv1.Buckety, backend config.Backend) (string, error) { - if bky.Spec.Name == "" { - return bky.Name, nil - } - return template.Resolve(bky.Spec.Name, template.Inputs{ - Name: bky.Name, - Namespace: bky.Namespace, - Labels: bky.Labels, - BackendDefaults: backend.Defaults, - }) -} - // majorOf parses the major number out of a SemVer string. // Returns 0 and the parse error if the input is malformed. func majorOf(v string) (int, error) { @@ -638,47 +586,12 @@ func majorOf(v string) (int, error) { } // patchStatus writes status only when something OTHER than -// condition messages changed since base. Message text is allowed -// to be volatile - provider errors embed per-attempt tokens (a -// GCS 403 mints a fresh troubleshooter errorId on every call) - -// and a message-only patch feeds the controller's own Buckety -// watch: patch -> watch event -> immediate reconcile -> fresh -// provider error -> new message -> patch, at whatever rate the -// provider answers (observed ~18/s), with workqueue backoff never -// engaging because watch events are Adds, not requeues -// (ISSUE_status_message_reconcile_loop.md). Skipping the write -// breaks the cycle structurally, whatever the next provider -// embeds: the message rides along with the next real transition, -// which says the same thing attempt #1 said. Cost: progress-style -// message refreshes (recursive-deletion counts) lag while their -// condition is otherwise unchanged. +// condition messages changed since base +// (status.ChangedBeyondMessages has the rationale; the loop it +// prevents is ISSUE_status_message_reconcile_loop.md). func (r *Reconciler) patchStatus(ctx context.Context, bky, base *bucketyv1.Buckety) error { - if !statusChangedBeyondMessages(&base.Status, &bky.Status) { + if !status.ChangedBeyondMessages(&base.Status, &bky.Status, func(s *bucketyv1.BucketyStatus) *[]metav1.Condition { return &s.Conditions }) { return nil } return r.Status().Patch(ctx, bky, client.MergeFrom(base)) } - -func statusChangedBeyondMessages(base, cur *bucketyv1.BucketyStatus) bool { - b, c := base.DeepCopy(), cur.DeepCopy() - for i := range b.Conditions { - b.Conditions[i].Message = "" - } - for i := range c.Conditions { - c.Conditions[i].Message = "" - } - return !equality.Semantic.DeepEqual(b, c) -} - -// setCond is a wrapper around meta.SetStatusCondition that stamps -// ObservedGeneration so consumers can tell which spec produced -// the condition. -func setCond(conds *[]metav1.Condition, t string, status metav1.ConditionStatus, reason, message string, observed int64) { - meta.SetStatusCondition(conds, metav1.Condition{ - Type: t, - Status: status, - Reason: reason, - Message: message, - ObservedGeneration: observed, - }) -} diff --git a/pkg/controller/buckety/reconciler_test.go b/pkg/controller/buckety/reconciler_test.go index d899c65..a81960d 100644 --- a/pkg/controller/buckety/reconciler_test.go +++ b/pkg/controller/buckety/reconciler_test.go @@ -17,6 +17,7 @@ import ( bucketyv1 "github.com/Yolean/buckety-controller/pkg/api/v1alpha1" "github.com/Yolean/buckety-controller/pkg/config" + "github.com/Yolean/buckety-controller/pkg/controller/status" "github.com/Yolean/buckety-controller/pkg/drivers/registry" ) @@ -79,8 +80,7 @@ func TestDecideProvenance(t *testing.T) { // its finalizer before releasing its own: owner-ref GC would // otherwise remove the access after the Buckety, whose absence // makes the access finalizer skip RevokeAccess - orphaning a live -// key on a Retain-surviving service account (checkit review -// finding 3, sharpened). +// key on a Retain-surviving service account. func TestDeleteWaitsForImplicitAccessRevocation(t *testing.T) { scheme := runtime.NewScheme() if err := clientgoscheme.AddToScheme(scheme); err != nil { @@ -187,7 +187,7 @@ func TestPatchStatusIgnoresMessageOnlyChanges(t *testing.T) { // Attempt #1: condition appears - patched. base := bky.DeepCopy() - setCond(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "InspectFailed", "403 errorId=aaa111", bky.Generation) + status.Set(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "InspectFailed", "403 errorId=aaa111", bky.Generation) if err := r.patchStatus(ctx, bky, base); err != nil { t.Fatal(err) } @@ -204,7 +204,7 @@ func TestPatchStatusIgnoresMessageOnlyChanges(t *testing.T) { // loop driver. Must not write. current := got.DeepCopy() base = got.DeepCopy() - setCond(¤t.Status.Conditions, "Ready", metav1.ConditionFalse, "InspectFailed", "403 errorId=bbb222", current.Generation) + status.Set(¤t.Status.Conditions, "Ready", metav1.ConditionFalse, "InspectFailed", "403 errorId=bbb222", current.Generation) if err := r.patchStatus(ctx, current, base); err != nil { t.Fatal(err) } @@ -222,7 +222,7 @@ func TestPatchStatusIgnoresMessageOnlyChanges(t *testing.T) { // message rides along. current = got.DeepCopy() base = got.DeepCopy() - setCond(¤t.Status.Conditions, "Ready", metav1.ConditionFalse, "EnsureFailed", "403 errorId=ccc333", current.Generation) + status.Set(¤t.Status.Conditions, "Ready", metav1.ConditionFalse, "EnsureFailed", "403 errorId=ccc333", current.Generation) if err := r.patchStatus(ctx, current, base); err != nil { t.Fatal(err) } diff --git a/pkg/controller/bucketyaccess/reconciler.go b/pkg/controller/bucketyaccess/reconciler.go index 5d9fc5c..3863eca 100644 --- a/pkg/controller/bucketyaccess/reconciler.go +++ b/pkg/controller/bucketyaccess/reconciler.go @@ -7,6 +7,7 @@ package bucketyaccess import ( "context" "fmt" + "time" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" @@ -26,15 +27,18 @@ import ( bucketyv1 "github.com/Yolean/buckety-controller/pkg/api/v1alpha1" "github.com/Yolean/buckety-controller/pkg/config" + "github.com/Yolean/buckety-controller/pkg/controller/status" "github.com/Yolean/buckety-controller/pkg/drivers/registry" ) // Reconciler reconciles BucketyAccess resources. type Reconciler struct { client.Client - Scheme *runtime.Scheme - Config *config.Loaded - RequeueAfter func() ctrl.Result + Scheme *runtime.Scheme + Config *config.Loaded + // Recheck is the periodic re-check cadence returned from every + // settled reconcile; zero disables it (tests). + Recheck time.Duration // Recorder emits Events alongside failure conditions so // `kubectl describe` tells the story; nil disables (tests). Recorder record.EventRecorder @@ -58,20 +62,11 @@ func (r *Reconciler) liveReader() client.Reader { return r.Client } -// eventIfTransition emits an Event only when the condition's -// (status, reason) pair differs from the pre-reconcile state, so -// steady-state requeues do not spam the event stream. Reason is -// part of the comparison: Ready staying False while its reason -// moves (e.g. WaitingForBuckety to SecretConflict) is a -// transition users need to see. -func (r *Reconciler) eventIfTransition(obj runtime.Object, baseConds []metav1.Condition, condType string, status metav1.ConditionStatus, condReason, eventType, eventReason, message string) { - if r.Recorder == nil { - return - } - if c := meta.FindStatusCondition(baseConds, condType); c != nil && c.Status == status && c.Reason == condReason { - return - } - r.Recorder.Event(obj, eventType, eventReason, message) +// surface sets a condition and, when that is a transition from +// base, emits an Event under the same reason. +func (r *Reconciler) surface(access, base *bucketyv1.BucketyAccess, typ string, st metav1.ConditionStatus, reason, msg, eventType string) { + status.Event(r.Recorder, access, base.Status.Conditions, typ, st, reason, eventType, reason, msg) + status.Set(&access.Status.Conditions, typ, st, reason, msg, access.Generation) } // SetupWithManager registers this controller with the manager. @@ -151,16 +146,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu // backend renames. base := access.DeepCopy() msg := fmt.Sprintf("cannot revoke principal %q: backend %q is not registered in buckety-controller.yaml; restore it to let this BucketyAccess go", access.Status.Principal, bky.Spec.Backend) - r.eventIfTransition(&access, base.Status.Conditions, "Ready", metav1.ConditionFalse, "BackendUnavailable", + status.Event(r.Recorder, &access, base.Status.Conditions, "Ready", metav1.ConditionFalse, "BackendUnavailable", corev1.EventTypeWarning, "DeletionBlocked", msg) - setCond(&access.Status.Conditions, "Ready", metav1.ConditionFalse, "BackendUnavailable", msg, access.Generation) + status.Set(&access.Status.Conditions, "Ready", metav1.ConditionFalse, "BackendUnavailable", msg, access.Generation) if err := r.patchStatus(ctx, &access, base); err != nil { return ctrl.Result{}, err } - if r.RequeueAfter != nil { - return r.RequeueAfter(), nil - } - return ctrl.Result{}, nil + return ctrl.Result{RequeueAfter: r.Recheck}, nil } if ok { if err := backend.Driver.RevokeAccess(ctx, access.Status.Principal); err != nil { @@ -204,13 +196,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu if bkyErr != nil { if apierrors.IsNotFound(bkyErr) { - r.eventIfTransition(&access, baseAccess.Status.Conditions, "Ready", metav1.ConditionFalse, "BucketyNotFound", - corev1.EventTypeWarning, "BucketyNotFound", - fmt.Sprintf("Buckety %q not found in namespace %q", access.Spec.BucketyRef.Name, access.Namespace)) - setCond(&access.Status.Conditions, "Ready", metav1.ConditionFalse, - "BucketyNotFound", - fmt.Sprintf("Buckety %q not found in namespace %q", access.Spec.BucketyRef.Name, access.Namespace), - access.Generation) + r.surface(&access, baseAccess, "Ready", metav1.ConditionFalse, "BucketyNotFound", + fmt.Sprintf("Buckety %q not found in namespace %q", access.Spec.BucketyRef.Name, access.Namespace), corev1.EventTypeWarning) return ctrl.Result{}, r.patchStatus(ctx, &access, baseAccess) } return ctrl.Result{}, bkyErr @@ -219,26 +206,18 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu // Buckety must be Ready and have its backend resource name // stamped before we can mint a Secret. if bky.Status.BackendResourceName == "" || !isReady(&bky) { - setCond(&access.Status.Conditions, "Ready", metav1.ConditionFalse, + status.Set(&access.Status.Conditions, "Ready", metav1.ConditionFalse, "WaitingForBuckety", "Buckety is not Ready yet; will retry", access.Generation) _ = r.patchStatus(ctx, &access, baseAccess) - if r.RequeueAfter != nil { - return r.RequeueAfter(), nil - } - return ctrl.Result{}, nil + return ctrl.Result{RequeueAfter: r.Recheck}, nil } backend, ok := r.Config.Lookup(bky.Status.Backend) if !ok { - r.eventIfTransition(&access, baseAccess.Status.Conditions, "Ready", metav1.ConditionFalse, "BackendUnavailable", - corev1.EventTypeWarning, "BackendUnavailable", - fmt.Sprintf("backend %q is not registered in buckety-controller.yaml", bky.Status.Backend)) - setCond(&access.Status.Conditions, "Ready", metav1.ConditionFalse, - "BackendUnavailable", - fmt.Sprintf("backend %q is not registered in buckety-controller.yaml", bky.Status.Backend), - access.Generation) + r.surface(&access, baseAccess, "Ready", metav1.ConditionFalse, "BackendUnavailable", + fmt.Sprintf("backend %q is not registered in buckety-controller.yaml", bky.Status.Backend), corev1.EventTypeWarning) return ctrl.Result{}, r.patchStatus(ctx, &access, baseAccess) } @@ -247,9 +226,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu // the Buckety does not exist yet, so this is the authoritative // check for BucketyAccess parameters. if err := backend.Driver.ValidateAccessParameters(access.Spec.Parameters); err != nil { - r.eventIfTransition(&access, baseAccess.Status.Conditions, "Ready", metav1.ConditionFalse, "InvalidParameters", - corev1.EventTypeWarning, "InvalidParameters", err.Error()) - setCond(&access.Status.Conditions, "Ready", metav1.ConditionFalse, "InvalidParameters", err.Error(), access.Generation) + r.surface(&access, baseAccess, "Ready", metav1.ConditionFalse, "InvalidParameters", err.Error(), corev1.EventTypeWarning) return ctrl.Result{}, r.patchStatus(ctx, &access, baseAccess) } @@ -258,9 +235,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu // serviceAccount) without knowing about CRDs. bkyParams, err := backend.ResolvedParameters(bky.Name, bky.Namespace, bky.Spec.Parameters) if err != nil { - r.eventIfTransition(&access, baseAccess.Status.Conditions, "Ready", metav1.ConditionFalse, "GrantFailed", - corev1.EventTypeWarning, "GrantFailed", err.Error()) - setCond(&access.Status.Conditions, "Ready", metav1.ConditionFalse, "GrantFailed", err.Error(), access.Generation) + r.surface(&access, baseAccess, "Ready", metav1.ConditionFalse, "ParameterTemplate", err.Error(), corev1.EventTypeWarning) return ctrl.Result{}, r.patchStatus(ctx, &access, baseAccess) } @@ -283,17 +258,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu switch { case getErr == nil && !metav1.IsControlledBy(&existing, &access): msg := fmt.Sprintf("secret %q exists and is not managed by this BucketyAccess; delete it or pick another credentialsSecretName", access.Spec.CredentialsSecretName) - r.eventIfTransition(&access, baseAccess.Status.Conditions, "Ready", metav1.ConditionFalse, "SecretConflict", corev1.EventTypeWarning, "SecretConflict", msg) - setCond(&access.Status.Conditions, "Ready", metav1.ConditionFalse, "SecretConflict", msg, access.Generation) + r.surface(&access, baseAccess, "Ready", metav1.ConditionFalse, "SecretConflict", msg, corev1.EventTypeWarning) if err := r.patchStatus(ctx, &access, baseAccess); err != nil { return ctrl.Result{}, err } // Periodic requeue notices when the conflicting Secret goes // away (an unowned Secret's deletion maps to no watch). - if r.RequeueAfter != nil { - return r.RequeueAfter(), nil - } - return ctrl.Result{}, nil + return ctrl.Result{RequeueAfter: r.Recheck}, nil case getErr != nil && !apierrors.IsNotFound(getErr): return ctrl.Result{}, getErr } @@ -310,9 +281,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu ExistingSecretData: existingData, }) if err != nil { - r.eventIfTransition(&access, baseAccess.Status.Conditions, "Ready", metav1.ConditionFalse, "GrantFailed", - corev1.EventTypeWarning, "GrantFailed", err.Error()) - setCond(&access.Status.Conditions, "Ready", metav1.ConditionFalse, "GrantFailed", err.Error(), access.Generation) + r.surface(&access, baseAccess, "Ready", metav1.ConditionFalse, "GrantFailed", err.Error(), corev1.EventTypeWarning) _ = r.patchStatus(ctx, &access, baseAccess) return ctrl.Result{}, err } @@ -345,9 +314,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu log.Info("secret write skipped; namespace is terminating") return ctrl.Result{}, nil } - r.eventIfTransition(&access, baseAccess.Status.Conditions, "Ready", metav1.ConditionFalse, "SecretWriteFailed", - corev1.EventTypeWarning, "SecretWriteFailed", err.Error()) - setCond(&access.Status.Conditions, "Ready", metav1.ConditionFalse, "SecretWriteFailed", err.Error(), access.Generation) + r.surface(&access, baseAccess, "Ready", metav1.ConditionFalse, "SecretWriteFailed", err.Error(), corev1.EventTypeWarning) _ = r.patchStatus(ctx, &access, baseAccess) return ctrl.Result{}, err } @@ -357,16 +324,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu // replaced credential now that the Secret carries its // successor. Without this, every re-mint orphans a live key // on the gcs SA until GCP's 10-keys-per-SA cap wedges - // Keys.Create permanently (checkit review finding 1). + // Keys.Create permanently. // Ordering makes it self-healing: status.principal keeps // naming the old key until revocation succeeds, so a failure // retries here while GrantAccess keeps returning the // already-written replacement. if old := access.Status.Principal; old != "" && old != res.Principal { if rerr := backend.Driver.RevokeAccess(ctx, old); rerr != nil { - r.eventIfTransition(&access, baseAccess.Status.Conditions, "Ready", metav1.ConditionFalse, "RevokeFailed", - corev1.EventTypeWarning, "RevokeFailed", rerr.Error()) - setCond(&access.Status.Conditions, "Ready", metav1.ConditionFalse, "RevokeFailed", rerr.Error(), access.Generation) + r.surface(&access, baseAccess, "Ready", metav1.ConditionFalse, "RevokeFailed", rerr.Error(), corev1.EventTypeWarning) _ = r.patchStatus(ctx, &access, baseAccess) return ctrl.Result{}, rerr } @@ -387,7 +352,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu // scoping per role and the user asked for something other // than ReadWrite. if !res.Scoped && access.Spec.Role != "" && access.Spec.Role != bucketyv1.RoleReadWrite { - setCond(&access.Status.Conditions, "ScopingNotImplemented", metav1.ConditionTrue, + status.Set(&access.Status.Conditions, "ScopingNotImplemented", metav1.ConditionTrue, "DriverIgnoresRole", fmt.Sprintf("driver %q v1alpha1 does not scope credentials per role; got root creds despite role=%q", backend.Driver.Name(), access.Spec.Role), access.Generation) @@ -395,18 +360,15 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu meta.RemoveStatusCondition(&access.Status.Conditions, "ScopingNotImplemented") } - r.eventIfTransition(&access, baseAccess.Status.Conditions, "Ready", metav1.ConditionTrue, "SecretMinted", + status.Event(r.Recorder, &access, baseAccess.Status.Conditions, "Ready", metav1.ConditionTrue, "SecretMinted", corev1.EventTypeNormal, "SecretMinted", fmt.Sprintf("secret %q minted for backend resource %q", access.Spec.CredentialsSecretName, bky.Status.BackendResourceName)) - setCond(&access.Status.Conditions, "Ready", metav1.ConditionTrue, "SecretMinted", "", access.Generation) + status.Set(&access.Status.Conditions, "Ready", metav1.ConditionTrue, "SecretMinted", "", access.Generation) access.Status.ObservedGeneration = access.Generation if err := r.patchStatus(ctx, &access, baseAccess); err != nil { return ctrl.Result{}, err } - if r.RequeueAfter != nil { - return r.RequeueAfter(), nil - } - return ctrl.Result{}, nil + return ctrl.Result{RequeueAfter: r.Recheck}, nil } // writeSecret creates or updates the access's credentials Secret @@ -443,45 +405,16 @@ func (r *Reconciler) writeSecret(ctx context.Context, access *bucketyv1.BucketyA } // patchStatus writes status only when something OTHER than -// condition messages changed since base; see the buckety -// reconciler's counterpart for the full rationale -// (ISSUE_status_message_reconcile_loop.md): a volatile provider -// error message would otherwise loop patch -> own-watch event -> -// reconcile -> new message at the provider's answer rate, with -// workqueue backoff bypassed. +// condition messages changed since base +// (status.ChangedBeyondMessages has the rationale; the loop it +// prevents is ISSUE_status_message_reconcile_loop.md). func (r *Reconciler) patchStatus(ctx context.Context, access, base *bucketyv1.BucketyAccess) error { - if !statusChangedBeyondMessages(&base.Status, &access.Status) { + if !status.ChangedBeyondMessages(&base.Status, &access.Status, func(s *bucketyv1.BucketyAccessStatus) *[]metav1.Condition { return &s.Conditions }) { return nil } return r.Status().Patch(ctx, access, client.MergeFrom(base)) } -func statusChangedBeyondMessages(base, cur *bucketyv1.BucketyAccessStatus) bool { - b, c := base.DeepCopy(), cur.DeepCopy() - for i := range b.Conditions { - b.Conditions[i].Message = "" - } - for i := range c.Conditions { - c.Conditions[i].Message = "" - } - return !equality.Semantic.DeepEqual(b, c) -} - func isReady(bky *bucketyv1.Buckety) bool { - for _, c := range bky.Status.Conditions { - if c.Type == "Ready" { - return c.Status == metav1.ConditionTrue - } - } - return false -} - -func setCond(conds *[]metav1.Condition, t string, status metav1.ConditionStatus, reason, message string, observed int64) { - meta.SetStatusCondition(conds, metav1.Condition{ - Type: t, - Status: status, - Reason: reason, - Message: message, - ObservedGeneration: observed, - }) + return meta.IsStatusConditionTrue(bky.Status.Conditions, "Ready") } diff --git a/pkg/controller/bucketyaccess/reconciler_test.go b/pkg/controller/bucketyaccess/reconciler_test.go index 7839bf7..258a164 100644 --- a/pkg/controller/bucketyaccess/reconciler_test.go +++ b/pkg/controller/bucketyaccess/reconciler_test.go @@ -21,64 +21,6 @@ import ( "github.com/Yolean/buckety-controller/pkg/drivers/registry" ) -// The gate must treat a reason change within the same status as a -// transition: Ready staying False while moving WaitingForBuckety -> -// SecretConflict is exactly the moment users need an Event (this -// suppression shipped once and was caught by e2e). -func TestEventIfTransition(t *testing.T) { - obj := &bucketyv1.BucketyAccess{} - base := []metav1.Condition{{ - Type: "Ready", Status: metav1.ConditionFalse, Reason: "WaitingForBuckety", - }} - - drain := func(rec *record.FakeRecorder) []string { - var out []string - for { - select { - case e := <-rec.Events: - out = append(out, e) - default: - return out - } - } - } - - rec := record.NewFakeRecorder(10) - r := &Reconciler{Recorder: rec} - - // Same status, same reason: suppressed. - r.eventIfTransition(obj, base, "Ready", metav1.ConditionFalse, "WaitingForBuckety", - corev1.EventTypeWarning, "WaitingForBuckety", "x") - if got := drain(rec); len(got) != 0 { - t.Fatalf("steady state emitted %v", got) - } - - // Same status, new reason: emitted. - r.eventIfTransition(obj, base, "Ready", metav1.ConditionFalse, "SecretConflict", - corev1.EventTypeWarning, "SecretConflict", "x") - if got := drain(rec); len(got) != 1 { - t.Fatalf("reason change emitted %v", got) - } - - // Status flip: emitted. - r.eventIfTransition(obj, base, "Ready", metav1.ConditionTrue, "SecretMinted", - corev1.EventTypeNormal, "SecretMinted", "x") - if got := drain(rec); len(got) != 1 { - t.Fatalf("status flip emitted %v", got) - } - - // Condition absent from base (first reconcile): emitted. - r.eventIfTransition(obj, nil, "Ready", metav1.ConditionFalse, "GrantFailed", - corev1.EventTypeWarning, "GrantFailed", "x") - if got := drain(rec); len(got) != 1 { - t.Fatalf("first-seen condition emitted %v", got) - } - - // Nil recorder: no panic. - (&Reconciler{}).eventIfTransition(obj, base, "Ready", metav1.ConditionTrue, "SecretMinted", - corev1.EventTypeNormal, "SecretMinted", "x") -} - // The manager cache only carries Secrets labelled LabelOwnedSecret // (issue #10), so writeSecret works from a live read and must (a) // stamp the label on creation, (b) stamp it onto owned Secrets @@ -319,8 +261,8 @@ func reconcilerRequest(ns, name string) reconcile.Request { // A re-mint that changes the principal must revoke the replaced // one AFTER the Secret write, or every lost/hand-edited Secret -// orphans a live key until the SA's 10-key cap wedges Keys.Create -// (checkit review finding 1). status.principal advances only once +// orphans a live key until the SA's 10-key cap wedges Keys.Create. +// status.principal advances only once // revocation succeeds, so failures retry. func TestReplacedPrincipalRevoked(t *testing.T) { scheme := runtime.NewScheme() diff --git a/pkg/controller/status/status.go b/pkg/controller/status/status.go new file mode 100644 index 0000000..8d1fea9 --- /dev/null +++ b/pkg/controller/status/status.go @@ -0,0 +1,77 @@ +// Package status holds the condition and event helpers the two +// reconcilers share: setting a condition with its observed +// generation, emitting an Event only on a transition, and +// deciding whether a status changed in anything but condition +// messages (the check that keeps volatile provider error text +// from driving a patch loop). +package status + +import ( + "k8s.io/apimachinery/pkg/api/equality" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/tools/record" +) + +// Set sets a condition, stamping ObservedGeneration so consumers +// can tell which spec produced it. +func Set(conds *[]metav1.Condition, typ string, st metav1.ConditionStatus, reason, msg string, gen int64) { + meta.SetStatusCondition(conds, metav1.Condition{ + Type: typ, + Status: st, + Reason: reason, + Message: msg, + ObservedGeneration: gen, + }) +} + +// Event emits an Event on rec (nil disables, for tests) unless +// base already carried the condition with the same (status, +// reason) pair, so steady-state requeues do not spam the stream. +// Reason is part of the comparison: Ready staying False while its +// reason moves (WaitingForBuckety to SecretConflict, say) is a +// transition users need to see. +func Event(rec record.EventRecorder, obj runtime.Object, base []metav1.Condition, typ string, st metav1.ConditionStatus, reason, eventType, eventReason, msg string) { + if rec == nil { + return + } + if c := meta.FindStatusCondition(base, typ); c != nil && c.Status == st && c.Reason == reason { + return + } + rec.Event(obj, eventType, eventReason, msg) +} + +// ChangedBeyondMessages reports whether two statuses differ in +// anything except condition messages. conds selects the +// conditions slice of the status struct S, whose other fields +// must be plain values (they are compared through a shallow +// copy). +// +// Message text is allowed to be volatile - provider errors embed +// per-attempt tokens (a GCS 403 mints a fresh troubleshooter +// errorId on every call) - and a message-only patch feeds the +// controller's own watch: patch -> event -> immediate reconcile +// -> fresh provider error -> new message -> patch, at whatever +// rate the provider answers, with workqueue backoff never +// engaging because watch events are Adds, not requeues. Skipping +// the write breaks that cycle structurally, whatever the next +// provider embeds: the message rides along with the next real +// transition, which says the same thing attempt #1 said. Cost: +// progress-style message refreshes (recursive-deletion counts) +// lag while their condition is otherwise unchanged. +func ChangedBeyondMessages[S any](base, cur *S, conds func(*S) *[]metav1.Condition) bool { + b, c := *base, *cur + *conds(&b) = withoutMessages(*conds(&b)) + *conds(&c) = withoutMessages(*conds(&c)) + return !equality.Semantic.DeepEqual(b, c) +} + +func withoutMessages(conds []metav1.Condition) []metav1.Condition { + out := make([]metav1.Condition, len(conds)) + for i, c := range conds { + c.Message = "" + out[i] = c + } + return out +} diff --git a/pkg/controller/status/status_test.go b/pkg/controller/status/status_test.go new file mode 100644 index 0000000..24b0405 --- /dev/null +++ b/pkg/controller/status/status_test.go @@ -0,0 +1,82 @@ +package status + +import ( + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/tools/record" + + bucketyv1 "github.com/Yolean/buckety-controller/pkg/api/v1alpha1" +) + +// The gate must treat a reason change within the same status as a +// transition: Ready staying False while moving WaitingForBuckety -> +// SecretConflict is exactly the moment users need an Event (this +// suppression shipped once and was caught by e2e). +func TestEventOnlyOnTransition(t *testing.T) { + obj := &bucketyv1.BucketyAccess{} + base := []metav1.Condition{{ + Type: "Ready", Status: metav1.ConditionFalse, Reason: "WaitingForBuckety", + }} + rec := record.NewFakeRecorder(10) + drain := func() int { + n := 0 + for { + select { + case <-rec.Events: + n++ + default: + return n + } + } + } + + Event(rec, obj, base, "Ready", metav1.ConditionFalse, "WaitingForBuckety", corev1.EventTypeWarning, "WaitingForBuckety", "x") + if n := drain(); n != 0 { + t.Fatalf("steady state emitted %d events", n) + } + Event(rec, obj, base, "Ready", metav1.ConditionFalse, "SecretConflict", corev1.EventTypeWarning, "SecretConflict", "x") + if n := drain(); n != 1 { + t.Fatalf("reason change emitted %d events", n) + } + Event(rec, obj, base, "Ready", metav1.ConditionTrue, "SecretMinted", corev1.EventTypeNormal, "SecretMinted", "x") + if n := drain(); n != 1 { + t.Fatalf("status flip emitted %d events", n) + } + Event(rec, obj, nil, "Ready", metav1.ConditionFalse, "GrantFailed", corev1.EventTypeWarning, "GrantFailed", "x") + if n := drain(); n != 1 { + t.Fatalf("first-seen condition emitted %d events", n) + } + // Nil recorder: no panic. + Event(nil, obj, base, "Ready", metav1.ConditionTrue, "SecretMinted", corev1.EventTypeNormal, "SecretMinted", "x") +} + +func TestChangedBeyondMessages(t *testing.T) { + conds := func(s *bucketyv1.BucketyAccessStatus) *[]metav1.Condition { return &s.Conditions } + base := &bucketyv1.BucketyAccessStatus{Principal: "p"} + Set(&base.Conditions, "Ready", metav1.ConditionFalse, "GrantFailed", "403 errorId=aaa", 1) + + same := *base + same.Conditions = append([]metav1.Condition(nil), base.Conditions...) + Set(&same.Conditions, "Ready", metav1.ConditionFalse, "GrantFailed", "403 errorId=bbb", 1) + if ChangedBeyondMessages(base, &same, conds) { + t.Error("message-only change reported as a change") + } + if base.Conditions[0].Message != "403 errorId=aaa" { + t.Error("comparison mutated its input") + } + + reason := *base + reason.Conditions = nil + Set(&reason.Conditions, "Ready", metav1.ConditionFalse, "SecretConflict", "403 errorId=aaa", 1) + if !ChangedBeyondMessages(base, &reason, conds) { + t.Error("reason change not reported") + } + + field := *base + field.Principal = "q" + if !ChangedBeyondMessages(base, &field, conds) { + t.Error("field change not reported") + } +} diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go index e426dae..a9ba4ae 100644 --- a/pkg/webhook/webhook.go +++ b/pkg/webhook/webhook.go @@ -20,7 +20,6 @@ import ( bucketyv1 "github.com/Yolean/buckety-controller/pkg/api/v1alpha1" "github.com/Yolean/buckety-controller/pkg/config" - "github.com/Yolean/buckety-controller/pkg/template" ) // Validator is the single handler the controller registers at @@ -78,7 +77,7 @@ func (v *Validator) validateBuckety(_ context.Context, req admission.Request) ad // re-resolution are irrelevant: status.backendResourceName is // sticky and admission must not reject unrelated updates. if bky.Status.BackendResourceName == "" { - resolved, err := resolveName(&bky, backend) + resolved, err := backend.ResolveName(bky.Spec.Name, bky.Name, bky.Namespace, bky.Labels) if err != nil { return admission.Denied(fmt.Sprintf("spec.name: %v", err)) } @@ -149,15 +148,3 @@ func (v *Validator) validateAccess(_ context.Context, req admission.Request) adm // the API server. Role enum is enforced by the CRD schema. return admission.Allowed("") } - -func resolveName(bky *bucketyv1.Buckety, backend config.Backend) (string, error) { - if bky.Spec.Name == "" { - return bky.Name, nil - } - return template.Resolve(bky.Spec.Name, template.Inputs{ - Name: bky.Name, - Namespace: bky.Namespace, - Labels: bky.Labels, - BackendDefaults: backend.Defaults, - }) -} From 2a2b17810afa7cbf5018dc456b6245e23948867d Mon Sep 17 00:00:00 2001 From: Staffan Olsson Date: Fri, 4 Sep 2026 07:40:20 +0200 Subject: [PATCH 11/21] drivers: drop stale version strings and review-thread pointers from messages and comments Two gcs error messages still said "gcs v0.1" on a 0.2 driver. Several comments cited "checkit review finding N" or a CI run number - pointers into conversations a reader of this repository cannot follow; the "why" each comment carried stays, the pointer goes. A dead `_ = v` in the kadm parameter switch is removed. Co-Authored-By: Claude Fable 5.1 --- pkg/drivers/gcs/driver.go | 8 ++++---- pkg/drivers/gcs/driver_test.go | 2 +- pkg/drivers/gcs/serviceaccount.go | 3 +-- pkg/drivers/gcs/serviceaccount_test.go | 2 +- pkg/drivers/kadm/driver.go | 1 - pkg/drivers/s3/driver_test.go | 2 +- 6 files changed, 8 insertions(+), 10 deletions(-) diff --git a/pkg/drivers/gcs/driver.go b/pkg/drivers/gcs/driver.go index b14b63c..fd6eb8c 100644 --- a/pkg/drivers/gcs/driver.go +++ b/pkg/drivers/gcs/driver.go @@ -151,8 +151,8 @@ type ServiceAccountsConfig struct { Endpoint string `json:"endpoint,omitempty"` // Insecure disables authentication on Endpoint, for emulators // and tests ONLY. A separate explicit flag so that an - // endpoint typo cannot silently turn credentials off - // (checkit review finding 4). Requires Endpoint. + // endpoint typo cannot silently turn credentials off. + // Requires Endpoint. Insecure bool `json:"insecure,omitempty"` } @@ -692,7 +692,7 @@ func (d *Driver) ValidateAccessParameters(params map[string]string) error { keys = append(keys, k) } sort.Strings(keys) - return fmt.Errorf("gcs v0.1 accepts no BucketyAccess parameters; got: %s", + return fmt.Errorf("gcs accepts no BucketyAccess parameters; got: %s", strings.Join(keys, ", ")) } @@ -709,7 +709,7 @@ var bucketNameRE = regexp.MustCompile(`^[a-z0-9][a-z0-9._-]*[a-z0-9]$`) // here. func (d *Driver) ValidateResourceName(name string) error { if len(name) < 3 || len(name) > 63 { - return fmt.Errorf("bucket name %q is %d characters; GCS requires 3-63 (longer dotted names need domain verification and are out of scope for gcs v0.1)", name, len(name)) + return fmt.Errorf("bucket name %q is %d characters; GCS requires 3-63 (longer dotted names need domain verification and are out of scope)", name, len(name)) } if !bucketNameRE.MatchString(name) { return fmt.Errorf("bucket name %q must be lowercase alphanumerics, dashes, underscores and dots, starting and ending alphanumeric", name) diff --git a/pkg/drivers/gcs/driver_test.go b/pkg/drivers/gcs/driver_test.go index 7d6abf6..42da236 100644 --- a/pkg/drivers/gcs/driver_test.go +++ b/pkg/drivers/gcs/driver_test.go @@ -301,7 +301,7 @@ func TestUpdateForDrift(t *testing.T) { // knob, drifted or not: a backend with replace-like update // semantics (fake-gcs-server resets fields absent from the // payload) must not un-converge settled values. Seen live as -// portable-blobs-cr flaking on versioning (run 29509508991): the +// portable-blobs-cr flaking on versioning: the // emulator drops UBLA/lifecycle, the resulting perpetual drift // update wiped the already-correct versioning flag. func TestUpdateForDriftCarriesAllManagedKnobs(t *testing.T) { diff --git a/pkg/drivers/gcs/serviceaccount.go b/pkg/drivers/gcs/serviceaccount.go index 46342ad..bb0c713 100644 --- a/pkg/drivers/gcs/serviceaccount.go +++ b/pkg/drivers/gcs/serviceaccount.go @@ -90,8 +90,7 @@ func (d *Driver) ensureServiceAccount(ctx context.Context, shortName, bucket str // soft-deleted SA holds the name. GCP reserves a // deleted SA's ID for ~30 days, so this state does // not converge on retries and the generic create - // error would hide the actual cause (checkit review - // finding 2). + // error would hide the actual cause. return fmt.Errorf("gcs: service account ID %q is reserved by a recently deleted account; GCP holds deleted SA names for ~30 days. Wait out the window, undelete it if its numeric unique ID is known (gcloud iam service-accounts undelete), or use a different parameters.serviceAccount", email) } } diff --git a/pkg/drivers/gcs/serviceaccount_test.go b/pkg/drivers/gcs/serviceaccount_test.go index c8abaf1..d7fa26e 100644 --- a/pkg/drivers/gcs/serviceaccount_test.go +++ b/pkg/drivers/gcs/serviceaccount_test.go @@ -638,7 +638,7 @@ func TestDeleteBucketyRemovesServiceAccount(t *testing.T) { // A soft-deleted SA holds its name for ~30 days: Create conflicts // while Get sees nothing. That state never converges on retries, // so the driver must name the tombstone instead of wrapping the -// misleading create/get error (checkit review finding 2). +// misleading create/get error. func TestEnsureServiceAccountTombstone(t *testing.T) { f, srv := newFakeGCP(t, "id-proj") d := saDriver(t, srv) diff --git a/pkg/drivers/kadm/driver.go b/pkg/drivers/kadm/driver.go index cac1619..c81adf5 100644 --- a/pkg/drivers/kadm/driver.go +++ b/pkg/drivers/kadm/driver.go @@ -183,7 +183,6 @@ func (d *Driver) ValidateParameters(params map[string]string) error { } case strings.HasPrefix(k, "config."): // Kafka topic-config keys; broker validates content. - _ = v default: return fmt.Errorf("unknown parameter %q (kadm v0.1 accepts: partitions, replicationFactor, config.*)", k) } diff --git a/pkg/drivers/s3/driver_test.go b/pkg/drivers/s3/driver_test.go index d491a86..a23906d 100644 --- a/pkg/drivers/s3/driver_test.go +++ b/pkg/drivers/s3/driver_test.go @@ -218,7 +218,7 @@ func TestLifecycleEqual(t *testing.T) { // versitygw answers HTTP 501 with its own error code // (VersioningNotConfigured) rather than NotImplemented; the -// fail-safe keys on the status code (seen live on run 29507386876). +// fail-safe keys on the status code (seen live in e2e). func TestIsNotImplemented(t *testing.T) { versitygw501 := fmt.Errorf("operation error S3: GetBucketVersioning: %w", &awshttp.ResponseError{ResponseError: &smithyhttp.ResponseError{ From 7f0a0ce14db9ba95790b74362f967a14e435843f Mon Sep 17 00:00:00 2001 From: Staffan Olsson Date: Fri, 4 Sep 2026 07:41:22 +0200 Subject: [PATCH 12/21] readiness gates on the webhook listener readyz answered Ping only, so the controller Pod was Ready - and in the webhook Service's endpoints - before its TLS listener was up. With failurePolicy Fail on the ValidatingWebhookConfiguration that window fails every Buckety apply during a rollout, and the e2e harness had grown two workarounds for it: a poll of the Endpoints object before each scenario, and a retry of "failed calling webhook" on apply. The webhook server's StartedChecker is now a readyz check, which is the readiness the Deployment's probe was meant to express. The Endpoints poll goes; the apply retry stays, because the apiserver resolves the Service through its own informer cache and that lag is not something the Pod can report. Co-Authored-By: Claude Fable 5.1 --- cmd/buckety/main.go | 21 +++++++++++++++------ test/e2e/run.sh | 42 ++++++++---------------------------------- 2 files changed, 23 insertions(+), 40 deletions(-) diff --git a/cmd/buckety/main.go b/cmd/buckety/main.go index d408ef8..98f761d 100644 --- a/cmd/buckety/main.go +++ b/cmd/buckety/main.go @@ -157,12 +157,6 @@ func main() { os.Exit(1) } - if enableWebhook { - (&bkhook.Validator{Config: loaded}).Register(mgr) - } else { - setupLog.Info("webhook disabled; per-driver parameter validation will surface on resource status only") - } - if err := mgr.AddHealthzCheck("ping", healthz.Ping); err != nil { setupLog.Error(err, "healthz registration failed") os.Exit(1) @@ -171,6 +165,21 @@ func main() { setupLog.Error(err, "readyz registration failed") os.Exit(1) } + if enableWebhook { + (&bkhook.Validator{Config: loaded}).Register(mgr) + // Ready only once the webhook listener answers. The + // ValidatingWebhookConfiguration has failurePolicy Fail, so + // a Pod that is Ready - and therefore in the Service's + // endpoints - before its TLS listener is up makes every + // Buckety apply during a rollout fail with "failed calling + // webhook". + if err := mgr.AddReadyzCheck("webhook", mgr.GetWebhookServer().StartedChecker()); err != nil { + setupLog.Error(err, "webhook readyz registration failed") + os.Exit(1) + } + } else { + setupLog.Info("webhook disabled; per-driver parameter validation will surface on resource status only") + } setupLog.Info("starting", "version", version, "drivers", registry.Versions()) if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil { diff --git a/test/e2e/run.sh b/test/e2e/run.sh index e4b2455..80ca956 100755 --- a/test/e2e/run.sh +++ b/test/e2e/run.sh @@ -166,31 +166,6 @@ scenario_matches_impl() { esac } -# The apiserver resolves the validating webhook through the -# Service's Endpoints. Scenarios that rescale or restart the -# controller (scaled-to-zero, misconfigured-startup, -# backend-stickiness) end with `rollout status`, which proves pod -# readiness but not Endpoints propagation - a short lag window in -# which the NEXT scenario's apply fails with "no endpoints -# available for service buckety-controller-webhook" (seen on run -# 29493728100: kadm/secret-conflict + webhook-fallback-validation -# failed at apply seconds after scaled-to-zero restored the -# controller). Wait for an address before each scenario's apply. -wait_webhook_endpoints() { - local deadline=$(( $(date +%s) + 60 )) - while (( $(date +%s) < deadline )); do - local ip - ip="$(kubectl -n "$CONTROLLER_NS" get endpoints buckety-controller-webhook -o jsonpath='{.subsets[0].addresses[0].ip}' 2>/dev/null || true)" - [[ -n "$ip" ]] && return 0 - sleep 2 - done - # Soft warning: an apply that needs the webhook will fail loudly - # anyway, and scenarios that do not touch the webhook should not - # be blocked here. - log "webhook endpoints not ready after 60s; proceeding" - return 0 -} - # run_scenario is invoked in an `if` condition, which makes bash # ignore `set -e` for everything inside the function body. Every # step therefore propagates failure explicitly with `|| return 1`; @@ -204,7 +179,6 @@ run_scenario() { ns="$(echo "$ns" | tr 'A-Z_' 'a-z-' | head -c 60 | sed 's/-$//')" log "=== scenario: $scenario -> $ns (impl=$impl) ===" - wait_webhook_endpoints kubectl create namespace "$ns" || return 1 # Render first: config-only scenarios (misconfigured-startup) @@ -216,14 +190,14 @@ run_scenario() { return 1 } if [[ -n "${rendered//[$'\n\r\t ']/}" ]]; then - # wait_webhook_endpoints above reads Endpoints fresh, but the - # apiserver resolves the webhook Service through an informer - # cache that can lag a beat after the controller pod churns - - # seen as "no endpoints available" on the first apply after - # scaled-to-zero (run 29602598294) despite the wait passing. - # The error is transient and apply is idempotent: retry it. - # Admission DENIALS say "denied the request", not "failed - # calling webhook", so real rejections still fail fast. + # The controller Pod is Ready only once its webhook listener + # answers, but the apiserver resolves the webhook Service + # through an informer cache that can lag a beat after the + # controller Pod churns - seen as "no endpoints available" on + # the first apply after scaled-to-zero. The error is transient + # and apply is idempotent: retry it. Admission DENIALS say + # "denied the request", not "failed calling webhook", so real + # rejections still fail fast. local applied=0 apply_out="" for _ in $(seq 1 15); do if apply_out="$(printf '%s\n' "$rendered" | kubectl apply -n "$ns" -f - 2>&1)"; then From c08a1cd6773436e5aaf29c6ec3d108392c7658d2 Mon Sep 17 00:00:00 2001 From: Staffan Olsson Date: Fri, 4 Sep 2026 07:41:22 +0200 Subject: [PATCH 13/21] deploy: the controller kustomization pins its namespace; bucketies lose create/delete The ClusterRoleBinding subject, the webhook clientConfig and the certgen Jobs all hard-code the buckety namespace, but deploy/kustomize/controller did not set `namespace:`, so applying base without the README's overlay put the Deployment and ServiceAccount in `default` while RBAC and admission pointed at `buckety` - a silent failure. The namespace is part of the contract; it is now set where the resources are. The controller never creates or deletes a Buckety (only the implicit BucketyAccess), so those verbs come off bucketies. Co-Authored-By: Claude Fable 5.1 --- deploy/kustomize/controller/clusterrole.yaml | 8 +++++++- deploy/kustomize/controller/kustomization.yaml | 7 +++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/deploy/kustomize/controller/clusterrole.yaml b/deploy/kustomize/controller/clusterrole.yaml index d91c1b2..b5a56b1 100644 --- a/deploy/kustomize/controller/clusterrole.yaml +++ b/deploy/kustomize/controller/clusterrole.yaml @@ -3,8 +3,14 @@ kind: ClusterRole metadata: name: buckety-controller rules: +# Bucketys are only ever read and patched (finalizer, status); +# the controller creates and deletes BucketyAccess (the implicit +# access) but never a Buckety. - apiGroups: [buckety.yolean.se] - resources: [bucketies, bucketyaccesses] + resources: [bucketies] + verbs: [get, list, watch, update, patch] +- apiGroups: [buckety.yolean.se] + resources: [bucketyaccesses] verbs: [get, list, watch, create, update, patch, delete] - apiGroups: [buckety.yolean.se] resources: [bucketies/status, bucketyaccesses/status] diff --git a/deploy/kustomize/controller/kustomization.yaml b/deploy/kustomize/controller/kustomization.yaml index 8190014..fba47b8 100644 --- a/deploy/kustomize/controller/kustomization.yaml +++ b/deploy/kustomize/controller/kustomization.yaml @@ -3,6 +3,13 @@ kind: Kustomization # Controller-side resources: RBAC, Service, Deployment, webhook # configuration. Apply against a cluster where ../crd/ is # already installed. ../base/ composes both. +# +# The namespace is part of the contract, not a default: the +# ClusterRoleBinding subject, the webhook clientConfig and the +# certgen Jobs all name `buckety`, and cannot follow a namespace +# transformer applied by an overlay only partially. Set here so +# every composition of this directory is self-consistent. +namespace: buckety resources: - serviceaccount.yaml - clusterrole.yaml From d68649048b842a00c7a016bcd97204f40a1c2300 Mon Sep 17 00:00:00 2001 From: Staffan Olsson Date: Fri, 4 Sep 2026 08:16:50 +0200 Subject: [PATCH 14/21] e2e: one controller deploy from the shipped release plus certgen; configs per implementation The harness kept four near-identical kustomize overlays (17 files: four copies of the same kustomization and args patch, three of the env patch, four hand-maintained "renamed" configs) and applied one per implementation. Each apply reset the image to the base's placeholder tag before `set image` rolled it again, leaving an ImagePullBackOff pod behind every time. It also minted its own webhook certificate with openssl and patched the caBundle by hand, so the certgen Jobs the release actually ships were never exercised by CI. Now there is one overlay - the release base, the shipped webhook-certgen, the e2e args and every backing service's credentials - applied once with the image set through a kustomize images transformer, and a config file per implementation. Switching implementation is what a platform does on a config change: swap the Secret, restart the controller. The renamed config the backend-stickiness scenario needs is derived from the same file with sed instead of being a fifth copy per implementation. 17 files become 6, and e2e deploys the way the README tells users to deploy. Co-Authored-By: Claude Fable 5.1 --- .gitignore | 1 + SPEC.md | 2 +- .../fakegcs.yaml} | 0 .../minio.yaml} | 0 .../redpanda.yaml} | 0 .../versitygw.yaml} | 0 test/e2e/overlay/deployment.yaml | 45 ++++++ test/e2e/overlay/kustomization.yaml | 21 +++ .../fakegcs/buckety-controller.renamed.yaml | 17 --- .../e2e/overlays/fakegcs/deployment-args.yaml | 16 -- test/e2e/overlays/fakegcs/deployment-env.yaml | 15 -- test/e2e/overlays/fakegcs/kustomization.yaml | 20 --- .../minio/buckety-controller.renamed.yaml | 27 ---- test/e2e/overlays/minio/deployment-args.yaml | 16 -- test/e2e/overlays/minio/deployment-env.yaml | 20 --- test/e2e/overlays/minio/kustomization.yaml | 20 --- .../redpanda/buckety-controller.renamed.yaml | 15 -- .../overlays/redpanda/deployment-args.yaml | 16 -- test/e2e/overlays/redpanda/kustomization.yaml | 16 -- .../versitygw/buckety-controller.renamed.yaml | 27 ---- .../overlays/versitygw/deployment-args.yaml | 16 -- .../overlays/versitygw/deployment-env.yaml | 20 --- .../e2e/overlays/versitygw/kustomization.yaml | 20 --- test/e2e/run.sh | 143 ++++++++++-------- 24 files changed, 150 insertions(+), 343 deletions(-) rename test/e2e/{overlays/fakegcs/buckety-controller.yaml => configs/fakegcs.yaml} (100%) rename test/e2e/{overlays/minio/buckety-controller.yaml => configs/minio.yaml} (100%) rename test/e2e/{overlays/redpanda/buckety-controller.yaml => configs/redpanda.yaml} (100%) rename test/e2e/{overlays/versitygw/buckety-controller.yaml => configs/versitygw.yaml} (100%) create mode 100644 test/e2e/overlay/deployment.yaml create mode 100644 test/e2e/overlay/kustomization.yaml delete mode 100644 test/e2e/overlays/fakegcs/buckety-controller.renamed.yaml delete mode 100644 test/e2e/overlays/fakegcs/deployment-args.yaml delete mode 100644 test/e2e/overlays/fakegcs/deployment-env.yaml delete mode 100644 test/e2e/overlays/fakegcs/kustomization.yaml delete mode 100644 test/e2e/overlays/minio/buckety-controller.renamed.yaml delete mode 100644 test/e2e/overlays/minio/deployment-args.yaml delete mode 100644 test/e2e/overlays/minio/deployment-env.yaml delete mode 100644 test/e2e/overlays/minio/kustomization.yaml delete mode 100644 test/e2e/overlays/redpanda/buckety-controller.renamed.yaml delete mode 100644 test/e2e/overlays/redpanda/deployment-args.yaml delete mode 100644 test/e2e/overlays/redpanda/kustomization.yaml delete mode 100644 test/e2e/overlays/versitygw/buckety-controller.renamed.yaml delete mode 100644 test/e2e/overlays/versitygw/deployment-args.yaml delete mode 100644 test/e2e/overlays/versitygw/deployment-env.yaml delete mode 100644 test/e2e/overlays/versitygw/kustomization.yaml diff --git a/.gitignore b/.gitignore index d32e137..c59157b 100644 --- a/.gitignore +++ b/.gitignore @@ -8,6 +8,7 @@ # Test artefacts *.test *.out +test/e2e/.work/ # Go workspace go.work diff --git a/SPEC.md b/SPEC.md index f9c4f29..100cce6 100644 --- a/SPEC.md +++ b/SPEC.md @@ -1142,7 +1142,7 @@ Required CI matrix for v1alpha1: Adding an implementation later (e.g. AWS S3 once the project has credentials and a budget) requires no example or harness changes, -only a new controller-config overlay under `test/e2e/overlays/` +only a new controller-config overlay under `test/e2e/configs/` and the corresponding GHA secret. ## Build and distribution diff --git a/test/e2e/overlays/fakegcs/buckety-controller.yaml b/test/e2e/configs/fakegcs.yaml similarity index 100% rename from test/e2e/overlays/fakegcs/buckety-controller.yaml rename to test/e2e/configs/fakegcs.yaml diff --git a/test/e2e/overlays/minio/buckety-controller.yaml b/test/e2e/configs/minio.yaml similarity index 100% rename from test/e2e/overlays/minio/buckety-controller.yaml rename to test/e2e/configs/minio.yaml diff --git a/test/e2e/overlays/redpanda/buckety-controller.yaml b/test/e2e/configs/redpanda.yaml similarity index 100% rename from test/e2e/overlays/redpanda/buckety-controller.yaml rename to test/e2e/configs/redpanda.yaml diff --git a/test/e2e/overlays/versitygw/buckety-controller.yaml b/test/e2e/configs/versitygw.yaml similarity index 100% rename from test/e2e/overlays/versitygw/buckety-controller.yaml rename to test/e2e/configs/versitygw.yaml diff --git a/test/e2e/overlay/deployment.yaml b/test/e2e/overlay/deployment.yaml new file mode 100644 index 0000000..4ad1f5e --- /dev/null +++ b/test/e2e/overlay/deployment.yaml @@ -0,0 +1,45 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: buckety-controller +spec: + template: + spec: + containers: + - name: controller + # Fast drift re-check for e2e: the oob-drift scenarios wait + # 90-300s for the periodic reconcile to repair out-of-band + # changes, which the production default of 5m never meets. + args: + - "-c" + - "/etc/buckety" + - "--periodic-recheck=15s" + # Credentials for every backing service at once; a config + # that does not reference a variable leaves it unused. The + # Secrets live in the controller namespace, created by + # backing-services/. STORAGE_EMULATOR_HOST routes the GCS + # client library to the emulator and skips ADC; it only + # matters when a gcs backend is configured. + env: + - name: MINIO_ROOT_USER + valueFrom: + secretKeyRef: + name: minio-server + key: root-user + - name: MINIO_ROOT_PASSWORD + valueFrom: + secretKeyRef: + name: minio-server + key: root-password + - name: VERSITYGW_ROOT_ACCESSKEY + valueFrom: + secretKeyRef: + name: versitygw-server + key: root-accesskey + - name: VERSITYGW_ROOT_SECRETKEY + valueFrom: + secretKeyRef: + name: versitygw-server + key: root-secretkey + - name: STORAGE_EMULATOR_HOST + value: fake-gcs.blobs.svc.cluster.local:8000 diff --git a/test/e2e/overlay/kustomization.yaml b/test/e2e/overlay/kustomization.yaml new file mode 100644 index 0000000..3a67e80 --- /dev/null +++ b/test/e2e/overlay/kustomization.yaml @@ -0,0 +1,21 @@ +apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +# The one controller deployment the e2e harness applies, for every +# implementation. What differs per implementation is only the +# controller config (configs/.yaml), which run.sh applies as +# the buckety-controller-config Secret and restarts the controller +# on - the same operation a platform does on a config change. +# +# webhook-certgen is the shipped no-cert-manager path +# (deploy/kustomize/release-tls-selfsigned composes the same two +# directories), so e2e exercises what users deploy instead of a +# harness-local certificate. +namespace: buckety +resources: +- ../../../deploy/kustomize/base +- ../../../deploy/kustomize/webhook-certgen +patches: +- path: deployment.yaml + target: + kind: Deployment + name: buckety-controller diff --git a/test/e2e/overlays/fakegcs/buckety-controller.renamed.yaml b/test/e2e/overlays/fakegcs/buckety-controller.renamed.yaml deleted file mode 100644 index 9667b75..0000000 --- a/test/e2e/overlays/fakegcs/buckety-controller.renamed.yaml +++ /dev/null @@ -1,17 +0,0 @@ -# yaml-language-server: $schema=https://raw.githubusercontent.com/Yolean/buckety-controller/main/pkg/config/schema/buckety-controller.schema.json -# Same connection details as buckety-controller.yaml but with the -# backend name changed from 'gcs' to 'gcs-renamed'. Used by the -# gcs/backend-stickiness scenario to prove status.backend -# stickiness survives a config rename. -backends: - -- name: gcs-renamed - driver: gcs - config: - project: e2e-project - endpoint: fake-gcs.blobs.svc.cluster.local:8000 - region: e2e-region-1 - accessKeyID: e2e-fake-hmac-id - secretAccessKey: e2e-fake-hmac-secret - defaults: - zone: e2e diff --git a/test/e2e/overlays/fakegcs/deployment-args.yaml b/test/e2e/overlays/fakegcs/deployment-args.yaml deleted file mode 100644 index 047466f..0000000 --- a/test/e2e/overlays/fakegcs/deployment-args.yaml +++ /dev/null @@ -1,16 +0,0 @@ -apiVersion: apps/v1 -kind: Deployment -metadata: - name: buckety-controller -spec: - template: - spec: - containers: - - name: controller - # Fast drift re-check for e2e: the oob-drift scenarios wait - # 90-300s for the periodic reconcile to repair out-of-band - # changes, which the production default of 5m never meets. - args: - - "-c" - - "/etc/buckety" - - "--periodic-recheck=15s" diff --git a/test/e2e/overlays/fakegcs/deployment-env.yaml b/test/e2e/overlays/fakegcs/deployment-env.yaml deleted file mode 100644 index 7769f4f..0000000 --- a/test/e2e/overlays/fakegcs/deployment-env.yaml +++ /dev/null @@ -1,15 +0,0 @@ -apiVersion: apps/v1 -kind: Deployment -metadata: - name: buckety-controller -spec: - template: - spec: - containers: - - name: controller - env: - # The GCS client library routes to the emulator and skips - # ADC when this is set; no GOOGLE_APPLICATION_CREDENTIALS - # needed in e2e. - - name: STORAGE_EMULATOR_HOST - value: fake-gcs.blobs.svc.cluster.local:8000 diff --git a/test/e2e/overlays/fakegcs/kustomization.yaml b/test/e2e/overlays/fakegcs/kustomization.yaml deleted file mode 100644 index dcb5f71..0000000 --- a/test/e2e/overlays/fakegcs/kustomization.yaml +++ /dev/null @@ -1,20 +0,0 @@ -apiVersion: kustomize.config.k8s.io/v1beta1 -kind: Kustomization -namespace: buckety -resources: -- ../../../../deploy/kustomize/base -secretGenerator: -- name: buckety-controller-config - files: - - buckety-controller.yaml -generatorOptions: - disableNameSuffixHash: true -patches: -- path: deployment-env.yaml - target: - kind: Deployment - name: buckety-controller -- path: deployment-args.yaml - target: - kind: Deployment - name: buckety-controller diff --git a/test/e2e/overlays/minio/buckety-controller.renamed.yaml b/test/e2e/overlays/minio/buckety-controller.renamed.yaml deleted file mode 100644 index 04bf043..0000000 --- a/test/e2e/overlays/minio/buckety-controller.renamed.yaml +++ /dev/null @@ -1,27 +0,0 @@ -# yaml-language-server: $schema=https://raw.githubusercontent.com/Yolean/buckety-controller/main/pkg/config/schema/buckety-controller.schema.json -# Same connection details as buckety-controller.yaml but with the -# backend name changed from 's3' to 's3-renamed'. Used by the -# s3/backend-stickiness scenario to prove status.backend stickiness -# survives a config rename. -backends: - -- name: s3-renamed - driver: s3 - config: - implementation: minio - endpoint: http://minio.blobs.svc.cluster.local:9000 - region: us-east-1 - forcePathStyle: true - accessKeyID: ${MINIO_ROOT_USER} - secretAccessKey: ${MINIO_ROOT_PASSWORD} - defaults: - zone: e2e - -- name: r2-fake - driver: s3 - config: - implementation: r2 - endpoint: https://example.invalid - region: auto - accessKeyID: not-used - secretAccessKey: not-used diff --git a/test/e2e/overlays/minio/deployment-args.yaml b/test/e2e/overlays/minio/deployment-args.yaml deleted file mode 100644 index 047466f..0000000 --- a/test/e2e/overlays/minio/deployment-args.yaml +++ /dev/null @@ -1,16 +0,0 @@ -apiVersion: apps/v1 -kind: Deployment -metadata: - name: buckety-controller -spec: - template: - spec: - containers: - - name: controller - # Fast drift re-check for e2e: the oob-drift scenarios wait - # 90-300s for the periodic reconcile to repair out-of-band - # changes, which the production default of 5m never meets. - args: - - "-c" - - "/etc/buckety" - - "--periodic-recheck=15s" diff --git a/test/e2e/overlays/minio/deployment-env.yaml b/test/e2e/overlays/minio/deployment-env.yaml deleted file mode 100644 index 224f8a1..0000000 --- a/test/e2e/overlays/minio/deployment-env.yaml +++ /dev/null @@ -1,20 +0,0 @@ -apiVersion: apps/v1 -kind: Deployment -metadata: - name: buckety-controller -spec: - template: - spec: - containers: - - name: controller - env: - - name: MINIO_ROOT_USER - valueFrom: - secretKeyRef: - name: minio-server - key: root-user - - name: MINIO_ROOT_PASSWORD - valueFrom: - secretKeyRef: - name: minio-server - key: root-password diff --git a/test/e2e/overlays/minio/kustomization.yaml b/test/e2e/overlays/minio/kustomization.yaml deleted file mode 100644 index dcb5f71..0000000 --- a/test/e2e/overlays/minio/kustomization.yaml +++ /dev/null @@ -1,20 +0,0 @@ -apiVersion: kustomize.config.k8s.io/v1beta1 -kind: Kustomization -namespace: buckety -resources: -- ../../../../deploy/kustomize/base -secretGenerator: -- name: buckety-controller-config - files: - - buckety-controller.yaml -generatorOptions: - disableNameSuffixHash: true -patches: -- path: deployment-env.yaml - target: - kind: Deployment - name: buckety-controller -- path: deployment-args.yaml - target: - kind: Deployment - name: buckety-controller diff --git a/test/e2e/overlays/redpanda/buckety-controller.renamed.yaml b/test/e2e/overlays/redpanda/buckety-controller.renamed.yaml deleted file mode 100644 index 032f738..0000000 --- a/test/e2e/overlays/redpanda/buckety-controller.renamed.yaml +++ /dev/null @@ -1,15 +0,0 @@ -# yaml-language-server: $schema=https://raw.githubusercontent.com/Yolean/buckety-controller/main/pkg/config/schema/buckety-controller.schema.json -# Same connection details as buckety-controller.yaml but with the -# backend name changed from 'kafka' to 'kafka-renamed'. Used by the -# kadm/backend-stickiness scenario to prove status.backend stickiness -# survives a config rename. -backends: - -- name: kafka-renamed - driver: kadm - config: - seedBrokers: - - redpanda.redpanda.svc.cluster.local:9093 - clientID: buckety-controller-e2e - defaults: - zone: e2e diff --git a/test/e2e/overlays/redpanda/deployment-args.yaml b/test/e2e/overlays/redpanda/deployment-args.yaml deleted file mode 100644 index 047466f..0000000 --- a/test/e2e/overlays/redpanda/deployment-args.yaml +++ /dev/null @@ -1,16 +0,0 @@ -apiVersion: apps/v1 -kind: Deployment -metadata: - name: buckety-controller -spec: - template: - spec: - containers: - - name: controller - # Fast drift re-check for e2e: the oob-drift scenarios wait - # 90-300s for the periodic reconcile to repair out-of-band - # changes, which the production default of 5m never meets. - args: - - "-c" - - "/etc/buckety" - - "--periodic-recheck=15s" diff --git a/test/e2e/overlays/redpanda/kustomization.yaml b/test/e2e/overlays/redpanda/kustomization.yaml deleted file mode 100644 index 58b20db..0000000 --- a/test/e2e/overlays/redpanda/kustomization.yaml +++ /dev/null @@ -1,16 +0,0 @@ -apiVersion: kustomize.config.k8s.io/v1beta1 -kind: Kustomization -namespace: buckety -resources: -- ../../../../deploy/kustomize/base -secretGenerator: -- name: buckety-controller-config - files: - - buckety-controller.yaml -generatorOptions: - disableNameSuffixHash: true -patches: -- path: deployment-args.yaml - target: - kind: Deployment - name: buckety-controller diff --git a/test/e2e/overlays/versitygw/buckety-controller.renamed.yaml b/test/e2e/overlays/versitygw/buckety-controller.renamed.yaml deleted file mode 100644 index 080d2e4..0000000 --- a/test/e2e/overlays/versitygw/buckety-controller.renamed.yaml +++ /dev/null @@ -1,27 +0,0 @@ -# yaml-language-server: $schema=https://raw.githubusercontent.com/Yolean/buckety-controller/main/pkg/config/schema/buckety-controller.schema.json -# Same connection details as buckety-controller.yaml but with the -# backend name changed from 's3' to 's3-renamed'. Used by the -# s3/backend-stickiness scenario to prove status.backend stickiness -# survives a config rename. -backends: - -- name: s3-renamed - driver: s3 - config: - implementation: versitygw - endpoint: http://versitygw.blobs.svc.cluster.local:9000 - region: us-east-1 - forcePathStyle: true - accessKeyID: ${VERSITYGW_ROOT_ACCESSKEY} - secretAccessKey: ${VERSITYGW_ROOT_SECRETKEY} - defaults: - zone: e2e - -- name: r2-fake - driver: s3 - config: - implementation: r2 - endpoint: https://example.invalid - region: auto - accessKeyID: not-used - secretAccessKey: not-used diff --git a/test/e2e/overlays/versitygw/deployment-args.yaml b/test/e2e/overlays/versitygw/deployment-args.yaml deleted file mode 100644 index 047466f..0000000 --- a/test/e2e/overlays/versitygw/deployment-args.yaml +++ /dev/null @@ -1,16 +0,0 @@ -apiVersion: apps/v1 -kind: Deployment -metadata: - name: buckety-controller -spec: - template: - spec: - containers: - - name: controller - # Fast drift re-check for e2e: the oob-drift scenarios wait - # 90-300s for the periodic reconcile to repair out-of-band - # changes, which the production default of 5m never meets. - args: - - "-c" - - "/etc/buckety" - - "--periodic-recheck=15s" diff --git a/test/e2e/overlays/versitygw/deployment-env.yaml b/test/e2e/overlays/versitygw/deployment-env.yaml deleted file mode 100644 index 66b8eaf..0000000 --- a/test/e2e/overlays/versitygw/deployment-env.yaml +++ /dev/null @@ -1,20 +0,0 @@ -apiVersion: apps/v1 -kind: Deployment -metadata: - name: buckety-controller -spec: - template: - spec: - containers: - - name: controller - env: - - name: VERSITYGW_ROOT_ACCESSKEY - valueFrom: - secretKeyRef: - name: versitygw-server - key: root-accesskey - - name: VERSITYGW_ROOT_SECRETKEY - valueFrom: - secretKeyRef: - name: versitygw-server - key: root-secretkey diff --git a/test/e2e/overlays/versitygw/kustomization.yaml b/test/e2e/overlays/versitygw/kustomization.yaml deleted file mode 100644 index dcb5f71..0000000 --- a/test/e2e/overlays/versitygw/kustomization.yaml +++ /dev/null @@ -1,20 +0,0 @@ -apiVersion: kustomize.config.k8s.io/v1beta1 -kind: Kustomization -namespace: buckety -resources: -- ../../../../deploy/kustomize/base -secretGenerator: -- name: buckety-controller-config - files: - - buckety-controller.yaml -generatorOptions: - disableNameSuffixHash: true -patches: -- path: deployment-env.yaml - target: - kind: Deployment - name: buckety-controller -- path: deployment-args.yaml - target: - kind: Deployment - name: buckety-controller diff --git a/test/e2e/run.sh b/test/e2e/run.sh index 80ca956..05d0c66 100755 --- a/test/e2e/run.sh +++ b/test/e2e/run.sh @@ -35,15 +35,12 @@ # KUBECONFIG Cluster the harness writes to. # CONTROLLER_NS Namespace the buckety-controller runs in. # Default: buckety. -# OVERLAYS_DIR Where per-implementation overlays live. -# Default: /test/e2e/overlays. # -# Each per-implementation overlay is a kustomize bundle that: -# - applies deploy/kustomize/base -# - patches the controller image (overlay or set-image at runtime) -# - provides a buckety-controller-config Secret with the -# backends this implementation should expose -# - applies any implementation-specific bootstrap manifests +# The controller is deployed once from overlay/ (the release base +# plus webhook-certgen, args and backing-service credentials). +# Each implementation then only swaps the controller config +# (configs/.yaml) and restarts the controller - the same +# operation a platform performs on a config change. # # Scenario discovery: every directory under examples//... # that contains both kustomization.yaml AND assert.sh is a @@ -56,7 +53,6 @@ set -euo pipefail here() { cd "$(dirname "${BASH_SOURCE[0]}")" && pwd; } HERE="$(here)" REPO="$(cd "$HERE/../.." && pwd)" -OVERLAYS_DIR="${OVERLAYS_DIR:-$HERE/overlays}" CONTROLLER_NS="${CONTROLLER_NS:-buckety}" KEEP_FAILED="${KEEP_FAILED:-false}" IMPLEMENTATIONS="${IMPLEMENTATIONS:-redpanda,versitygw,minio,fakegcs}" @@ -69,6 +65,16 @@ declare -A IMPL_DRIVER=( [minio]=s3 [fakegcs]=gcs ) +# Map implementation -> the backend name its config declares for +# the driver-agnostic scenarios. backend-stickiness renames it to +# prove status.backend stickiness; the renamed config is derived +# from configs/.yaml rather than kept as a second copy. +declare -A IMPL_BACKEND=( + [redpanda]=kafka + [versitygw]=s3 + [minio]=s3 + [fakegcs]=gcs +) log() { printf '[run.sh] %s\n' "$*" >&2; } fail() { printf '[run.sh][ERR] %s\n' "$*" >&2; exit 1; } @@ -89,58 +95,67 @@ sideload_image() { # ---- per-implementation lifecycle ---------------------------- -apply_overlay() { - local impl="$1" - local overlay="$OVERLAYS_DIR/$impl" - [[ -d "$overlay" ]] || fail "overlay missing for implementation '$impl': $overlay" - log "applying overlay $overlay" - kubectl apply -k "$overlay" - ensure_webhook_tls - # Overlays ship the baked-in default image; CI and local k3d both push - # to a per-run registry, so patch the deployment to CONTROLLER_IMAGE - # before rollout instead of carrying a stale tag. - kubectl -n "$CONTROLLER_NS" set image deploy/buckety-controller \ - controller="$CONTROLLER_IMAGE" - kubectl -n "$CONTROLLER_NS" rollout status deploy/buckety-controller --timeout=180s -} +WORK="$(mktemp -d)" +trap 'rm -rf "$WORK" "$HERE/.work"' EXIT -# The base overlay ships a ValidatingWebhookConfiguration that -# expects cert-manager to populate caBundle. In e2e there is no -# cert-manager, so generate a one-shot self-signed cert, mount it -# into the controller, and patch the VWC's caBundle to trust it. -# parameter-mutation and similar scenarios actively probe the -# webhook so disabling it via --enable-webhook=false would mask -# real regressions. -WEBHOOK_TLS_DIR="" -ensure_webhook_tls() { - if [[ -z "$WEBHOOK_TLS_DIR" ]]; then - command -v openssl >/dev/null || fail "openssl required for self-signed webhook TLS" - log "generating self-signed webhook TLS" - WEBHOOK_TLS_DIR="$(mktemp -d)" - openssl req -x509 -newkey rsa:2048 -days 365 -nodes \ - -keyout "$WEBHOOK_TLS_DIR/tls.key" -out "$WEBHOOK_TLS_DIR/tls.crt" \ - -subj "/CN=buckety-controller-webhook.${CONTROLLER_NS}.svc" \ - -addext "subjectAltName=DNS:buckety-controller-webhook.${CONTROLLER_NS}.svc,DNS:buckety-controller-webhook.${CONTROLLER_NS}.svc.cluster.local" \ - >/dev/null 2>&1 - fi - kubectl -n "$CONTROLLER_NS" create secret tls buckety-controller-webhook-tls \ - --cert="$WEBHOOK_TLS_DIR/tls.crt" --key="$WEBHOOK_TLS_DIR/tls.key" \ +# apply_config installs a config file as the controller's config +# Secret - the operation a platform performs on a config change, +# and the one the config-swapping scenarios perform too. +apply_config() { + kubectl -n "$CONTROLLER_NS" create secret generic buckety-controller-config \ + --from-file=buckety-controller.yaml="$1" \ --dry-run=client -o yaml | kubectl apply -f - - local ca_bundle - ca_bundle="$(base64 -w0 <"$WEBHOOK_TLS_DIR/tls.crt")" - kubectl patch validatingwebhookconfiguration buckety-controller --type=json \ - -p="[{\"op\":\"replace\",\"path\":\"/webhooks/0/clientConfig/caBundle\",\"value\":\"${ca_bundle}\"}]" } -# No teardown between implementations. Deleting the overlay would -# delete the CRDs while scenario CRs (which carry finalizers) may -# still be terminating in their namespaces; the CRD then hangs in -# Terminating and the next implementation's apply is rejected with -# "create not allowed while custom resource definition is -# terminating". apply_overlay converges the config Secret and the -# Deployment in place instead; kubectl apply prunes the env vars a -# previous overlay patched in, because they are absent from the -# next overlay's last-applied configuration. +# switch_config points the controller at implementation $1's +# config and derives the renamed variant for backend-stickiness. +# Sets E2E_ORIGINAL_CONFIG / E2E_RENAMED_CONFIG for the scenarios. +switch_config() { + local impl="$1" backend="${IMPL_BACKEND[$1]}" + E2E_ORIGINAL_CONFIG="$HERE/configs/$impl.yaml" + E2E_RENAMED_CONFIG="$WORK/$impl.renamed.yaml" + sed "s/^- name: ${backend}\$/- name: ${backend}-renamed/" "$E2E_ORIGINAL_CONFIG" >"$E2E_RENAMED_CONFIG" + grep -q -- "^- name: ${backend}-renamed" "$E2E_RENAMED_CONFIG" \ + || fail "configs/$impl.yaml declares no backend named '$backend'" + log "controller config: $E2E_ORIGINAL_CONFIG" + apply_config "$E2E_ORIGINAL_CONFIG" +} + +# deploy_controller applies overlay/ once, with CONTROLLER_IMAGE +# set through a kustomize images transformer: `kubectl set image` +# after an apply would first roll out the base's placeholder tag +# (and leave an ImagePullBackOff pod behind on every re-apply). +# The certgen Jobs mint the webhook TLS Secret; the controller +# restarts until it exists and turns Ready only once its webhook +# listener answers, which is what `rollout status` waits for. +deploy_controller() { + [[ "$CONTROLLER_IMAGE" != *@* ]] \ + || fail "CONTROLLER_IMAGE must be a tag reference, got a digest: $CONTROLLER_IMAGE" + local name="${CONTROLLER_IMAGE%:*}" tag="${CONTROLLER_IMAGE##*:}" + # Generated next to overlay/ (gitignored): kustomize wants the + # resource path relative, and refuses one above its root. + mkdir -p "$HERE/.work" + cat >"$HERE/.work/kustomization.yaml" < Date: Fri, 4 Sep 2026 08:17:15 +0200 Subject: [PATCH 15/21] e2e: shared bodies for the driver-agnostic scenarios; helper pods pinned and in the scenario namespace backend-stickiness, misconfigured-startup and scaled-to-zero test controller behaviour that has nothing to do with the driver, and their three copies each were byte-identical apart from a backend name or a Secret key list - the kadm and s3 copies of backend-stickiness had already drifted in structure. Their bodies now live in test/e2e/lib.sh next to the helpers they use (controller_config_apply/restore, the operation every config-swapping scenario had reimplemented); each assert.sh stays runnable on its own and reads as the one line it asserts. Helper pods (rpk, aws-cli, curl) ran in the controller or the kafka namespace and lingered there when `--rm` did not get to run; they now run in the scenario namespace, cleaned up with it and inspectable next to the resources under test. aws-cli was the one floating tag in a suite where everything else is digest-pinned; it is pinned in one place in lib.sh and in the two consumer Jobs. Co-Authored-By: Claude Fable 5.1 --- examples/gcs/backend-stickiness/assert.sh | 61 +----- examples/gcs/misconfigured-startup/assert.sh | 67 +----- examples/gcs/scaled-to-zero/assert.sh | 24 +-- examples/kadm/backend-stickiness/README.md | 7 +- examples/kadm/backend-stickiness/assert.sh | 65 +----- examples/kadm/misconfigured-startup/assert.sh | 67 +----- examples/kadm/parameter-mutation/assert.sh | 2 +- examples/kadm/scaled-to-zero/assert.sh | 24 +-- examples/s3/backend-stickiness/assert.sh | 61 +----- examples/s3/happy-path/consumer-job.yaml | 2 +- examples/s3/misconfigured-startup/assert.sh | 67 +----- examples/s3/oob-drift/assert.sh | 8 +- examples/s3/retention-policy/assert.sh | 8 +- examples/s3/scaled-to-zero/assert.sh | 24 +-- examples/s3/scaled-to-zero/consumer-job.yaml | 2 +- test/e2e/lib.sh | 203 ++++++++++++++++-- 16 files changed, 224 insertions(+), 468 deletions(-) diff --git a/examples/gcs/backend-stickiness/assert.sh b/examples/gcs/backend-stickiness/assert.sh index bca1057..7e86906 100755 --- a/examples/gcs/backend-stickiness/assert.sh +++ b/examples/gcs/backend-stickiness/assert.sh @@ -2,62 +2,5 @@ set -euo pipefail . "${E2E_LIB:-$(cd "$(dirname "$0")/../../../test/e2e" && pwd)}/lib.sh" -wait_ready buckety/sticky-orig 120s -sticky_backend="$(kc get buckety/sticky-orig -o jsonpath='{.status.backend}')" -[[ "$sticky_backend" == "gcs" ]] \ - || fail "status.backend stamped as '$sticky_backend', expected 'gcs'" - -: "${E2E_RENAMED_CONFIG:?harness must set E2E_RENAMED_CONFIG to a yaml file that defines backend 'gcs-renamed' instead of 'gcs'}" -: "${E2E_ORIGINAL_CONFIG:?harness must set E2E_ORIGINAL_CONFIG to the unmodified controller config}" - -apply_config() { - kcg -n "$E2E_CONTROLLER_NS" create secret generic buckety-controller-config \ - --from-file=buckety-controller.yaml="$1" \ - --dry-run=client -o yaml | kcg apply -f - - rollout_restart deploy/buckety-controller - kcg -n "$E2E_CONTROLLER_NS" rollout status deploy/buckety-controller --timeout=60s -} - -restore() { - log "restoring original controller config" - apply_config "$E2E_ORIGINAL_CONFIG" -} -trap restore EXIT - -log "swapping controller config: gcs -> gcs-renamed" -apply_config "$E2E_RENAMED_CONFIG" - -wait_condition buckety/sticky-orig BackendUnavailable True 90s -[[ "$(condition_status buckety/sticky-orig Ready)" == "False" ]] \ - || fail "sticky-orig Ready should be False under BackendUnavailable" -still_sticky="$(kc get buckety/sticky-orig -o jsonpath='{.status.backend}')" -[[ "$still_sticky" == "gcs" ]] \ - || fail "status.backend mutated to '$still_sticky'; stickiness violated" - -# A fresh Buckety against the renamed backend still works. -log "applying new Buckety against gcs-renamed" -kc apply -f "$(dirname "$0")/renamed.yaml" -wait_ready buckety/sticky-new 120s - -# Deletion with retentionPolicy=Delete blocks while the backend is -# missing: removing the finalizer would silently orphan the bucket. -log "deleting sticky-orig while its backend is missing; expecting the deletion to block" -kc delete buckety/sticky-orig --wait=false -blocked="" -for _ in $(seq 1 20); do - blocked="$(kc get buckety/sticky-orig \ - -o jsonpath='{.status.conditions[?(@.type=="Ready")].message}' 2>/dev/null || true)" - [[ "$blocked" == *"deletion blocked"* ]] && break - sleep 3 -done -[[ "$blocked" == *"deletion blocked"* ]] \ - || fail "sticky-orig deletion did not surface 'deletion blocked' (Ready message: '$blocked')" -kc get buckety/sticky-orig >/dev/null 2>&1 \ - || fail "sticky-orig disappeared while its backend was missing; the bucket would be orphaned" - -# Restoring the backend unblocks the deletion and removes the bucket. -restore -kc wait --for=delete buckety/sticky-orig --timeout=90s \ - || fail "sticky-orig deletion did not complete after the backend was restored" - -log "backend-stickiness PASS" +# Body in test/e2e/lib.sh: controller behaviour, identical for every driver. +backend_stickiness_scenario gcs "$(dirname "$0")/renamed.yaml" diff --git a/examples/gcs/misconfigured-startup/assert.sh b/examples/gcs/misconfigured-startup/assert.sh index 38f8eb1..bb73d28 100755 --- a/examples/gcs/misconfigured-startup/assert.sh +++ b/examples/gcs/misconfigured-startup/assert.sh @@ -2,68 +2,5 @@ set -euo pipefail . "${E2E_LIB:-$(cd "$(dirname "$0")/../../../test/e2e" && pwd)}/lib.sh" -: "${E2E_ORIGINAL_CONFIG:?harness must set E2E_ORIGINAL_CONFIG to a valid buckety-controller.yaml on disk}" - -dir="$(cd "$(dirname "$0")" && pwd)" -broken_dir="$dir/broken-configs" -expectations="$dir/expectations.txt" - -restore() { - log "restoring original controller config" - kcg -n "$E2E_CONTROLLER_NS" create secret generic buckety-controller-config \ - --from-file=buckety-controller.yaml="$E2E_ORIGINAL_CONFIG" \ - --dry-run=client -o yaml | kcg apply -f - - rollout_restart deploy/buckety-controller - kcg -n "$E2E_CONTROLLER_NS" rollout status deploy/buckety-controller --timeout=60s -} -trap restore EXIT - -apply_broken() { - local file="$1" - kcg -n "$E2E_CONTROLLER_NS" create secret generic buckety-controller-config \ - --from-file=buckety-controller.yaml="$file" \ - --dry-run=client -o yaml | kcg apply -f - - rollout_restart deploy/buckety-controller -} - -# Iterate variants, each MUST produce a log line matching the -# documented regex on the controller's most recent attempt. -while read -r variant regex; do - [[ "$variant" =~ ^# ]] && continue - [[ -z "$variant" ]] && continue - file="$broken_dir/$variant" - [[ -f "$file" ]] || fail "broken-config variant not found: $file" - - log "applying broken config: $variant (expect regex: /$regex/)" - apply_broken "$file" - - # Wait until ANY pod's logs (current or previous container) - # carry the expected regex. During a rollout there can be two - # pods (old + new); after the new one has crashed once it lands - # in CrashLoopBackOff and the message is in either log stream. - deadline=$(( $(date +%s) + 90 )) - matched=0 - while (( $(date +%s) < deadline )); do - pods=$(kcg -n "$E2E_CONTROLLER_NS" get pods \ - -l app.kubernetes.io/name=buckety-controller \ - -o jsonpath='{range .items[*]}{.metadata.name} {end}' 2>/dev/null || true) - for pod in $pods; do - for arg in "" "--previous"; do - if kcg -n "$E2E_CONTROLLER_NS" logs "$pod" $arg 2>/dev/null \ - | grep -E "$regex" >/dev/null 2>&1; then - matched=1; break 3 - fi - done - done - sleep 2 - done - - if (( matched == 0 )); then - log "------ controller logs ($variant) ------" - kcg -n "$E2E_CONTROLLER_NS" logs -l app.kubernetes.io/name=buckety-controller \ - --tail=200 --previous 2>/dev/null || true - fail "variant '$variant' did not produce log matching /$regex/" - fi -done < "$expectations" - -log "misconfigured-startup PASS" +# Body in test/e2e/lib.sh: controller behaviour, identical for every driver. +misconfigured_startup_scenario "$(cd "$(dirname "$0")" && pwd)" diff --git a/examples/gcs/scaled-to-zero/assert.sh b/examples/gcs/scaled-to-zero/assert.sh index 9876b0b..75cd70c 100755 --- a/examples/gcs/scaled-to-zero/assert.sh +++ b/examples/gcs/scaled-to-zero/assert.sh @@ -2,25 +2,5 @@ set -euo pipefail . "${E2E_LIB:-$(cd "$(dirname "$0")/../../../test/e2e" && pwd)}/lib.sh" -wait_ready buckety/dial-tone 120s -secret_has_keys dial-tone-bucket endpoint bucket project region accessKeyID secretAccessKey - -# Scale operator to zero before the consumer Job runs. -log "scaling buckety-controller to 0 in $E2E_CONTROLLER_NS" -kcg -n "$E2E_CONTROLLER_NS" scale deploy/buckety-controller --replicas=0 -kcg -n "$E2E_CONTROLLER_NS" wait --for=delete pod \ - -l app.kubernetes.io/name=buckety-controller --timeout=60s - -# Apply the consumer Job while the operator is down. -log "applying consumer Job (operator is down)" -kc apply -f "$(dirname "$0")/consumer-job.yaml" -kc wait --for=condition=Complete --timeout=120s job/dial-tone-roundtrip \ - || { kc logs job/dial-tone-roundtrip >&2 || true; fail "roundtrip Job failed while operator was scaled to 0"; } - -# Restore replica count so subsequent scenarios see a running -# controller. -log "restoring buckety-controller to 1" -kcg -n "$E2E_CONTROLLER_NS" scale deploy/buckety-controller --replicas=1 -kcg -n "$E2E_CONTROLLER_NS" rollout status deploy/buckety-controller --timeout=60s - -log "scaled-to-zero PASS" +# Body in test/e2e/lib.sh: controller behaviour, identical for every driver. +scaled_to_zero_scenario dial-tone dial-tone-bucket "$(dirname "$0")/consumer-job.yaml" dial-tone-roundtrip endpoint bucket project region accessKeyID secretAccessKey diff --git a/examples/kadm/backend-stickiness/README.md b/examples/kadm/backend-stickiness/README.md index fc77320..cf33a93 100644 --- a/examples/kadm/backend-stickiness/README.md +++ b/examples/kadm/backend-stickiness/README.md @@ -34,6 +34,7 @@ surfaces. 4. Restore the controller config so subsequent scenarios are not affected. -The config-rotation dance lives in the assert script because -this scenario, by design, mutates the controller's deploy-time -state and must restore it. +The config-rotation dance (`backend_stickiness_scenario` in +`test/e2e/lib.sh`, shared by every driver's copy of this +scenario) mutates the controller's deploy-time state by design +and restores it on exit. diff --git a/examples/kadm/backend-stickiness/assert.sh b/examples/kadm/backend-stickiness/assert.sh index 10d8c5d..11f2cb6 100755 --- a/examples/kadm/backend-stickiness/assert.sh +++ b/examples/kadm/backend-stickiness/assert.sh @@ -2,66 +2,5 @@ set -euo pipefail . "${E2E_LIB:-$(cd "$(dirname "$0")/../../../test/e2e" && pwd)}/lib.sh" -wait_ready buckety/sticky-orig 120s -sticky_backend="$(kc get buckety/sticky-orig -o jsonpath='{.status.backend}')" -[[ "$sticky_backend" == "kafka" ]] \ - || fail "status.backend stamped as '$sticky_backend', expected 'kafka'" - -# Save current controller config Secret and patch a rename. -# The harness provides a renamed buckety-controller.yaml under -# $E2E_RENAMED_CONFIG; this scenario applies it, restarts the -# controller, and restores the original at the end. -: "${E2E_RENAMED_CONFIG:?harness must set E2E_RENAMED_CONFIG to a yaml file that defines backend 'kafka-renamed' instead of 'kafka'}" -: "${E2E_ORIGINAL_CONFIG:?harness must set E2E_ORIGINAL_CONFIG to the unmodified controller config}" - -restore() { - log "restoring original controller config" - kcg -n "$E2E_CONTROLLER_NS" create secret generic buckety-controller-config \ - --from-file=buckety-controller.yaml="$E2E_ORIGINAL_CONFIG" \ - --dry-run=client -o yaml | kcg apply -f - - rollout_restart deploy/buckety-controller - kcg -n "$E2E_CONTROLLER_NS" rollout status deploy/buckety-controller --timeout=60s -} -trap restore EXIT - -log "swapping controller config: kafka -> kafka-renamed" -kcg -n "$E2E_CONTROLLER_NS" create secret generic buckety-controller-config \ - --from-file=buckety-controller.yaml="$E2E_RENAMED_CONFIG" \ - --dry-run=client -o yaml | kcg apply -f - -rollout_restart deploy/buckety-controller -kcg -n "$E2E_CONTROLLER_NS" rollout status deploy/buckety-controller --timeout=60s - -wait_condition buckety/sticky-orig BackendUnavailable True 90s -[[ "$(condition_status buckety/sticky-orig Ready)" == "False" ]] \ - || fail "sticky-orig Ready should be False under BackendUnavailable" -still_sticky="$(kc get buckety/sticky-orig -o jsonpath='{.status.backend}')" -[[ "$still_sticky" == "kafka" ]] \ - || fail "status.backend mutated to '$still_sticky'; stickiness violated" - -# A fresh Buckety against the renamed backend still works. -log "applying new Buckety against kafka-renamed" -kc apply -f "$(dirname "$0")/renamed.yaml" -wait_ready buckety/sticky-new 120s - -# Deletion with retentionPolicy=Delete blocks while the backend is -# missing: removing the finalizer would silently orphan the topic. -log "deleting sticky-orig while its backend is missing; expecting the deletion to block" -kc delete buckety/sticky-orig --wait=false -blocked="" -for _ in $(seq 1 20); do - blocked="$(kc get buckety/sticky-orig \ - -o jsonpath='{.status.conditions[?(@.type=="Ready")].message}' 2>/dev/null || true)" - [[ "$blocked" == *"deletion blocked"* ]] && break - sleep 3 -done -[[ "$blocked" == *"deletion blocked"* ]] \ - || fail "sticky-orig deletion did not surface 'deletion blocked' (Ready message: '$blocked')" -kc get buckety/sticky-orig >/dev/null 2>&1 \ - || fail "sticky-orig disappeared while its backend was missing; the topic would be orphaned" - -# Restoring the backend unblocks the deletion and removes the topic. -restore -kc wait --for=delete buckety/sticky-orig --timeout=90s \ - || fail "sticky-orig deletion did not complete after the backend was restored" - -log "backend-stickiness PASS" +# Body in test/e2e/lib.sh: controller behaviour, identical for every driver. +backend_stickiness_scenario kafka "$(dirname "$0")/renamed.yaml" diff --git a/examples/kadm/misconfigured-startup/assert.sh b/examples/kadm/misconfigured-startup/assert.sh index 38f8eb1..bb73d28 100755 --- a/examples/kadm/misconfigured-startup/assert.sh +++ b/examples/kadm/misconfigured-startup/assert.sh @@ -2,68 +2,5 @@ set -euo pipefail . "${E2E_LIB:-$(cd "$(dirname "$0")/../../../test/e2e" && pwd)}/lib.sh" -: "${E2E_ORIGINAL_CONFIG:?harness must set E2E_ORIGINAL_CONFIG to a valid buckety-controller.yaml on disk}" - -dir="$(cd "$(dirname "$0")" && pwd)" -broken_dir="$dir/broken-configs" -expectations="$dir/expectations.txt" - -restore() { - log "restoring original controller config" - kcg -n "$E2E_CONTROLLER_NS" create secret generic buckety-controller-config \ - --from-file=buckety-controller.yaml="$E2E_ORIGINAL_CONFIG" \ - --dry-run=client -o yaml | kcg apply -f - - rollout_restart deploy/buckety-controller - kcg -n "$E2E_CONTROLLER_NS" rollout status deploy/buckety-controller --timeout=60s -} -trap restore EXIT - -apply_broken() { - local file="$1" - kcg -n "$E2E_CONTROLLER_NS" create secret generic buckety-controller-config \ - --from-file=buckety-controller.yaml="$file" \ - --dry-run=client -o yaml | kcg apply -f - - rollout_restart deploy/buckety-controller -} - -# Iterate variants, each MUST produce a log line matching the -# documented regex on the controller's most recent attempt. -while read -r variant regex; do - [[ "$variant" =~ ^# ]] && continue - [[ -z "$variant" ]] && continue - file="$broken_dir/$variant" - [[ -f "$file" ]] || fail "broken-config variant not found: $file" - - log "applying broken config: $variant (expect regex: /$regex/)" - apply_broken "$file" - - # Wait until ANY pod's logs (current or previous container) - # carry the expected regex. During a rollout there can be two - # pods (old + new); after the new one has crashed once it lands - # in CrashLoopBackOff and the message is in either log stream. - deadline=$(( $(date +%s) + 90 )) - matched=0 - while (( $(date +%s) < deadline )); do - pods=$(kcg -n "$E2E_CONTROLLER_NS" get pods \ - -l app.kubernetes.io/name=buckety-controller \ - -o jsonpath='{range .items[*]}{.metadata.name} {end}' 2>/dev/null || true) - for pod in $pods; do - for arg in "" "--previous"; do - if kcg -n "$E2E_CONTROLLER_NS" logs "$pod" $arg 2>/dev/null \ - | grep -E "$regex" >/dev/null 2>&1; then - matched=1; break 3 - fi - done - done - sleep 2 - done - - if (( matched == 0 )); then - log "------ controller logs ($variant) ------" - kcg -n "$E2E_CONTROLLER_NS" logs -l app.kubernetes.io/name=buckety-controller \ - --tail=200 --previous 2>/dev/null || true - fail "variant '$variant' did not produce log matching /$regex/" - fi -done < "$expectations" - -log "misconfigured-startup PASS" +# Body in test/e2e/lib.sh: controller behaviour, identical for every driver. +misconfigured_startup_scenario "$(cd "$(dirname "$0")" && pwd)" diff --git a/examples/kadm/parameter-mutation/assert.sh b/examples/kadm/parameter-mutation/assert.sh index ae87b01..7cfc5b0 100755 --- a/examples/kadm/parameter-mutation/assert.sh +++ b/examples/kadm/parameter-mutation/assert.sh @@ -34,7 +34,7 @@ log "checking broker for retention.ms=3600000 on topic '$topic_name'" bootstrap="$(secret_value cfg-mut-topic bootstrap)" broker_retention="" for _ in $(seq 1 12); do - broker_retention="$(kcg run -n "${E2E_KAFKA_NAMESPACE:-redpanda}" --rm -i --restart=Never --quiet \ + broker_retention="$(kc run --rm -i --restart=Never --quiet \ --image=ghcr.io/yolean/redpanda:v24.2.22@sha256:5132085d4fe35b0fd6ddedc7f0fe3d3ba7be12c5e3829e1a2b986cd41b1d3538 \ "rpk-cfgcheck-$RANDOM" -- \ topic describe -c "$topic_name" --brokers "$bootstrap" 2>/dev/null \ diff --git a/examples/kadm/scaled-to-zero/assert.sh b/examples/kadm/scaled-to-zero/assert.sh index 4e84be9..936d4ef 100755 --- a/examples/kadm/scaled-to-zero/assert.sh +++ b/examples/kadm/scaled-to-zero/assert.sh @@ -2,25 +2,5 @@ set -euo pipefail . "${E2E_LIB:-$(cd "$(dirname "$0")/../../../test/e2e" && pwd)}/lib.sh" -wait_ready buckety/dial-tone 120s -secret_has_keys dial-tone-topic bootstrap topic - -# Scale operator to zero before the consumer Job runs. -log "scaling buckety-controller to 0 in $E2E_CONTROLLER_NS" -kcg -n "$E2E_CONTROLLER_NS" scale deploy/buckety-controller --replicas=0 -kcg -n "$E2E_CONTROLLER_NS" wait --for=delete pod \ - -l app.kubernetes.io/name=buckety-controller --timeout=60s - -# Apply the consumer Job while the operator is down. -log "applying consumer Job (operator is down)" -kc apply -f "$(dirname "$0")/consumer-job.yaml" -kc wait --for=condition=Complete --timeout=120s job/dial-tone-roundtrip \ - || { kc logs job/dial-tone-roundtrip >&2 || true; fail "roundtrip Job failed while operator was scaled to 0"; } - -# Restore replica count so subsequent scenarios see a running -# controller. -log "restoring buckety-controller to 1" -kcg -n "$E2E_CONTROLLER_NS" scale deploy/buckety-controller --replicas=1 -kcg -n "$E2E_CONTROLLER_NS" rollout status deploy/buckety-controller --timeout=60s - -log "scaled-to-zero PASS" +# Body in test/e2e/lib.sh: controller behaviour, identical for every driver. +scaled_to_zero_scenario dial-tone dial-tone-topic "$(dirname "$0")/consumer-job.yaml" dial-tone-roundtrip bootstrap topic diff --git a/examples/s3/backend-stickiness/assert.sh b/examples/s3/backend-stickiness/assert.sh index e01cc34..081f091 100755 --- a/examples/s3/backend-stickiness/assert.sh +++ b/examples/s3/backend-stickiness/assert.sh @@ -2,62 +2,5 @@ set -euo pipefail . "${E2E_LIB:-$(cd "$(dirname "$0")/../../../test/e2e" && pwd)}/lib.sh" -wait_ready buckety/sticky-orig 120s -sticky_backend="$(kc get buckety/sticky-orig -o jsonpath='{.status.backend}')" -[[ "$sticky_backend" == "s3" ]] \ - || fail "status.backend stamped as '$sticky_backend', expected 's3'" - -: "${E2E_RENAMED_CONFIG:?harness must set E2E_RENAMED_CONFIG to a yaml file that defines backend 's3-renamed' instead of 's3'}" -: "${E2E_ORIGINAL_CONFIG:?harness must set E2E_ORIGINAL_CONFIG to the unmodified controller config}" - -apply_config() { - kcg -n "$E2E_CONTROLLER_NS" create secret generic buckety-controller-config \ - --from-file=buckety-controller.yaml="$1" \ - --dry-run=client -o yaml | kcg apply -f - - rollout_restart deploy/buckety-controller - kcg -n "$E2E_CONTROLLER_NS" rollout status deploy/buckety-controller --timeout=60s -} - -restore() { - log "restoring original controller config" - apply_config "$E2E_ORIGINAL_CONFIG" -} -trap restore EXIT - -log "swapping controller config: s3 -> s3-renamed" -apply_config "$E2E_RENAMED_CONFIG" - -wait_condition buckety/sticky-orig BackendUnavailable True 90s -[[ "$(condition_status buckety/sticky-orig Ready)" == "False" ]] \ - || fail "sticky-orig Ready should be False under BackendUnavailable" -still_sticky="$(kc get buckety/sticky-orig -o jsonpath='{.status.backend}')" -[[ "$still_sticky" == "s3" ]] \ - || fail "status.backend mutated to '$still_sticky'; stickiness violated" - -# A fresh Buckety against the renamed backend still works. -log "applying new Buckety against s3-renamed" -kc apply -f "$(dirname "$0")/renamed.yaml" -wait_ready buckety/sticky-new 120s - -# Deletion with retentionPolicy=Delete blocks while the backend is -# missing: removing the finalizer would silently orphan the bucket. -log "deleting sticky-orig while its backend is missing; expecting the deletion to block" -kc delete buckety/sticky-orig --wait=false -blocked="" -for _ in $(seq 1 20); do - blocked="$(kc get buckety/sticky-orig \ - -o jsonpath='{.status.conditions[?(@.type=="Ready")].message}' 2>/dev/null || true)" - [[ "$blocked" == *"deletion blocked"* ]] && break - sleep 3 -done -[[ "$blocked" == *"deletion blocked"* ]] \ - || fail "sticky-orig deletion did not surface 'deletion blocked' (Ready message: '$blocked')" -kc get buckety/sticky-orig >/dev/null 2>&1 \ - || fail "sticky-orig disappeared while its backend was missing; the bucket would be orphaned" - -# Restoring the backend unblocks the deletion and removes the bucket. -restore -kc wait --for=delete buckety/sticky-orig --timeout=90s \ - || fail "sticky-orig deletion did not complete after the backend was restored" - -log "backend-stickiness PASS" +# Body in test/e2e/lib.sh: controller behaviour, identical for every driver. +backend_stickiness_scenario s3 "$(dirname "$0")/renamed.yaml" diff --git a/examples/s3/happy-path/consumer-job.yaml b/examples/s3/happy-path/consumer-job.yaml index 695cb50..257daba 100644 --- a/examples/s3/happy-path/consumer-job.yaml +++ b/examples/s3/happy-path/consumer-job.yaml @@ -9,7 +9,7 @@ spec: restartPolicy: Never containers: - name: roundtrip - image: public.ecr.aws/aws-cli/aws-cli:latest + image: public.ecr.aws/aws-cli/aws-cli:2.36.39@sha256:df8b292f3ae0092a2861eb4ad37f46d755ffd082615f99bb404bb0d118e6e1f1 env: - name: AWS_ENDPOINT_URL valueFrom: { secretKeyRef: { name: orders-bucket, key: endpoint } } diff --git a/examples/s3/misconfigured-startup/assert.sh b/examples/s3/misconfigured-startup/assert.sh index 38f8eb1..bb73d28 100755 --- a/examples/s3/misconfigured-startup/assert.sh +++ b/examples/s3/misconfigured-startup/assert.sh @@ -2,68 +2,5 @@ set -euo pipefail . "${E2E_LIB:-$(cd "$(dirname "$0")/../../../test/e2e" && pwd)}/lib.sh" -: "${E2E_ORIGINAL_CONFIG:?harness must set E2E_ORIGINAL_CONFIG to a valid buckety-controller.yaml on disk}" - -dir="$(cd "$(dirname "$0")" && pwd)" -broken_dir="$dir/broken-configs" -expectations="$dir/expectations.txt" - -restore() { - log "restoring original controller config" - kcg -n "$E2E_CONTROLLER_NS" create secret generic buckety-controller-config \ - --from-file=buckety-controller.yaml="$E2E_ORIGINAL_CONFIG" \ - --dry-run=client -o yaml | kcg apply -f - - rollout_restart deploy/buckety-controller - kcg -n "$E2E_CONTROLLER_NS" rollout status deploy/buckety-controller --timeout=60s -} -trap restore EXIT - -apply_broken() { - local file="$1" - kcg -n "$E2E_CONTROLLER_NS" create secret generic buckety-controller-config \ - --from-file=buckety-controller.yaml="$file" \ - --dry-run=client -o yaml | kcg apply -f - - rollout_restart deploy/buckety-controller -} - -# Iterate variants, each MUST produce a log line matching the -# documented regex on the controller's most recent attempt. -while read -r variant regex; do - [[ "$variant" =~ ^# ]] && continue - [[ -z "$variant" ]] && continue - file="$broken_dir/$variant" - [[ -f "$file" ]] || fail "broken-config variant not found: $file" - - log "applying broken config: $variant (expect regex: /$regex/)" - apply_broken "$file" - - # Wait until ANY pod's logs (current or previous container) - # carry the expected regex. During a rollout there can be two - # pods (old + new); after the new one has crashed once it lands - # in CrashLoopBackOff and the message is in either log stream. - deadline=$(( $(date +%s) + 90 )) - matched=0 - while (( $(date +%s) < deadline )); do - pods=$(kcg -n "$E2E_CONTROLLER_NS" get pods \ - -l app.kubernetes.io/name=buckety-controller \ - -o jsonpath='{range .items[*]}{.metadata.name} {end}' 2>/dev/null || true) - for pod in $pods; do - for arg in "" "--previous"; do - if kcg -n "$E2E_CONTROLLER_NS" logs "$pod" $arg 2>/dev/null \ - | grep -E "$regex" >/dev/null 2>&1; then - matched=1; break 3 - fi - done - done - sleep 2 - done - - if (( matched == 0 )); then - log "------ controller logs ($variant) ------" - kcg -n "$E2E_CONTROLLER_NS" logs -l app.kubernetes.io/name=buckety-controller \ - --tail=200 --previous 2>/dev/null || true - fail "variant '$variant' did not produce log matching /$regex/" - fi -done < "$expectations" - -log "misconfigured-startup PASS" +# Body in test/e2e/lib.sh: controller behaviour, identical for every driver. +misconfigured_startup_scenario "$(cd "$(dirname "$0")" && pwd)" diff --git a/examples/s3/oob-drift/assert.sh b/examples/s3/oob-drift/assert.sh index b6e74d6..5949cfc 100755 --- a/examples/s3/oob-drift/assert.sh +++ b/examples/s3/oob-drift/assert.sh @@ -11,8 +11,8 @@ log "initial bucket=$bucket" s3_bucket_exists "$bucket" "$endpoint" "$access" "$secret" log "out-of-band: deleting bucket $bucket directly" -kcg run -n "$E2E_CONTROLLER_NS" --rm -i --restart=Never --quiet \ - --image=public.ecr.aws/aws-cli/aws-cli:latest \ +kc run --rm -i --restart=Never --quiet \ + --image="$AWSCLI_IMAGE" \ --env="AWS_ACCESS_KEY_ID=$access" \ --env="AWS_SECRET_ACCESS_KEY=$secret" \ --env="AWS_REGION=us-east-1" \ @@ -23,8 +23,8 @@ kcg run -n "$E2E_CONTROLLER_NS" --rm -i --restart=Never --quiet \ deadline=$(( $(date +%s) + 90 )) recreated=0 while (( $(date +%s) < deadline )); do - if kcg run -n "$E2E_CONTROLLER_NS" --rm -i --restart=Never --quiet \ - --image=public.ecr.aws/aws-cli/aws-cli:latest \ + if kc run --rm -i --restart=Never --quiet \ + --image="$AWSCLI_IMAGE" \ --env="AWS_ACCESS_KEY_ID=$access" \ --env="AWS_SECRET_ACCESS_KEY=$secret" \ --env="AWS_REGION=us-east-1" \ diff --git a/examples/s3/retention-policy/assert.sh b/examples/s3/retention-policy/assert.sh index e70173b..3b8aaf0 100755 --- a/examples/s3/retention-policy/assert.sh +++ b/examples/s3/retention-policy/assert.sh @@ -18,8 +18,8 @@ s3_bucket_exists "$drop_bucket" "$endpoint" "$access" "$secret" # just DeleteBucket on an already-empty bucket. log "writing objects into $drop_bucket before deletion" for key in doomed/one.txt doomed/two.txt; do - kcg run -n "$E2E_CONTROLLER_NS" --rm -i --restart=Never --quiet \ - --image=public.ecr.aws/aws-cli/aws-cli:latest \ + kc run --rm -i --restart=Never --quiet \ + --image="$AWSCLI_IMAGE" \ --env="AWS_ACCESS_KEY_ID=$access" \ --env="AWS_SECRET_ACCESS_KEY=$secret" \ --env="AWS_REGION=us-east-1" \ @@ -37,8 +37,8 @@ s3_bucket_exists "$keep_bucket" "$endpoint" "$access" "$secret" log "deleting Buckety/drop-me (Delete, recursive)" kc delete buckety/drop-me --wait=true --timeout=90s resource_absent secret/drop-me-bucket 30s -if kcg run -n "$E2E_CONTROLLER_NS" --rm -i --restart=Never --quiet \ - --image=public.ecr.aws/aws-cli/aws-cli:latest \ +if kc run --rm -i --restart=Never --quiet \ + --image="$AWSCLI_IMAGE" \ --env="AWS_ACCESS_KEY_ID=$access" \ --env="AWS_SECRET_ACCESS_KEY=$secret" \ "awscli-deleted-$RANDOM" -- \ diff --git a/examples/s3/scaled-to-zero/assert.sh b/examples/s3/scaled-to-zero/assert.sh index 52a7c54..0db294d 100755 --- a/examples/s3/scaled-to-zero/assert.sh +++ b/examples/s3/scaled-to-zero/assert.sh @@ -2,25 +2,5 @@ set -euo pipefail . "${E2E_LIB:-$(cd "$(dirname "$0")/../../../test/e2e" && pwd)}/lib.sh" -wait_ready buckety/dial-tone 120s -secret_has_keys dial-tone-bucket endpoint bucket accessKeyID secretAccessKey - -# Scale operator to zero before the consumer Job runs. -log "scaling buckety-controller to 0 in $E2E_CONTROLLER_NS" -kcg -n "$E2E_CONTROLLER_NS" scale deploy/buckety-controller --replicas=0 -kcg -n "$E2E_CONTROLLER_NS" wait --for=delete pod \ - -l app.kubernetes.io/name=buckety-controller --timeout=60s - -# Apply the consumer Job while the operator is down. -log "applying consumer Job (operator is down)" -kc apply -f "$(dirname "$0")/consumer-job.yaml" -kc wait --for=condition=Complete --timeout=120s job/dial-tone-roundtrip \ - || { kc logs job/dial-tone-roundtrip >&2 || true; fail "roundtrip Job failed while operator was scaled to 0"; } - -# Restore replica count so subsequent scenarios see a running -# controller. -log "restoring buckety-controller to 1" -kcg -n "$E2E_CONTROLLER_NS" scale deploy/buckety-controller --replicas=1 -kcg -n "$E2E_CONTROLLER_NS" rollout status deploy/buckety-controller --timeout=60s - -log "scaled-to-zero PASS" +# Body in test/e2e/lib.sh: controller behaviour, identical for every driver. +scaled_to_zero_scenario dial-tone dial-tone-bucket "$(dirname "$0")/consumer-job.yaml" dial-tone-roundtrip endpoint bucket accessKeyID secretAccessKey diff --git a/examples/s3/scaled-to-zero/consumer-job.yaml b/examples/s3/scaled-to-zero/consumer-job.yaml index 3ee8cdf..48e2d2c 100644 --- a/examples/s3/scaled-to-zero/consumer-job.yaml +++ b/examples/s3/scaled-to-zero/consumer-job.yaml @@ -9,7 +9,7 @@ spec: restartPolicy: Never containers: - name: roundtrip - image: public.ecr.aws/aws-cli/aws-cli:latest + image: public.ecr.aws/aws-cli/aws-cli:2.36.39@sha256:df8b292f3ae0092a2861eb4ad37f46d755ffd082615f99bb404bb0d118e6e1f1 env: - name: AWS_ENDPOINT_URL valueFrom: { secretKeyRef: { name: dial-tone-bucket, key: endpoint } } diff --git a/test/e2e/lib.sh b/test/e2e/lib.sh index f019e02..4aa9e41 100755 --- a/test/e2e/lib.sh +++ b/test/e2e/lib.sh @@ -35,6 +35,11 @@ export KUBECONFIG kc() { kubectl -n "$E2E_NAMESPACE" "$@"; } kcg() { kubectl "$@"; } +# Images the helper pods run. Pinned like everything else the +# suite pulls: a floating tag flakes every helper at once when the +# upstream image changes. +AWSCLI_IMAGE="public.ecr.aws/aws-cli/aws-cli:2.36.39@sha256:df8b292f3ae0092a2861eb4ad37f46d755ffd082615f99bb404bb0d118e6e1f1" + log() { printf '[assert] %s\n' "$*" >&2; } fail() { printf '[assert][FAIL] %s\n' "$*" >&2; exit 1; } @@ -79,7 +84,7 @@ rpk_topic_list() { local bootstrap="${2:-${E2E_KAFKA_BOOTSTRAP:-redpanda.${kns}.svc.cluster.local:9093}}" # ghcr.io/yolean/redpanda's ENTRYPOINT is rpk; pass args # without the leading `rpk` to avoid `rpk rpk topic ...`. - kcg run -n "$kns" --rm -i --restart=Never --quiet \ + kc run --rm -i --restart=Never --quiet \ --image=ghcr.io/yolean/redpanda:v24.2.22@sha256:5132085d4fe35b0fd6ddedc7f0fe3d3ba7be12c5e3829e1a2b986cd41b1d3538 \ "rpk-check-$RANDOM" -- \ topic list --brokers "$bootstrap" &1 @@ -152,8 +157,7 @@ rollout_restart() { rpk_run() { local prefix="$1" shift - local kns="${E2E_KAFKA_NAMESPACE:-redpanda}" - kcg run -n "$kns" --rm -i --restart=Never --quiet \ + kc run --rm -i --restart=Never --quiet \ --image=ghcr.io/yolean/redpanda:v24.2.22@sha256:5132085d4fe35b0fd6ddedc7f0fe3d3ba7be12c5e3829e1a2b986cd41b1d3538 \ "rpk-${prefix}-$RANDOM" -- \ "$@" 2>&1 @@ -189,8 +193,8 @@ s3_bucket_exists() { local bucket="$1" endpoint="$2" access="$3" secret="$4" log "verifying S3 bucket '$bucket' at $endpoint" local out - if ! out="$(kcg run -n "$E2E_CONTROLLER_NS" --rm -i --restart=Never --quiet \ - --image=public.ecr.aws/aws-cli/aws-cli:latest \ + if ! out="$(kc run --rm -i --restart=Never --quiet \ + --image="$AWSCLI_IMAGE" \ --env="AWS_ACCESS_KEY_ID=$access" \ --env="AWS_SECRET_ACCESS_KEY=$secret" \ "awscli-check-$RANDOM" -- \ @@ -206,8 +210,8 @@ s3_bucket_exists() { s3_api() { local endpoint="$1" access="$2" secret="$3" shift 3 - kcg run -n "$E2E_CONTROLLER_NS" --rm -i --restart=Never --quiet \ - --image=public.ecr.aws/aws-cli/aws-cli:latest \ + kc run --rm -i --restart=Never --quiet \ + --image="$AWSCLI_IMAGE" \ --env="AWS_ACCESS_KEY_ID=$access" \ --env="AWS_SECRET_ACCESS_KEY=$secret" \ "awscli-api-$RANDOM" -- \ @@ -286,7 +290,7 @@ resource_absent() { # values are bare hosts (schemes are the consumer's choice); # these emulator-coupled helpers speak plain http. gcs_api() { - kcg run -n "$E2E_CONTROLLER_NS" --rm -i --restart=Never --quiet \ + kc run --rm -i --restart=Never --quiet \ --image=curlimages/curl:8.17.0@sha256:935d9100e9ba842cdb060de42472c7ca90cfe9a7c96e4dacb55e79e560b3ff40 \ "curl-check-$RANDOM" -- \ -sfS "$1" &1 @@ -315,7 +319,7 @@ gcs_bucket_exists_quiet() { # Deletes the bucket directly (out-of-band mutation). gcs_bucket_delete() { local bucket="$1" endpoint="$2" - kcg run -n "$E2E_CONTROLLER_NS" --rm -i --restart=Never --quiet \ + kc run --rm -i --restart=Never --quiet \ --image=curlimages/curl:8.17.0@sha256:935d9100e9ba842cdb060de42472c7ca90cfe9a7c96e4dacb55e79e560b3ff40 \ "curl-oob-$RANDOM" -- \ -sfS -X DELETE "http://$endpoint/storage/v1/b/$bucket" +# Installs as the controller config Secret and restarts the +# controller - the operation a platform performs on a config +# change. Scenarios that swap the config trap +# controller_config_restore so later scenarios see the original. +controller_config_apply() { + kcg -n "$E2E_CONTROLLER_NS" create secret generic buckety-controller-config \ + --from-file=buckety-controller.yaml="$1" \ + --dry-run=client -o yaml | kcg apply -f - + rollout_restart deploy/buckety-controller + kcg -n "$E2E_CONTROLLER_NS" rollout status deploy/buckety-controller --timeout=120s +} + +controller_config_restore() { + log "restoring original controller config" + controller_config_apply "${E2E_ORIGINAL_CONFIG:?harness must set E2E_ORIGINAL_CONFIG}" +} + +# controller_set_image +controller_set_image() { + log "switching controller image to $1" + kcg -n "$E2E_CONTROLLER_NS" set image deploy/buckety-controller "controller=$1" + kcg -n "$E2E_CONTROLLER_NS" rollout status deploy/buckety-controller --timeout=120s +} + +# ---- shared scenario bodies ---- +# +# Some scenarios exercise controller behaviour that does not +# depend on the driver, and their assert.sh files were +# byte-identical across drivers apart from a name or a key list. +# Their bodies live here so a fix lands once; each +# examples///assert.sh stays runnable on its +# own and reads as the one-line statement of what it asserts. + +# backend_stickiness_scenario +# SPEC §End-to-end coverage #7. buckety/sticky-orig was applied +# against ; the harness's E2E_RENAMED_CONFIG declares +# -renamed instead, and targets it. +backend_stickiness_scenario() { + local backend="$1" renamed_cr="$2" + : "${E2E_RENAMED_CONFIG:?harness must set E2E_RENAMED_CONFIG (a config declaring '${backend}-renamed' instead of '${backend}')}" + : "${E2E_ORIGINAL_CONFIG:?harness must set E2E_ORIGINAL_CONFIG}" + + wait_ready buckety/sticky-orig 120s + local sticky_backend + sticky_backend="$(kc get buckety/sticky-orig -o jsonpath='{.status.backend}')" + [[ "$sticky_backend" == "$backend" ]] \ + || fail "status.backend stamped as '$sticky_backend', expected '$backend'" + + trap controller_config_restore EXIT + log "swapping controller config: $backend -> $backend-renamed" + controller_config_apply "$E2E_RENAMED_CONFIG" + + wait_condition buckety/sticky-orig BackendUnavailable True 90s + [[ "$(condition_status buckety/sticky-orig Ready)" == "False" ]] \ + || fail "sticky-orig Ready should be False under BackendUnavailable" + local still_sticky + still_sticky="$(kc get buckety/sticky-orig -o jsonpath='{.status.backend}')" + [[ "$still_sticky" == "$backend" ]] \ + || fail "status.backend mutated to '$still_sticky'; stickiness violated" + + # A fresh Buckety against the renamed backend still works. + log "applying new Buckety against $backend-renamed" + kc apply -f "$renamed_cr" + wait_ready buckety/sticky-new 120s + + # Deletion with retentionPolicy=Delete blocks while the backend + # is missing: removing the finalizer would silently orphan the + # backend resource. + log "deleting sticky-orig while its backend is missing; expecting the deletion to block" + kc delete buckety/sticky-orig --wait=false + local blocked="" + for _ in $(seq 1 20); do + blocked="$(kc get buckety/sticky-orig \ + -o jsonpath='{.status.conditions[?(@.type=="Ready")].message}' 2>/dev/null || true)" + [[ "$blocked" == *"deletion blocked"* ]] && break + sleep 3 + done + [[ "$blocked" == *"deletion blocked"* ]] \ + || fail "sticky-orig deletion did not surface 'deletion blocked' (Ready message: '$blocked')" + kc get buckety/sticky-orig >/dev/null 2>&1 \ + || fail "sticky-orig disappeared while its backend was missing; the backend resource would be orphaned" + + # Restoring the backend unblocks the deletion. + controller_config_restore + kc wait --for=delete buckety/sticky-orig --timeout=90s \ + || fail "sticky-orig deletion did not complete after the backend was restored" + + log "backend-stickiness PASS" +} + +# misconfigured_startup_scenario +# SPEC §End-to-end coverage #10. Every /broken-configs/ +# listed in /expectations.txt is installed as the controller +# config; the controller MUST refuse to start with a log line +# matching the listed regex. +misconfigured_startup_scenario() { + local dir="$1" broken_dir="$1/broken-configs" expectations="$1/expectations.txt" + : "${E2E_ORIGINAL_CONFIG:?harness must set E2E_ORIGINAL_CONFIG}" + trap controller_config_restore EXIT + + local variant regex file + while read -r variant regex; do + [[ "$variant" =~ ^# ]] && continue + [[ -z "$variant" ]] && continue + file="$broken_dir/$variant" + [[ -f "$file" ]] || fail "broken-config variant not found: $file" + + log "applying broken config: $variant (expect regex: /$regex/)" + kcg -n "$E2E_CONTROLLER_NS" create secret generic buckety-controller-config \ + --from-file=buckety-controller.yaml="$file" \ + --dry-run=client -o yaml | kcg apply -f - + rollout_restart deploy/buckety-controller + + # Wait until ANY pod's logs (current or previous container) + # carry the expected regex. During a rollout there can be two + # pods (old + new); after the new one has crashed once it lands + # in CrashLoopBackOff and the message is in either log stream. + local deadline=$(( $(date +%s) + 90 )) matched=0 pods pod arg + while (( $(date +%s) < deadline )); do + pods=$(kcg -n "$E2E_CONTROLLER_NS" get pods \ + -l app.kubernetes.io/name=buckety-controller \ + -o jsonpath='{range .items[*]}{.metadata.name} {end}' 2>/dev/null || true) + for pod in $pods; do + for arg in "" "--previous"; do + if kcg -n "$E2E_CONTROLLER_NS" logs "$pod" $arg 2>/dev/null \ + | grep -E "$regex" >/dev/null 2>&1; then + matched=1; break 3 + fi + done + done + sleep 2 + done + if (( matched == 0 )); then + log "------ controller logs ($variant) ------" + kcg -n "$E2E_CONTROLLER_NS" logs -l app.kubernetes.io/name=buckety-controller \ + --tail=200 --previous 2>/dev/null || true + fail "variant '$variant' did not produce log matching /$regex/" + fi + done < "$expectations" + + log "misconfigured-startup PASS" +} + +# scaled_to_zero_scenario +# SPEC §End-to-end coverage #5. The consumer Job in +# round-trips through the backend using only the Secret, with the +# controller scaled to zero. +scaled_to_zero_scenario() { + local bky="$1" secret="$2" job_file="$3" job="$4" + shift 4 + wait_ready "buckety/$bky" 120s + secret_has_keys "$secret" "$@" + + log "scaling buckety-controller to 0 in $E2E_CONTROLLER_NS" + kcg -n "$E2E_CONTROLLER_NS" scale deploy/buckety-controller --replicas=0 + kcg -n "$E2E_CONTROLLER_NS" wait --for=delete pod \ + -l app.kubernetes.io/name=buckety-controller --timeout=60s + + log "applying consumer Job (operator is down)" + kc apply -f "$job_file" + kc wait --for=condition=Complete --timeout=120s "job/$job" \ + || { kc logs "job/$job" >&2 || true; fail "roundtrip Job failed while operator was scaled to 0"; } + + # Restore replica count so subsequent scenarios see a running + # controller. + log "restoring buckety-controller to 1" + kcg -n "$E2E_CONTROLLER_NS" scale deploy/buckety-controller --replicas=1 + kcg -n "$E2E_CONTROLLER_NS" rollout status deploy/buckety-controller --timeout=120s + + log "scaled-to-zero PASS" +} From 682cfad15b1b6582d163d9e655364c04d0cffe86 Mon Sep 17 00:00:00 2001 From: Staffan Olsson Date: Fri, 4 Sep 2026 08:17:40 +0200 Subject: [PATCH 16/21] e2e: driver-version derives every version from the binary; gcs gains the scenario The workflow hard-coded E2E_VERSION_BASE=0.1.0 for every driver and built the rotation images with all drivers at 0.1.1 and 1.0.0. gcs has been at 0.2.0 since driver 0.2 shipped, so its "patch" rotation was a minor downgrade - unnoticed because gcs had no driver-version scenario at all - and the first minor bump of kadm or s3 would have failed CI in a workflow that SPEC #5 says must know nothing about scenarios. test/e2e/build-rotation-images.sh now reads `buckety --version` and builds dv-patch (X.Y.Z+1 per driver) and dv-major ((X+1).0.0) from that. The scenario, one shared body for all three drivers, takes the base from the status.driverBuildVersion the base image stamps and derives the patch expectation from it. Nothing in the harness carries a driver version any more, and gcs is covered. Co-Authored-By: Claude Fable 5.1 --- .github/workflows/e2e.yaml | 29 +++------ examples/gcs/driver-version/README.md | 10 +++ examples/gcs/driver-version/assert.sh | 6 ++ examples/gcs/driver-version/buckety.yaml | 11 ++++ .../gcs/driver-version/kustomization.yaml | 4 ++ examples/kadm/driver-version/README.md | 24 ++++---- examples/kadm/driver-version/assert.sh | 61 +------------------ examples/s3/driver-version/README.md | 4 +- examples/s3/driver-version/assert.sh | 61 +------------------ test/e2e/build-rotation-images.sh | 37 +++++++++++ test/e2e/lib.sh | 53 ++++++++++++++++ test/e2e/run.sh | 12 ++-- 12 files changed, 152 insertions(+), 160 deletions(-) create mode 100644 examples/gcs/driver-version/README.md create mode 100644 examples/gcs/driver-version/assert.sh create mode 100644 examples/gcs/driver-version/buckety.yaml create mode 100644 examples/gcs/driver-version/kustomization.yaml create mode 100755 test/e2e/build-rotation-images.sh diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml index 64806b7..90317bf 100644 --- a/.github/workflows/e2e.yaml +++ b/.github/workflows/e2e.yaml @@ -117,23 +117,13 @@ jobs: exit 1 fi - # The driver-version scenario rotates the controller between a - # base, a patch-bumped and a major-bumped driver build (SPEC.md - # scenario #9, docs/SCAFFOLDING.md "Driver version stamping"). - # These images exist only for that rotation: built here with - # per-driver -X flags and pushed straight to the run's local - # registry. They are not digest-pinned or published. + # The driver-version scenario rotates the controller through a + # patch-bumped and a major-bumped build of every driver. + # build-rotation-images.sh derives the versions from the binary, + # so this workflow knows nothing about drivers (SPEC §E2E + # harness #5). The images are not digest-pinned or published. - name: Build and push driver-version rotation images - run: | - set -euxo pipefail - for v in 0.1.1 1.0.0; do - scripts/build-image.sh "dv-${v}" "./oci-dv-${v}" \ - -X github.com/Yolean/buckety-controller/pkg/drivers/kadm.version=${v} \ - -X github.com/Yolean/buckety-controller/pkg/drivers/s3.version=${v} \ - -X github.com/Yolean/buckety-controller/pkg/drivers/gcs.version=${v} - crane push --insecure "./oci-dv-${v}" \ - "localhost:5000/yolean/buckety-controller:dv-${v}" - done + run: test/e2e/build-rotation-images.sh localhost:5000/yolean/buckety-controller - name: Run e2e harness env: @@ -144,12 +134,9 @@ jobs: # hostname so node containerd can pull it. PUSH_AS: localhost:5000/yolean/buckety-controller:${{ steps.release.outputs.tag }} CONTROLLER_IMAGE: k3d-buckety-registry:5000/yolean/buckety-controller:${{ steps.release.outputs.tag }} - E2E_VERSION_BASE: 0.1.0 - E2E_VERSION_PATCH: 0.1.1 - E2E_VERSION_MAJOR: 1.0.0 E2E_IMAGE_BASE: k3d-buckety-registry:5000/yolean/buckety-controller:${{ steps.release.outputs.tag }} - E2E_IMAGE_PATCH: k3d-buckety-registry:5000/yolean/buckety-controller:dv-0.1.1 - E2E_IMAGE_MAJOR: k3d-buckety-registry:5000/yolean/buckety-controller:dv-1.0.0 + E2E_IMAGE_PATCH: k3d-buckety-registry:5000/yolean/buckety-controller:dv-patch + E2E_IMAGE_MAJOR: k3d-buckety-registry:5000/yolean/buckety-controller:dv-major run: ./test/e2e/run.sh - name: Surface controller logs on failure diff --git a/examples/gcs/driver-version/README.md b/examples/gcs/driver-version/README.md new file mode 100644 index 0000000..c204742 --- /dev/null +++ b/examples/gcs/driver-version/README.md @@ -0,0 +1,10 @@ +# gcs / driver-version + +**Scenario:** SPEC "Driver versioning" for the gcs driver, the +same image rotation as `examples/kadm/driver-version`. + +Patch bump auto-applies and advances `status.driverBuildVersion`; +major bump surfaces `DriverVersionIncompatible` and pauses +reconcile with `status.driverMajor` unchanged. The expected +versions derive from what the base image stamps, so the scenario +needs no knowledge of the driver's current version. diff --git a/examples/gcs/driver-version/assert.sh b/examples/gcs/driver-version/assert.sh new file mode 100644 index 0000000..7cf6cb4 --- /dev/null +++ b/examples/gcs/driver-version/assert.sh @@ -0,0 +1,6 @@ +#!/usr/bin/env bash +set -euo pipefail +. "${E2E_LIB:-$(cd "$(dirname "$0")/../../../test/e2e" && pwd)}/lib.sh" + +# Body in test/e2e/lib.sh: controller behaviour, identical for every driver. +driver_version_scenario pinned diff --git a/examples/gcs/driver-version/buckety.yaml b/examples/gcs/driver-version/buckety.yaml new file mode 100644 index 0000000..a65ba1e --- /dev/null +++ b/examples/gcs/driver-version/buckety.yaml @@ -0,0 +1,11 @@ +# yaml-language-server: $schema=https://raw.githubusercontent.com/Yolean/buckety-controller/main/schema/buckety-gcs.schema.json +apiVersion: buckety.yolean.se/v1alpha1 +kind: Buckety +metadata: + name: pinned +spec: + backend: gcs + retentionPolicy: Delete + defaultAccess: + role: ReadWrite + credentialsSecretName: pinned-bucket diff --git a/examples/gcs/driver-version/kustomization.yaml b/examples/gcs/driver-version/kustomization.yaml new file mode 100644 index 0000000..86932e6 --- /dev/null +++ b/examples/gcs/driver-version/kustomization.yaml @@ -0,0 +1,4 @@ +apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +resources: +- buckety.yaml diff --git a/examples/kadm/driver-version/README.md b/examples/kadm/driver-version/README.md index 1e0375e..a2a2a6a 100644 --- a/examples/kadm/driver-version/README.md +++ b/examples/kadm/driver-version/README.md @@ -25,21 +25,23 @@ maintainer pins a compatible binary or migrates the resource. **Harness requirements:** -The harness must build (or sideload) three versions of the -controller image, stamped via `-ldflags '-X -main.driverVersion=...'` per `contain.yaml`. The version -strings to use are conveyed to this scenario via the -`E2E_VERSION_BASE`, `E2E_VERSION_PATCH`, and -`E2E_VERSION_MAJOR` env vars (e.g. `0.1.0`, `0.1.1`, `1.0.0`). -The image refs that contain those versions come in via -`E2E_IMAGE_BASE`, `E2E_IMAGE_PATCH`, `E2E_IMAGE_MAJOR`. +The harness must build (or sideload) three controller images: +the base, and for every driver at version `X.Y.Z` a patch bump +`X.Y.Z+1` and a major bump `(X+1).0.0` +(`test/e2e/build-rotation-images.sh` derives these from +`buckety --version`). The image refs come in via +`E2E_IMAGE_BASE`, `E2E_IMAGE_PATCH`, `E2E_IMAGE_MAJOR`; the +scenario derives its expected versions from the +`status.driverBuildVersion` the base image stamps, so nothing in +the harness needs to know a driver's current version. **Assertions** (`assert.sh`): 1. After initial apply: `status.driverMajor` matches the - major of `E2E_VERSION_BASE`. -2. After patch-rotate: `status.driverBuildVersion` == - `E2E_VERSION_PATCH`; `driverMajor` unchanged; Ready=True. + major of `status.driverBuildVersion`. +2. After patch-rotate: `status.driverBuildVersion` is the base + with its patch number incremented; `driverMajor` unchanged; + Ready=True. 3. After major-rotate: `DriverVersionIncompatible=True`; `driverMajor` unchanged; Ready=False. 4. After restore: Ready=True again. diff --git a/examples/kadm/driver-version/assert.sh b/examples/kadm/driver-version/assert.sh index 3a63d11..7cf6cb4 100755 --- a/examples/kadm/driver-version/assert.sh +++ b/examples/kadm/driver-version/assert.sh @@ -2,62 +2,5 @@ set -euo pipefail . "${E2E_LIB:-$(cd "$(dirname "$0")/../../../test/e2e" && pwd)}/lib.sh" -# This scenario needs three controller images with rotated driver -# versions (see .github/workflows/e2e.yaml "rotation images" step -# for the build recipe). Without them there is nothing meaningful -# to assert - switching the Deployment to a nonexistent image would -# wedge the controller for every scenario after this one - so skip -# loudly rather than fail. -if [[ -z "${E2E_IMAGE_BASE:-}" || -z "${E2E_IMAGE_PATCH:-}" || -z "${E2E_IMAGE_MAJOR:-}" ]]; then - log "driver-version SKIPPED: E2E_IMAGE_BASE/PATCH/MAJOR not set (CI provides these; build rotated images to run locally)" - exit 0 -fi -: "${E2E_VERSION_BASE:?must be set alongside E2E_IMAGE_BASE, e.g. 0.1.0}" -: "${E2E_VERSION_PATCH:?must be set alongside E2E_IMAGE_PATCH, e.g. 0.1.1}" -: "${E2E_VERSION_MAJOR:?must be set alongside E2E_IMAGE_MAJOR, e.g. 1.0.0}" - -set_image() { - local img="$1" - log "switching controller image to $img" - kcg -n "$E2E_CONTROLLER_NS" set image deploy/buckety-controller "controller=$img" - kcg -n "$E2E_CONTROLLER_NS" rollout status deploy/buckety-controller --timeout=60s -} - -restore() { set_image "$E2E_IMAGE_BASE"; } -trap restore EXIT - -set_image "$E2E_IMAGE_BASE" -wait_ready buckety/pinned 120s - -base_major="${E2E_VERSION_BASE%%.*}" -stamped_major="$(kc get buckety/pinned -o jsonpath='{.status.driverMajor}')" -[[ "$stamped_major" == "$base_major" ]] \ - || fail "status.driverMajor=$stamped_major, expected $base_major" - -build_version="$(kc get buckety/pinned -o jsonpath='{.status.driverBuildVersion}')" -[[ "$build_version" == "$E2E_VERSION_BASE" ]] \ - || fail "status.driverBuildVersion=$build_version, expected $E2E_VERSION_BASE" - -# Patch bump: auto-applied; buildVersion advances; major unchanged. -set_image "$E2E_IMAGE_PATCH" -for _ in $(seq 1 30); do - bv="$(kc get buckety/pinned -o jsonpath='{.status.driverBuildVersion}' 2>/dev/null || echo)" - [[ "$bv" == "$E2E_VERSION_PATCH" ]] && break - sleep 2 -done -[[ "$bv" == "$E2E_VERSION_PATCH" ]] \ - || fail "after patch-rotate, driverBuildVersion=$bv, expected $E2E_VERSION_PATCH" -[[ "$(kc get buckety/pinned -o jsonpath='{.status.driverMajor}')" == "$base_major" ]] \ - || fail "driverMajor changed after patch bump" -[[ "$(condition_status buckety/pinned Ready)" == "True" ]] \ - || fail "Ready not True after patch-rotate" - -# Major bump: incompatible; surfaces DriverVersionIncompatible. -set_image "$E2E_IMAGE_MAJOR" -wait_condition buckety/pinned DriverVersionIncompatible True 90s -[[ "$(kc get buckety/pinned -o jsonpath='{.status.driverMajor}')" == "$base_major" ]] \ - || fail "driverMajor changed under major bump" -[[ "$(condition_status buckety/pinned Ready)" == "False" ]] \ - || fail "Ready should be False under DriverVersionIncompatible" - -log "driver-version PASS" +# Body in test/e2e/lib.sh: controller behaviour, identical for every driver. +driver_version_scenario pinned diff --git a/examples/s3/driver-version/README.md b/examples/s3/driver-version/README.md index e83d0a8..b0ddc3f 100644 --- a/examples/s3/driver-version/README.md +++ b/examples/s3/driver-version/README.md @@ -1,8 +1,8 @@ # s3 / driver-version **Scenario:** SPEC "Driver versioning" for the s3 driver, same -image-rotation shape as `examples/kadm/driver-version` (the CI -rotation images inject both drivers' version vars). +image-rotation shape as `examples/kadm/driver-version` (the +rotation images bump every driver's version). Patch bump auto-applies and advances `status.driverBuildVersion`; major bump surfaces `DriverVersionIncompatible` and pauses diff --git a/examples/s3/driver-version/assert.sh b/examples/s3/driver-version/assert.sh index 3a63d11..7cf6cb4 100755 --- a/examples/s3/driver-version/assert.sh +++ b/examples/s3/driver-version/assert.sh @@ -2,62 +2,5 @@ set -euo pipefail . "${E2E_LIB:-$(cd "$(dirname "$0")/../../../test/e2e" && pwd)}/lib.sh" -# This scenario needs three controller images with rotated driver -# versions (see .github/workflows/e2e.yaml "rotation images" step -# for the build recipe). Without them there is nothing meaningful -# to assert - switching the Deployment to a nonexistent image would -# wedge the controller for every scenario after this one - so skip -# loudly rather than fail. -if [[ -z "${E2E_IMAGE_BASE:-}" || -z "${E2E_IMAGE_PATCH:-}" || -z "${E2E_IMAGE_MAJOR:-}" ]]; then - log "driver-version SKIPPED: E2E_IMAGE_BASE/PATCH/MAJOR not set (CI provides these; build rotated images to run locally)" - exit 0 -fi -: "${E2E_VERSION_BASE:?must be set alongside E2E_IMAGE_BASE, e.g. 0.1.0}" -: "${E2E_VERSION_PATCH:?must be set alongside E2E_IMAGE_PATCH, e.g. 0.1.1}" -: "${E2E_VERSION_MAJOR:?must be set alongside E2E_IMAGE_MAJOR, e.g. 1.0.0}" - -set_image() { - local img="$1" - log "switching controller image to $img" - kcg -n "$E2E_CONTROLLER_NS" set image deploy/buckety-controller "controller=$img" - kcg -n "$E2E_CONTROLLER_NS" rollout status deploy/buckety-controller --timeout=60s -} - -restore() { set_image "$E2E_IMAGE_BASE"; } -trap restore EXIT - -set_image "$E2E_IMAGE_BASE" -wait_ready buckety/pinned 120s - -base_major="${E2E_VERSION_BASE%%.*}" -stamped_major="$(kc get buckety/pinned -o jsonpath='{.status.driverMajor}')" -[[ "$stamped_major" == "$base_major" ]] \ - || fail "status.driverMajor=$stamped_major, expected $base_major" - -build_version="$(kc get buckety/pinned -o jsonpath='{.status.driverBuildVersion}')" -[[ "$build_version" == "$E2E_VERSION_BASE" ]] \ - || fail "status.driverBuildVersion=$build_version, expected $E2E_VERSION_BASE" - -# Patch bump: auto-applied; buildVersion advances; major unchanged. -set_image "$E2E_IMAGE_PATCH" -for _ in $(seq 1 30); do - bv="$(kc get buckety/pinned -o jsonpath='{.status.driverBuildVersion}' 2>/dev/null || echo)" - [[ "$bv" == "$E2E_VERSION_PATCH" ]] && break - sleep 2 -done -[[ "$bv" == "$E2E_VERSION_PATCH" ]] \ - || fail "after patch-rotate, driverBuildVersion=$bv, expected $E2E_VERSION_PATCH" -[[ "$(kc get buckety/pinned -o jsonpath='{.status.driverMajor}')" == "$base_major" ]] \ - || fail "driverMajor changed after patch bump" -[[ "$(condition_status buckety/pinned Ready)" == "True" ]] \ - || fail "Ready not True after patch-rotate" - -# Major bump: incompatible; surfaces DriverVersionIncompatible. -set_image "$E2E_IMAGE_MAJOR" -wait_condition buckety/pinned DriverVersionIncompatible True 90s -[[ "$(kc get buckety/pinned -o jsonpath='{.status.driverMajor}')" == "$base_major" ]] \ - || fail "driverMajor changed under major bump" -[[ "$(condition_status buckety/pinned Ready)" == "False" ]] \ - || fail "Ready should be False under DriverVersionIncompatible" - -log "driver-version PASS" +# Body in test/e2e/lib.sh: controller behaviour, identical for every driver. +driver_version_scenario pinned diff --git a/test/e2e/build-rotation-images.sh b/test/e2e/build-rotation-images.sh new file mode 100755 index 0000000..c19296b --- /dev/null +++ b/test/e2e/build-rotation-images.sh @@ -0,0 +1,37 @@ +#!/usr/bin/env bash +# +# Build and push the two controller images the driver-version +# scenario rotates through: +# +# build-rotation-images.sh +# +# e.g. build-rotation-images.sh localhost:5000/yolean/buckety-controller +# pushes :dv-patch and :dv-major. +# +# Versions are derived from the binary itself (`buckety --version`) +# so no file needs to know what each driver's current version is: +# for every driver X.Y.Z, dv-patch carries X.Y.(Z+1) and dv-major +# (X+1).0.0. The scenario derives the same expectations from the +# status.driverBuildVersion the base image stamps. + +set -euo pipefail + +REF="${1:?usage: build-rotation-images.sh }" +here() { cd "$(dirname "${BASH_SOURCE[0]}")" && pwd; } +cd "$(here)/../.." + +# "driver " lines -> -X flags per rotation. +patch_flags=() major_flags=() +while read -r kind name ver; do + [[ "$kind" == driver ]] || continue + IFS=. read -r x y z <<<"$ver" + pkg="github.com/Yolean/buckety-controller/pkg/drivers/${name}.version" + patch_flags+=("-X" "${pkg}=${x}.${y}.$((z + 1))") + major_flags+=("-X" "${pkg}=$((x + 1)).0.0") +done < <(go run ./cmd/buckety --version) +(( ${#patch_flags[@]} > 0 )) || { echo "no drivers reported by --version" >&2; exit 1; } + +scripts/build-image.sh dv-patch ./oci-dv-patch "${patch_flags[@]}" >/dev/null +crane push --insecure ./oci-dv-patch "${REF}:dv-patch" +scripts/build-image.sh dv-major ./oci-dv-major "${major_flags[@]}" >/dev/null +crane push --insecure ./oci-dv-major "${REF}:dv-major" diff --git a/test/e2e/lib.sh b/test/e2e/lib.sh index 4aa9e41..1b40213 100755 --- a/test/e2e/lib.sh +++ b/test/e2e/lib.sh @@ -495,6 +495,59 @@ backend_stickiness_scenario() { log "backend-stickiness PASS" } +# driver_version_scenario +# SPEC §End-to-end coverage #9. Needs E2E_IMAGE_BASE/PATCH/MAJOR +# (test/e2e/build-rotation-images.sh); skips loudly otherwise - +# switching to a nonexistent image would wedge the controller for +# every scenario after this one. The base driver version is +# whatever the base image stamps, and the rotation images derive +# from the same binary (X.Y.Z+1 and (X+1).0.0 per driver), so the +# expectations derive from status too: nothing here knows what +# any driver's current version is. +driver_version_scenario() { + local name="$1" + if [[ -z "${E2E_IMAGE_BASE:-}" || -z "${E2E_IMAGE_PATCH:-}" || -z "${E2E_IMAGE_MAJOR:-}" ]]; then + log "driver-version SKIPPED: E2E_IMAGE_BASE/PATCH/MAJOR not set (CI provides these; test/e2e/build-rotation-images.sh builds them locally)" + exit 0 + fi + trap 'controller_set_image "$E2E_IMAGE_BASE"' EXIT + + controller_set_image "$E2E_IMAGE_BASE" + wait_ready "buckety/$name" 120s + local base x y z + base="$(kc get "buckety/$name" -o jsonpath='{.status.driverBuildVersion}')" + IFS=. read -r x y z <<<"$base" + [[ -n "$z" ]] || fail "status.driverBuildVersion '$base' is not X.Y.Z" + local patch="$x.$y.$((z + 1))" + [[ "$(kc get "buckety/$name" -o jsonpath='{.status.driverMajor}')" == "$x" ]] \ + || fail "status.driverMajor is not the major of driverBuildVersion $base" + + # Patch bump: auto-applied; buildVersion advances; major unchanged. + controller_set_image "$E2E_IMAGE_PATCH" + local bv="" + for _ in $(seq 1 30); do + bv="$(kc get "buckety/$name" -o jsonpath='{.status.driverBuildVersion}' 2>/dev/null || echo)" + [[ "$bv" == "$patch" ]] && break + sleep 2 + done + [[ "$bv" == "$patch" ]] \ + || fail "after patch-rotate, driverBuildVersion=$bv, expected $patch" + [[ "$(kc get "buckety/$name" -o jsonpath='{.status.driverMajor}')" == "$x" ]] \ + || fail "driverMajor changed after patch bump" + [[ "$(condition_status "buckety/$name" Ready)" == "True" ]] \ + || fail "Ready not True after patch-rotate" + + # Major bump: incompatible; surfaces DriverVersionIncompatible. + controller_set_image "$E2E_IMAGE_MAJOR" + wait_condition "buckety/$name" DriverVersionIncompatible True 90s + [[ "$(kc get "buckety/$name" -o jsonpath='{.status.driverMajor}')" == "$x" ]] \ + || fail "driverMajor changed under major bump" + [[ "$(condition_status "buckety/$name" Ready)" == "False" ]] \ + || fail "Ready should be False under DriverVersionIncompatible" + + log "driver-version PASS" +} + # misconfigured_startup_scenario # SPEC §End-to-end coverage #10. Every /broken-configs/ # listed in /expectations.txt is installed as the controller diff --git a/test/e2e/run.sh b/test/e2e/run.sh index 05d0c66..f52e545 100755 --- a/test/e2e/run.sh +++ b/test/e2e/run.sh @@ -27,11 +27,10 @@ # `kubectl logs` (SPEC.md section "E2E harness # and parity" #3). # E2E_IMAGE_BASE / E2E_IMAGE_PATCH / E2E_IMAGE_MAJOR -# E2E_VERSION_BASE / E2E_VERSION_PATCH / E2E_VERSION_MAJOR -# Controller images built with rotated driver -# versions, for the driver-version scenario. CI -# builds and pushes these; when unset the -# scenario logs SKIPPED and exits 0. +# Controller images built with the drivers' +# versions rotated (build-rotation-images.sh), +# for the driver-version scenario. When unset +# the scenario logs SKIPPED and exits 0. # KUBECONFIG Cluster the harness writes to. # CONTROLLER_NS Namespace the buckety-controller runs in. # Default: buckety. @@ -245,9 +244,6 @@ run_scenario() { E2E_KAFKA_BOOTSTRAP="${E2E_KAFKA_BOOTSTRAP:-redpanda.redpanda.svc.cluster.local:9093}" \ E2E_ORIGINAL_CONFIG="$E2E_ORIGINAL_CONFIG" \ E2E_RENAMED_CONFIG="$E2E_RENAMED_CONFIG" \ - E2E_VERSION_BASE="${E2E_VERSION_BASE:-}" \ - E2E_VERSION_PATCH="${E2E_VERSION_PATCH:-}" \ - E2E_VERSION_MAJOR="${E2E_VERSION_MAJOR:-}" \ E2E_IMAGE_BASE="${E2E_IMAGE_BASE:-}" \ E2E_IMAGE_PATCH="${E2E_IMAGE_PATCH:-}" \ E2E_IMAGE_MAJOR="${E2E_IMAGE_MAJOR:-}" \ From bfe1e720c34d823a04775275168fc2f55390c076 Mon Sep 17 00:00:00 2001 From: Staffan Olsson Date: Fri, 4 Sep 2026 08:17:40 +0200 Subject: [PATCH 17/21] e2e: kadm oob-drift grows the partition count on the broker The unreconcilable half of the scenario patched spec.parameters from 3 to 2 partitions - a spec mutation, which is what parameter-mutation covers, not SPEC #4's "mutate the backend directly". Its README conceded the substitution. The driver compares the live partition count on every reconcile, so the real out-of-band case works: `rpk topic add-partitions` grows the topic to 5 while the spec still says 3, Kafka cannot shrink it back, and the controller surfaces ParameterDrift instead of trying. The retention re-check loop is bounded to 90s like the s3 and gcs copies, instead of 60 helper-pod round trips. Co-Authored-By: Claude Fable 5.1 --- examples/kadm/oob-drift/README.md | 17 ++++++++--------- examples/kadm/oob-drift/assert.sh | 22 +++++++++++++--------- 2 files changed, 21 insertions(+), 18 deletions(-) diff --git a/examples/kadm/oob-drift/README.md b/examples/kadm/oob-drift/README.md index c021491..495e892 100644 --- a/examples/kadm/oob-drift/README.md +++ b/examples/kadm/oob-drift/README.md @@ -15,17 +15,16 @@ place, e.g. an attempted partition-count shrink). - Direct broker call: `rpk topic alter-config --set retention.ms=1` (reconcilable in-place; controller reapplies the spec's value). -- Direct broker call: attempt to shrink to fewer partitions - via an out-of-band ALTER (not actually possible on Kafka, - but the SPEC scenario covers the assertion shape for it). - For v1alpha1, simulate by reducing `partitions` in spec from - 3 to 2: controller surfaces `ParameterDrift=True` and pauses, - rather than attempting an unsafe shrink. +- Direct broker call: `rpk topic add-partitions --num 2` + grows the topic from 3 to 5 partitions. Kafka cannot shrink a + partition count, so the spec's 3 can never be re-applied: the + controller surfaces `ParameterDrift=True` and pauses rather + than attempting an unsafe shrink. **Assertions** (`assert.sh`): 1. After the out-of-band retention change, the controller reapplies the spec value within the reconcile window. -2. After reducing `partitions` in the spec from 3 to 2, the - Buckety surfaces `ParameterDrift=True` with a message - naming `partitions`. `Ready` flips to False. +2. After the out-of-band partition growth, the Buckety + surfaces `ParameterDrift=True` with a message naming + `partitions`. `Ready` flips to False. diff --git a/examples/kadm/oob-drift/assert.sh b/examples/kadm/oob-drift/assert.sh index 4200afe..dbcd1ff 100755 --- a/examples/kadm/oob-drift/assert.sh +++ b/examples/kadm/oob-drift/assert.sh @@ -10,14 +10,16 @@ bootstrap="$(secret_value drift-topic bootstrap)" # directly poke retention.ms=1 on the broker, then wait for the # controller's next periodic re-check to restore it. log "out-of-band: setting retention.ms=1 directly on broker" -kcg run -n "${E2E_KAFKA_NAMESPACE:-redpanda}" --rm -i --restart=Never --quiet \ +kc run --rm -i --restart=Never --quiet \ --image=ghcr.io/yolean/redpanda:v24.2.22@sha256:5132085d4fe35b0fd6ddedc7f0fe3d3ba7be12c5e3829e1a2b986cd41b1d3538 \ "rpk-oob-set-$RANDOM" -- \ topic alter-config "$topic_name" --set retention.ms=1 --brokers "$bootstrap" log "expecting controller to reconcile retention.ms back to 3600000" -for _ in $(seq 1 60); do - current="$(kcg run -n "${E2E_KAFKA_NAMESPACE:-redpanda}" --rm -i --restart=Never --quiet \ +deadline=$(( $(date +%s) + 90 )) +current="" +while (( $(date +%s) < deadline )); do + current="$(kc run --rm -i --restart=Never --quiet \ --image=ghcr.io/yolean/redpanda:v24.2.22@sha256:5132085d4fe35b0fd6ddedc7f0fe3d3ba7be12c5e3829e1a2b986cd41b1d3538 \ "rpk-oob-check-$RANDOM" -- \ topic describe -c "$topic_name" --brokers "$bootstrap" 2>/dev/null \ @@ -26,13 +28,15 @@ for _ in $(seq 1 60); do sleep 5 done [[ "$current" == "3600000" ]] \ - || fail "controller did not reapply retention.ms=3600000 (broker reports $current)" + || fail "controller did not reapply retention.ms=3600000 within 90s (broker reports $current)" -# Spec-level unsafe change. Reduce partitions in the spec; the -# driver cannot shrink Kafka partitions in place, so surfaces -# ParameterDrift instead of attempting destructive action. -log "spec change: reducing partitions from 3 to 2 (unsafe)" -kc patch buckety/drift --type=merge -p '{"spec":{"parameters":{"partitions":"2"}}}' +# Out-of-band unreconcilable change: grow the partition count on +# the broker. Kafka cannot shrink partitions, so the spec's 3 can +# never be re-applied; the controller surfaces ParameterDrift and +# waits for a human instead of attempting anything destructive. +log "out-of-band: adding 2 partitions on the broker (3 -> 5)" +rpk_run oob-parts topic add-partitions "$topic_name" --num 2 --brokers "$bootstrap" Date: Fri, 4 Sep 2026 08:17:40 +0200 Subject: [PATCH 18/21] docs: reconcile README, SPEC and scaffolding with what ships MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - README said two drivers; there are three. It recommended --enable-webhook=false for clusters without cert-manager while the release-tls-selfsigned composition and its README call that strongly discouraged; it now points at the certgen path, and says that webhook-disabled mode also drops driver-level parameter immutability, which nothing else enforced. It listed adoption as a non-goal, as did SPEC's non-goals, while SPEC §Adoption specifies it and the code implements it. Its drift sentence said the opposite of SPEC. - SPEC used role ReadOnly (the enum is Reader), a main.driverVersion ldflag that never existed, a per-key error map ValidateParameters never returned, and a status.driverVersion field that is driverMajor + driverBuildVersion. Its Ready clause required the Buckety to wait for access-side Secrets, which the access reconciler's own wait for Buckety Ready makes circular; the clause now states the implemented split. Two observable behaviours were undocumented: message-only status changes are not written (a88198d), and the reasons and events the controller actually emits. - SCAFFOLDING's first sentence on webhook TLS still described webhook-disabled mode as the no-cert-manager answer. Co-Authored-By: Claude Fable 5.1 --- README.md | 45 +++++++++++++++++++------------------ SPEC.md | 54 +++++++++++++++++++++++++++++++-------------- docs/SCAFFOLDING.md | 4 ++-- 3 files changed, 62 insertions(+), 41 deletions(-) diff --git a/README.md b/README.md index cfc6e0c..bb1fe5c 100644 --- a/README.md +++ b/README.md @@ -4,8 +4,8 @@ A small in-cluster operator that provisions named resources on backing services (Kafka topics, S3 buckets) and mints `secretKeyRef`-friendly credentials Secrets for consumers. -> **Status: v1alpha1 shipped.** Both drivers (`kadm`, `s3`) are -> implemented and covered by the e2e suite in CI. Every commit on +> **Status: v1alpha1 shipped.** Three drivers (`kadm`, `s3`, +> `gcs`) are implemented and covered by the e2e suite in CI. Every commit on > `main` pins a reproducible image build in > [`deploy/kustomize/release/`](./deploy/kustomize/release), and > the same digest is pushed to `ghcr.io/yolean/buckety-controller` @@ -45,8 +45,9 @@ here. The deviations are operational, not architectural: - **Recovery.** Standard Kubernetes status conditions (`Ready`, `Reconciling`, `BackendUnavailable`, `ParameterDrift`, `BlockedByAccesses`) cover the diagnostic - surface. Out-of-band drift on the backend is surfaced - explicitly rather than silently re-reconciled. + surface. Out-of-band drift on the backend is reapplied where + the driver can, and surfaced as `ParameterDrift` where it + cannot (a partition-count shrink, a bucket location). - **Portability.** Backend choice (VersityGW vs MinIO vs AWS S3) is a deploy-time cluster-maintainer decision, not part of the API. Consumer YAML moves between clusters with different @@ -140,23 +141,24 @@ The base includes a `ValidatingWebhookConfiguration` whose CA is injected by cert-manager (`cert-manager.io/inject-ca-from`), and the webhook server needs a TLS Secret named `buckety-controller-webhook-tls`. On clusters without -cert-manager the controller cannot start in its default shape. -The cert-less recipe: - -1. Vendor `deploy/kustomize/crd/` and `deploy/kustomize/controller/` - separately (skip the composite base). -2. Drop `webhook.yaml` from your controller overlay (or patch the - `ValidatingWebhookConfiguration` out). -3. Pass `--enable-webhook=false` to the controller via a - Deployment args patch. No `Certificate`, no TLS Secret needed. - -In webhook-disabled mode, per-driver parameter and resolved-name -validation runs in the reconcile loop and surfaces as -`Ready=False` with reason `InvalidParameters` or `NameInvalid` on -the resource's status (plus a Warning Event), instead of failing -the apply. CRD-level CEL still enforces `spec.backend` / -`spec.name` / `bucketyRef` / `credentialsSecretName` immutability -and the `role` / `retentionPolicy` enums regardless. +cert-manager, vendor `deploy/kustomize/release-tls-selfsigned/` +instead of `release/`: it adds two `kube-webhook-certgen` Jobs +that mint a self-signed serving certificate into that Secret and +patch the webhook's `caBundle`, with no external infrastructure +([`deploy/kustomize/webhook-certgen/README.md`](./deploy/kustomize/webhook-certgen/README.md)). +The e2e suite deploys the controller this way. + +Running with `--enable-webhook=false` (and `webhook.yaml` dropped +from the overlay) is a last resort, not a recommended shape: +per-driver parameter and resolved-name validation then runs in +the reconcile loop and surfaces as `Ready=False` with reason +`InvalidParameters` or `NameInvalid` (plus a Warning Event) +instead of failing the apply, and driver-level parameter +immutability (a gcs bucket's `location`, its `serviceAccount`) +is not enforced at all. CRD-level CEL still enforces +`spec.backend` / `spec.name` / `bucketyRef` / +`credentialsSecretName` immutability and the `role` / +`retentionPolicy` enums regardless. ## Configure @@ -494,7 +496,6 @@ The full list is in [`SPEC.md`](./SPEC.md). Highlights: - No MySQL driver. - No cross-namespace `bucketyRef`. - No hot-reload of `buckety-controller.yaml`. -- No adoption of pre-existing backing resources. ## Filing issues diff --git a/SPEC.md b/SPEC.md index 100cce6..a2f40d5 100644 --- a/SPEC.md +++ b/SPEC.md @@ -261,7 +261,7 @@ spec: bucketyRef: name: orders credentialsSecretName: orders-reader - role: ReadOnly + role: Reader parameters: consumerGroupPrefix: "tenant1-orders-reporting-" @@ -278,7 +278,7 @@ credentials drawn from the backend's root config. The CRD shape already supports per-access scoping for v1alpha2 — drivers are expected to ignore role/parameter values they don't yet implement and surface a `ScopingNotImplemented` condition rather -than silently treating ReadOnly as ReadWrite. +than silently treating Reader as ReadWrite. ## Implicit access (`defaultAccess`) @@ -377,8 +377,8 @@ creation. Audit the fleet before adding one. Each driver carries a SemVer (`major.minor.patch`) advertised by the compiled binary (typically injected at build time via -`-ldflags '-X main.driverVersion=...'`; see *Build and -distribution*). At first reconcile the resource's +`-ldflags '-X github.com/Yolean/buckety-controller/pkg/drivers/.version=...'`; +see *Build and distribution*). At first reconcile the resource's `status.driverMajor` is stamped from this version and is **sticky** thereafter, exactly like `status.backend`. The full running version is mirrored to `status.driverBuildVersion` on @@ -644,10 +644,12 @@ call, guided by the chosen operator SDK. Idempotent for the same `(Buckety, BucketyAccess)` pair. - `RevokeAccess` — removes the backend-side principal (no-op in v1alpha1), idempotent on NotFound. -- `ValidateParameters` — used by the admission webhook; returns - a per-key error map. -- `Version()` — returns the driver's SemVer for - `status.driverVersion` stamping. +- `ValidateParameters` — used by the admission webhook and, with + the webhook disabled, by the reconciler; returns an error + naming the offending key. +- `Version()` — returns the driver's SemVer, from which + `status.driverMajor` (sticky) and `status.driverBuildVersion` + are stamped. - Driver also exposes its OpenAPI schemas separately (see Schemas). @@ -667,13 +669,21 @@ specified: (Kafka partition count can grow but not shrink), the driver surfaces `ParameterDrift` and waits for human resolution. - `status.observedGeneration` advances when `metadata.generation` - has been fully reconciled, including downstream Secret state - (not just the backend resource). Until then, - `status.observedGeneration` lags `metadata.generation` and - `Ready=False`. -- The controller MUST be honest in `status.conditions`: - `Ready=False` until both the backend resource AND any - access-side Secrets are in sync. + has been fully reconciled. Until then it lags + `metadata.generation` and `Ready=False`. +- The controller MUST be honest in `status.conditions`. A + `Buckety` is `Ready` when its backend resource matches the spec + and its implicit access (if any) has been materialised; the + access-side Secret has its own `Ready` on the `BucketyAccess`. + The two cannot depend on each other: an access waits for its + `Buckety` to be `Ready` before minting, so a `Buckety` that + waited for the Secret would never get there. +- Condition messages may lag the latest attempt: a status patch + is written only when something other than a message changed, + because a message carrying a per-attempt token (a provider's + request or error id) would otherwise re-trigger the controller + through its own watch at the provider's answer rate. The + condition's status and reason are always current. ## Off the data path @@ -1181,7 +1191,6 @@ and the corresponding GHA secret. Secret output > gcs driver), but role scoping remains unimplemented. - Cross-namespace `bucketyRef`. -- Adopting backing resources that already exist outside Buckety. - Quota enforcement. - Hot-reload of `buckety-controller.yaml` (envsubst is startup-only; rotating credentials requires re-rolling the @@ -1223,7 +1232,18 @@ contract above: 5. **Status conditions catalog.** Minimum required: `Ready`, `Reconciling`, `BackendUnavailable`, `DriverVersionIncompatible`, `ParameterDrift`, `BlockedByAccesses`, `ScopingNotImplemented`. - Maintainer can add more. + Maintainer can add more. Implemented `Ready=False` reasons + beyond the failure ones: `Provisioning` (the backend needs a + moment, e.g. IAM propagation of a fresh service account; + prompt requeue, bounded), `DeletingContents` and + `RevokingAccesses` (teardown in progress), `SecretConflict` + (the credentials Secret exists and is not ours), + `BackendResourceExists` (adoption refused), `InspectFailed`. + Events beyond condition transitions: `Adopted`, + `RetainedOnDelete`, `DeletionBlocked`, and `PrincipalReplaced` + on a `BucketyAccess` whose credential was re-minted and the + previous one revoked - the signal to restart consumers that + loaded the Secret once. ## What to do before writing code diff --git a/docs/SCAFFOLDING.md b/docs/SCAFFOLDING.md index d23aa7e..37e213c 100644 --- a/docs/SCAFFOLDING.md +++ b/docs/SCAFFOLDING.md @@ -74,8 +74,8 @@ versioning. ## Webhook TLS -cert-manager when present; webhook-disabled mode for platforms -without it. The kustomize base annotates the +cert-manager when present; the shipped certgen Jobs for +platforms without it. The kustomize base annotates the `ValidatingWebhookConfiguration` with `cert-manager.io/inject-ca-from: buckety/buckety-controller-webhook`; overlays that include `deploy/kustomize/controller/webhook.yaml` From 1c29abdef8d362f4fc6700b65d178d01a58fbf12 Mon Sep 17 00:00:00 2001 From: Staffan Olsson Date: Fri, 4 Sep 2026 08:19:29 +0200 Subject: [PATCH 19/21] e2e: deploying the controller is idempotent across harness invocations Running run.sh twice against one cluster (one implementation per invocation, which local development wants) left the second run with the first run's controller config and, briefly, a webhook whose CA bundle nobody trusted: apply does not restart a Deployment whose spec did not change, does not re-run a completed certgen Job, and strips the caBundle that the webhook-fallback scenario's restore had written into last-applied. The certgen Jobs are now recreated on every deploy, the patch Job is awaited before any scenario runs, and the controller is restarted regardless of whether the Deployment changed. Co-Authored-By: Claude Fable 5.1 --- test/e2e/run.sh | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/test/e2e/run.sh b/test/e2e/run.sh index f52e545..2122063 100755 --- a/test/e2e/run.sh +++ b/test/e2e/run.sh @@ -143,8 +143,20 @@ images: newTag: "$tag" EOF log "deploying controller as $CONTROLLER_IMAGE" + # Idempotent across invocations against one cluster. A previous + # run may have left the certgen Jobs behind (they self-delete + # after five minutes) and apply does not re-run an unchanged + # Job, while re-applying the webhook configuration can drop the + # caBundle a scenario's restore wrote into last-applied - so the + # Jobs are recreated every time and the patch must land before + # the webhook is trusted. The config Secret is applied before + # this and is not part of the overlay, so an unchanged + # Deployment would keep the config its Pod started with: restart + # regardless. + kubectl -n "$CONTROLLER_NS" delete job buckety-webhook-certgen-create buckety-webhook-certgen-patch --ignore-not-found kubectl apply -k "$HERE/.work" - kubectl -n "$CONTROLLER_NS" rollout status deploy/buckety-controller --timeout=180s + kubectl -n "$CONTROLLER_NS" wait --for=condition=complete job/buckety-webhook-certgen-patch --timeout=180s + restart_controller } # restart_controller picks up a swapped config. Deleting the From 069a84c9fff1a7bba5581346048b1f63577ff929 Mon Sep 17 00:00:00 2001 From: Staffan Olsson Date: Fri, 4 Sep 2026 08:37:14 +0200 Subject: [PATCH 20/21] e2e: SCENARIOS filter for iterating on one scenario locally Re-running a single failing scenario meant running the whole implementation (a dozen scenarios, several controller rollouts). SCENARIOS=adoption,backend-stickiness restricts discovery to the named scenario directories; unset, nothing changes. Co-Authored-By: Claude Fable 5.1 --- test/e2e/run.sh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/e2e/run.sh b/test/e2e/run.sh index 2122063..df68825 100755 --- a/test/e2e/run.sh +++ b/test/e2e/run.sh @@ -10,6 +10,10 @@ # Inputs (env): # IMPLEMENTATIONS Comma-separated. Default: redpanda,versitygw,minio,fakegcs. # Each maps via $IMPL_DRIVER below to a driver. +# SCENARIOS Optional comma-separated scenario directory +# names (e.g. adoption,backend-stickiness) to run +# instead of every scenario; for iterating on one +# locally. # CONTROLLER_IMAGE Cluster-side image reference the deployment is # patched to before rollout. Required. # OCI_DIR Optional local OCI layout to push before applying. @@ -302,6 +306,7 @@ for impl in "${impl_list[@]}"; do for scenario in "${scenario_list[@]}"; do [[ -z "$scenario" ]] && continue scenario_matches_impl "$scenario" "$impl" "$driver" || continue + [[ -z "${SCENARIOS:-}" || ",$SCENARIOS," == *",$(basename "$scenario"),"* ]] || continue if run_scenario "$scenario" "$impl" "$driver"; then results["$impl/$scenario"]=PASS else From 0de962fcb89197ae82ebdbd7e0b933ba9e377756 Mon Sep 17 00:00:00 2001 From: Staffan Olsson Date: Fri, 4 Sep 2026 08:45:16 +0200 Subject: [PATCH 21/21] release: pin the improvements build Every commit since the fix branch that touched the binary rides on this pin; CI rebuilds and asserts the digest. Co-Authored-By: Claude Fable 5.1 --- deploy/kustomize/release/kustomization.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/deploy/kustomize/release/kustomization.yaml b/deploy/kustomize/release/kustomization.yaml index 6c6dce0..370a865 100644 --- a/deploy/kustomize/release/kustomization.yaml +++ b/deploy/kustomize/release/kustomization.yaml @@ -7,7 +7,7 @@ kind: Kustomization resources: - ../base images: -- digest: sha256:6cc01b8163ff6dee01768b5b2f02d5af9fffb4156e2f621ed362c86c16e5edab +- digest: sha256:19e3172abce9c8be53ea19fbcd60ffc3ec636b11c3457af56e829d063e877efe name: ghcr.io/yolean/buckety-controller newName: ghcr.io/yolean/buckety-controller - newTag: 20260904T052038Z + newTag: 20260904T064507Z