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

Implemented authentication via EC2 instance role for ECR #1186

Merged
merged 2 commits into from
Apr 6, 2019

Conversation

sithmein
Copy link
Contributor

@sithmein sithmein commented Apr 3, 2019

This PR adds support for EC2 instance roles as described in issue #1177.
I didn't add any tests since they would only work when run on a correctly configured EC2 instance.

@codecov
Copy link

codecov bot commented Apr 3, 2019

Codecov Report

Merging #1186 into master will decrease coverage by 0.2%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #1186      +/-   ##
============================================
- Coverage     52.27%   52.06%   -0.21%     
  Complexity     1484     1484              
============================================
  Files           150      150              
  Lines          7906     7934      +28     
  Branches       1183     1185       +2     
============================================
- Hits           4133     4131       -2     
- Misses         3369     3397      +28     
- Partials        404      406       +2
Impacted Files Coverage Δ Complexity Δ
...bric8/maven/docker/access/ecr/EcrExtendedAuth.java 88.37% <0%> (-2.11%) 10 <2> (+2)
...o/fabric8/maven/docker/util/AuthConfigFactory.java 62.5% <0%> (-14.75%) 69 <0> (+6)
...n/java/io/fabric8/maven/docker/util/ImageName.java 93.47% <0%> (-0.14%) 39% <0%> (-2%)
.../io/fabric8/maven/docker/service/BuildService.java 31.49% <0%> (+2.82%) 9% <0%> (ø) ⬇️
...a/io/fabric8/maven/docker/util/DockerFileUtil.java 75.6% <0%> (+5.76%) 11% <0%> (-6%) ⬇️

Copy link
Collaborator

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

Thanks a lot !

Looks good to me with some minor comment on validation and encoding.

Also, could you please add a Changelog entry too please ?


// read instance role
try (InputStream is = response.getEntity().getContent()) {
instanceRole = IOUtils.toString(is, StandardCharsets.UTF_8);
Copy link
Collaborator

Choose a reason for hiding this comment

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

any validation of this instance role needed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What exactly do you mean with "validation" in this context?


// get temporary credentials
request = new HttpGet(
"http://169.254.169.254/latest/meta-data/iam/security-credentials/" + instanceRole);
Copy link
Collaborator

Choose a reason for hiding this comment

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

URL encoding required as instanceRole is an externally (aka 'tainted') data ?

Copy link
Collaborator

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

Thanks ! lgtm except some minor formatting issue. If you could fix that, we are ready to merge.

try (InputStream is = response.getEntity().getContent()) {
instanceRole = IOUtils.toString(is, StandardCharsets.UTF_8);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

formatting seems to be messed up a bit

@sithmein sithmein force-pushed the support-for-ec2-instance-roles branch from 68b0bf9 to ebed8a6 Compare April 6, 2019 12:43
)

Signed-off-by: Thorsten Meinl <thorsten.meinl@knime.com>
@sithmein sithmein force-pushed the support-for-ec2-instance-roles branch from ebed8a6 to 2261002 Compare April 6, 2019 12:46
Copy link
Collaborator

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

thanks, looks good to me !

@rhuss rhuss merged commit ebb1aa4 into fabric8io:master Apr 6, 2019
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.

4 participants