Add AWS Elastic Container Service authentication support #876

toelen · 2017-08-29T02:21:27Z

No description provided.

codecov-io · 2017-08-29T02:43:09Z

Codecov Report

Merging #876 into master will decrease coverage by 0.32%.
The diff coverage is 66.66%.

@@             Coverage Diff              @@
##             master     #876      +/-   ##
============================================
- Coverage     66.39%   66.06%   -0.33%     
- Complexity      742      778      +36     
============================================
  Files           166      171       +5     
  Lines          3139     3300     +161     
  Branches        357      376      +19     
============================================
+ Hits           2084     2180      +96     
- Misses          898      952      +54     
- Partials        157      168      +11

ben-gineer · 2017-09-14T11:27:04Z

It would be great to see this in the docker client

davidxia

@toelen Thanks so much for this PR.

In general, I'm not sure if we want to add more vendor specific libraries to this repo or if we want to pull out this support to separate libraries that can be plugged in.

See spotify/dockerfile-maven#54.

@mattnworb thoughts?

davidxia · 2017-11-12T19:03:45Z

src/main/java/com/spotify/docker/client/auth/ecr/ContainerRegistryAuthSupplier.java

+  private static final Logger log = LoggerFactory.getLogger(ContainerRegistryAuthSupplier.class);
+
+  /**
+   * Constructs a ContainerRegistryAuthSupplier using the Application Default Credentials.


Is it accurate to use "Application Default Credentials" here or should we simply call it "default AWS credentials"?

IMO, AWS Default Credential Provider Chain is probably the most correct.

davidxia · 2017-11-12T19:08:58Z

src/main/java/com/spotify/docker/client/auth/ecr/ContainerRegistryAuthSupplier.java

+      return RegistryConfigs.empty();
+    }
+
+    final Map<String, RegistryAuth> configs = new HashMap<String, RegistryAuth>(1);


nit: You can omit the explicit type args here in new HashMap<>.

davidxia · 2017-11-12T19:19:08Z

src/test/java/com/spotify/docker/client/auth/ecr/ContainerRegistryAuthSupplierTest.java

+      ecrClient, clock, minimumExpirationSecs, credentials));
+
+  private static Matcher<RegistryAuth> matchesAccessToken(final AuthorizationData accessToken) {
+    String decoded = new String(Base64.decode(accessToken.getAuthorizationToken()));


nit: You can also add final here.

davidxia · 2017-11-12T19:20:01Z

src/test/java/com/spotify/docker/client/auth/ecr/ContainerRegistryAuthSupplierTest.java

+  private final DateTime expiration = new DateTime(2017, 5, 23, 16, 25);
+  private final String tokenValue = new String(encode("test-username:test-password".getBytes()));
+
+  private AmazonECR ecrClient = mock(AmazonECR.class);


nit: Can also be final.

davidxia · 2017-11-12T19:20:15Z

src/test/java/com/spotify/docker/client/auth/ecr/ContainerRegistryAuthSupplierTest.java

+
+  @Before
+  public void before() {
+    GetAuthorizationTokenResult res = new GetAuthorizationTokenResult()


nit: Can be final.

davidxia · 2017-11-12T20:03:55Z

src/main/java/com/spotify/docker/client/auth/ecr/ContainerRegistryAuthSupplier.java

+
+    // Don't refresh if expiration time hasn't been provided.
+    if (expirationTime == null) {
+      return true;


Do we need to test this? Will accessToken.getExpiresAt() ever be null?

It's technically nullable - https://github.com/aws/aws-sdk-java/blob/master/aws-java-sdk-ecr/src/main/java/com/amazonaws/services/ecr/model/AuthorizationData.java

davidxia · 2017-11-12T20:05:49Z

src/test/java/com/spotify/docker/client/auth/ecr/ContainerRegistryAuthSupplierTest.java

+  private final Clock clock = mock(Clock.class);
+  private final int minimumExpirationSecs = 30;
+  private final EcrCredentials credentials = spy(new EcrCredentials(ecrClient, authData1));
+  private final ContainerRegistryAuthSupplier supplier = spy(new ContainerRegistryAuthSupplier(


I don't think you need spy() here. See https://gist.github.com/davidxia/ad4b35ae0777c30726afabe0a77490c0

mhermosi · 2018-01-09T21:30:40Z

Any news on this MR? any time frame for its availability?

smuryginim · 2018-03-01T13:10:03Z

Hey team. Let merge this PR?

davidxia · 2018-03-01T15:48:25Z

This PR doesn't seem ready yet to merge. I at least still had some comments and unresolved questions above. Original author hasn't responded yet.

toelen · 2018-03-02T08:28:48Z

@davidxia I fixed your remarks on https://github.com/toelen/docker-client/tree/feat/ecr-auth

mattnworb · 2018-03-02T15:40:28Z

Sorry for the late reply.

In general, I'm not sure if we want to add more vendor specific libraries to this repo or if we want to pull out this support to separate libraries that can be plugged in.

See spotify/dockerfile-maven#54.

@mattnworb thoughts?

I think the best thing would be to structure these things as different modules, meaning change docker-client to be a multi-module maven build with a "core" module that contains all the existing code minus the GCR part, which can go in a separate module, and then ECR or other RegistryAuthSuppliers can all go in different modules. But I am indifferent about if that is a prerequisite for merging this change or not.

acmcelwee · 2018-03-08T17:28:10Z

src/main/java/com/spotify/docker/client/auth/ecr/ContainerRegistryAuthSupplier.java

+  private static final Logger log = LoggerFactory.getLogger(ContainerRegistryAuthSupplier.class);
+
+  /**
+   * Constructs a ContainerRegistryAuthSupplier using the Application Default Credentials.


IMO, AWS Default Credential Provider Chain is probably the most correct.

acmcelwee · 2018-03-08T18:22:45Z

src/main/java/com/spotify/docker/client/auth/ecr/ContainerRegistryAuthSupplier.java

+  public RegistryAuth authFor(final String imageName) throws DockerException {
+    final String[] imageParts = imageName.split("/", 2);
+
+    if ((imageParts.length < 2) || !imageParts[0].contains(".dkr.ecr.")


What about just doing a regex on the ECR repo format? Something along the lines of ^\d+\.dkr\.ecr\.[a-z-0-9]+\.amazonaws\.com\/

acmcelwee · 2018-03-08T18:48:23Z

src/test/java/com/spotify/docker/client/auth/ecr/ContainerRegistryAuthSupplierTest.java

+        expiration.minusSeconds(minimumExpirationSecs + 1).getMillis());
+
+    assertThat(
+        supplier.authFor("1234567890.dkr.ecr.eu-west-1.amazonaws.com/foobar/barfoo:latest"),


All of these image names seem to be the wrong format, to me. Shouldn't it be 1234567890.dkr.ecr.eu-west-1.amazonaws.com/foobar:latest?

acmcelwee · 2018-03-08T18:48:43Z

src/test/java/com/spotify/docker/client/auth/ecr/ContainerRegistryAuthSupplierTest.java

+    doReturn(true).when(supplier).needsRefresh(Matchers.any(AuthorizationData.class));
+
+    assertThat(
+        supplier.authFor("1234567890.dkr.ecr.eu-west-1.amazonaws.com/foobar/barfoo:latest"),


Same image name format comment as above

acmcelwee · 2018-03-08T18:48:46Z

src/test/java/com/spotify/docker/client/auth/ecr/ContainerRegistryAuthSupplierTest.java

+    doReturn(true).when(supplier).needsRefresh(Matchers.any(AuthorizationData.class));
+
+    assertThat(
+        supplier.authFor("1234567890.dkr.ecr.eu-west-1.amazonaws.com/foobar/barfoo:latest"),


Same image name format comment as above

acmcelwee · 2018-03-08T18:48:54Z

src/test/java/com/spotify/docker/client/auth/ecr/ContainerRegistryAuthSupplierTest.java

+    exception.expect(DockerException.class);
+    exception.expectCause(is(ex));
+
+    supplier.authFor("1234567890.dkr.ecr.eu-west-1.amazonaws.com/example/foobar:1.2.3");


Same image name format comment as above

acmcelwee · 2018-03-08T18:50:53Z

src/main/java/com/spotify/docker/client/auth/ecr/ContainerRegistryAuthSupplier.java

+
+    // Don't refresh if expiration time hasn't been provided.
+    if (expirationTime == null) {
+      return true;


It's technically nullable - https://github.com/aws/aws-sdk-java/blob/master/aws-java-sdk-ecr/src/main/java/com/amazonaws/services/ecr/model/AuthorizationData.java

sigpwned · 2018-04-21T14:05:39Z

Oops! I just submitted a different pull request #1013 for ECR authentication. I missed this in my search. I was searching for ECR and ECS. Hopefully now this comment will flag for future searchers.

It looks like this review may have stalled out. If I can do some work to push it through, or if my other pull request looks OK, it would be great to get ECR support into this project soon. Very happy to help do that — just LMK how!

davidxia · 2018-05-20T15:16:54Z

src/main/java/com/spotify/docker/client/auth/ecr/ContainerRegistryAuthSupplier.java

+  ContainerRegistryAuthSupplier(final AmazonECR ecr, final Clock clock,
+                                final long minimumExpiryMillis, final EcrCredentials credentials) {
+    Preconditions.checkArgument(ecr != null, "ecr");
+    this.ecr = ecr;


Do we need to pass ecr here? I see it's used as a sync object below, but can't you just lock on something else?

davidxia · 2018-05-20T15:30:51Z

@toelen Sorry, I didn't see your comment here.

@davidxia I fixed your remarks on https://github.com/toelen/docker-client/tree/feat/ecr-auth

Can you update this PR with those changes?

davidxia · 2018-05-30T00:27:42Z

src/main/java/com/spotify/docker/client/auth/ecr/EcrCredentials.java

+    return authorizationData;
+  }
+
+  void refresh() throws IOException {


You don't need the IOException here.

davidxia · 2018-05-30T00:27:55Z

src/main/java/com/spotify/docker/client/auth/ecr/EcrCredentials.java

+    this.authorizationData = authorizationData;
+  }
+
+  public AuthorizationData getAuthorizationData() {


Can be package-private.

davidxia · 2018-05-30T00:31:19Z

src/main/java/com/spotify/docker/client/auth/ecr/EcrCredentials.java

+
+  void refresh() throws IOException {
+    GetAuthorizationTokenResult authorizationToken = ecr
+        .getAuthorizationToken(new GetAuthorizationTokenRequest());


javadocs for GetAuthorizationTokenRequest() say

A list of AWS account IDs that are associated with the registries for which to get authorization tokens. If you do not specify a registry, the default registry is assumed.

Will the user ever need to use registries other than the default?

cenacsf · 2018-07-17T12:09:32Z

Can We get this merge please , We also eagerly waiting for this support

davidxia · 2018-07-17T13:39:41Z

@cenacsf There's this and #1013. The last update was my comment here #1013 (comment). It seems like both of these PRs have stalled. If someone's willing to drive #1013 (comment) to completion, I'm happy to review and merge.

Leen Toelen added 4 commits August 7, 2017 20:50

add AWS Elastic Container Service authentication supporta

fb1e23c

fix: remove GCR warnings in ECR auth provider

bd89003

fix: updated headers on ecr auth provider

08aa679

fix: don't rely on default encoding

13f3bea

ben-gineer mentioned this pull request Sep 15, 2017

Support Docker external credential stores spotify/dockerfile-maven#24

Closed

davidxia reviewed Nov 12, 2017

View reviewed changes

mattnworb mentioned this pull request Mar 2, 2018

Un-hardcode Google Repository in AbstractDockerMojo and add AWS ECR support spotify/dockerfile-maven#54

Closed

acmcelwee reviewed Mar 8, 2018

View reviewed changes

davidxia reviewed May 20, 2018

View reviewed changes

davidxia mentioned this pull request May 20, 2018

Add authentication support for Amazon ECR #1013

Closed

davidxia mentioned this pull request May 20, 2018

Add support for AWS ECR variant of RegistryAuthSupplier #1019

Closed

davidxia reviewed May 30, 2018

View reviewed changes

johnflavin mentioned this pull request Aug 4, 2018

Refactor Config-file-based auth #1051

Merged

toelen closed this Nov 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AWS Elastic Container Service authentication support #876

Add AWS Elastic Container Service authentication support #876

toelen commented Aug 29, 2017

codecov-io commented Aug 29, 2017 •

edited by codecov bot

Loading

ben-gineer commented Sep 14, 2017

davidxia left a comment

davidxia Nov 12, 2017

acmcelwee Mar 8, 2018

davidxia Nov 12, 2017

davidxia Nov 12, 2017

davidxia Nov 12, 2017

davidxia Nov 12, 2017

davidxia Nov 12, 2017

acmcelwee Mar 8, 2018

davidxia Nov 12, 2017

mhermosi commented Jan 9, 2018

smuryginim commented Mar 1, 2018

davidxia commented Mar 1, 2018 •

edited

Loading

toelen commented Mar 2, 2018

mattnworb commented Mar 2, 2018

acmcelwee Mar 8, 2018

acmcelwee Mar 8, 2018

acmcelwee Mar 8, 2018

acmcelwee Mar 8, 2018

acmcelwee Mar 8, 2018

acmcelwee Mar 8, 2018

acmcelwee Mar 8, 2018

sigpwned commented Apr 21, 2018 •

edited

Loading

davidxia May 20, 2018

davidxia commented May 20, 2018

davidxia May 30, 2018

davidxia May 30, 2018

davidxia May 30, 2018

cenacsf commented Jul 17, 2018

davidxia commented Jul 17, 2018

Add AWS Elastic Container Service authentication support #876

Add AWS Elastic Container Service authentication support #876

Conversation

toelen commented Aug 29, 2017

codecov-io commented Aug 29, 2017 • edited by codecov bot Loading

Codecov Report

ben-gineer commented Sep 14, 2017

davidxia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhermosi commented Jan 9, 2018

smuryginim commented Mar 1, 2018

davidxia commented Mar 1, 2018 • edited Loading

toelen commented Mar 2, 2018

mattnworb commented Mar 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sigpwned commented Apr 21, 2018 • edited Loading

Choose a reason for hiding this comment

davidxia commented May 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cenacsf commented Jul 17, 2018

davidxia commented Jul 17, 2018

codecov-io commented Aug 29, 2017 •

edited by codecov bot

Loading

davidxia commented Mar 1, 2018 •

edited

Loading

sigpwned commented Apr 21, 2018 •

edited

Loading