SUP-4325: Implement mixed-format env file for automatic environment variable propagation#3430
SUP-4325: Implement mixed-format env file for automatic environment variable propagation#3430Mykematt wants to merge 4 commits into
Conversation
…ariable propagation - Add OTEL tracing variables to env file before writing - Move 29 agent config variables to be included in BUILDKITE_ENV_FILE - Implement mixed format: names-only for agent config, key=value for job vars - Enable automatic propagation to any plugin using propagate-environment - Maintain security boundaries by excluding sensitive tokens and agent paths - Validated with real pipeline execution showing 92 variables propagated Testing: - All tests pass (go test ./...) - Code formatted (go fmt ./...) - Real-world validation with standard plugins using propagate-environment
|
Thanks @Mykematt. This sounds like a backwards-incompatible change for anyone parsing the env file with the expectation that every variable has a value. Is that right? |
|
@DrJosh9000 oh yeah. That is correct. This way, there is no need to implement any changes piece-meal in different plugins. |
DrJosh9000
left a comment
There was a problem hiding this comment.
I think that this cannot be merged as-is, for two reasons.
One is the backwards compatibility with user scripts or programs that parse the env file. While they can now migrate to the better-defined JSON version of the file, this requires a recent agent version and work on their part to reimplement. They could just as easily adapt to the additional valueless env vars in the file, but either way we don't know whose builds we will break by doing this.
Two is the change to propagated environment variables itself. A user could have a pipeline set up with the Docker and Docker Compose plugins case (and any other plugin that might be using the env file), and, without making any change in version to their plugins, be surprised when they upgrade their agent version, with a bunch of extra env vars in their jobs. They may be relying on certain env vars not existing.
Both the Docker and Docker Compose plugin Readmes specifically call out:
Important: only pipeline environment variables will be propagated
and these extra env vars aren't "pipeline environment variables" per se, and that may be something someone is relying on.
To merge this, I would suggest either:
- writing an additional file that could be read by a future release of the Docker and Docker Compose plugins, or
- writing them to the existing env file under an experiment (see
internal/experiments).
…ironment variable propagation - Add PropagateAgentVars experiment flag in internal/experiments/experiments.go - Wrap agent config variables in experiment check in agent/job_runner.go - When enabled, includes 29+ agent config variables in BUILDKITE_ENV_FILE - Validated end-to-end with test pipeline showing successful propagation
3e3f1d5 to
39ebbc1
Compare
|
@DrJosh9000 thanks for the in-depth review. I have implemented the second approach you mentioned. Tested here |
DrJosh9000
left a comment
There was a problem hiding this comment.
Thanks, but this still needs quite a bit of work in its current form. I note that the tests now fail.
I think the idea here is: if the experiment is enabled, add some env var names to the start of the env file. Is that right?
If so, that seems like the part of the implementation that should be made conditional (i.e. conditionally add var names to the file), not what's implemented here (conditionally add env vars to env).
| } | ||
|
|
||
| // Pipeline signing variables - conditionally set based on experiment flag | ||
| if experiments.IsEnabled(ctx, experiments.PropagateAgentVars) { |
There was a problem hiding this comment.
This if condition is the same as on line 456, so they could be combined into a single if.
Modified job_runner.go to write agent config vars to env file when propagate-agent-vars experiment is enabled. All variables still set in environment for backward compatibility - only the env file writing is conditional. Reorganized existing BUILDKITE_GIT_* vars and added ~29 agent config variables to the propagation list. No new environment variables added. Testing: - Build 236: All integration tests pass - Docker/Docker Compose/Artifacts plugins work with propagation - Go tests pass except TestProcessOutputPTY_PTYRawExperiment (pre-existing flaky PTY test unrelated to env var changes) Addresses review feedback on backward compatibility and variable scoping.
|
Thanks @DrJosh9000 once again. I've pushed the changes that you requested. Added environment variable propagation for plugins. Modified job_runner.go to write agent config vars to env file when propagate-agent-vars experiment is enabled. All variables still set in environment for backward compatibility - only the env file writing is conditional. Reorganized existing BUILDKITE_GIT_* vars and added ~29 agent config variables to the propagation list. No new environment variables added. Variable groupings:
Testing:
Addresses review feedback on backward compatibility and variable scoping. |
|
@Mykematt Thanks, sorry for the delay in responding. This still has problems. If the experiment is not enabled, then all the agent config vars that are not present in the env file today, but are unconditionally added to the map before writing the file, would be written to the file. Ideally the file would be 100% unchanged unless the experiment is active. Rather than go back and forth again, I've taken a stab at implementing this myself here: #3471. Would you mind testing it to ensure it suits your purposes? |
Description
This PR implements automatic environment variable propagation for OTEL tracing variables and other agent configuration variables to any plugin using
propagate-environment: true. Agent config variables are written as names-only (allowing containers to pull values from the host environment) and job-specific variables are written as key=value pairs.Problem: OTEL tracing variables (
BUILDKITE_TRACING_*) and other agent config variables were not automatically available to plugins usingpropagate-environment: true, requiring manual plugin configuration for each variable.Solution: Modified the agent to include these variables in
BUILDKITE_ENV_FILEbefore writing, enabling automatic propagation to any plugin that usespropagate-environment: truewithout requiring plugin changes.Alternatives considered: Manual plugin-level propagation, but this requires maintaining variable lists in multiple plugins and doesn't scale well.
Context
Linear ticket SUP-4325: "[Pinterest/Docker & Docker Compose] Propagate BUILDKITE_TRACING_* Env Vars"
Changes
BUILDKITE_ENV_FILEBUILDKITE_TRACING_BACKENDBUILDKITE_TRACING_SERVICE_NAMEBUILDKITE_TRACING_PROPAGATE_TRACEPARENTBUILDKITE_TRACING_TRACEPARENTBUILDKITE_TRACE_CONTEXT_ENCODINGImpact: Any plugin using
propagate-environment: truenow automatically receives 92 variables instead of requiring manual configuration for each one.Testing
go test ./...). Buildkite employees may check this if the pipeline has run automatically.go fmt ./...)