feat(operator): add migration orchestration controller - #346
Siddhant-K-code wants to merge 32 commits into
Conversation
Replace Helm hook-based migrations with a lightweight Kubernetes operator that watches OpenFGA Deployments, detects version changes, and runs migrations as regular Jobs. - Go operator using controller-runtime (no CRDs) - Helm subchart with opt-in via operator.enabled (default false) - Dedicated migration ServiceAccount (separate from runtime) - Auto-recovery on database failure (delete/retry cycle) - GitHub Actions workflow for multi-arch image builds - Integration test values for local Kubernetes clusters Resolves #211, #107, #120, #100, #126
- Harden pod security (runAsNonRoot, seccompProfile, drop ALL caps) - Find container by name instead of index to handle sidecars - Skip migration for memory datastore - Persist retry-after annotation before Job deletion to survive re-enqueue - Clear MigrationFailed condition on success - Propagate imagePullSecrets and securityContext to migration Jobs - Remove unused RBAC rules (secrets, serviceaccounts) - Add POD_NAMESPACE downward API for namespace-scoped watch default - Remove no-op migration values (timeout, backoffLimit, resources) - Fix migration SA helper to require name when create=false - Guard operator logic on both operator.enabled and migration.enabled - Build and load operator image into kind for chart-testing CI - Add path filters to operator workflow - Fix ADR inaccuracies (retry strategy, default-enabled wording) - Pin Dockerfile base image to golang:1.26.2
- Render extraInitContainers in operator mode (previously skipped) - Add version label to migration Jobs and delete stale Jobs on image change - Use namespaced Role/RoleBinding when watchAllNamespaces is false - Replace Status().Update with Status().Patch to avoid write conflicts - Fix logger.Error(nil, ...) to logger.Info for expected failure state - Wire desiredVersion param into buildMigrationJob for version tracking - Update RBAC: deployments/status verb from update to patch - Don't force replicas: 0 for memory engine in operator mode - Guard migration SA creation on migration.enabled - Document both required labels and mutable tag limitation in README
- Add opt-in annotation (openfga.dev/migration-enabled) so the operator only manages migrations for explicitly opted-in Deployments - Propagate volumes, volumeMounts, and envFrom from the Deployment to migration Jobs for TLS certs and file-based credentials - Remove watchAllNamespaces option; operator is now always namespace-scoped - Update ADR-004 dependency example to match actual file:// reference - Add test for migration-not-enabled skip behavior
The migration Job should only receive explicitly filtered OPENFGA_DATASTORE_* env vars, not the full EnvFrom from the source Deployment which could leak non-datastore secrets.
- Wrap deployment annotations in conditional to avoid emitting empty annotations: field which produces an invalid manifest - Store full version in annotation (openfga.dev/desired-version) and truncate label to 63 chars to support digest-pinned images - Align operator image default to ghcr.io/openfga/openfga-operator to match CI publishing target
- Return error on retry-after annotation patch failure to prevent Job churn that bypasses the 60s cooldown - Add test for stale-Job version mismatch deletion path
…values overflowing)
- Pin Dockerfile base images by digest for reproducible builds - Fix version label fallback comparison for digest-pinned images by sanitizing desiredVersion before comparing to the label value
…y migration-specific volumes
Replace checks on job.Status.Failed >= backoffLimit with isJobConditionTrue(job, batchv1.JobFailed), and job.Status.Succeeded with batchv1.JobComplete. The Job controller sets conditions atomically when it makes its final decision, avoiding races where the operator acts on intermediate counter states before Kubernetes has finished cleaning up.
The old Helm-templated migration Job uses datastore.migrations.resources for resource limits, but the operator-built Job had none. Inherit the main container's Resources to maintain parity and prevent unbounded resource consumption during migrations.
Four new tests covering previously untested code paths: - StaleJob_LabelOnlyFallback: version-mismatch detection when Job has only a label (no annotation), exercising the sanitized-label fallback - JobSucceeded_UpdatesExistingConfigMap: ConfigMap update path when a prior version's ConfigMap already exists - ScaleToZero_NilAnnotationsMap: scaleDeploymentToZero correctly stores desired-replicas when the annotation was not previously set - JobInProgress_Requeues: in-progress Job triggers 10s requeue without scaling up or modifying the Deployment
The test validates that scaleDeploymentToZero stores the current replica count in the desired-replicas annotation before zeroing, not that it handles a nil annotations map.
When concurrent reconciles race between the GET and CREATE, the second create returns AlreadyExists. Treat this as benign and requeue to poll the existing Job instead of returning a hard error that produces noisy reconcile failures in the controller logs.
Address review findings: add allowPrivilegeEscalation: false for restricted PSS compliance, set default resource requests/limits, add values.schema.json validation, use stable selectorLabels on pod template to prevent spurious rollouts, add .helmignore, and improve Chart.yaml metadata and NOTES.txt with migration commands.
The operator was scaling Deployments to 0 replicas during every migration, causing a full outage on every helm upgrade — a regression from the existing rolling update behavior. OpenFGA already gates readiness on schema version (MinimumSupportedDatastoreSchemaRevision in sqlcommon.IsReady), so new pods naturally block until migration completes while old pods keep serving. Use Helm's lookup function to preserve the live replica count on upgrade (falling back to replicas: 0 on fresh install where no Deployment exists). Remove scaleDeploymentToZero from the operator reconcile loop. Update ADR-002 to document the rationale and the readiness gate dependency.
…ent template - findOpenFGAContainer now reads the openfga.dev/container-name annotation emitted by the chart, and returns an error when the target container is missing instead of silently falling back to the first container in the pod spec. - migration_controller surfaces that error to the reconciler instead of logging and skipping, so misconfigured Deployments are visible. - deployment.yaml emits the new container-name annotation, collapses the replica-preservation logic to a single branch (both previous branches already preserved existing replicas), and uses selectorLabels on the pod template to avoid chart-version churn in pod labels across upgrades. - values.yaml documents the openfga-operator subchart values passthrough and clarifies migration service account behavior.
…s stale, its JobComplete would write the wrong version into the status ConfigMap
The Job controller sets JobFailureTarget as soon as it decides a Job will fail (backoff limit reached, active deadline exceeded, etc.) — JobFailed only flips after pods finish terminating, which can take up to BackoffLimit × ActiveDeadlineSeconds. Previously the operator only watched JobFailed, so a broken migration took ~15 minutes (with chart defaults) before MigrationFailed appeared on the Deployment. Treat either condition as "failed" and add a regression test.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
Siddhant-K-code
changed the title
There was a problem hiding this comment.
Pull request overview
Adds a new operator/ Go module implementing a controller-runtime Kubernetes operator responsible for orchestrating OpenFGA datastore migrations via Jobs and recording completed versions in a status ConfigMap, including unit tests and build artifacts (Dockerfile/Makefile).
Changes:
- Introduces
MigrationReconcilerto detect OpenFGA version changes, create/observe migration Jobs, update migration-status ConfigMaps, and manage retry cooldowns. - Adds controller-focused unit tests covering success/failure, stale resources, collisions, and retry behavior.
- Adds operator module scaffolding and developer tooling (go.mod/go.sum, main entrypoint, Dockerfile, Makefile, README, .dockerignore).
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| operator/README.md | Operator purpose, local development/testing instructions, flags, and limitations. |
| operator/Makefile | Build/test/vet/fmt and docker build/push targets for the operator. |
| operator/internal/controller/migration_controller.go | Core reconciliation logic for migrations, retries, and resource lifecycle. |
| operator/internal/controller/migration_controller_test.go | Unit tests covering migration orchestration and edge cases. |
| operator/internal/controller/helpers.go | Shared helpers for image parsing, job/configmap creation/update, scaling, and deletion preconditions. |
| operator/go.mod | New Go module definition and dependencies for controller-runtime + Kubernetes APIs. |
| operator/go.sum | Dependency checksums for the new module. |
| operator/Dockerfile | Multi-stage, pinned-image build producing a distroless static operator image. |
| operator/cmd/main.go | Operator manager setup, flags, cache scoping, and health endpoints. |
| operator/.dockerignore | Docker build context exclusions for the operator directory. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…-code-authored-migration-controller
SoulPancake
left a comment
SoulPancake
left a comment
There was a problem hiding this comment.
ensureDeploymentScaledenforcesdesired-replicason every reconcile, not just after migration. Anykubectl scaleis instantly reverted (the scale event itself triggers the reconcile), and with autoscaling blocked in operator mode that's the only scaling left. Also snaps back the live replica count the chart'slookupdeliberately preserves, right after the Job succeeds. Make it a one-shot gate — only scale when replicas is 0 — and add a test that a non-zero live count is left alone.- Stale-version/legacy-hook Jobs are deleted with background propagation while possibly still running, and the replacement lands ~5s later → two concurrent
openfga migrateruns while the old pod terminates. Foreground propagation fixes it: the Job lingers until pods are gone, so recreation waits naturally.
Nits: cmd/main.go fails gofmt -l; module path should be github.com/openfga/helm-charts/operator (operator lives here per ADR-003) unless extraction is planned; updateMigrationStatus overwrites existing.Labels
Make migration Job replacement deletion-safe, preserve stable Helm adoption across chart upgrades, and avoid status or replica churn. Harden startup validation and namespace scoping, preserve ConfigMap labels, and use the repository-local module path.
|
Addressed the controller review in
The same commit also fixes the namespace-scope fallback, active-deadline validation, status-patch durability, idempotent failure-condition clearing, and cross-chart-version legacy Job adoption. Full tests, race tests, vet, and build pass. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 8 comments.
Suppressed comments (1)
operator/internal/controller/migration_controller.go:217
- This status merge patch can silently replace concurrently updated Deployment conditions because it does not include a resource-version precondition. Use an optimistic-lock merge patch and let a conflict retry the condition clear.
statusPatch := client.MergeFrom(deployment.DeepCopy())
Persist terminal results before arming cleanup, retain failed Jobs through cooldown and diagnostics, and serialize replacement behind foreground deletion. Repair status ownership, lock condition patches, preserve pull policy, and revalidate controller labels.
|
Closing this stack for now. We're going to focus first on updating, reviewing, and fixing #309. |
Summary
Adds the Go controller that coordinates OpenFGA datastore migrations before application rollout.
Stack
Native stack 351. Open #345 to view the GitHub stack map. Review bottom to top:
mainReplaces closed #337 and carries the controller portion of source #331. Helm packaging and chart integration remain in later layers.
Attribution
The 24 original controller commits are replayed with Ed Milic (@emilic) as their actual Git author, preserving their original author dates, messages, and commit boundaries. GitHub links the replayed commits to
@emilic.Validation
make vet testinoperatorgit diff --check