Defaulting to the `latest` tag if no tag was specified. #826

ybettan · 2024-06-03T12:48:22Z

Defaulting to the latest tag if no tag was specified.

In the current code, we handle differently build/sign and deploying a
pre-built kmod.

This is the current behavior:

If a build/sign section is set in the module, and the image tag
isn't specified, then we will return an error.
If there are no build/sign section, and the image tag isn't
specified, then we will default to the latest tag.

This change is adjusting the behavior of both workflow to always defaul
to the latest image tag if it is not specified in the
Module/ManagedClusterModule.

This is achieved by adding a new mutating-webhook for the
Module&ManangedClusterModule objects.

This change is also adding the nullable attributes to some API fields
in order for it to work.

/cc @qbarrand @yevgeny-shnaidman

The +nullable kubebuilder annotation is required until this PR is merged.

k8s-ci-robot · 2024-06-03T12:48:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ybettan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ybettan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

netlify · 2024-06-03T12:48:39Z

✅ Deploy Preview for kubernetes-sigs-kmm ready!

Name	Link
🔨 Latest commit	`0a2b042`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kmm/deploys/66844a1e09167b0008dfb043
😎 Deploy Preview	https://deploy-preview-826--kubernetes-sigs-kmm.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

ybettan · 2024-06-03T12:49:12Z

/hols
The webhook isn't popping the warning for some reason.

codecov-commenter · 2024-06-03T12:51:10Z

Codecov Report

Attention: Patch coverage is 69.56522% with 14 lines in your changes missing coverage. Please review.

Project coverage is 71.63%. Comparing base (fa23a9b) to head (0a2b042).
Report is 76 commits behind head on main.

Files	Patch %	Lines
...ternal/webhook/hub/managedclustermodule_mutator.go	70.58%	5 Missing ⚠️
internal/webhook/module_mutator.go	80.00%	5 Missing ⚠️
cmd/webhook-server/main.go	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #826      +/-   ##
==========================================
- Coverage   79.09%   71.63%   -7.47%     
==========================================
  Files          51       68      +17     
  Lines        5109     4519     -590     
==========================================
- Hits         4041     3237     -804     
- Misses        882     1087     +205     
- Partials      186      195       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yevgeny-shnaidman · 2024-06-03T13:10:34Z

internal/webhook/module.go

+		if km.ContainerImage != "" {
+			imgToCheck = km.ContainerImage
+		}
+		if !strings.ContainsRune(imgToCheck, '@') && !strings.ContainsRune(imgToCheck, ':') {


why can't we just return ImageHashOrTagNotSet from here? Why do we have to wait for the loop to finish?

If we return directly then we will only warn about a single potential missing tag in a single kernelMapping instead of appending them all to 1 warning.

Why do we need a warning in the webhook? Container image is either valid or not.

I think we should have a new mutating webhook that sets the latest tag instead of returning warnings.

yevgeny-shnaidman · 2024-06-03T13:15:18Z

internal/webhook/module.go

+		if km.ContainerImage != "" {
+			imgToCheck = km.ContainerImage
+		}
+		if !strings.ContainsRune(imgToCheck, '@') && !strings.ContainsRune(imgToCheck, ':') {


what do you think about using NewTag and NewDigest to verify the correctness of the image? Those functions are part of the go-containerregistry/pkg/name package, and they make a little bit more verifications than just the presence of @ or :

qbarrand · 2024-06-04T07:45:22Z

internal/webhook/module.go


 	"github.com/go-logr/logr"
+	multierror "github.com/hashicorp/go-multierror"


I think we should use errors.Join from the standard library instead.

qbarrand · 2024-06-04T07:46:43Z

internal/webhook/module.go

+		if km.ContainerImage != "" {
+			imgToCheck = km.ContainerImage
+		}
+		if !strings.ContainsRune(imgToCheck, '@') && !strings.ContainsRune(imgToCheck, ':') {


I think we should have a new mutating webhook that sets the latest tag instead of returning warnings.

In the current code, we handle differently build/sign and deploying a pre-built kmod. This is the current behavior: * If a `build`/`sign` section is set in the module, and the image tag isn't specified, then we will return an error. * If there are no `build`/`sign` section, and the image tag isn't specified, then we will default to the `latest` tag. This change is adjusting the behavior of both workflow to always default to the `latest` image tag if it is not specified in the `Module`/`ManagedClusterModule`. This is achieved by adding a new mutating-webhook for the `Module`&`ManangedClusterModule` objects. This change is also adding the `nullable` attributes to some API fields in order for it to work. Signed-off-by: Yoni Bettan <yonibettan@gmail.com>

Signed-off-by: Yoni Bettan <yonibettan@gmail.com>

k8s-ci-robot · 2024-07-02T18:43:21Z

@ybettan: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kernel-module-management-check-commits-count	`0a2b042`	link	true	`/test pull-kernel-module-management-check-commits-count`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

ybettan · 2024-07-02T18:46:22Z

/hold
This fix will fail for any field in the CR that is an optional pointer/slice and was not initiated because of this issue unless the +nullable kubebuilder annotation is added to it.

This is very fragile and will require an API bump for each field that we forgot to check.

@yevgeny-shnaidman @TomerNewman How about we just add a validation-webhook rule to refuse Module/ManagedClusterModule in which an image with no has or tag was specified?

We can proceed with this mutating webhook without the +nullable annotation once the PR fixing the issue is merged in https://github.com/kubernetes-sigs/structured-merge-diff.

ybettan · 2024-07-03T14:32:17Z

/close
Closing in favor of #853

k8s-ci-robot · 2024-07-03T14:32:22Z

@ybettan: Closed this PR.

In response to this:

/close
Closing in favor of #853

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot requested review from qbarrand and yevgeny-shnaidman June 3, 2024 12:48

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 3, 2024

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 3, 2024

yevgeny-shnaidman reviewed Jun 3, 2024

View reviewed changes

qbarrand reviewed Jun 4, 2024

View reviewed changes

ybettan force-pushed the latest-tag branch from 365ad21 to b87d8f9 Compare June 27, 2024 08:05

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 27, 2024

ybettan force-pushed the latest-tag branch 8 times, most recently from 4227a87 to 9acd668 Compare July 2, 2024 08:33

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 2, 2024

ybettan force-pushed the latest-tag branch 4 times, most recently from 5711d0f to f001c8d Compare July 2, 2024 13:59

ybettan mentioned this pull request Jul 2, 2024

Specifying {} to remove nested fields should yield {}, not nil kubernetes-sigs/structured-merge-diff#259

Open

tmp - Making some API fields nullable

0a2b042

Signed-off-by: Yoni Bettan <yonibettan@gmail.com>

ybettan force-pushed the latest-tag branch from f001c8d to 0a2b042 Compare July 2, 2024 18:42

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 2, 2024

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 2, 2024

ybettan mentioned this pull request Jul 2, 2024

Refusing Modules with container images without a sha or a tag. #853

Merged

k8s-ci-robot closed this Jul 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Defaulting to the `latest` tag if no tag was specified. #826

Defaulting to the `latest` tag if no tag was specified. #826

ybettan commented Jun 3, 2024 •

edited

Loading

k8s-ci-robot commented Jun 3, 2024

netlify bot commented Jun 3, 2024 •

edited

Loading

ybettan commented Jun 3, 2024

codecov-commenter commented Jun 3, 2024 •

edited

Loading

yevgeny-shnaidman Jun 3, 2024

ybettan Jun 3, 2024

yevgeny-shnaidman Jun 3, 2024

qbarrand Jun 4, 2024

yevgeny-shnaidman Jun 3, 2024

qbarrand Jun 4, 2024

qbarrand Jun 4, 2024

k8s-ci-robot commented Jul 2, 2024 •

edited

Loading

ybettan commented Jul 2, 2024

ybettan commented Jul 3, 2024

k8s-ci-robot commented Jul 3, 2024


		"github.com/go-logr/logr"
		multierror "github.com/hashicorp/go-multierror"

Defaulting to the latest tag if no tag was specified. #826

Defaulting to the latest tag if no tag was specified. #826

Conversation

ybettan commented Jun 3, 2024 • edited Loading

k8s-ci-robot commented Jun 3, 2024

netlify bot commented Jun 3, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kmm ready!

ybettan commented Jun 3, 2024

codecov-commenter commented Jun 3, 2024 • edited Loading

Codecov Report

yevgeny-shnaidman Jun 3, 2024

Choose a reason for hiding this comment

ybettan Jun 3, 2024

Choose a reason for hiding this comment

yevgeny-shnaidman Jun 3, 2024

Choose a reason for hiding this comment

qbarrand Jun 4, 2024

Choose a reason for hiding this comment

yevgeny-shnaidman Jun 3, 2024

Choose a reason for hiding this comment

qbarrand Jun 4, 2024

Choose a reason for hiding this comment

qbarrand Jun 4, 2024

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 2, 2024 • edited Loading

ybettan commented Jul 2, 2024

ybettan commented Jul 3, 2024

k8s-ci-robot commented Jul 3, 2024

Defaulting to the `latest` tag if no tag was specified. #826

Defaulting to the `latest` tag if no tag was specified. #826

ybettan commented Jun 3, 2024 •

edited

Loading

netlify bot commented Jun 3, 2024 •

edited

Loading

codecov-commenter commented Jun 3, 2024 •

edited

Loading

k8s-ci-robot commented Jul 2, 2024 •

edited

Loading