Repository navigation
Review all code using a newer model - #8
Merged
Merged
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…et 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…nfig 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…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 <noreply@anthropic.com>
…essages 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…se 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 <noreply@anthropic.com>
…figs 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 <noreply@anthropic.com>
…ned 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 <noreply@anthropic.com>
…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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bulk of this codebase was written with Opus 4.7 and it has proven to work well for the given scope, but I asked Fable to do a thorough code review and "look for ways to simplify and improve".