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

[JENKINS-67572] Allow docker digest in image names #93

Merged
merged 5 commits into from
Jan 27, 2022

Conversation

viceice
Copy link
Member

@viceice viceice commented Jan 12, 2022

Allow docker digests in image names, as since c069b79 it's no longer allowed in docker-workflow-plugin to pin docker images.

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@viceice viceice marked this pull request as ready for review January 12, 2022 20:54
j3t referenced this pull request Jan 17, 2022
@PandarinDev
Copy link

Great job, looking forward to this getting merged! Currently our workflows are failing because of this regression and we cannot really downgrade our plugins so hoping that this will be released ASAP.

@j3t
Copy link
Member

j3t commented Jan 18, 2022

As a workaround, you can disable the validation see https://github.com/jenkinsci/docker-commons-plugin/blob/master/src/main/java/org/jenkinsci/plugins/docker/commons/credentials/ImageNameValidator.java#L48

@viceice
Copy link
Member Author

viceice commented Jan 18, 2022

@j3t I know, but that fully disables all checks again and we are vulnerable again. So i hope this gets merge and released soon.

@rsandell @oleg-nenashev Any changes to get this done sooner than later?

@jsnod
Copy link

jsnod commented Jan 21, 2022

As a workaround, you can disable the validation see https://github.com/jenkinsci/docker-commons-plugin/blob/master/src/main/java/org/jenkinsci/plugins/docker/commons/credentials/ImageNameValidator.java#L48

@j3t We are starting Jenkins with -Dorg.jenkinsci.plugins.docker.commons.credentials.ImageNameValidator.SKIP=true but we are still failing on validation, is there some other way to enable this SKIP workaround?

@viceice
Copy link
Member Author

viceice commented Jan 21, 2022

I think you are probably also hit by

…/ImageNameValidatorTest.java

Co-authored-by: Robert Sandell <rsandell@cloudbees.com>
@viceice
Copy link
Member Author

viceice commented Jan 27, 2022

OK, fixed tests and now validating against oci spec. So please review again. 🤗

@rsandell rsandell changed the title fix: allow docker digest in image names [JENKINS-67572] [JENKINS-67572] Allow docker digest in image names Jan 27, 2022
@rsandell rsandell merged commit e23a91d into jenkinsci:master Jan 27, 2022
@viceice viceice deleted the fix/valid-image-name branch January 27, 2022 15:42
@viceice
Copy link
Member Author

viceice commented Jan 27, 2022

♥️

@viceice
Copy link
Member Author

viceice commented Jan 27, 2022

Thanks, updated jenkins and removed workaround and everything works 🎉

@ccayg-sainsburys
Copy link

ccayg-sainsburys commented Feb 2, 2022

Is this possibly fixing some cases but not all or has something here / nearby changed that means my configuration no longer works?

I'm seeing: ERROR: Name must follow the pattern &#039;^[a-zA-Z0-9]+((\.|_|__|-+)[a-zA-Z0-9]+)*$&#039;
following DOCKER_IMAGE = docker.build('${ECR_REGISTRY}/${ECR_REPO}:${COMMIT_HASH}')

Where the registry variable is pulled from credentials while the repo and hash are defined within the pipeline itself.
This should equate to what I believe is a normal ECR reference per https://docs.aws.amazon.com/AmazonECR/latest/userguide/docker-push-ecr-image.html of the form 123456789012.dkr.ecr.eu-west-1.amazonaws.com/service-foo:0a1b23c

with v1.19 of this plugin and v1.28 of docker-workflow on linux

actually I guess that's JENKINS-67633

@TBBle
Copy link

TBBle commented Feb 3, 2022

@ccayg-sainsburys Yeah, this is almost certainly JENKINS-67633. I've left a comment there so it's more-visible to users with that issue.

@ccayg-sainsburys
Copy link

@ccayg-sainsburys Yeah, this is almost certainly JENKINS-67633. I've left a comment there so it's more-visible to users with that issue.

Thanks - and the suggested fix there does work but because one of the values comes from credentials it triggers:

10:33:27  Warning: A secret was passed to "withEnv" using Groovy String interpolation, which is insecure.
10:33:27  		 Affected argument(s) used the following variable(s): [ECR_REGISTRY]
10:33:27  		 See https://jenkins.io/redirect/groovy-string-interpolation for details.

I might just replace with shell commands at this point to be honest.

@TBBle
Copy link

TBBle commented Feb 3, 2022

For data from credentials, don't use withEnv, use the specialised credential-handling step withCredentials to expose the value as an env-var. That should fix that warning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants