-
Notifications
You must be signed in to change notification settings - Fork 300
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Convey env vars between k8s containers #2851
Conversation
BUILDKITE_AGENT_ACCESS_TOKEN is one of them, so we no longer need to pass that individually. It was also present as a field in Status for some reason? I've included some cleanup in startKubernetesClient.
f070c6f
to
36940e5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No test coverage on this code path makes me a bit nervous but the change looks very reasonable and solid.
It's good to put more controls in agent.
clicommand/bootstrap.go
Outdated
for n, v := range env.FromSlice(regResp.Env).Dump() { | ||
if err := os.Setenv(n, v); err != nil { | ||
return err | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only issues I can think of for this are:
- now many environment variables set by our k8s controller as part of the pod spec will be entirely useless. We might want to eliminate those logic in k8s stack.
- Is there any environment variables that agent container have access but bootstrap containers don't/shouldn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Probably!
- In theory, everything the agent container passes this way to the bootstrap container would, if not using the k8s stack, be set specifically on the bootstrap subprocess forked by the agent. So I don't imagine there's anything the bootstrap shouldn't have access to. Definitely something to think about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Point 2 was a good callout - after thinking on it for a bit, some of the vars normally set by the agent would interfere with the variables set by the k8s stack if they were set after the bootstrap is forked.
Too many Kubernetes-flavoured local APIs!
b166629
to
f7f77fe
Compare
This would interfere with how the k8s stack operates. See comment.
f7f77fe
to
15f4a0e
Compare
I did some testing after merge (it was slightly easier to grab an edge build from Docker Hub last night) and I discovered that despite setting env vars as the first thing inside the bootstrap command, it's still not early enough. urfave/cli uses the |
Description
Pass environment vars through the k8s socket
Context
https://linear.app/buildkite/issue/PS-72/make-buildkite-agent-id-available-within-k8s-stack-containers
Changes
AccessToken string
withEnv []string
processEnv
to the Kubernetes socket serverTesting
go test ./...
). Buildkite employees may check this if the pipeline has run automatically.go fmt ./...
)