Add authentication support for Amazon ECR

[sigpwned]

Hello,

I'm interested in using the (wonderful!) https://github.com/spotify/dockerfile-maven project to manage my docker images during the build process, but I noticed that it doesn't support ECR. Adding ECR support to this project seems like the proper first step; once it's included here, support can more easily be added to that project next.

I checked for open and closed issues and pull requests about ECR and didn't find any on this project. I apologize if they're there, but I missed them! If that's the case, please point me in the right direction.

The ECR implementation is heavily based off the GCR implementation. I've added what feels like the natural analog to the GCR features for ECR, including unit tests. Again, please let me know if I've misunderstood or overlooked some important aspect of the authorization piece!

I hope this patch looks OK and makes sense! I'm happy to make any changes you need.

[codecov-io]

Codecov Report

Merging #1013 into master will decrease coverage by 1.01%.
The diff coverage is 38.65%.

@@             Coverage Diff              @@
##             master    #1013      +/-   ##
============================================
- Coverage     66.86%   65.84%   -1.02%     
- Complexity      761      779      +18     
============================================
  Files           173      177       +4     
  Lines          3175     3294     +119     
  Branches        366      383      +17     
============================================
+ Hits           2123     2169      +46     
- Misses          895      961      +66     
- Partials        157      164       +7

[davidxia]

@sigpwned Thanks for your PR. I haven't looked super closely at yours yet. I wanted to ask first if there are any meaningful or significant differences between yours and #876 and which parts of this PR you think I should focus on reviewing. The big differences that stood out to me between the two are that yours has more indirection and a bit more error checking on the AuthorizationData that's returned. Any reason you don't go for the seemingly more straightforward approach with using EcrCredentials directly like #876 does?

[sigpwned]

As long as ECR authentication makes its way into the plugin, it doesn't make much difference to me which patch is used.

Regarding some choices I made in my implementation, to minimize friction, I've tried to model this implementation against the GCR implementation, which is (ostensibly) already known-good and trusted code since it's integrated in the codebase. If you compare this ECR implementation to the GCR implementation, you should find them to be essentially identical. In my experience, this reduces technical risk and maintenance cost.

Obviously you may feel differently, and you're the maintainer so you're the boss! 😄

As long as ECR support gets added, I'm a happy camper.

Thanks for your help! I'm sure you're doing this during your free time. It's much appreciated.

[sigpwned]

Just checking in here. Is there anything I can do to help, either on this PR or on #876?

[davidxia]

@sigpwned Sorry for the delay. I was asking about whether we need the extra Authenticator, EcrAuthenticator, and EcrAuthenticatorFactory classes? I like how there's less code in #876 since it tries to just implement RegistryAuthSupplier.

But I like how your PR is more flexible in the various ways that the user can provide ECR credentials to ElasticContainerRegistryAuthSupplier. #876 only allows AmazonECRClientBuilder.defaultClient() and with AmazonECR.

If you could combine my two suggestions or make a case for the extra classes here, I'd be happy to review more closely before merging. Since I haven't used ECR at all, let me know if you've tested and the results.

[t1ernan]

@mattnworb Has #1051 made the need for this obsolete or does the underlying docker client still require code changes to enable ECR Auth support?

[mattnworb]

I haven't used AWS ECR at all, so it would be great if someone else could verify this, but I think supporting other registries (like ECR) via the credential-helpers is the best long-term approach. If that works for AWS ECR then I would consider this obsolete, yeah.

[acmcelwee]

@tiernanscully @mattnworb I used ECR, and I just did a quick test to wire up https://github.com/spotify/dockerfile-maven, which uses this project, and it does, indeed, seem to be solved by #1051, once I setup the appropriate ecr credHelpers entry.

[davidxia]

@acmcelwee Thanks for trying it out. Closing as this issue seems to be solved.