YARN-11364. Docker Container to accept docker Image name with sha256 digest #5092
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🎊 +1 overall
This message was automatically generated. |
|
LGTM. |
ashutoshcipher
force-pushed
the
YARN-11364
branch
from
2dbc4a0
to
5a780f0
Compare
|
🎊 +1 overall
This message was automatically generated. |
ashutoshcipher
changed the title
ashutoshcipher
changed the title
cnauroth
requested changes
Oct 31, 2022
There was a problem hiding this comment.
@ashutoshcipher , thank you for the patch. I entered a few comments.
| @@ -208,6 +208,8 @@ public class DockerLinuxContainerRuntime extends OCIContainerRuntime { | |||
| private static final Pattern dockerImagePattern = | |||
| Pattern.compile(DOCKER_IMAGE_PATTERN); | |||
|
|
|||
| private static final Pattern DOCKER_DIGEST_PATTERN = Pattern.compile("^sha256:[a-z0-9]{32,}$"); | |||
There was a problem hiding this comment.
This allows "sha256:" followed by anywhere from 32 to an infinite number of characters. The maximum length of a SHA-256 is 64 characters. I believe Docker allows referencing a shortened SHA-256 as small as 12 characters, though I'm having trouble finding documentation to back that up.
Maybe we should go with either {32,64} or {12,64}?
There was a problem hiding this comment.
Makes sense, Lets go with {12,64}
| "123456789123.dkr.ecr.us-east-1.amazonaws.com/emr-docker-examples:pyspark-example" | ||
| + "@sha256:f1d4ae3f7261a72e98c6ebefe9985cf10a0ea5bd762585a43e0700ed99863807"}; | ||
|
|
||
| String[] invalidNames = {"Ubuntu", "ubuntu || fedora", "ubuntu#", "myregistryhost:50AB0/ubuntu", |
There was a problem hiding this comment.
I suggest also adding new negative tests here to make sure DOCKER_DIGEST_PATTERN doesn't match inputs that we don't want it to match. For example:
// Invalid: contains "@sha256" but doesn't really contain a digest.
"123456789123.dkr.ecr.us-east-1.amazonaws.com/emr-docker-examples:pyspark-example@sha256"
// Invalid: digest is too short.
"123456789123.dkr.ecr.us-east-1.amazonaws.com/emr-docker-examples:pyspark-example@sha256:f1d4"
// Invalid: digest is too long (depending on if we take my code review feedback on the regex).
"123456789123.dkr.ecr.us-east-1.amazonaws.com/emr-docker-examples:pyspark-example@sha256:f1d4ae3f7261a72e98c6ebefe9985cf10a0ea5bd762585a43e0700ed99863807f"
There was a problem hiding this comment.
Thanks, will add these negative test cases.
Thanks @cnauroth for review and comments. I will address them in my next commit. |
|
🎊 +1 overall
This message was automatically generated. |
|
@cnauroth, I have addressed your comments. Please help in reviewing it again. Thanks. |
cnauroth
approved these changes
Nov 1, 2022
There was a problem hiding this comment.
+1
Thank you for incorporating the code review feedback, @ashutoshcipher . I can commit this in a little while, targeting trunk, branch-3.3 and branch-3.2.
|
@ashutoshcipher , thank you for the contribution. I have committed this to trunk, branch-3.3 and branch-3.2 (after resolving a merge conflict). @slfan1989 , thank you for your code review. |
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
…digest (apache#5092) Co-authored-by: Ashutosh Gupta <ashugpt@amazon.com> Reviewed-by: slfan1989 <55643692+slfan1989@users.noreply.github.com> Signed-off-by: Chris Nauroth <cnauroth@apache.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
JIRA - YARN-11364
Docker Container to accept docker Image name with sha256 digest.
While using docker and passing the using sha256 digest as docker image. It should be valid and not throw error
eg -
should be valid and not throw error.
How was this patch tested?
UT
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?