🌱 wip: add kube-api-linter, fix linter issues #2058

everettraven · 2025-06-26T19:24:15Z

Description

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>

openshift-ci · 2025-06-26T19:24:23Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign tmshort for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

netlify · 2025-06-26T19:24:47Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`c5710f6`
🔍 Latest deploy log	https://app.netlify.com/projects/olmv1/deploys/685d9e6244a1c00008d8dbd4
😎 Deploy Preview	https://deploy-preview-2058--olmv1.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 project configuration.

everettraven · 2025-06-26T19:25:27Z

api/v1/clustercatalog_types.go

 	Spec ClusterCatalogSpec `json:"spec"`

 	// status contains information about the state of the ClusterCatalog such as:
 	//   - Whether or not the catalog contents are being served via the catalog content HTTP server
 	//   - Whether or not the ClusterCatalog is progressing to a new state
 	//   - A reference to the source from which the catalog contents were retrieved
 	// +optional
-	Status ClusterCatalogStatus `json:"status,omitempty"`
+	Status ClusterCatalogStatus `json:"status,omitempty"` //nolint: kubeapilinter


This nolint is because of a bug (I think) in kubeapilinter suggesting that status fields in top-level Kind objects should be pointers

I think the linter is raising it right here.

It should be a pointer because:

.status is optional and only set by the controller.

Value types serialize as status: {} even when empty.
Pointer types allow the field to be omitted (cleaner JSON).

Follows Kubernetes API conventions — e.g. Deployment, CronJob use *Status.

The user does not set that just the controllers

The linter is correct, optional structs, unless they have omitzero, must be pointers.

Otherwise the structured client will marshal {} and that may have a semantic difference (e.g. CEL rules are triggered here where they may not have been before)

Sounds like there were recent discussions with Kubernetes API reviewers that this makes sense as-is that I was unaware of and thought there were discussions going the other way that i was referencing.

This should indeed be a pointer.

everettraven · 2025-06-26T19:28:47Z

api/v1/clustercatalog_types.go

-	// +kubebuilder:validation:minimum:=-2147483648
-	// +kubebuilder:validation:maximum:=2147483647
+	// +kubebuilder:validation:Minimum:=-2147483648
+	// +kubebuilder:validation:Maximum:=2147483647


This was a typo that meant this was not properly being enforced. This is now a breaking change as these APIs have already been released as v1.

I'll leave it up to the maintainers as to how this should be handled. Some options I can quickly think of:

make the change, expect ratcheting validations to take over where used in production. Seems unlikely that users could have reasonably specified something out of these bounds.

don't make the change. Leave unvalidated.

IHMO, we should make this change.
It is a bug, not a breaking change.
We are not validating something that we should validate.

Also, given that this is the entire range of a valid int32... The odds of someone hitting an issue because we fixed this typo is very, very, very low

Just because it is a bug does not make it a non-breaking change. This can be both, but it is most certainly a "breaking change" in the sense that you are adding restrictions to something that previously had no restrictions.

Whether or not this is a reasonably safe change is an entirely separate question.

everettraven · 2025-06-26T19:30:00Z

api/v1/clustercatalog_types.go

-	// +kubebuilder:validation:minimum:=-2147483648
-	// +kubebuilder:validation:maximum:=2147483647
+	// +kubebuilder:validation:Minimum:=-2147483648
+	// +kubebuilder:validation:Maximum:=2147483647
 	// +optional
 	Priority int32 `json:"priority"`


This should be a pointer with omitempty as the 0 value is a valid value, but that would be a breaking change. Also leaving up to maintainers how they might want to handle this.

I think we have a bug in the linter here.

I am applying Kubebuilder to all scenarios, so we can discuss each one to shape not only Kubebuilder scaffolds and samples, but also the linter.

But IMO, this should not be a pointer because:

Zero (0) is a meaningful default, not absence.

The spec comment already treats 0 as the intended default.

We also have +kubebuilder:default:=0, which works perfectly with a value type.

There's no need to distinguish between unset and zero.

So, IHMO it should be fine as:

// +optional
+kubebuilder:default:=0
Priority int32 json:"priority"

pinging @JoelSpeed he might can help us to know more about

This is a very special case, and while I agree with what @camilamacedo86 is saying, there are some nuances.

The field is optional, so in general, we would want to be able to tell between unset and the zero value.

The field has a default, this means that for any client, should they omit the value, it would be set by the API server to 0.

If the value were non-zero, it's obvious that this needs to be a pointer with omitempty (I hope that's obvious? Challenge me otherwise and I'll explain).

The nuance that is missed here is who is doing the defaulting.

If we leave this field as a non-pointer, non-omitempty, this means that the structured client happens to marshal the default value when non other opinion is set.

If this were a pointer+omitempty, then the structured client would not marshal anything, and the API server would default the value for them, and on future round trips, the value would be set, because 0 is not a nil pointer.

So, for this case, yes, what is there currently works, but it's not "correct". The structured client has no way to specify "I have no opinion, please apply a default value".

IMO, this should be pointer and omitempty and have the API server apply the default, which would be consistent with any other field that is optional and has a valid zero value.

Thank you very much for the detailed and very comprehensive info

Is priority truly optional here then? If it is truly optional then it should be a pointer to distinguish between unset and the valid zero value.

Based on what you've said, it sounds to me like this field is actually required but allows the zero value.

everettraven · 2025-06-26T19:30:57Z

api/v1/clustercatalog_types.go

 	Type SourceType `json:"type"`
 	// image is a field containing resolution information for a catalog sourced from an image.
 	// This field must be set when type is Image, and forbidden otherwise.
-	Image *ResolvedImageSource `json:"image"`
+	// +optional
+	Image *ResolvedImageSource `json:"image,omitempty"`


I believe adding the omitempty would technically be a breaking change here as well

You are right, but in this case we have a check already see;

operator-controller/api/v1/clustercatalog_types.go

Lines 230 to 231 in c5710f6

// +union

// +kubebuilder:validation:XValidation:rule="has(self.type) && self.type == 'Image' ? has(self.image) : !has(self.image)",message="image is required when source type is Image, and forbidden otherwise"

Adding omitempty ensures that when Image is nil, it’s not serialized at all, which is aligned with the above validation rule and union semantics as far I get it. So, I think it’s fair to call this a small bug fix in the CRD/spec — we’re not changing the API behavior, just ensuring that invalid/unnecessary output doesn't get emitted.

c/c @joelanford wdyt?

Since, without omitempty, this would render to null, and the field does not have +nullable, any request from a structured client that did not set this would have been rejected.

You've also added the optional tag which takes this from required to optional, which is correct from the DU perspective, and as @camilamacedo86 points out, doesn't matter anyway because of the CEL.

As far as I can tell, requests from structured clients before and after this change behave in the same way (reject or accept, error message may vary), +1 to go ahead

@camilamacedo86 That sounds right to me. If there is no way for this to be set to image: null today, then adding the omitempty would not change serialization.

everettraven · 2025-06-26T19:32:09Z

api/v1/clustercatalog_types.go

@@ -344,7 +349,7 @@ type ImageSource struct {
 	// When omitted, the image will not be polled for new content.
 	// +kubebuilder:validation:Minimum:=1
 	// +optional
-	PollIntervalMinutes *int `json:"pollIntervalMinutes,omitempty"`
+	PollIntervalMinutes *int `json:"pollIntervalMinutes,omitempty"` //nolint: kubeapilinter


Nolint here because it wants this to be either int32 or int64 and not a pointer which would be a breaking change. Although I should probably remove the nolint

So, in your POV, should we consider it a bug fix and move forward?
I think it might be acceptable; it will not change the expected value for end users, but we will need to change our internal code.

@joelanford WDYT?
Could we do the change?

everettraven · 2025-06-26T19:34:46Z

api/v1/clusterextension_types.go

@@ -244,6 +243,7 @@ type CatalogFilter struct {
 	// For more information on semver, please see https://semver.org/
 	//
 	// +kubebuilder:validation:MaxLength:=64
+	// +kubebuilder:validation:MinLength:=1


Breaking change because explicitly setting version: "" is no longer valid, but IIRC the original intention was to actually have it so you either don't provide this or explicitly provide a range so this would be more appropriate.

Ratcheting should take effect here, but again - up to maintainer discretion.

What happens if we set "version: """ today?
If that is not allowed already, then we might be able to move forward here as a bug fix.
We are just trying to catch the invalid scenario before.

From what I understand, this would be accepted as a valid input.

everettraven · 2025-06-26T19:37:58Z

api/v1/clusterextension_types.go

@@ -369,7 +369,8 @@ type PreflightConfig struct {
 	//
 	// The CRD Upgrade Safety pre-flight check safeguards from unintended
 	// consequences of upgrading a CRD, such as data loss.
-	CRDUpgradeSafety *CRDUpgradeSafetyPreflightConfig `json:"crdUpgradeSafety"`
+	// +optional
+	CRDUpgradeSafety *CRDUpgradeSafetyPreflightConfig `json:"crdUpgradeSafety,omitempty"`


I think that adding omitempty here is also a breaking change because it changes serialization (i.e crdUpgradeSafety: null as opposed to it jusst being totally missing.

That being said, the CEL validation and the fact that the parent field is omitempty might make it so that this is never actually serialized with a null value, meaning this might be OK.

I think you are right.
@joelanford is the right person here to 🔨 .
WDYT?

This field isn't nullable so an empty value would have marshalled to null and would have been rejected, right?

camilamacedo86 · 2025-06-27T07:28:02Z

.custom-gcl.yml

+destination: ./bin
+plugins:
+- module: 'sigs.k8s.io/kube-api-linter'
+  version: 'v0.0.0-20250626111229-e719da12d840' # Replace with the latest version


We are not using version: v1.64.8 any more
We need config it with 2x

That doesn't seem to be true based on what I gathered from the bingo files that set golangci-lint to 1.64.8.

If this recently changed, I can look into updating this. I'll hold off on this until other items that I think are more important are addressed in the PR.

When you opened the PR, it was using Go 1.64.8 :-)
But that’s no longer the case now.

The issue is that we can’t use .custom-gcl.yml with GolangCI-Lint v2.x.
You can see how I was testing it in Kubebuilder here:
https://github.com/kubernetes-sigs/kubebuilder/pull/4888/files

I don’t think it’s the best way to install it, but it might help as a reference.

Golangci-lint version still shows as 1.64.8 on main to me:

operator-controller/.bingo/golangci-lint.mod

Line 5 in c322aaa

require github.com/golangci/golangci-lint v1.64.8 // cmd/golangci-lint

OH, we did not merged : https://github.com/operator-framework/operator-controller/pull/2045/files
^ can you help me out, get this one merged so we do it right?

I no longer consider myself a maintainer of this project so I'll defer that to current maintainers to get it in. I'll update the configuration in the PR accordingly once that lands.

api/v1/clustercatalog_types.go

openshift-merge-robot · 2025-06-27T22:34:36Z

PR needs rebase.

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.

wip: add kube-api-linter, fix linter issues

c5710f6

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 26, 2025

everettraven commented Jun 26, 2025

View reviewed changes

camilamacedo86 reviewed Jun 27, 2025

View reviewed changes

api/v1/clustercatalog_types.go Show resolved Hide resolved

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2025

	// +union
	// +kubebuilder:validation:XValidation:rule="has(self.type) && self.type == 'Image' ? has(self.image) : !has(self.image)",message="image is required when source type is Image, and forbidden otherwise"

🌱 wip: add kube-api-linter, fix linter issues #2058

Are you sure you want to change the base?

🌱 wip: add kube-api-linter, fix linter issues #2058

Uh oh!

Conversation

everettraven commented Jun 26, 2025

Description

Reviewer Checklist

Uh oh!

openshift-ci bot commented Jun 26, 2025

Uh oh!

netlify bot commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for olmv1 ready!

Uh oh!

everettraven Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

netlify bot commented Jun 26, 2025 •

edited

Loading

everettraven Jun 26, 2025 •

edited

Loading

camilamacedo86 Jun 27, 2025 •

edited

Loading

camilamacedo86 Jun 27, 2025 •

edited

Loading