Fix Helm chart pre-mounted Terraform binary path mismatch#11880
Open
willdavsmith wants to merge 6 commits into
Open
Fix Helm chart pre-mounted Terraform binary path mismatch#11880willdavsmith wants to merge 6 commits into
willdavsmith wants to merge 6 commits into
Conversation
The terraform-init init container in dynamic-rp and rp deployments was
writing the pre-downloaded Terraform binary to {terraform.path}/terraform
with a marker at {terraform.path}/.terraform-source. However,
ensureGlobalTerraformBinary in pkg/recipes/terraform/install.go looks for
the binary at /terraform/.terraform-global/terraform with a marker at
/terraform/.terraform-global/.terraform-ready.
Because the paths did not match, the runtime always fell through to
downloadAndInstallTerraform and downloaded Terraform from
releases.hashicorp.com on every cold start, silently wasting the work
done by the init container.
Update both init container scripts to write the binary and marker file
to the paths expected by install.go. The legacy path is kept as a copy
for any out-of-tree tooling that may still reference it.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Helm chart init container scripts for rp and dynamic-rp to write the pre-downloaded Terraform binary and readiness marker into the global path expected by the Terraform installer logic (pkg/recipes/terraform/install.go), preventing unnecessary re-downloads on cold start.
Changes:
- Write Terraform to
<terraform.path>/.terraform-global/terraformand create<terraform.path>/.terraform-global/.terraform-ready. - Retain the legacy copy at
<terraform.path>/terraformand legacy marker<terraform.path>/.terraform-source. - Add an in-script comment pointing maintainers to the Go constants to reduce future drift.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| deploy/Chart/templates/rp/deployment.yaml | Update terraform init container to populate the global Terraform binary/marker paths (and keep legacy copies). |
| deploy/Chart/templates/dynamic-rp/deployment.yaml | Same global-path update for dynamic-rp terraform init container (and keep legacy copies). |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11880 +/- ##
==========================================
+ Coverage 51.41% 51.71% +0.29%
==========================================
Files 716 726 +10
Lines 45106 45608 +502
==========================================
+ Hits 23191 23585 +394
- Misses 19719 19796 +77
- Partials 2196 2227 +31 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…andling for marker touch
…tests
Copilot review flagged that even with the global path hard-coded in the
init script, the volume mount and pre-mount directory creation still use
.Values.<svc>.terraform.path. If a user overrides that value, the shared
emptyDir is no longer mounted at /terraform and the runtime (which
hard-codes /terraform/.terraform-global in pkg/recipes/terraform/install.go)
will not see the pre-downloaded binary, silently reintroducing the
download-on-cold-start bug.
Fix:
- Add a Helm 'fail' guard in both rp/deployment.yaml and
dynamic-rp/deployment.yaml that errors at template-render time when
<svc>.terraform.path != "/terraform" and terraform pre-download is
enabled. This makes the unsupported override loud instead of silent.
- Add a new helm-unittest suite (tests/terraform_init_test.yaml) that
asserts the rendered init container script writes to
/terraform/.terraform-global/{terraform,.terraform-ready}, verifies
the fail guard fires for both deployments, and verifies the guard is
bypassed when global.terraform.enabled=false.
Match the convention used in deploy/Chart/tests/helpers_test.yaml so each assertion explicitly states which rendered template it validates, rather than relying on the test-level 'template:' key. The suite-level 'templates:' list is kept so helm-unittest knows which templates are in scope for the suite.
The previous fail guard only fired when global.terraform.enabled=true, but the runtime hard-codes /terraform regardless: the rp and dynamic-rp configmaps both set terraform.path: "/terraform", and pkg/recipes/terraform/install.go uses /terraform/.terraform-global. Allowing the override when pre-download is disabled produced an inconsistent deployment (volume mounted elsewhere while the runtime still looked in /terraform). Move the fail guard out of the global.terraform.enabled block to the top of both deployment templates so it runs on every render. Update the test suite to assert failure even when pre-download is disabled, and update the error message to reference the configmap as well so the root cause is discoverable from the failure.
After PR radius-project#11880 fixed the binary-path mismatch, the dynamic-rp and rp containers actually consume the pre-mounted Terraform binary written by the chart's terraform-init init container. However, the init script was still fetching 'latest' from the HashiCorp API while the runtime download fallback (pkg/recipes/terraform/install.go) pins to terraformVersion (1.14.9) in version.go. This silent version mismatch broke Test_TerraformRecipe_Context and Test_TerraformRecipe_KubernetesRedis in functional tests on every commit of this PR: 'latest' Terraform handled the recipe modules' 'context' variable injection differently than 1.14.9. Fix: - Add global.terraform.version (default '1.14.9') to deploy/Chart/values.yaml. - Update both terraform-init init container scripts to build the download URL from $TERRAFORM_VERSION instead of querying the HashiCorp 'latest' API. global.terraform.downloadUrl still overrides. - Add helm-unittest assertions that the rendered script contains TERRAFORM_VERSION="1.14.9". - Add TestTerraformVersionMatchesHelmChart Go test that asserts the chart default matches terraformVersion in version.go, so future bumps of either side fail fast in CI instead of in functional tests.
Radius functional test overviewClick here to see the test run details
Test Status⌛ Building Radius and pushing container images for functional tests... |
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
The
terraform-initinit container in thedynamic-rpandrpdeployments writes the pre-downloaded Terraform binary to a path that the runtime doesn't look at, so the containers silently re-download Terraform fromreleases.hashicorp.comon every cold start.Root cause
Two paths must agree, and they don't:
Producer —
deploy/Chart/templates/{dynamic-rp,rp}/deployment.yaml:{{ .Values.<svc>.terraform.path }}/terraform→/terraform/terraform{{ .Values.<svc>.terraform.path }}/.terraform-sourceConsumer —
pkg/recipes/terraform/install.go:ensureGlobalTerraformBinaryonly checksdefaultGlobalMarkerFile. When absent it falls through todownloadAndInstallTerraform, which callsreleases.hashicorp.com. The pre-mounted binary at/terraform/terraformis never consulted.Fix
Change the chart, not the Go code. The Go constants are also referenced by tests and non-K8s install paths — the chart is the side that diverged.
The init container scripts now write the binary and marker to the paths expected by
install.go:$GLOBAL_DIR/terraform$GLOBAL_DIR/.terraform-readyA copy at the legacy
{terraform.path}/terraformpath and the.terraform-sourcemarker are retained for any out-of-tree tooling that may reference them.A comment is added pointing at the Go constants so future drift is caught at review time.
Verification
End-to-end check after install (should report whatever version was set via
global.terraform.downloadUrl, not the version compiled intopkg/recipes/terraform/version.go):Scope
install.go.downloadAndInstallTerraformis preserved.