Skip to content
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

Merged
merged 4 commits into from
Jun 26, 2024
Merged

Conversation

DrJosh9000
Copy link
Contributor

@DrJosh9000 DrJosh9000 commented Jun 25, 2024

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

  • In kubernetes/kubernetes.go: Replace AccessToken string with Env []string
  • In agent/job_runner.go: Provide processEnv to the Kubernetes socket server
  • In clicommand/bootstrap.go: Connect the socket as the very first thing it does, to get env vars in time for configuring the executor
  • Some cleanup

Testing

  • Tests have run locally (with go test ./...). Buildkite employees may check this if the pipeline has run automatically.
  • Code is formatted (with go fmt ./...)

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.
@DrJosh9000 DrJosh9000 requested a review from a team June 25, 2024 08:12
Copy link
Contributor

@zhming0 zhming0 left a 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.

Comment on lines 411 to 415
for n, v := range env.FromSlice(regResp.Env).Dump() {
if err := os.Setenv(n, v); err != nil {
return err
}
}
Copy link
Contributor

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:

  1. 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.
  2. Is there any environment variables that agent container have access but bootstrap containers don't/shouldn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Probably!
  2. 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.

Copy link
Contributor Author

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.

clicommand/bootstrap.go Outdated Show resolved Hide resolved
Too many Kubernetes-flavoured local APIs!
This would interfere with how the k8s stack operates. See comment.
@DrJosh9000 DrJosh9000 merged commit 2ece143 into main Jun 26, 2024
1 check passed
@DrJosh9000 DrJosh9000 deleted the ps-72-k8s-env-var-conveyor branch June 26, 2024 06:58
@DrJosh9000
Copy link
Contributor Author

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 EnvVar field on each flag as the flag default, so the env vars would need to be set even earlier in the process (before urfave/cli processes the flag definitions).

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.

None yet

2 participants