Isolate docker configuration per step

[patrobinson]

This avoids a couple of issues:

Allow steps to use credentials from other steps

People may forget to configure their pipelines with ECR login and wonder why the build sometimes fails with "No basic auth credentials", when in fact their pipeline was never configured correctly.
By isolating credentials we make this behaviour more predictable, incorrectly configured pipelines will never work (rather than sometimes not working).

Causes steps to use incorrect credentials

If they are over-written by another step running concurrently on the same agent. If Step A has read/write credentials and Step B has only read credentials, if Step A logs in first, then Step B logs in immediately after, future commands run by Step A have only read credentials.

[toolmantim]

Sounds very similar to buildkite-plugins/docker-login-buildkite-plugin#31

I'd think the current behaviour, where it leaks docker login credentials between jobs, would be considered a straight up bug, and deserves a fix without introducing another stack parameter? What do you think @patrobinson?

I think at the moment, I'd suggest we update that plugin PR to use mktemp -d as you've done here, merge and release that, and then have the elastic stack use that plugin? And not introduce any new stack parameter?

[patrobinson]

@toolmantim there's a risk that we break builds that depend on credential caching, as they do not explicitly authenticate.
We know that some builds in our environment already depend on this and we'll have to role it out carefully and fix builds that break as a result of it.

That's why I think it needs a parameter, it is a breaking change that can impact builds. It might be worth in the future making it a default.

[toolmantim]

Thanks for the extra context @patrobinson — that's good to hear!

I was considering this more of a credential leak / security fix, so although it was breaking, it was necessarily breaking it because some pipelines might be unexpectedly running with elevated privileges.

In that light, I'd lean more towards having it as a breaking change, but perhaps a minor version bump to go along with it?

[patrobinson]

I'm happy if you want me to make this turned on without a parameter

[pda]

Hmm. I think v4.x should have the parameter, and master (and future v5.x) should remove the parameter. I'll figure out the best sequence of commit/merge/backport to make it so! Thanks @patrobinson.

[toolmantim]

I’ll get a new major version of the docker login published with the change, and you can switch that version accordingly from the stack.

[patrobinson]

@toolmantim any update on this?

[toolmantim]

I just did some more digging into this, and trying to fix it at the Docker Login plugin level. I think the approach in this pull request is actually a better way to go, given it needs to affect both the ECR plugin and the Docker plugin.

[patrobinson]

FWIW we've been running this in production for a few months and it works well and prevents a race condition that was happening rather frequently as we run ~30,000 jobs a week and 6-8 agents per instance.

[toolmantim]

Good to hear! We're reviewing this for you now @patrobinson

[yob]

@patrobinson - a version of this has merged in #756 and we're aiming to include it in v5.0.0. Thanks for the PR, and for your patience 🙏