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

bug: add support for image tag and digest #4792

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

emosbaugh
Copy link
Contributor

@emosbaugh emosbaugh commented Jul 24, 2024

Description

When adding an envoy image with tag and digest I get a failure:

invalid node config: spec: network: nodeLocalLoadBalancing.envoyProxy.image.version: Invalid value: "1.29.5-r0@sha256:7856e4d51f6631b1121c79ec93958897f1d90a52ce8d1d3513cff73df6821a2b": must match regular expression: ^[\w][\w.-]{0,127}$

This change adds support for an optional digested image reference.

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

How Has This Been Tested?

  • Manual test
  • Auto test added

Checklist:

  • My code follows the style guidelines of this project
  • My commit messages are signed-off
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

@emosbaugh emosbaugh force-pushed the add-support-for-image-tag-digest branch 2 times, most recently from 8d88104 to cb43fe5 Compare July 24, 2024 04:30
Signed-off-by: Ethan Mosbaugh <ethan@replicated.com>
@emosbaugh emosbaugh force-pushed the add-support-for-image-tag-digest branch from cb43fe5 to f7f641b Compare July 24, 2024 04:36
@emosbaugh emosbaugh marked this pull request as ready for review July 24, 2024 04:36
@emosbaugh emosbaugh requested a review from a team as a code owner July 24, 2024 04:36
@emosbaugh emosbaugh requested review from kke and juanluisvaladas July 24, 2024 04:36
@twz123 twz123 added bug Something isn't working area/configuration backport/release-1.30 PR that needs to be backported/cherrypicked to the release-1.30 branch labels Jul 24, 2024
@twz123 twz123 changed the title feat: add support for image tag and digest bug: add support for image tag and digest Jul 24, 2024
@twz123
Copy link
Member

twz123 commented Jul 24, 2024

Darn! That was definitely an oversight. Surprising that there's no ready-made constant for this in github.com/distribution/reference. I wonder if it makes sense to simply use a single string instead of the image/version struct in a potential config version v2. This is how it's done in upstream Kubernetes anyways. Then we could use github.com/distribution/reference.ReferenceRegexp.

Fixes #2428.

Copy link
Member

@twz123 twz123 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!

@twz123 twz123 merged commit 6d083b0 into k0sproject:main Jul 24, 2024
83 checks passed
@k0s-bot
Copy link

k0s-bot commented Jul 24, 2024

Backport failed for release-1.30, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-1.30
git worktree add -d .worktree/backport-4792-to-release-1.30 origin/release-1.30
cd .worktree/backport-4792-to-release-1.30
git switch --create backport-4792-to-release-1.30
git cherry-pick -x f7f641b7360a66682d202a86ef65780a9725a23d

@sgalsaleh
Copy link

Is it possible to backport this fix to 1.28 as well?

@twz123
Copy link
Member

twz123 commented Jul 24, 2024

Is it possible to backport this fix to 1.28 as well?

This is going into all supported releases, i.e. down to 1.27.

@emosbaugh
Copy link
Contributor Author

@twz123 I wanted to call out that ClusterImages.Validate does not call ImageSpec.Validate. Validate is only called for the envoy image. I unintentionally pushed this test which would have failed had it been called. Is there any follow up action you would like me to take here like to remove the test? Thanks for the quick turnaround!

@twz123
Copy link
Member

twz123 commented Jul 24, 2024

I wanted to call out that ClusterImages.Validate does not call ImageSpec.Validate.

That explains why this didn't crop up earlier.

Is there any follow up action you would like me to take here like to remove the test?

Actually validating the images in ClusterImages.Validate seems reasonable to me. PRs are welcome, as always 🙃

@emosbaugh emosbaugh mentioned this pull request Jul 28, 2024
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/configuration backport/release-1.30 PR that needs to be backported/cherrypicked to the release-1.30 branch bug Something isn't working config-2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants