diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml index bae5190..90317bf 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 @@ -130,30 +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 - 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 - 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: @@ -164,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 @@ -214,26 +181,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/.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/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 8980e95..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 @@ -816,12 +826,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 @@ -1136,7 +1152,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 @@ -1175,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 @@ -1186,8 +1201,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 @@ -1217,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/cmd/buckety/main.go b/cmd/buckety/main.go index 956aa75..98f761d 100644 --- a/cmd/buckety/main.go +++ b/cmd/buckety/main.go @@ -134,36 +134,29 @@ 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"), + 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) } - 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) @@ -172,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/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/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 diff --git a/deploy/kustomize/release/kustomization.yaml b/deploy/kustomize/release/kustomization.yaml index e643889..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:0b083f26c3472d42e444a83fdaaf34948f123bec39c8c4271b4caf1275dd17ef +- digest: sha256:19e3172abce9c8be53ea19fbcd60ffc3ec636b11c3457af56e829d063e877efe name: ghcr.io/yolean/buckety-controller newName: ghcr.io/yolean/buckety-controller - newTag: 20260729T151707Z + newTag: 20260904T064507Z 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` 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/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/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/gcs/service-account/README.md b/examples/gcs/service-account/README.md index 846f185..25978ce 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, 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. + +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/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/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/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/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" /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/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/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/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/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 78e8ad3..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,29 +38,37 @@ 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 + // 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 } -// 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 +// 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 } - if c := meta.FindStatusCondition(baseConds, condType); c != nil && c.Status == status && c.Reason == condReason { - return - } - r.Recorder.Event(obj, eventType, eventReason, message) + return r.Client +} + +// 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 @@ -102,26 +109,38 @@ 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 // 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 @@ -130,13 +149,13 @@ 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. 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()) } @@ -154,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 } @@ -165,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", @@ -206,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 } @@ -234,52 +246,46 @@ 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) { + 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. - 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) + // 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.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 } @@ -292,27 +298,30 @@ 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 } -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. + // 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 @@ -327,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 { @@ -338,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 @@ -367,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 { @@ -397,22 +403,17 @@ 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) - setCond(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "BackendUnavailable", "deletion blocked", 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) + status.Event(r.Recorder, bky, base.Status.Conditions, "BackendUnavailable", metav1.ConditionTrue, unavailableReason, + corev1.EventTypeWarning, "DeletionBlocked", msg) + 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 @@ -422,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 } @@ -437,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 } @@ -463,6 +460,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 { @@ -503,19 +506,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 == "" { @@ -529,26 +529,37 @@ 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) - setCond(&bky.Status.Conditions, "Ready", metav1.ConditionFalse, "BackendUnavailable", "reconcile paused", bky.Generation) +func (r *Reconciler) surfaceBackendUnavailable(ctx context.Context, bky, base *bucketyv1.Buckety, reason, msg string) (ctrl.Result, error) { + status.Event(r.Recorder, bky, base.Status.Conditions, "BackendUnavailable", metav1.ConditionTrue, reason, + corev1.EventTypeWarning, "BackendUnavailable", msg) + 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) } +// 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 @@ -564,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) { @@ -588,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 33ac86d..a81960d 100644 --- a/pkg/controller/buckety/reconciler_test.go +++ b/pkg/controller/buckety/reconciler_test.go @@ -3,17 +3,21 @@ 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" 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" 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" ) @@ -76,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 { @@ -184,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) } @@ -201,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) } @@ -219,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) } @@ -246,8 +249,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 +262,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 +351,243 @@ 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) + } +} + +// 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) + } +} + +// 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) + } + } +} diff --git a/pkg/controller/bucketyaccess/reconciler.go b/pkg/controller/bucketyaccess/reconciler.go index 190097d..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 { @@ -190,15 +182,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 @@ -207,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 @@ -222,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) } @@ -250,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) } @@ -261,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) } @@ -286,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 } @@ -313,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 } @@ -329,6 +295,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 @@ -336,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 } @@ -348,19 +324,26 @@ 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 } + // 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 @@ -369,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) @@ -377,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 @@ -425,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 940364d..258a164 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" @@ -20,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 @@ -160,6 +103,7 @@ type grantRecorder struct { req *registry.GrantRequest data map[string][]byte principal string + minted bool revoked []string revokeErr error } @@ -177,7 +121,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) @@ -317,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() @@ -351,6 +295,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 +303,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 +313,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 +351,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 +366,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. @@ -493,3 +466,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/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/drivers/gcs/driver.go b/pkg/drivers/gcs/driver.go index c80b4cd..fd6eb8c 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 @@ -149,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"` } @@ -526,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 @@ -536,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 } @@ -544,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 } @@ -689,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, ", ")) } @@ -706,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 99c375d..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) } } @@ -165,15 +164,37 @@ 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). -func (d *Driver) ensureAccessKey(ctx context.Context, email string, existing map[string][]byte) ([]byte, string, error) { +// 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. +// +// 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 { @@ -183,30 +204,41 @@ 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 { - 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, false, 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. + } + 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 c4bddf8..d7fa26e 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"]) + } } } @@ -592,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) @@ -762,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/kadm/driver.go b/pkg/drivers/kadm/driver.go index 6c93811..c81adf5 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) @@ -182,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) } @@ -255,6 +255,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 +319,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 { diff --git a/pkg/drivers/registry/registry.go b/pkg/drivers/registry/registry.go index b112134..648380a 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 } @@ -186,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 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..a23906d 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 @@ -207,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{ 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, - }) -} 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 \ 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/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/lib.sh b/test/e2e/lib.sh index f019e02..1b40213 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" +} + +# 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 +# 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" +} 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 e4b2455..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. @@ -27,23 +31,19 @@ # `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. -# 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 +56,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 +68,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 +98,79 @@ 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" </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 +209,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 +220,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 @@ -254,11 +258,8 @@ run_scenario() { E2E_BACKEND_ZONE="${E2E_BACKEND_ZONE:-e2e}" \ E2E_KAFKA_NAMESPACE="${E2E_KAFKA_NAMESPACE:-redpanda}" \ E2E_KAFKA_BOOTSTRAP="${E2E_KAFKA_BOOTSTRAP:-redpanda.redpanda.svc.cluster.local:9093}" \ - E2E_ORIGINAL_CONFIG="$OVERLAYS_DIR/$impl/buckety-controller.yaml" \ - E2E_RENAMED_CONFIG="$OVERLAYS_DIR/$impl/buckety-controller.renamed.yaml" \ - E2E_VERSION_BASE="${E2E_VERSION_BASE:-}" \ - E2E_VERSION_PATCH="${E2E_VERSION_PATCH:-}" \ - E2E_VERSION_MAJOR="${E2E_VERSION_MAJOR:-}" \ + E2E_ORIGINAL_CONFIG="$E2E_ORIGINAL_CONFIG" \ + E2E_RENAMED_CONFIG="$E2E_RENAMED_CONFIG" \ E2E_IMAGE_BASE="${E2E_IMAGE_BASE:-}" \ E2E_IMAGE_PATCH="${E2E_IMAGE_PATCH:-}" \ E2E_IMAGE_MAJOR="${E2E_IMAGE_MAJOR:-}" \ @@ -280,7 +281,7 @@ run_scenario() { sideload_image -declare -i fails=0 +declare -i fails=0 deployed=0 declare -A results IFS=',' read -ra impl_list <<<"$IMPLEMENTATIONS" for impl in "${impl_list[@]}"; do @@ -290,7 +291,13 @@ for impl in "${impl_list[@]}"; do log "============================================================" log "implementation=$impl driver=$driver" log "============================================================" - apply_overlay "$impl" + switch_config "$impl" + if (( deployed )); then + restart_controller + else + deploy_controller + deployed=1 + fi # The scenario list is materialised up front instead of streamed # on stdin, so nothing a scenario runs can consume the remainder @@ -299,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