Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The deploy workflow is missing key hardening/robustness pieces (notably GHCR/buildx setup and clearer required-variable preflight), which could cause avoidable deployment failures.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new manual GitHub Actions workflow to deploy an already-released ghcr.io/zitadel/nextgen:<version> image to the GCP Cloud Run service, including optional Secret Manager version sync and a concise deployment report in the workflow summary.
Changes:
- Introduces
.github/workflows/deploy.ymlwithworkflow_dispatchinputs for release version, target environment, and optional secret sync. - Mirrors the multi-arch GHCR release image into Artifact Registry and updates the Cloud Run service to use it.
- Optionally pushes GitHub environment secrets into Secret Manager as new versions and reports the deployed revision.
File summaries
| File | Description |
|---|---|
| .github/workflows/deploy.yml | New manual Cloud Run deployment workflow: image mirroring, optional secret sync, and service update/reporting. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 3
- 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
fforootd
requested changes
Sep 4, 2026
fforootd
left a comment
fforootd
left a comment
Member
There was a problem hiding this comment.
Two blockers on the master-key mount (L6, L29); the rest are nits. Copilot's packages: read and Buildx comments don't apply: the package is public and imagetools create needs no builder.
Generated by Claude Code
| --service-account="$RUN_SA" \ | ||
| --port=8080 \ | ||
| --set-env-vars="NEXTGEN_SERVER_DATA_DIR=${DATA_DIR}" \ | ||
| --set-secrets="NEXTGEN_DATABASE_POSTGRES=zitadel-database-postgres-${ENVIRONMENT}:latest,${DATA_DIR}/master-keys/master-key.pem=zitadel-master-key-${ENVIRONMENT}:latest" |
Member
There was a problem hiding this comment.
Verify on a real revision before merge: in gen2 root owns the volume and the image runs as uid 65532; and the key-dir scan skips only IsDir(), so a projected layout with a ..data symlink would be read as a key file and fail startup. ls -la the mount.
Generated by Claude Code
Contributor
Author
There was a problem hiding this comment.
Taken seriously, and it turned into its own PR: #1161.
I could not test a real revision — nothing is deployed with a mount yet — so I reproduced the layout locally instead, and the ..data half of your concern is a genuine bug in the server today:
..2026_09_05_12_00_00.123456 DirEntry.IsDir()=true os.Stat().IsDir()=true
..data DirEntry.IsDir()=false os.Stat().IsDir()=true
master-key.pem DirEntry.IsDir()=false os.Stat().IsDir()=false
..data is a symlink, so DirEntry.IsDir() reports false, the key-dir scan adopts it as a key named ..data, and startup dies with failed to read encryption key file ".../..data": is a directory. Confirmed end-to-end by running the server against that directory.
#1161 skips dot-prefixed entries and stats through symlinks, with three tests each confirmed to fail beforehand. Worth having regardless of Cloud Run: any self-hoster projecting keys through a Kubernetes secret hits it, and a stray .DS_Store previously became the deployment's only master key, since a non-empty directory also suppresses generation.
Leaving this thread open for the ownership half, which I still cannot check: gen2 root owning the volume against uid 65532. Once a revision is up with the mount, ls -la on it settles that — but with #1161 in, the projection layout is no longer a merge gate either way.
Contributor
Author
There was a problem hiding this comment.
Update: #1161 is merged, so the ..data half of this is fixed on main — the key-directory scan now skips dot-prefixed entries and stats through symlinks, with regression tests for the projected layout.
Leaving this thread open for the half I still cannot verify: whether gen2's root-owned volume is readable by uid 65532. That needs a live revision with the mount, which does not exist yet. It is no longer a merge gate — #1161 makes both projection layouts safe — but it is a real unknown that would surface as a failed revision on first deploy, so it seems worth keeping visible until someone can ls -la the mount.
fforootd
reviewed
Sep 4, 2026
fforootd
left a comment
fforootd
left a comment
Member
There was a problem hiding this comment.
Cloud Run imports public GHCR images directly since the July 14 GA, so the mirror step can go and with it the docker registry login, the mgmt project variable, and the deploy identity's need for artifactregistry.writer. The image stays public, so no remote repository is needed. Inline suggestions below; the master-key mount also needs the ignore_changes fix on #1132 or the next infra apply strips it.
Generated by Claude Code
muhlemmer
force-pushed
the
ci/gcp-deploy-workflow
branch
from
September 5, 2026 12:50
982b6a9 to
ab4f691
Compare
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>
Adds the workflow the infra tier always assumed existed. infra/README.md describes a "GitHub Actions deploy workflow" owning the Cloud Run image, env and secrets — those fields sit under `lifecycle.ignore_changes` in the cloud-run module precisely so OpenTofu does not fight it — but the workflow was never written, which is why nothing has been deployed since the original infra PR was closed. Three things it has to do that are not obvious: - Mirror the image. Releases publish to ghcr.io, and Cloud Run pulls only from Artifact Registry or GCR. `docker buildx imagetools create` copies the multi-arch manifest as-is; a plain pull/push would flatten it to the runner's architecture. - Mount the master key as a file, not an env var. `server.master_keys` is a map keyed by key ID and Viper cannot populate map keys from the environment, so NEXTGEN_SERVER_MASTER_KEYS_* is silently ignored; the server does discover keys in its master-key directory by filename. Without a provisioned key it generates a throwaway one per instance and logs that it is "for local/dev only" — on Cloud Run that would leave project KEKs wrapped by a key the next instance does not have. - Leave the migrate job alone. The released binary has no `migrate` subcommand — its only command is `server`, which runs pool.Migrate(ctx) at startup — so running zitadel-migrate-<env> would start a server that never exits and fail on the job timeout. Secret values come from the environment's GitHub secrets and are pushed to Secret Manager as new versions, gated behind a `sync_secrets` input so ordinary deploys do not churn versions. Values are piped over stdin, never passed as argv. The containers and IAM bindings come from the `secrets` OpenTofu module. Manual dispatch: a `release: published` trigger would not fire, because the release is created by release-publish.yml using GITHUB_TOKEN and events raised by that token do not start new workflow runs. Validated by parsing the workflow YAML, exercising the secret-sync shell against a stubbed gcloud (value arrives on stdin; a missing secret fails loudly), and confirming against ghcr.io/zitadel/nextgen:1.0.0-alpha.20 that a key placed in the master-key directory is adopted — the server reaches "server listening for requests" and "migrate keys to latest master key" with no dev-only warning. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…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>
#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>
muhlemmer
force-pushed
the
ci/gcp-deploy-workflow
branch
from
September 9, 2026 15:27
ab4f691 to
e475431
Compare
muhlemmer
marked this pull request as draft
September 9, 2026 15:58
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.
Adds the workflow the infra tier always assumed existed.
infra/README.mddescribes a "GitHub Actions deploy workflow" owning the Cloud Run image, env and secrets — those fields sit underlifecycle.ignore_changesin thecloud-runmodule precisely so OpenTofu does not fight it — but the workflow was never written. That is why nothing has been deployed since #188 was closed; the abandoned PR said so itself: "For now I only moved over the infra pipeline as we have no releases yet in this repo." We have releases now.Three things it has to do that are not obvious from the outside:
ghcr.io, and Cloud Run pulls only from Artifact Registry or GCR.docker buildx imagetools createcopies the multi-arch manifest as-is; a plain pull/push would flatten it to the runner's architecture.server.master_keysis a map keyed by key ID, and Viper cannot populate map keys from the environment —NEXTGEN_SERVER_MASTER_KEYS_*is silently ignored. The server does discover keys in its master-key directory by filename. Without a provisioned key it generates a throwaway one per instance and logs that it is "for local/dev only"; on Cloud Run, where that directory is ephemeral, project KEKs would end up wrapped by a key the next instance does not have.migratesubcommand — its only command isserver, which runspool.Migrate(ctx)at startup — so runningzitadel-migrate-<env>would start a server that never exits and fail on the job's 600s timeout.Secrets never touch the repository. Values live in the environment's GitHub secrets and are pushed to Secret Manager as new versions, gated behind a
sync_secretsinput so ordinary deploys do not churn versions. They are piped over stdin, never passed as argv.Manual dispatch on purpose: a
release: publishedtrigger would not fire, because the release is created byrelease-publish.ymlusingGITHUB_TOKEN, and events raised by that token do not start new workflow runs.Validation
gcloud: the value arrives on stdin (65 bytes of PEM, newlines intact) and never as an argv entry; a missing environment secret fails loudly with an::error::annotation.ghcr.io/zitadel/nextgen:1.0.0-alpha.20: with a key placed in the master-key directory the server reachesserver listening for requestsand logsmigrate keys to latest master key, with no dev-only warning. The stock image with no key logscreated server master key file … (generated for local/dev only).Not run: the workflow itself. It needs the GCP identity and the secret containers from the companion PR, and this environment has no
gcloudand no credentials.Release notes / changeset
No changeset required — no shipped behavior changed. Deployment wiring only, which is why the title is
cirather thanfeat.Notes
secretVersionAdderbinding. Merge that one first; this workflow will fail at the sync step without it.sync_secrets: true, and thedevenvironment needs two secrets set:MASTER_KEY_PEM(an RSA private key in PEM, the same PKCS#1 shape the server generates) andDATABASE_POSTGRES_DSN(built from thepostgres_private_ipandpostgres_database_nameoutputs).run.appURL — test through the load balancer atdev.zitadel.io.GCP_DEV_PROJECT_IDrepository variable is new; the others (GCP_WIF_PROVIDER,GCP_WIF_SERVICE_ACCOUNT,GCP_MGMT_PROJECT_ID,AR_REGION) already exist. The first step fails with a clear message if any are missing.workflow_run; a realmigratesubcommand so the migration job can be wired; and Spanner support, which needs thespanner_integrationbuild tag question settled before it can run on a released image at all.🤖 Generated with Claude Code