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

Optionally enable the ecr credential helper plugin #763

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bshelton229
Copy link
Contributor

@bshelton229 bshelton229 commented Nov 3, 2020

This allows optionally enabling https://github.com/awslabs/amazon-ecr-credential-helper as mentioned in #676 . This plugin's cache should write to the appropriate home folder and should be shared between builds as it's not in the ephemeral DOCKER_HOME. That's my desired behavior, but I'm not 100% sure that's what everyone would want. I think this has the benefit over the ECR plugin of both maintaining a cache between builds and also working against any ECR repo you have access to without having to pre-populate the appropriate registry IDs. I should note I've had a little bit of trouble with this plugin's interactions with docker-compose, but that was years ago. I believe the docker project has resolved those issues with how it calls credential helpers at this point. This worked with the docker and docker-compose plugins for me in testing.

Overview:

  • Installs the credential helper no matter the plugin setting. The binary won't do anything when it's not used and it's only 8mb and some change.
  • Writes an empty {} config.yaml in the temp DOCKER_HOME so it can be modified later by various things if necessary.
  • Follows the standard

Known issues:

I have almost no Windows knowledge. I'm hoping to get some feedback on this general pattern before attempting to get this working in Windows at well.

I'm not sure if this is better off being made into a more general plugin and then incorporated into this stack via git submodules or not? I opted to bake it right in.

This configuration also precludes any other docker auth, like dockerhub. To make that work it would need to configure the credHelpers object to only work on certain configured ECR URLs. That would probably be best done in a separate plugin that was incorporated and configured via a git submodule? We currently only use ECR as an authenticated repo, so this meets that need.

@yob
Copy link
Contributor

yob commented Nov 4, 2020

disclaimer: I'm not super familiar with docker credential stores or helpers, so some of this comment might be naive.

This is great, thanks for starting the discussion. Automatically using the instance credentials when accessing ECR seems like a great usability win.

Installs the credential helper no matter the plugin setting

👍

I think this has the benefit over the ECR plugin of both maintaining a cache between builds

Could you describe the caching in more detail? What will be cached across builds, the credentials?

I'm not sure if this is better off being made into a more general plugin and then incorporated into this stack via git submodules or not? I opted to bake it right in.
This configuration also precludes any other docker auth, like dockerhub. To make that work it would need to configure the credHelpers object to only work on certain configured ECR URLs. That would probably be best done in a separate plugin that was incorporated and configured via a git submodule? We currently only use ECR as an authenticated repo, so this meets that need.

Given the new rate limits being imposed on unauthenticated docker hub pulls as of yesterday, I think it'd be a shame to make it impossible to authenticate to docker hub.

I guess that means credHelpers are our best bet, but we'd need a way to specify the specific registries that will use ecr-login 🤔

I'd want to run this idea by some colleagues before we commit to it, but what if we added a new ENV var to the existing ecr-login plugin that looks something like this: BUILDKITE_PLUGIN_ECR_CREDHELPER_REGISTRIES="aws_account_id1.dkr.ecr.region.amazonaws.com,aws_account_id2.dkr.ecr.region.amazonaws.com"?

That'd be useful for folks who use the ecr plugin directly. For folks using it via the elastic stack though, it'd mean setting the new ENV in an environment hook. Similar to the existing elastic stack docs. It's a shame wild card credHelpers aren't possible (like *.amazonaws.com).

Would that work for your team? I know it doesn't align with this comment :(

I think this has the benefit over the ECR plugin of both maintaining a cache between builds and also working against any ECR repo you have access to without having to pre-populate the appropriate registry IDs

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.

2 participants