Repository navigation
Conversation
83333e7 to
2e5ee4b
Compare
There was a problem hiding this comment.
Copilot was unable to review this pull request because there is no eligible user to bill.
An enterprise owner can allow the organization to be billed for reviews like this by enabling Copilot code review for users without a license.
alliscode
left a comment
alliscode
left a comment
There was a problem hiding this comment.
I found several production security and operability issues around database permissions, secret handling, setup identity, storage readiness, and release logging. Please address the inline comments.
| owner = var.owner | ||
| managed_by = "terraform" | ||
| } | ||
| database_url = "postgresql://replicaadmin:${replace(urlencode(var.postgres_admin_password), "+", "%20")}@${azurerm_postgresql_flexible_server.database.fqdn}:5432/replica?sslmode=verify-full" |
There was a problem hiding this comment.
This gives the long-running Hub process the PostgreSQL administrator account, not just the one-time setup job. If the app is compromised, that credential can run migrations, change roles, or drop schemas. Could we create a separate least-privilege runtime principal and reserve the admin or migration credential for setup?
There was a problem hiding this comment.
Hub now receives a Key Vault-backed replica_runtime URL, and setup receives a separate replica_migration URL. Fresh-installation role provisioning is an explicit private-network operational script, never workload startup. Setup refreshes runtime DML/sequence/function grants after migrations without granting permanent schema creation, role administration, or ledger writes. Neither workload receives the administrator credential. See 280faa7 and the database guide.
| } | ||
| } | ||
|
|
||
| variable "runtime_secrets" { |
There was a problem hiding this comment.
Can we pass managed secret references here instead of raw secret values? Every value in this map is stored in Terraform state and copied into Container Apps secrets. For the Azure profile, Hub should use its workload identity to resolve Key Vault or equivalent references so state contains IDs, not application secrets.
There was a problem hiding this comment.
Replaced raw runtime values with existing Key Vault references resolved natively by Container Apps using the assigned identity. Grants are scoped to individual secrets, including when setup/runtime secrets share a vault. Terraform does not read application secret values. The provisioning administrator password still enters protected state, and the guide explicitly covers it and historical state retention. See 280faa7.
|
|
||
| identity { | ||
| type = "UserAssigned" | ||
| identity_ids = [azurerm_user_assigned_identity.hub.id] |
There was a problem hiding this comment.
Could setup use its own identity and only the settings it needs? This job gets the same Blob contributor identity as Hub, and the blocks below also inject every runtime secret, even though setup only runs database and job migrations. A separate setup identity with image-pull and migration access would reduce the blast radius.
There was a problem hiding this comment.
Setup now has its own identity and optional ACR pull grant, with access only to its migration secret. It receives only DATABASE_URL, NODE_ENV, RUNTIME_MODE, and the fixed runtime-grant target. Artifact access and additional runtime secrets remain exclusive to Hub. See 280faa7.
| signal: AbortSignal.timeout(10_000), | ||
| }) | ||
| await response.body?.cancel() | ||
| if (response.status === 200) return |
There was a problem hiding this comment.
/api/ready currently checks PostgreSQL and the workers, but startup only creates the Blob client; it never verifies the managed identity, role assignment, account, or container. This can release the deployment lock while artifact storage is unusable. Could readiness include a bounded, non-writing storage check when Azure storage is configured?
There was a problem hiding this comment.
Configured storage now participates in API and combined readiness. Azure checks the configured container properties using managed identity, with an abort signal and a two-second outer deadline; failures return 503 through the readiness route. The check never writes/deletes blobs and does not claim to prove write/delete authorization. Worker readiness remains initialization-only as agreed. See 280faa7.
| try { | ||
| await deployHub() | ||
| } catch (error) { | ||
| console.error("Deployment stopped:", error instanceof Error ? error.message : "Unknown failure") |
There was a problem hiding this comment.
execFileSync includes the full command line in error.message. Some commands above pass the release lease ID as an argument, so a failed lease release or delete can print that ID here. Could the command wrapper turn child-process failures into a safe message that leaves out the arguments?
There was a problem hiding this comment.
The command wrapper now captures stdout/stderr and replaces child failures with a static executable-specific error without arguments, raw diagnostics, or a cause. JSON parse failures are sanitized too. Lease retention and recovery behavior remain unchanged. See 280faa7.
Provision durable Azure resources with Terraform and serialize maintenance releases around the separate setup job. Document protected state, ownership, recovery, and teardown without changing the devcontainer. Co-authored-by: Ben Thomas <25218250+alliscode@users.noreply.github.com> Co-authored-by: Nishiki <1121087+nshki@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace Python release orchestration and tests with native Node.js TypeScript. Keep Terraform's native tests and existing release safety gates. Co-authored-by: Ben Thomas <25218250+alliscode@users.noreply.github.com> Co-authored-by: Nishiki <1121087+nshki@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use separate database roles and workload identities, native Key Vault references, bounded artifact readiness, and private command diagnostics. Co-authored-by: Ben Thomas <25218250+alliscode@users.noreply.github.com> Co-authored-by: Nishiki <1121087+nshki@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2e5ee4b to
280faa7
Compare
Why
Operators need an Azure Hub installation with durable data and setup before rollout. This is the third layer of #171; application code remains independent of Terraform and Azure deployment.
What changes
Terraform under
infrastructure/azure/provisions Container Apps, private PostgreSQL, authenticated Blob storage, separate Hub/setup identities, logging, and a manual setup job. Operators supply an existing image digest, a separate protected state backend, and existing Key Vault secret references. Only Hub receives artifact access; setup receives only its migration credential and required settings.A one-time private-network script creates separate migration and runtime database roles. Setup applies migrations and refreshes runtime data permissions before rollout. Administrator credentials never enter either workload. Application secret values stay outside Terraform; the PostgreSQL provisioning password still requires protected state.
One TypeScript release script holds a deployment lock, stops old replicas, runs setup, then starts the same digest. Readiness includes a bounded, non-writing storage check. Command failures do not expose arguments or child output. This deliberately uses a maintenance window, not zero-downtime rollout or scale-out. The guide covers ownership, secrets, domains, recovery, and guarded teardown.
Real Azure provisioning, identity access, revision behavior, custom TLS, restore, and teardown remain unverified. Credential-free checks do not replace that tenant exercise.
Created from a Slack conversation with @alliscode.