Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
OpenTofu Plan — devPushed by @muhlemmer |
Contributor
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new infra workflow masks tofu plan failures due to missing pipefail, which can cause CI to report success and post misleading plan output.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR restores and modernizes the repository’s GCP infrastructure-as-code by adding an OpenTofu-based two-tier setup (management project + per-environment workload project), including a new secrets module and CI automation to plan/apply the dev environment.
Changes:
- Adds
infra/OpenTofu root module plus reusable modules for networking, Cloud Run, load balancing, DNS, Cloud SQL, certificates, IAM, and secrets. - Adds
infra/mgmt/OpenTofu configuration for shared management-project resources (Artifact Registry, CI service accounts, and WIF). - Adds a GitHub Actions workflow to run
tofu planon PRs andtofu applyon merges for the dev environment, plus drift-detection plans for mgmt changes.
File summaries
| File | Description |
|---|---|
.github/workflows/infra.yml |
CI workflow to plan/apply dev infra and plan mgmt tier for drift/guardrails |
infra/.gitignore |
Ignores local OpenTofu/Terraform artifacts and state files |
infra/.terraform.lock.hcl |
Provider lockfile for environment-tier OpenTofu configuration |
infra/README.md |
Documentation for infra architecture, workflow, secrets, and migrations |
infra/bootstrap.sh |
One-time helper script to create the GCS state bucket |
infra/environments/dev.tfbackend |
Backend config for dev state prefix |
infra/environments/dev.tfvars |
Dev environment variables (project/domain/runtime sizing) |
infra/environments/prod.tfbackend |
Backend config for prod state prefix |
infra/environments/prod.tfvars |
Prod environment variable template |
infra/main.tf |
Environment root module wiring all infra submodules |
infra/outputs.tf |
Exposes key infra outputs (DB, LB, secrets, etc.) for consumers/ops |
infra/providers.tf |
Provider and backend configuration for environment tier |
infra/variables.tf |
Input variables for environment tier configuration |
infra/mgmt/.terraform.lock.hcl |
Provider lockfile for mgmt-tier OpenTofu configuration |
infra/mgmt/main.tf |
Mgmt project resources (AR, CI SAs, WIF, cross-project bindings) |
infra/mgmt/mgmt.tfbackend |
Backend config for mgmt state prefix |
infra/mgmt/mgmt.tfvars |
Mgmt variables (mgmt project, repo, env project ids) |
infra/mgmt/outputs.tf |
Outputs needed for CI/repo variables (WIF provider, SA emails, AR URL) |
infra/mgmt/providers.tf |
Provider and backend configuration for mgmt tier |
infra/mgmt/variables.tf |
Input variables for mgmt tier configuration |
infra/modules/certificate-map/main.tf |
Certificate Manager map + baseline DNS authorization and cert entries |
infra/modules/certificate-map/outputs.tf |
Exposes cert map IDs and DNS challenge records |
infra/modules/certificate-map/variables.tf |
Inputs for certificate-map module |
infra/modules/cloud-run/main.tf |
Cloud Run service + migrate job scaffolding with drift-ignores for deploy-owned fields |
infra/modules/cloud-run/outputs.tf |
Exposes Cloud Run service/job names and URI |
infra/modules/cloud-run/variables.tf |
Inputs for cloud-run module |
infra/modules/dns/main.tf |
Managed zone + A/AAAA + wildcard records + cert challenge records |
infra/modules/dns/outputs.tf |
Exposes DNS zone name servers and zone name |
infra/modules/dns/variables.tf |
Inputs for dns module |
infra/modules/iam/main.tf |
Runtime and migrator service accounts plus key project IAM bindings |
infra/modules/iam/outputs.tf |
Exposes runtime/migrator SA emails |
infra/modules/iam/variables.tf |
Inputs for iam module |
infra/modules/load-balancer/main.tf |
External managed LB resources (addresses, NEGs, backend services, URL maps, proxies, forwarding rules) |
infra/modules/load-balancer/moved.tf |
moved blocks to preserve state across renames |
infra/modules/load-balancer/outputs.tf |
Exposes LB IPs and key resource names |
infra/modules/load-balancer/variables.tf |
Inputs for load-balancer module |
infra/modules/network/main.tf |
VPC, subnet, router, and Cloud NAT setup |
infra/modules/network/outputs.tf |
Exposes network/subnet IDs and NAT IP |
infra/modules/network/variables.tf |
Inputs for network module |
infra/modules/postgres/main.tf |
Cloud SQL Postgres instance + DB plus private service access plumbing |
infra/modules/postgres/outputs.tf |
Exposes SQL instance connection info and private IP |
infra/modules/postgres/variables.tf |
Inputs for postgres module |
infra/modules/project/main.tf |
Enables required APIs in the target project |
infra/modules/project/outputs.tf |
Exposes the enabled API list |
infra/modules/project/variables.tf |
Inputs for project module |
infra/modules/secrets/main.tf |
Secret Manager secret containers + IAM bindings (no secret values) |
infra/modules/secrets/outputs.tf |
Exposes Secret Manager secret IDs for runtime/deploy integration |
infra/modules/secrets/variables.tf |
Inputs for secrets module |
Review details
Files not reviewed (2)
- infra/.terraform.lock.hcl: Generated file
- infra/mgmt/.terraform.lock.hcl: Generated file
Suppressed comments (1)
infra/mgmt/main.tf:85
- This comment line also includes garbled replacement characters ("���"). Please replace them so the file uses consistent UTF-8 characters throughout.
# ─── Service Account: github-deploy (app deploy CI) ���────────────────────────
- Files reviewed: 46/48 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This was referenced Sep 3, 2026
muhlemmer
commented
Sep 3, 2026
muhlemmer
added a commit
that referenced
this pull request
Sep 3, 2026
Review question on #1132. `containers.image` is required to create a google_cloud_run_v2_service, but the deploy workflow owns which release runs, so image sits under lifecycle.ignore_changes. That makes the value a create-time bootstrap only — read once when the service does not exist, never again. The plan CI runs on this PR shows it: the live service runs the released image, the config still says `hello`, and no image change is proposed. Also notes on the job that its placeholder is still what actually runs, since there is no `migrate` subcommand to point it at (#1138). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fforootd
requested changes
Sep 4, 2026
fforootd
left a comment
fforootd
left a comment
Member
There was a problem hiding this comment.
The secrets module is right: containers without values, accessor for the runtime, version-adder for deploy, and the README matches the runtime code. The blockers are in the surrounding automation. Two of them let the auto-apply job destroy dev data or the dev master key, and the mgmt drift job has never actually run. Each fix is a few lines; details inline.
Context for the mgmt tier: #1133 will deploy straight from GHCR, which Cloud Run has supported for public images since the July 14 GA. That leaves the Artifact Registry repo and its three bindings without a consumer. Not for this PR, since it imports live state and the mgmt plan should stay empty; see the note on the repo resource.
Non-blocking, fine as a follow-up issue: no ssl_policy on the HTTPS proxy so TLS 1.0 is accepted, the secrets module doesn't pass deletion_protection, load_balancer and dns miss depends_on = [module.project], the cacheable path rule targets routes the server doesn't serve, google-beta is required but unused, and the README's step 1 path and variable list don't match the workflows: GCP_DEV_PROJECT_ID is missing, while GCP_MGMT_PROJECT_ID and AR_REGION only served the mirror step #1133 is dropping.
Generated by Claude Code
muhlemmer
added a commit
that referenced
this pull request
Sep 5, 2026
Review question on #1132. `containers.image` is required to create a google_cloud_run_v2_service, but the deploy workflow owns which release runs, so image sits under lifecycle.ignore_changes. That makes the value a create-time bootstrap only — read once when the service does not exist, never again. The plan CI runs on this PR shows it: the live service runs the released image, the config still says `hello`, and no image change is proposed. Also notes on the job that its placeholder is still what actually runs, since there is no `migrate` subcommand to point it at (#1138). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
muhlemmer
force-pushed
the
ci/gcp-dev-infra
branch
from
September 5, 2026 12:47
8d9ef14 to
744aaa0
Compare
Contributor
OpenTofu Plan — mgmtMgmt applies are manual — pull this branch and run |
muhlemmer
added a commit
that referenced
this pull request
Sep 8, 2026
) ## Summary The Go binary now has a `migrate` subcommand so schema changes can run as their own step and exit, which is what the Cloud Run migrate job in #734/#1138 needs. - `nextgen migrate` loads the same config as `server`, connects, applies pending goose migrations, and exits 0 (including no-op reruns). - `nextgen` and `nextgen server` still start the HTTP server. They no longer migrate unless `--migrate` is passed. - Local and image entrypoints keep applying schema: `zitadel start` spawns `--migrate`, the Dockerfile `CMD` is `--migrate`, and `moon run workspace:server` injects the flag. Closes #1138. ## Validation - `go test ./cmd/server/ -run 'TestCommandHelpListsMigrate|TestMigrateCommandAppliesSchemaIdempotently|TestStartDatabaseSkipsMigrationsUnlessRequested'` (pass after review follow-up; migrate logs `stream=runtime`) - `go test ./cmd/server/` (full package) - `moon run server:format` - `corepack pnpm --filter @zitadel/cli exec vitest run tests/unit/lib/local-server/binary.test.ts` (status metadata includes injected `--migrate`) - Earlier on this PR: `moon run server:test`, `moon run cli:test`, `node scripts/check-pr-title.mjs --title "feat(migration): add migrate subcommand and opt-in --migrate flag"`, `corepack pnpm exec changeset status --since origin/main` - Live `go run . migrate` against a temp SQLite dir: first run applies, second run exits 0, `goose_db_version` and application tables present - `go run . --help` lists `migrate`; `go run . migrate --help` exposes `--config` ## Release notes / changeset Changeset: `.changeset/nextgen-migrate-command.md` — operators can run `nextgen migrate` to apply schema and exit; `server` migrates only with `--migrate`; `zitadel start` and the published image still migrate. Lists `@zitadel/server` (minor; fixed group bumps together). No extra changeset for review follow-ups: logging, Close handling, SilenceErrors, and recording `--migrate` in `zitadel status` metadata are polish on the same feature. The run-server `completion` skip is contributor-script only. ## Notes - Review follow-up: `migrate` now calls `setUpLogging` (same OTel metrics as `run`, StreamRuntime, metrics shutdown, no HTTP). Close errors after a successful migrate are `zlog` debug-only and do not change the exit code. `SilenceErrors: true` on root, `server`, and `migrate`. UI builds skipped for `completion` in the run-server wrappers. Help test asserts both the `migrate` subcommand and `--migrate` flag; skip-migrations test uses `t.Context()`. Local binary runtime metadata now stores the same argv that was spawned, including `--migrate`. - Cloud Run job wiring (`gcloud run jobs update` / `execute --wait`, migrator `secretAccessor`) is still follow-up after #1132/#1133. This PR only adds the binary command. - Spanner migrate-in-release-image remains #1137. - No oclif `zitadel migrate`; `@zitadel/server migrate` works via argv passthrough. - `loadConfig` is reused unchanged, so `migrate` still creates `data_dir` and may mint a local master key. --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Silvan <adlerhurst@users.noreply.github.com> Co-authored-by: Tim Möhlmann <tim+github@zitadel.com>
muhlemmer
added a commit
that referenced
this pull request
Sep 9, 2026
## Summary Keys discovered in the master key directory are identified by file name, and the scan skipped only directories. A Kubernetes-style projected secret volume — the shape both Cloud Run and GKE mount secrets with — lays the directory out like this: ``` ..2026_09_05_12_00_00.123456/master-key.pem the real file ..data -> ..2026_09_05_12_00_00.123456 symlink to that directory master-key.pem -> ..data/master-key.pem symlink to the key ``` `..data` is a **symlink**, so `DirEntry.IsDir()` reports `false` for it and it was adopted as a key named `..data`. Startup then died in `buildMasterKey`: ``` Error: failed to create Crypter: server: failed to read encryption key file ".../master-keys/..data": read .../master-keys/..data: is a directory ``` Found while wiring the GCP deployment (#734, #1132, #1133), but it is not GCP-specific: any self-hoster mounting keys through a Kubernetes secret hits it today. Two changes in `ensureServerMasterKey`: - **Skip dot-prefixed entries.** A key's file name doubles as its key ID, so no key an operator authored starts with a dot — while plenty of machinery does: projected volumes, editor swap files, `.DS_Store`. - **`os.Stat` the path instead of reading `DirEntry.Info()`.** `DirEntry` reports on the link itself, so a symlinked directory looks like a regular file. Following the link also means the modification time that picks the newest key is the key's, not the link's. ## Deliberate choices **Entries that cannot be stat'ed still fail startup.** Silently skipping something in this directory is the more dangerous behaviour: it can bring the server up under a *different* key than intended, orphaning every KEK the real one wrapped. Only what is positively identifiable as not-a-key — dot-prefixed, or a directory — is skipped. **Not filtering by extension.** `MasterKeyConfig.File` accepts a PEM or a JWK, so the file name carries no reliable extension contract. Dot-prefix is the right granularity. **Scope is one call site.** The only other `os.ReadDir`/`WalkDir` loops in non-test Go are `api/cmd/gen_event_schemas` and `api/cmd/gen_openapi_errors`, both build-time generators walking repo source. This is the only runtime scan of an operator- or platform-supplied directory. ## Validation - `go test ./cmd/server/ -count=1` — full package passes. - Three new tests, **each confirmed to fail before the change**: | test | failure on old code | | --- | --- | | projected volume layout | `map[..data:… master-key.pem:…] should have 1 item(s), but has 2` | | symlinked directory, no dot prefix | `map[linked-dir:… master-key.pem:…] should have 1 item(s), but has 2` | | stray `.DS_Store` | `map[".DS_Store":…] does not contain "master-key.pem"` | - `gofmt -l` and `go vet ./cmd/server/` clean. - Reproduced end-to-end before fixing: running the server against the projected layout produced exactly the `is a directory` startup failure above. The `.DS_Store` case is the nastiest of the three. Because a non-empty directory also suppresses generation, the old code did not merely ignore the stray file — it made `.DS_Store` the deployment's one master key, marked for encryption. ## Release notes / changeset Changeset: `.changeset/master-key-dir-hidden-entries.md` — `@zitadel/server` patch. This corrects shipped behavior a user could have hit, which is why the title is `fix`. ## Notes - Independent of #1132 and #1133; it needs no infrastructure and can land on its own. - It does de-risk #1133, which mounts the master key as a file into this directory. Whether Cloud Run actually uses the `..data` projection is still unconfirmed — nothing is deployed with a mount yet — and this change makes both layouts safe either way, so that stops being a merge gate. - Related: #655 / #1151 (a flag to refuse startup rather than generate a key). Complementary — that one governs whether generation happens at all; this one governs what counts as an existing key. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Recovers the infrastructure-as-code from the closed PR #188 (feat/tofu-infra), rebased onto current main. The resources it declares are already applied in zitadel-dev-492704 — the branch was abandoned before merge, so the code describing live infrastructure was never on main. Changes on top of the abandoned branch: - Adds a `secrets` module. It owns the Secret Manager containers and the access bindings but never the values, so no credential material reaches OpenTofu state while who may read each secret stays reviewable in code. Values come from GitHub environment secrets, pushed as new versions by the deploy workflow. - Grants the runtime SA `secretAccessor` and the github-deploy SA `secretVersionAdder` on those two secrets. Version adder, not admin: CI can write a new version but cannot read one back, delete the secret, or change who has access. - Rewrites the README's secrets section, which documented NEXTGEN_SERVER_ENCRYPTION_KEY — a variable that no longer exists. It was replaced by the rotatable master-key config, which is a map and therefore cannot be set from an environment variable at all. - Documents that the zitadel-migrate-<env> job is deliberately unwired: the released binary has no `migrate` subcommand, so migrations run at server startup. Validated with `tofu validate` and `tofu fmt -check -recursive`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
All four inherited from PR #188 and caught by Copilot review. - `tofu plan ... | tee` ran without pipefail in the plan and apply jobs, so the pipeline's exit status came from tee: a failed plan passed the step and posted a plan comment that looked fine. Confirmed with `bash -e -c 'false | tee'`, which exits 0. The mgmt-plan job already set pipefail, which is what makes this an oversight rather than a deliberate choice. - infra/mgmt/main.tf carried six U+FFFD replacement characters in two section rules, from a bad copy-paste in the original branch. Restored the box drawing characters and re-evened both rules to the file's 79-column convention. - The resource-ownership table credited `infra/mgmt/` with the GCS state bucket, but no bucket resource exists there — bootstrap.sh creates it with gcloud, deliberately, since it has to exist before the backend it stores. Split it into its own row so nobody expects OpenTofu to reconcile drift on it. Revalidated with `tofu validate` (both tiers) and `tofu fmt -check -recursive`; re-parsed the workflow to confirm every `tofu plan` step now sets pipefail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review question on #1132. `containers.image` is required to create a google_cloud_run_v2_service, but the deploy workflow owns which release runs, so image sits under lifecycle.ignore_changes. That makes the value a create-time bootstrap only — read once when the service does not exist, never again. The plan CI runs on this PR shows it: the live service runs the released image, the config still says `hello`, and no image change is proposed. Also notes on the job that its placeholder is still what actually runs, since there is no `migrate` subcommand to point it at (#1138). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four blockers from review, two of which let the auto-apply job destroy data. Cloud SQL could be replaced. disk_size was pinned while disk_autoresize is on, so once the disk grows past the configured floor the provider reads config-below-state as a shrink (ForceNewIfChange on settings.0.disk_size) and plans a destroy/recreate — with deletion_protection = false on dev, the apply job would carry it out. Confirmed still at 20 GB in the project today, so this is armed rather than fired. disk_size is now ignored after create. The master key mount could be stripped. `--set-secrets=<path>=secret` creates template.volumes and volume_mounts, and ignore_changes covered neither, so the first apply after a deploy would drop the mount and the server would mint a throwaway key under the same ID master-key.pem — every KEK wrapped by the real key then fails to decrypt. Rather than widen ignore_changes, the module now declares the volume itself: a mount that is OpenTofu's own state cannot be stripped by OpenTofu. That choice has a consequence worth stating, because it is the trap it replaces: `gcloud run services update --set-secrets` is declarative over volumes as well as env, so a deploy passing it for the DSN alone would delete the module's mount. The module therefore also owns the DSN env secret, env comes out of ignore_changes, and the deploy workflow shrinks to `--image` only. OpenTofu owns configuration; CI owns the release. The mgmt drift gate had never run. The default depth-1 checkout has neither SHA, so `git diff` died with "fatal: bad object", and inside an `if` condition — where set -e does not fire — that failure was read as "nothing changed". Job 100744516053 shows it: on the PR that adds all of infra/mgmt/, every step after detection was skipped and the job still reported success. Now fetch-depth: 0, and the diff runs on its own line so git's exit code fails the step instead of being swallowed. The mgmt plan discarded its own exit code. -detailed-exitcode returns 2 for "there are changes", which under the runner's bash -e killed the step before PLAN_EXIT was written, skipping the comment and summary steps in exactly the case they exist for. Verified against a stubbed tofu: 0 and 2 now record and pass, 1 records and fails. Also, on the review's non-blocking points: dropped certificatemanager.editor, compute.loadBalancerAdmin and dns.admin from the runtime SA. They were granted for an "infra.rs" runtime that does not exist in this repository — no Rust, and no such clients in go.mod — so on the identity terminating internet traffic they were blast radius and nothing else. The README's setup steps and variable list now match what the workflows actually read, including the environment-scoped GCP_PROJECT_ID and GCP_REGION. Validated with tofu validate and tofu fmt -check -recursive, by re-parsing the workflow, and by exercising the new exit-code handling against a stub. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Moving the secret references into OpenTofu introduced a first-apply failure: the same apply creates the secret containers and points the Cloud Run service at them, but a revision cannot start against a secret that has no versions — and the containers are created empty, on purpose, because OpenTofu must never hold a secret value. Merging as-is would have failed the apply it triggers. The volume, its mount and the DSN env reference are now gated on runtime_secrets_ready, default false, so a new environment applies cleanly and the sequence becomes explicit: apply, seed the secrets through the deploy workflow, then flip the flag. The flip is a one-line change whose plan shows the mount being added, which makes the riskiest step the one that gets read before it runs. Documented under "First deploy" in the README. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A comment added in 8d9ef14 said the placeholder image is what the job still runs. The project shows otherwise: image is under ignore_changes, and the job carries an old zitadel image set out of band. The operative point is unchanged — nothing invokes the job, because there is no `migrate` subcommand (#1138) — but the detail was wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
With the change detection fixed, the mgmt plan ran for the first time and immediately reported a destroy: google_project_iam_member.infra_apply_spanner ["dev"] is in state and applied, but absent from this configuration, so the plan proposed removing a live IAM binding. It was added to the mgmt tier after the original infra branch was abandoned, which is why the branch does not carry it. Declaring it here keeps the plan honest about what exists. Whether a CI identity should hold spanner.admin at all is a separate question, tracked in #1163. Note the plan that surfaced this did not finish: it aborted on a 403 for serviceusage.services.list on zitadel-ops, so only part of the state was refreshed. There may be further drift behind that error, and it cannot be seen until infra-apply can read the mgmt project. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#1152 landed a `migrate` subcommand and, in the same change, made `server` stop migrating unless `--migrate` is passed. That second half breaks this configuration as written: the live service carries args ["server"] from an old console deploy, which overrides the image's CMD of ["--migrate"], so a current release would have served against an unmigrated schema. The fix is the design the job was always for, and #1152 says as much — the subcommand exists because of #734/#1138. - The job runs `args = ["migrate"]`, and carries the DSN and the same VPC access the service has. `gcloud run jobs execute --wait` in the deploy workflow makes a failed migration a failed deploy rather than a broken revision. - The service declares `args = ["server"]` explicitly and `args` comes out of ignore_changes. The contract — the job migrates, the service serves — is now stated in code rather than inherited from whatever argv a container was last left with. - Both carry NEXTGEN_SERVER_GENERATE_MASTER_KEY=false, gated with the mount. Refusing to generate is only safe once a key is actually mounted; setting it outside that gate would leave the service unable to start at all while runtime_secrets_ready is still false. The job also mounts the master key, which it does not use — `migrate` runs goose and exits without building a crypter. It is there because `migrate` loads the same configuration as `server`, and with generation disabled (#1151, landing shortly) a start that finds no key fails inside loadConfig, which `migrate` calls too. So the mount keeps the job startable rather than serving the migration, and should come off if that stops being true; the grant and the volume are commented as a pair and a follow-up issue tracks it. Validated with tofu validate and tofu fmt -check -recursive. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
muhlemmer
force-pushed
the
ci/gcp-dev-infra
branch
from
September 9, 2026 15:26
ca3dc6c to
ebc3892
Compare
muhlemmer
added a commit
that referenced
this pull request
Sep 9, 2026
…Tofu Addresses the review on this PR. The mirror step is gone. It existed because "Cloud Run pulls only from Artifact Registry or GCR" — which is no longer true: Cloud Run imports public GHCR images directly. That removes the mirror, the registry login and the mgmt project variable, and with them the deploy identity's need for artifactregistry.writer. The existence check stays, and the workflow now resolves the digest and deploys that, so the one-hour cache Cloud Run keeps for public registries cannot come into it and the revision records exactly what shipped. Verified against 1.0.0-alpha.20: the tag resolves to sha256:ac57b5f1…, and a bogus tag fails the step. The deploy sets only --image. #1132 now declares the master-key volume and the DSN reference, and `--set-secrets` is declarative over volumes as well as env, so passing it here for the DSN alone would have deleted the mount that exists to stop the server minting a throwaway key. Dropped --service-account and --port for the same reason — OpenTofu owns them and `services update` keeps what it is not asked to change — and NEXTGEN_SERVER_DATA_DIR, which the Dockerfile already sets. Dropped the rotation claim from sync_secrets. A new version under the same file name keeps the same key ID, so old kids resolve to the new key and MigrateToLatestMasterKey has nothing to do; rotation needs a new ID alongside the old one (docs/operations/encryption-keys.md). The input now says what it actually does: seed the secrets once, before the environment's runtime_secrets_ready flag is flipped. The required-variables check now covers GCP_WIF_PROVIDER and GCP_WIF_SERVICE_ACCOUNT, and the project and region come from environment-scoped GCP_PROJECT_ID and GCP_REGION. The job already runs under `environment:`, so this makes the environment input mean something instead of borrowing an Artifact Registry variable as the Cloud Run region. Also corrected a comment added in 8d9ef14: the migrate job is not sitting on the hello-job placeholder, it points at an old zitadel/zitadel image. Either way it is not invoked, because the binary has no migrate subcommand (#1138). Validated by parsing the workflow, resolving the digest against the real published image, and exercising the required-variables check with values present and missing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
muhlemmer
added a commit
that referenced
this pull request
Sep 9, 2026
#1152 made `server` stop migrating unless --migrate is passed, and added the `migrate` subcommand the Cloud Run job was always meant to run. Without this step a deploy would roll a revision onto a schema that was never applied — the service in #1132 deliberately runs `args = ["server"]`, which overrides the image's CMD of ["--migrate"]. The job is pointed at the same digest as the service and executed with --wait, so a failed migration fails the deploy. Without --wait the service would roll anyway and the first broken query would be the notification. Also drops the stale comment claiming migrations run at server startup, and reports the job in the run summary so a deploy says what applied the schema. Validated by parsing the workflow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The follow-up issue now exists; reference it directly instead of pointing readers at another file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
muhlemmer
marked this pull request as draft
September 9, 2026 15:38
Contributor
Author
|
Waiting for #1151 to land, so we can cleanup the masterkey regeneration warnings. Other comments are resolved. |
Contributor
Author
Note for review: this apply takes dev down until the first deployThe plan removes two env vars from the running service and adds nothing back: That is deliberate, and the downtime has been accepted. Recording why here so it does not read as an oversight in the plan output. Why it happens. Moving the master-key mount into OpenTofu (the fix for the Why it is not worked around. What is actually lost. The serving revision is a June build ( The window closes at step 4:
Steps 1 and 4 are the two applies; everything between them is dev being down on purpose. |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Part of #734.
Recovers the infrastructure-as-code from the closed #188 (
feat/tofu-infra), rebased onto currentmain.The important thing about that PR: the resources it declares are already applied in
zitadel-dev-492704. The CI plan comment on #188 shows every resourceRefreshing state...against real IDs, and the final plan was0 to add, 1 to change, 0 to destroy(the one change being cosmetic drift —cpu "1000m" → "1", short network names → fully qualified). The branch was abandoned before merge, somainhas never carried the code that describes our live infrastructure. State lives ings://zitadel-ops-tofu-stateunder prefixinfra/dev.Rather than rebase ten commits with merge commits in them, this takes the tree as a single commit off current
main— the original PR is closed and its history is not worth preserving.Changes on top of the abandoned branch:
secretsmodule. It owns the Secret Manager containers and the access bindings but never the values. A secret with no versions carries no credential material, so nothing sensitive reaches OpenTofu state, while who may read each secret stays reviewable in code. Values come from GitHub environment secrets (see the companion PR).secretAccessor; thegithub-deploySA getssecretVersionAdder— version adder, not admin, so CI can write a new version but cannot read one back, delete the secret, or change who has access.NEXTGEN_SERVER_ENCRYPTION_KEY, a variable that no longer exists — it was replaced by the rotatable master-key config, which is a map and therefore cannot be set from an environment variable at all. The rewritten section explains why the master key has to arrive as a mounted file.zitadel-migrate-<env>has no work to do: the released binary has nomigratesubcommand, so migrations run at server startup.Nothing here changes any GCP resource on its own — merging runs
tofu planon the mgmt tier andtofu applyon dev, which should be close to a no-op apart from the new (empty) secret containers and their bindings.Validation
tofu validate—Success! The configuration is valid.tofu fmt -check -recursive— clean.tofu init -backend=falseagainst the committed lock file.Not run:
tofu planagainst the real project. This environment has nogcloudand no GCP credentials, so the plan will first appear on this PR from CI.Release notes / changeset
No changeset required — no shipped behavior changed. Deployment wiring only, which is why the title is
cirather thanfeat.Notes
secretsmodule most closely. It is the only genuinely new code; everything else is feat(infra): tofu migration #188's, reviewed there.spanner.googleapis.comis not even in the enabled-APIs set — but it is not abandoned leftovers, as I first read it. It belongs to the benchmark work tracked in Weekly real-instance Spanner benchmark: we cannot tell a Spanner regression from an emulator artifact #1011, which plans a recurring real-instance run against it. Treat it as live infrastructure with an owner: do not delete it. Bringing it under OpenTofu is worth doing and belongs with Weekly real-instance Spanner benchmark: we cannot tell a Spanner regression from an emulator artifact #1011, not here. It does share the namezitadel-devwith the Cloud SQL instance, which makes console URLs easy to misread.infra/environments/prod.tfvarsis inherited from feat(infra): tofu migration #188 and still contains placeholders. Dev is the only wired environment.🤖 Generated with Claude Code