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

AWS CLI Verison 2 support #40

Merged
merged 3 commits into from
Mar 5, 2020
Merged

AWS CLI Verison 2 support #40

merged 3 commits into from
Mar 5, 2020

Conversation

pda
Copy link
Contributor

@pda pda commented Mar 3, 2020

awscli v2.0.0 removed the aws ecr get-login command, in favour of the aws ecr get-login-password command which was introduced very recently in v1.17.10 via aws/aws-cli#4874 . In order for ecr-buildkite-plugin to support new and slightly-less-new versions of the AWS CLI, we need to decide which to use by version-checking.

Additionally, the new get-login-password no longer provides the ECR registry address(es), so we need to build them with knowledge of the AWS account ID and region.

AWS region is determined by:

  • (legacy registry-region plugin config)
  • region plugin config
  • AWS_DEFAULT_REGION environment
  • us-east-1 default.

AWS account IDs are determined by:

  • account-ids plugin config (YAML array, or legacy comma-separated)
  • aws sts get-caller-identity

If multiple account-ids are configured, a single aws ecr get-login-password call is made, and multiple docker login ... are made; one for each account/registry. This is correct according to aws/aws-cli#4874 (comment) and https://docs.aws.amazon.com/AmazonECR/latest/userguide/Registries.html#registry_auth:

The password that is returned is valid for any registry that your IAM principal has access to. We have updated our documentation to be more explicit about the authorization data that is returned in the get-authorization-token and get-login-password commands.

Fixes #37

@pda pda requested a review from a team March 3, 2020 11:30
Copy link
Contributor

@toolmantim toolmantim left a comment

Choose a reason for hiding this comment

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

Amazing work!

Related to my question in #39, could we drop support for aws-cli < 1.17.10 in this plugin release? Seems reasonable to me.

@pda
Copy link
Contributor Author

pda commented Mar 4, 2020

I think that would cause quite a bit of chaos, since 1.17.10 was only tagged 28 days ago.

We could however drop support for < 1.11.91 re. the --no-include-email nonsense, although that flag also has implications on docker version compatibility. But those are both from ~2017 so pretty safe to drop them soon, I imagine.

pda added 2 commits March 4, 2020 11:23
This commit adds the foundations to version-check awscli for
ecr get-login-password support, and send that down a different code
path. However the implementation is currently the same, and will not
work for awscli v2+
For awscli >= 1.17.10 we now use `aws ecr get-login-password` instead of
the deprecated (removed in 2.0.0) `aws ecr get-login`.  As a result, we
need to build the registry address, which means determining the AWS
region and account ID.

If the AWS region is not specified in the existing plugin config
options, AWS_DEFAULT_REGION is used, which default to us-east-1.

If the AWS account ID is not specified in the existing plugin config
options, it is (hopefully) found with `aws sts get-caller-identity`.
Copy link
Contributor

@toolmantim toolmantim left a comment

Choose a reason for hiding this comment

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

Cool, yeah having thought that through, that would be chaos.

🚀

@pda
Copy link
Contributor Author

pda commented Mar 4, 2020

Rebased, tests still look good:

 ✓ ECR login; configured account ID, configured region
 ✓ ECR login; configured account ID, configured legacy registry-region
 ✓ ECR login; configured account ID, discovered region
 ✓ ECR login; configured account ID, default region
 ✓ ECR login; multiple account IDs
 ✓ ECR login; multiple comma-separated account IDs
 ✓ ECR login; discovered account ID
 ✓ ECR login (v1.17.10; after get-login-password was added, before get-login was removed)
 ✓ ECR login (before aws cli 1.17.10 in which get-login-password was added)
 ✓ ECR login (before aws cli 1.17.10) (without --no-include-email)
 ✓ ECR login (before aws cli 1.17.10) with Account IDS
 ✓ ECR login (before aws cli 1.17.10) with Comma-delimited Account IDS (older aws-cli)
 ✓ ECR login (before aws cli 1.17.10) with Comma-delimited Account IDS (newer aws-cli)
 ✓ ECR login (before aws cli 1.17.10) with region specified
 ✓ ECR login (before aws cli 1.17.10) with region and registry id's
 ✓ ECR login (before aws cli 1.17.10) with error, and then retry until success
 ✓ ECR login (before aws cli 1.17.10) with error, and then retry until failure
 ✓ ECR login (before aws cli 1.17.10) doesn't disclose credentials
 ✓ version_a_gte_b: basic: major less; false
 ✓ version_a_gte_b: basic: major more; true
 ✓ version_a_gte_b: basic: major same, minor less; false
 ✓ version_a_gte_b: basic: major same, minor more; true
 ✓ version_a_gte_b: basic: major same, minor same, patch same; true
 ✓ version_a_gte_b: basic: major same, minor same, patch more; true
 ✓ version_a_gte_b: basic: major same, minor same, patch less; false
 ✓ version_a_gte_b: specific: 1.11.40 >= 1.11.91; false
 ✓ version_a_gte_b: specific: 2.0.2 >= 1.11.91; true
 ✓ version_a_gte_b: specific: 2.0.2 >= 2.0.0; true

28 tests, 0 failures

Otherwise AWS_DEFAULT_REGION (which defaults to us-east-1) is always
used for 'aws ecr get-login-password', but 'docker login' runs
against the region specified by plugin config. If they differ, the
docker login fails.

Leaving region completely unspecified produces a warning, but still
defaults to us-east-1 for compatibility.
@pda
Copy link
Contributor Author

pda commented Mar 4, 2020

I found a bug in my implementation; potential AWS region mismatch between the aws ecr get-login-password call (used AWS_DEFAULT_REGION) and the docker login endpoint (used the plugin-config region). Fixed.

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.

Incompatible with awscli v2
2 participants