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

Enhance Fix #541: Allow @sha256 digest for tags in FROM #1176

Merged
merged 4 commits into from
Apr 6, 2019

Conversation

mmkonrad
Copy link
Contributor

ImageName.java and respective test class adjusted to be able to handle
image names with tag AND sha256 digest like:
image_name:image_tag@sha256

Signed-off-by: Marcus Konrad M.M.Konrad@web.de

ImageName.java and respective test class adjusted to be able to handle
image names with tag AND sha256 digest like:
image_name:image_tag@sha256<digest>

Signed-off-by: Marcus Konrad <M.M.Konrad@web.de>
@codecov
Copy link

codecov bot commented Mar 19, 2019

Codecov Report

Merging #1176 into master will increase coverage by 0.01%.
The diff coverage is 93.75%.

@@             Coverage Diff              @@
##             master    #1176      +/-   ##
============================================
+ Coverage     52.31%   52.32%   +0.01%     
- Complexity     1473     1475       +2     
============================================
  Files           150      150              
  Lines          7855     7857       +2     
  Branches       1169     1171       +2     
============================================
+ Hits           4109     4111       +2     
  Misses         3343     3343              
  Partials        403      403
Impacted Files Coverage Δ Complexity Δ
...n/java/io/fabric8/maven/docker/util/ImageName.java 93.61% <93.75%> (+0.13%) 41 <3> (+2) ⬆️

2 similar comments
@codecov
Copy link

codecov bot commented Mar 19, 2019

Codecov Report

Merging #1176 into master will increase coverage by 0.01%.
The diff coverage is 93.75%.

@@             Coverage Diff              @@
##             master    #1176      +/-   ##
============================================
+ Coverage     52.31%   52.32%   +0.01%     
- Complexity     1473     1475       +2     
============================================
  Files           150      150              
  Lines          7855     7857       +2     
  Branches       1169     1171       +2     
============================================
+ Hits           4109     4111       +2     
  Misses         3343     3343              
  Partials        403      403
Impacted Files Coverage Δ Complexity Δ
...n/java/io/fabric8/maven/docker/util/ImageName.java 93.61% <93.75%> (+0.13%) 41 <3> (+2) ⬆️

@codecov
Copy link

codecov bot commented Mar 19, 2019

Codecov Report

Merging #1176 into master will increase coverage by 0.01%.
The diff coverage is 93.75%.

@@             Coverage Diff              @@
##             master    #1176      +/-   ##
============================================
+ Coverage     52.31%   52.32%   +0.01%     
- Complexity     1473     1475       +2     
============================================
  Files           150      150              
  Lines          7855     7857       +2     
  Branches       1169     1171       +2     
============================================
+ Hits           4109     4111       +2     
  Misses         3343     3343              
  Partials        403      403
Impacted Files Coverage Δ Complexity Δ
...n/java/io/fabric8/maven/docker/util/ImageName.java 93.61% <93.75%> (+0.13%) 41 <3> (+2) ⬆️

@codecov
Copy link

codecov bot commented Mar 19, 2019

Codecov Report

Merging #1176 into master will decrease coverage by 0.04%.
The diff coverage is 93.75%.

@@             Coverage Diff              @@
##             master    #1176      +/-   ##
============================================
- Coverage     52.36%   52.32%   -0.05%     
+ Complexity     1482     1475       -7     
============================================
  Files           150      150              
  Lines          7889     7857      -32     
  Branches       1176     1171       -5     
============================================
- Hits           4131     4111      -20     
+ Misses         3354     3343      -11     
+ Partials        404      403       -1
Impacted Files Coverage Δ Complexity Δ
...n/java/io/fabric8/maven/docker/util/ImageName.java 93.61% <93.75%> (+0.13%) 41 <3> (+2) ⬆️
...8/maven/docker/config/BuildImageConfiguration.java 82.2% <0%> (-0.43%) 60% <0%> (-1%)
...bric8/maven/docker/assembly/DockerFileBuilder.java 90.33% <0%> (-0.41%) 77% <0%> (-4%)
...config/handler/property/PropertyConfigHandler.java 74.33% <0%> (-0.23%) 116% <0%> (-2%)
...va/io/fabric8/maven/docker/service/RunService.java 51.91% <0%> (-0.21%) 25% <0%> (ø)
...ic8/maven/docker/config/RunImageConfiguration.java 91.02% <0%> (-0.17%) 50% <0%> (-1%)
...bric8/maven/docker/access/ContainerHostConfig.java 85.14% <0%> (-0.15%) 40% <0%> (-1%)
...aven/docker/config/handler/property/ConfigKey.java 100% <0%> (ø) 10% <0%> (ø) ⬇️
.../java/io/fabric8/maven/docker/model/Container.java 100% <0%> (ø) 0% <0%> (ø) ⬇️
...bric8/maven/docker/assembly/DockerFileKeyword.java 100% <0%> (ø) 5% <0%> (ø) ⬇️
... and 4 more

@mmkonrad
Copy link
Contributor Author

Updated PR without indent issues for Fabric8 MVN Plugin Issue 1386

@@ -64,25 +64,35 @@ public ImageName(String fullName, String givenTag) {
throw new NullPointerException("Image name must not be null");
}

// set digest to null as default
digest = null;
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the intention of this change, why do you need tag when you're not passing it in REST call to docker daemon?

Copy link
Member

Choose a reason for hiding this comment

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

Do you need it only for ImageName?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean why only ImageName.java ist modified but not e.g. UrlBuilder.java?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, since this would only have an effect on ImageName object and this won't be propagated to api server. Maybe I misunderstood your use case earlier and delayed it because I wasn't sure how docker api server handles digests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. I did not dive deep into the entire architecture. (i do not have enough time) As I said in the F8 MVN thread, it is not final.
I just wanted to make a start towards solving our/my request instead of just asking all the time when it will be done.
If you have some hints where to look and probably some docs about the missing parts, that would be great.

Copy link
Member

Choose a reason for hiding this comment

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

Right now we're ignoring tag and sending image digest in place of tag, need to check in docker api on how to pass both(but I'm still not sure why do we need both)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure whether others have the necessity for this.
But we do.
We have multiple applications with each several docker images being involved.
We have the demand, to know (and control) exactly what base images are used and whether we adapt newer versions/patches/updates or not.
In case we decide to adapt a change in a base image, follow-up processes will be triggered.
Going with the latest image does not work for us.
Instead we will use an open-source tool called "renovate" to pin the hash digest to the used base images.
This tool then will then check if there is any change to the base image, based on the hash digest that is pinned to the image and its tag.
This can be an update within the same tag e.g. version 1.2.3.4-alpine, a minor patch and also major patches.
Version tags like 1.2.3.4-alpine may consist of different version even though it is displayed to the outside as version 1.2.3.4-alpine.
Even these small changes within the same version may cause issues and side-effects to us.
But then we may also have to adapt to certain patches and updates at some time.
Right now multiple use-cases exist for us.
Relying on image tags AND hash digests together, gives us the possibility to meet the demand of controlling what updates/patches/changes we want to adapt.
Additionally fabric8 is a major component in the build processes of our docker images.
As a result we need the docker-maven-plugin to allow tags AND hash digest in the FROM statement.

Maybe there is a better solution out there, but time and available resource do not allow it right now to rethink and reconstruct all of our processes just to meet that demand.
Therefore it is the convenient way to enable the docker-maven-plugin to handle tags and hash digests together in the FROM statement.

Copy link

@schuch schuch Mar 21, 2019

Choose a reason for hiding this comment

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

@mmkonrad so the only reason for specifying the tag and the digest together is that a tool like https://github.com/renovatebot/renovate can automatically update the digest of a tag, if a tag changed, right?

Imho when pulling the "pinned" image only the digest needs to be sent to the registry.

According to the author of renovatebot, when specifiying tag and digest in an image reference, the tag will be indeed ignored:

One useful trick know is that you don’t have to remove the tag if you want to add a digest. If both are present then the tag is ignored, so you can leave it in for human readability, e.g

(refering to https://renovatebot.com/blog/docker-mutable-tags)

But i don't know if the pulling client is handling this by not sending the tag or if the pull client can send both and the registry just ignores it. I did not find any specification for that. May be we can try to look how the docker daemon behaves on this by looking at pull requests with a man-in-the-middle proxy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@schuch Thank you for pointing this out.
Human readability is indeed the key factor.

If a source tag is updated with a new semver version, you will receive a PR with a new FROM line that looks something like FROM node:8.10.1@sha256:eks9abc802b2a8464b3bfc1f8c3c409f89e9b70a31f1dccce70bd14mxwris. i.e. tag has changed and digest has changed. If only the digest is updated, you would see that too (and the PR will tell you that it’s a digest update, not a version update).

@rohanKanojia Sorry for not stating it so clearly from the beginning

Copy link
Member

Choose a reason for hiding this comment

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

@mmkonrad: Hi, Sorry for late reply(I saw this in notifications but somehow forgot to reply). Are you trying to say that you're okay with tag to stay just in ImageName class(like you've done here)? I think we're good to merge this then ;-) . I think we should handle this case as well and simply ignore tag even if it's parsed properly.

Copy link
Member

@rohanKanojia rohanKanojia left a comment

Choose a reason for hiding this comment

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

Basic usage of having a tag in image specification along with digest and ignoring it while sending to the server seems good to me.

@rohanKanojia
Copy link
Member

@rhuss : Could you please share your thoughts? I think we can merge this.

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.

Looks good to me. Didn't know that tag and digest are both valid for the name. Let's merge it.

@rhuss rhuss merged commit ba7f204 into fabric8io:master Apr 6, 2019
@mmkonrad
Copy link
Contributor Author

mmkonrad commented Apr 8, 2019

Thanks a lot!

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