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

always use a fresh docker config for each step #756

Merged
merged 7 commits into from
Oct 26, 2020

Conversation

yob
Copy link
Contributor

@yob yob commented Oct 26, 2020

This improves job isolation, preventing jobs from relying on authentication configured in other jobs.

This is based on PR #678, but we've removed the stack parameter because the v5.0.0 release is pending and we're comfortable making a small breaking change.

Patrick Robinson and others added 2 commits October 26, 2020 16:37
This avoids potential race condition that:
- Allow steps to use credential from other steps
- Causes steps to use incorrect credentials, if they are over-written by another step
This improves job isolation, preventing jobs from relying on
authentication configured in other jobs.

This is based on PR #678, but we've removed the stack parameter because
the v5.0.0 release is pending and we're comfortable making a small
breaking change.
Copy link
Contributor

@chloeruka chloeruka left a comment

Choose a reason for hiding this comment

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

Straightforward change to improve step isolation. We've discussed this and decided it's better to make this (possibly) breaking change now as part of v5 release, and to bugfix it later if necessary. Original PR (#678) says this has been working well for them for quite some time.

@lox
Copy link
Contributor

lox commented Oct 26, 2020

Couldn't we do this in the environment hook, or a bootstrap script?

@yob
Copy link
Contributor Author

yob commented Oct 26, 2020

@lox are you suggesting this doesn't need to be baked in for everyone?

yob and others added 3 commits October 26, 2020 17:05
Setting it directly in the agent environment hook means we don't need to
escape the eval. It also means we won't cat it out into build output
with the rest of cfn-env.

While I was there I also made the same change to windows.
This also fixes a shellcheck error SC2155
Instead of setting DOCKER_CONFIG directly, set
BUILDKITE_DOCKER_CONFIG_TEMP_DIRECTORY and hydrate DOCKER_CONFIG for
plugin steps. if this gets overriden by the user, they can manage their
own DOCKER_CONFIG cleanup activities and we will always remove the
temporary directory from the agent instance filesystem.
@chloeruka chloeruka self-assigned this Oct 26, 2020
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

3 participants