Use AWS SDK to fetch AWS credentials

[sebastiankirsch]

As discussed in #1310, credentials for ECR would be fetched via AWS SDK.

The dependency is optional, yet it would only introduce this:

[INFO] +- com.amazonaws:aws-java-sdk-core:jar:1.11.696:compile
[INFO] |  +- com.fasterxml.jackson.core:jackson-databind:jar:2.6.7.3:compile
[INFO] |  |  +- com.fasterxml.jackson.core:jackson-annotations:jar:2.6.0:compile
[INFO] |  |  \- com.fasterxml.jackson.core:jackson-core:jar:2.6.7:compile

i.e. outside of the SDK itself, only Jackson.
So: would you consider making this a full dependency, which would allow to remove 170loc in AuthConfigFactory and some more in the according test? It would also allow to get rid of the AwsSigner4 and AwsSigner4Request classes, since that functionality is found in the SDK as well.

No matter what, this will make Jackson available, and with GSON already in the mix, maybe you want to prevent using Jackson via checkstyle or SonarCloud or something? Especially as long as it's optional.

I'll try to come up with a meaningful test for this...

[sebastiankirsch]

My tests indicate that <optional>true</optional> doesn't seem to have an effect. I know that this works as expected for "normal" projects; maybe it's different for plugins?
So, it's either a no-go for this PR, incorporate the AWS SDK fully, or try to find a work-around with sub-modules and dynamic class loading...

[rhuss]

I think it would be best to avoid direct type references to the SDK to avoid any issues in cases when those types are not available on the class path.

Instead, just use full reflection for everything:

Class prChainClass = Class.forName("com.amazonaws.auth.DefaultAWSCredentialsProviderChain");
Object chain = prChainClass.getConstructor(new Class[0]).newInstance();
....

(just as an example).

That way it would be also possible to use the SDK as plugin dependencies directly within the plugin declaration and there is no need for usion optional dependencies.

[codecov]

Codecov Report

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

@@            Coverage Diff            @@
##             master    #1311   +/-   ##
=========================================
  Coverage     56.29%   56.29%           
  Complexity     1816     1816           
=========================================
  Files           158      158           
  Lines          8653     8653           
  Branches       1335     1335           
=========================================
  Hits           4871     4871           
  Misses         3307     3307           
  Partials        475      475

Impacted Files	Coverage Δ	Complexity Δ
...o/fabric8/maven/docker/util/AuthConfigFactory.java	71.57% <ø> (ø)	82 <0> (ø)	⬇️
...bric8/maven/docker/access/ecr/EcrExtendedAuth.java	90.69% <100%> (ø)	11 <1> (ø)	⬇️

[rhuss]

Thanks a lot ! Looks really good, would love to merge it soonish and make a next release quite quickly.

Some minor review points though, please see below.

[rhuss]

Are there any licensing issues ? Under which license the aws-java-sdk-core is licensed (so that it allows code copying with contribution ?). Don't wanna get into struggle because of licensing (was there already :)

Is there any chance that we could implement this without reusing SDK code ?

[rhuss]

I know its only a test class, but still this could pose some issue for some folks. The good thing is that we are talking here about a tool which is not part the of any delivered application that builds with it.

[sebastiankirsch]

The AWS SDK is licensed under Apache License 2.0. Afaik that's one of the most liberal licenses you could ask for.

I could remove this, but then there wouldn't be a test at all. What do you think?

[rhuss]

Ok, APL is ok. I had once a case where a GPL code snippet sneaked in (in another project). Was quite some hazzle to get this out again. I'm fine with it now (I will add some extra comment here)

[sebastiankirsch]

Yeah, licensing can be a pain...

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
1 Code Smell

82.4% Coverage
0.0% Duplication

[rhuss]

lgtm, thanks again !

[sebastiankirsch]

@rhuss You're too fast for me: https://github.com/sebastiankirsch/docker-maven-plugin/commit/f5a514b9993e3b3a50ca2c4a51879ae4dd555ee2

[rhuss]

@rhuss You're too fast for me: sebastiankirsch@f5a514b

sorry, this doesn't happen often these days ;-) I added your commit to master, as I already cut a new 0.33.0 release. So will make it into v0.34.0 then ...