Fix #1317: update ECR AuthorizationToken URL

[joaori]

No description provided.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

100.0% Coverage
0.0% Duplication

[codecov]

Codecov Report

Merging #1318 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff            @@
##             master   #1318   +/-   ##
========================================
  Coverage      56.2%   56.2%           
  Complexity     1809    1809           
========================================
  Files           157     157           
  Lines          8625    8625           
  Branches       1332    1332           
========================================
  Hits           4848    4848           
  Misses         3302    3302           
  Partials        475     475

Impacted Files	Coverage Δ	Complexity Δ
...bric8/maven/docker/access/ecr/EcrExtendedAuth.java	90.69% <100%> (ø)	11 <1> (ø)	⬇️

[rhuss]

Looks good to me but I wonder whether it is backwards compatible? I.e. does it break authentication for older configs? I'm a bit surprised that there was not a bug issue previously if this AWS API change broke every d-m-p access to AWS out there. Unfortunately, I have no quick way to test this as I am not an AWS user.

@sebastiankirsch would you mind taking a quick look on this PR whether it looks good to you ?

[joaori]

I don't think there's any backward incompatibility.

From what I can tell, AWS seems to be serving ECR API calls in both ecr and api.ecr.

The ECR Endpoints and Quotas still references (old) ecr endpoint.

However, seems that both python and java SDK's are already using api.ecr. And VPC Endpoints also mention it.

d-m-p will only fails on AWS if the following conditions are met:

1. It has an IAM Role defined
2. EC2 instance is on a private only subnet

Condition 1 will make AuthConfig to always be defined based on instance's AWS credentials.
This is also quite annoying as it won't allow for fallback to ~/.docker/config.json credentials (assuming I'd want to deal with repo authentication externally and skip extended authentication).

Condition 2 forces having VPC Endpoints (api.ecr.. for API calls and dkr.ecr.. for push / pulls) created on the VPC so that endpoints can resolve to private IPs.

Setting up a NAT and / or proxy would still make it work I guess. Probably the reason why it hasn't been reported before.

[sebastiankirsch]

I can confirm that the ECR API endpoint was updated to said value.

Since AWS itself is well-aware that there are older clients out there (the change to the SDK they provide isn't even a year old: aws/aws-sdk-java@895452f), I don't expect an upgrade/backward-compatibility issue there.

@joaori did anything not work for you, or why did you put in the effort of adapting this?

[joaori]

I've been using the plugin for quite a while now (kudos to you all!) using a similar setup as stated. With the only exception that the instance has internet access through a (whitelisted) proxy server.

So it's using VPC endpoints to allow private access to ECR but still had to rely on the proxy for the d-m-p plugin to be able to auth using the (old) ecr endpoint.

I tried configuring it to simply bypass the plugin's auth behaviour but wasn't successful due to the issue explained above: the instance has an IAM role, so ~/.docker/config.json fallback is never considered.
(execution output for mvn -X deploy:build on #1317 description)

Ended up patching the code with the new api.ecr endpoint :)
I've been using the patched plugin and seems to be working as expected so far.

[rhuss]

Ok, convinced :-) Let's update the end point then.

[sebastiankirsch]

I assume one could use com.amazonaws:aws-java-sdk-ecr to fetch the ECR authentication token instead of relying on the custom-written code incorporated in the plugin.
Similar to what I did in #1311 - would probably solve this and other "niche" use-cases...