Skip to content

Helm chart follow-ups from PR #1481 review (worker isolation, migration readiness, port handling, post-processor) #1486

Open

Description

@duffney

Context

PR #1481 added the Helm chart that deploys the Scope app onto the Bicep-provisioned AKS cluster (#1477). Review surfaced several findings; the P1/quick ones were fixed directly in that PR (worker-scoped identity wiring, mutable latest image tags, migration image pinning, publish-images.yml path filters). The remaining findings below are valid but need more design work or a product decision, so they're tracked here for a follow-up PR instead of blocking #1481.

Remaining issues

1. Separate benchmark execution from the backend workload identity (P1)

Both ACP workers (coder-acp-copilot, coder-acp-claude-code) run benchmark-controlled agent code in the same container/pod that mounts the kv-secrets CSI volume and the common-secret envFrom. Per the companion Bicep work (#1477), the worker identity currently has Key Vault Secrets User (read-only) because the workers need their BYO GitHub token/Anthropic API key from the vault — but that means untrusted agent subprocess code (which merges the parent environment and shares its filesystem) can also read those secrets.

Needs: split the trusted queue processor from the untrusted executor (no creds/federated token in the executor, automountServiceAccountToken: false on the executor), which requires worker code changes beyond the chart. Already documented as a known limitation in deploy/helm/scope/WORKERS.md.

2. Preserve the API's required-migration readiness barrier (P1)

api/deployment.yaml's readiness probe (like its liveness probe) hits /health, which always returns 200 — it should use /ready, which checks required migrations. Combined with the db-migrate Job's current post-install,post-upgrade hook ordering, new API pods can receive traffic before required schema changes finish, and stay Ready even if the migration hook fails outright.

Needs: redesign the hook ordering (the Job currently runs post-install/post-upgrade specifically to avoid a chicken-and-egg problem with the ServiceAccount/SecretProviderClass not existing yet pre-install — see the comment in templates/jobs/db-migrate.yaml) alongside switching the readiness probe to /ready, so helm --wait doesn't deadlock.

3. Preserve existing worker settings when bootstrapping a single worker (P2)

scripts/bootstrap-workers.sh --skip-copilot --values-file <original-core-values-file> re-applies the original install-time values file without merging in a prior bootstrap run's worker overrides (e.g. workers.coderAcpCopilot.enabled=true). Since the original values file doesn't contain those overrides, Helm falls back to the chart default and disables the worker that's supposed to be left alone.

Needs: merge current chart defaults with the release's existing overrides when re-running bootstrap for a subset of workers (e.g. something like --reset-then-reuse-values, or reading helm get values to build the merge instead of relying solely on a user-supplied file). Add a sequential single-worker bootstrap regression test.

4. Include the post-processor in the completed-run pipeline (P2)

Both ACP workers enqueue to post-processor-queue and set run.postProcessorStatus: queued on completion, but neither this chart nor publish-images.yml includes apps/workers/post-processor or its version-registration mechanism. Enabling report-generator alone isn't sufficient — completed runs queue for post-processing with no consumer deployed, and automatic report triggering never happens.

Needs: publish/deploy the post-processor (new chart Deployment + add it to the publish workflow, with its version-registration step), or explicitly document/disable this pipeline for the minimal deployment.

5. Keep configurable ports consistent with the shipped nginx config (P2)

The portal image bakes in nginx.conf with listen 80 and proxy_pass http://api:80. The chart exposes portal.port/api.port as configurable values, but changing either doesn't update the baked-in nginx config, so overriding them breaks the portal (502s or unhealthy readiness).

Needs: either template the nginx listen/upstream config at container start (e.g. via apps/portal/docker-entrypoint.sh, which already does some env-driven config), or stop exposing portal.port/api.port as overridable since the shipped image can't honor arbitrary values.

Reference

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area: cicdBuild, test, release, and deployment pipelines.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions