[AE-1241] publish new version of deployer #4
Conversation
WalkthroughAdds Terraform variables for nginx and feature flags, updates the deployer Docker base and tooling, bumps Helm chart versions, changes the PostgreSQL Helm release/config and PVC settings, introduces new secrets and nginx ingress wiring, removes some Makefile targets, and adds expanded Helm values files for deployment. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer/CI
participant TF as Terraform root
participant Module as stackgen module
participant Helm as Helm provider
participant K8s as Kubernetes
note over Dev,TF: Apply with new variables
Dev->>TF: terraform apply (nginx_config, enable_feature)
TF->>Module: pass nginx_config & enable_feature
Module->>Helm: render values (appcd YAML includes nginx, enable_ingress, secrets)
Helm->>K8s: install/upgrade charts (postgresql, temporal, dex, appcd, etc.)
Helm->>K8s: create Secrets (appcd_client_id/secret) and PVCs, configure Ingress rules
note right of K8s: Ingress and service routes provisioned per rendered values
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
deployer-image/terraform/modules/stackgen-installation/main.tf (2)
223-237: Hardcoding enable_ingress = true ignores module input.Use var.enable_ingress so callers can disable ingress.
- enable_ingress : true + enable_ingress : var.enable_ingress
158-166: SCM secret created with possibly empty data.If scm_configuration does not match github/gitlab/azuredev, local.final_scm_secrets is {}, and creating a Secret with empty data can fail. Gate resource on presence of keys.
-resource "kubernetes_secret" "appcd_scm_secrets" { +resource "kubernetes_secret" "appcd_scm_secrets" { + count = length(keys(local.final_scm_secrets)) > 0 ? 1 : 0 depends_on = [kubernetes_namespace.this] metadata { - name = "appcd-scm-secrets" - namespace = var.namespace + name = "appcd-scm-secrets" + namespace = var.namespace } - data = local.final_scm_secrets + data = local.final_scm_secrets }Also adjust references to this secret to handle count (e.g., only include when present).
deployer-image/terraform/values/temporal.yaml (1)
218-227: Fix invalid Ingress host and annotation usage
- Host must not include a path. Use
hosts: ["${domain}"]and define/internal/workloadunder apaths:block.- Remove
nginx.org/mergeable-ingress-type: minion/location-snippetsunless you pair them with a master Ingress for the NGINX Inc controller.- For community ingress-nginx, replace with
nginx.ingress.kubernetes.io/rewrite-targetand path-based routing.
🧹 Nitpick comments (6)
deployer-image/terraform/values/temporal.yaml (1)
28-31: Hard-coded ServiceMonitor label risks non-discovery.Label release: kube-prometheus-stack may not match your Prometheus stack release. Make this configurable or derive from var to avoid silent non-scrape.
- additionalLabels: - release: kube-prometheus-stack + additionalLabels: + release: ${prometheus_release_label}deployer-image/terraform/values/appcd-final.yaml (1)
1-4: Avoid committing environment-specific rendered values.This looks like a rendered artifact (final values for test.stackgen.local). Consider generating it in CI or .gitignoring it to prevent drift and accidental reuse in other environments.
Also applies to: 24-27, 49-62, 88-94, 95-117, 118-143, 144-159, 160-168, 165-174
deployer-image/terraform/values/appcd.yaml (2)
103-111: Probes disabled for iac-gen — reduces resilience.Disabling liveness/readiness can mask crashes and slow startups. Recommend enabling with conservative thresholds.
probes: - liveness: - enabled: false - readiness: - enabled: false + liveness: + enabled: true + initialDelaySeconds: 30 + periodSeconds: 10 + timeoutSeconds: 5 + failureThreshold: 6 + readiness: + enabled: true + initialDelaySeconds: 10 + periodSeconds: 10 + timeoutSeconds: 5 + failureThreshold: 6
24-27: Duplicated Temporal config blocks.You set both global.temporal and top-level temporal. If only one is consumed by charts, drop the duplicate to avoid drift.
Also applies to: 111-113
deployer-image/terraform/modules/stackgen-installation/main.tf (2)
73-95: Hardcoded GHCR dockerconfig values."username": "github_username" and "email": "support" are placeholders. Consider passing via variables and masking in state. Also prefer stringData for clarity.
- data = { + string_data = { ".dockerconfigjson" = jsonencode({ "auths" = { "https://ghcr.io" = { - "username" = "github_username" + "username" = var.ghcr_username "password" = var.STACKGEN_PAT - "email" = "support" + "email" = var.ghcr_email "auth" = base64encode("${var.ghcr_username}:${var.STACKGEN_PAT}") } } }) }
2-9: Pinning temporal_helm_version in locals reduces flexibility.Expose as a variable with default to ease upgrades/rollbacks.
variable "temporal_helm_version" { type = string default = "0.57.0" }Use var.temporal_helm_version in locals.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.gitignore(1 hunks)deployer-image/Dockerfile(2 hunks)deployer-image/Makefile(1 hunks)deployer-image/marketplace/manifests/application.yaml.template(1 hunks)deployer-image/marketplace/schema.yaml(1 hunks)deployer-image/terraform/Makefile(0 hunks)deployer-image/terraform/main.tf(2 hunks)deployer-image/terraform/modules/stackgen-installation/database.tf(2 hunks)deployer-image/terraform/modules/stackgen-installation/main.tf(8 hunks)deployer-image/terraform/modules/stackgen-installation/variables.tf(2 hunks)deployer-image/terraform/values/appcd-final.yaml(1 hunks)deployer-image/terraform/values/appcd.yaml(2 hunks)deployer-image/terraform/values/temporal.yaml(4 hunks)deployer-image/terraform/variables.tf(1 hunks)
💤 Files with no reviewable changes (1)
- deployer-image/terraform/Makefile
🔇 Additional comments (8)
.gitignore (1)
1-6: Solid Terraform ignore additions.
Covers the usual tfvars, state, cache, and lock artifacts so they stay out of git. Nicely done.deployer-image/terraform/values/temporal.yaml (2)
90-97: Double-check metrics scraping across all services.server.metrics.serviceMonitor is wired to ${enable_ops}, but per-service blocks have empty serviceMonitor objects. Confirm the chart enables monitors for frontend/history/matching/worker when enable_ops is true, or explicitly enable them to ensure coverage.
Also applies to: 112-119, 133-140, 155-161
61-61: No issue found. Thedriver: "postgres12"value is correct for Temporal Helm chart 0.57.0. Both the Temporal Helm charts README and self-hosted documentation confirm "postgres12" as the correct SQL driver value for PostgreSQL persistence configuration.deployer-image/terraform/values/appcd.yaml (2)
116-119: WORM storage tied to artifacts_support flag — verify semantics.iac-gen.storage.worm.enabled uses ${enable_feature.artifacts_support}. Confirm this is intentional (artifact support enabling WORM) vs. an independent toggle.
5-11: nginx config passthrough: ensure template keys align.Values read ${nginx.client_max_body_size}. Confirm var.nginx_config has client_max_body_size and the chart actually consumes nginx.* keys; otherwise this block is a no-op.
deployer-image/terraform/modules/stackgen-installation/main.tf (3)
65-72: Client ID/secret generation looks good.random_id for appcd_client_id/secret and wiring into appcd-secrets is sound.
Also applies to: 119-128
42-53: Dex chart bump to 0.19.1Version bump looks fine; values mapping unchanged.
182-198: Temporal depends_on includes Postgres — good ordering.Ensures DB exists before chart install.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
deployer-image/Dockerfile (3)
47-47: Pin pip dependencies for reproducible builds.Upgrading pip/setuptools/wheel to "latest" without version constraints makes builds non-reproducible and harder to audit. Consider specifying versions or at least a known-good baseline compatible with your environment:
- python3 -m pip install --upgrade pip setuptools wheel --no-cache-dir && \ + python3 -m pip install --upgrade pip==24.3.1 setuptools==75.1.0 wheel==0.42.0 --no-cache-dir && \Verify these versions address CVE-2024-35195 and CVE-2025-47273 before pinning.
9-11: Address yq version mismatch between stages.Stage 1 (test_schema_creator) uses Alpine's default yq version, while Stage 2 explicitly pins YQ_VERSION=4.44.6. This inconsistency could lead to schema processing differences. Either pin yq in Stage 1 or document why different versions are acceptable:
- RUN apk add --no-cache yq && \ + RUN apk add --no-cache yq==${YQ_VERSION} && \Or, if Alpine's yq version differs, ensure your schema processing logic is version-agnostic.
14-14: Verify base image stability and deprecation path.Line 14 updates to Debian 11 (good for security patches), and line 25 retains the
:latesttag for deployer_envsubst. While Debian 11 receives long-term support, the:latesttag in Stage 2 creates a reproducibility gap. Consider pinning to a specific image digest or version tag if available from Google Cloud Marketplace.Also applies to: 25-25
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
deployer-image/Dockerfile(2 hunks)
🔇 Additional comments (1)
deployer-image/Dockerfile (1)
30-52: Good security and cleanup practices.The expanded install block with system upgrades, security-focused package selection, and cleanup (removing wget/unzip and apt lists) effectively reduces the image attack surface and aligns with container security best practices. The explicit pip upgrade also addresses known CVEs.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
deployer-image/Dockerfile (1)
30-52: Add checksum verification for Terraform and yq downloads.Terraform and yq binaries are downloaded without integrity verification (checksums or GPG signatures) at lines 40–44. This reintroduces the MITM attack vector flagged in a previous review and leaves the build vulnerable to supply-chain compromise. Add verification to match HashiCorp and yq security best practices:
# Install Terraform wget https://releases.hashicorp.com/terraform/${TERRAFORM_VERSION}/terraform_${TERRAFORM_VERSION}_linux_amd64.zip && \ + wget https://releases.hashicorp.com/terraform/${TERRAFORM_VERSION}/terraform_${TERRAFORM_VERSION}_SHA256SUMS && \ + sha256sum -c terraform_${TERRAFORM_VERSION}_SHA256SUMS | grep linux_amd64 && \ unzip terraform_${TERRAFORM_VERSION}_linux_amd64.zip -d /usr/local/bin/ && \ rm terraform_${TERRAFORM_VERSION}_linux_amd64.zip && \ # Install yq with specific version wget -O /usr/local/bin/yq https://github.com/mikefarah/yq/releases/download/v${YQ_VERSION}/yq_linux_amd64 && \ + wget -O /usr/local/bin/yq.sha256 https://github.com/mikefarah/yq/releases/download/v${YQ_VERSION}/yq_linux_amd64.sha256 && \ + sha256sum -c /usr/local/bin/yq.sha256 && \ chmod +x /usr/local/bin/yq && \ rm /usr/local/bin/yq.sha256 && \Positive notes: The addition of
apt-get upgrade(line 32) to apply security patches and the pip upgrade (line 47) to fix CVE-2024-35195 and CVE-2025-47273 are good security improvements. The comprehensive cleanup block (lines 49–52) properly removes build tools and APT cache.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
deployer-image/Dockerfile(3 hunks)
🔇 Additional comments (1)
deployer-image/Dockerfile (1)
14-14: Base image security update.Upgrading from
debian10todebian11improves security posture and provides access to more recent packages and patches.
Summary by CodeRabbit
New Features
Chores
Chores