Skip to content

Feature/fix tf runs with artifacts#49

Open
sttomm wants to merge 3 commits into
mainfrom
feature/fix-tf-runs-with-artifacts
Open

Feature/fix tf runs with artifacts#49
sttomm wants to merge 3 commits into
mainfrom
feature/fix-tf-runs-with-artifacts

Conversation

@sttomm

@sttomm sttomm commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

This PR contains a few fixes that were identified during e2e integration of preflight runs

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adjusts how Terraform/OpenTofu runs handle predecessor plan artifacts and ephemeral credentials, to better support preflight DETECT → APPLY workflows and avoid embedding revoked tokens into saved plans.

Changes:

  • Switch meshStack HTTP backend authentication from backend-config headers (Bearer) to TF_HTTP_USERNAME/TF_HTTP_PASSWORD (Basic) so credentials are re-read fresh at plan/apply time.
  • Make plan-artifact vs fresh-apply paths explicit in user-visible step logs, and adapt scenario tests accordingly.
  • Deprecate run-scoped tfvars values for DETECT and saved-plan replays to avoid plan/input mismatches; add/adjust unit tests for these invariants.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tf-block-runner/tfrun/worker_scenario_test.go Updates expected user-visible logs to include the new “fresh apply” notice.
tf-block-runner/tfrun/tfcmd.go Moves backend auth to TF_HTTP_* env vars and changes meshStack variable emission/decls for saved-plan invariants.
tf-block-runner/tfrun/tfcmd_test.go Adds/updates tests for tfvars emission behavior and TF_HTTP_* env setup; updates backend golden expectations.
tf-block-runner/tfrun/tfapply.go Makes “apply predecessor plan” vs “fresh apply” user-visible via SystemMessage logs.
tf-block-runner/tfrun/testdata/backend.golden.tf Updates golden backend config to no longer contain auth headers.
go-meshapi-client/meshapi/client.go Removes same-origin enforcement for authenticated artifact downloads (security-sensitive).
go-meshapi-client/meshapi/client_test.go Removes the regression test that ensured cross-origin artifact URLs are rejected.
Comments suppressed due to low confidence (1)

go-meshapi-client/meshapi/client.go:131

  • DownloadArtifact now sends the client's Authorization header to whatever absolute URL is provided. Since artifactURL comes from an API response, this enables bearer-token leakage and runner-mediated SSRF if a malicious/buggy server ever returns a cross-origin href. Please restore a same-origin check (scheme + host match with c.baseURL) before calling setHeaders / issuing the request.
func (c *Client) DownloadArtifact(artifactURL string, w io.Writer) error {
	req, err := http.NewRequest(http.MethodGet, artifactURL, nil)
	if err != nil {
		return fmt.Errorf("download artifact %s: failed to create request: %w", artifactURL, err)
	}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread go-meshapi-client/meshapi/client_test.go
Comment thread tf-block-runner/tfrun/tfcmd.go
Comment thread tf-block-runner/tfrun/tfcmd.go Outdated

@grubmeshi grubmeshi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, see some stylistic improvements.

Comment thread go-meshapi-client/meshapi/client.go
Comment thread tf-block-runner/tfrun/tfapply.go
Comment thread tf-block-runner/tfrun/tfapply.go Outdated
Comment thread tf-block-runner/tfrun/tfcmd.go Outdated
Comment thread tf-block-runner/tfrun/tfcmd.go Outdated
Comment thread tf-block-runner/tfrun/tfcmd.go Outdated
Comment thread tf-block-runner/tfrun/tfcmd.go Outdated
Comment thread tf-block-runner/tfrun/tfcmd.go Outdated
Comment thread tf-block-runner/tfrun/tfcmd.go Outdated
Comment thread tf-block-runner/tfrun/tfcmd_test.go
@sttomm sttomm force-pushed the feature/fix-tf-runs-with-artifacts branch from eb6ea68 to 427ca79 Compare July 3, 2026 07:10
sttomm added 3 commits July 3, 2026 15:41
OpenTofu embeds the backend config verbatim into any saved plan, so a
Bearer header baked into the backend block would leak into saved plans
and could be replayed long after the token is revoked (a predecessor
plan may now be applied days/weeks after the DETECT run that created
it). Supply the run token via TF_HTTP_USERNAME/TF_HTTP_PASSWORD instead,
which OpenTofu re-reads fresh at every plan/apply. meshfed-api's
TfStateRunTokenBasicAuthFilter recognizes the fixed sentinel username
and rewrites the Basic password back to a Bearer token for the state
endpoint.
…y with artifact runs

A saved plan embeds the exact variable values it was created with, but
terraform still re-reads any *.auto.tfvars on disk when applying a saved
plan and rejects the apply with "Mismatch between input and plan variable
value" whenever a value differs. This runner regenerates its meshStack
auto.tfvars with run-specific values (e.g. meshstack_building_block_run_id)
on every run, which necessarily differ between the DETECT run that produced
the plan and the APPLY run consuming it, so they are now removed from the auto tfvars in that case

Also add PrintlnUser/PrintflnUser to surface which apply path was taken
(predecessor plan vs. fresh apply) in the SystemMessage the user sees in
meshStack, not just the local runner logs.
this did not work locally as we use the mux proxy that runs on a different port than what is provided as localhost urls from teh hal link. In the end, it was a very safe guard solution before, but I don't see it being relevant for our application.
@sttomm sttomm force-pushed the feature/fix-tf-runs-with-artifacts branch from 427ca79 to 442876f Compare July 3, 2026 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants