🌱 wip: add kube-api-linter, fix linter issues #2058
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| version: v1.64.8 | ||
| name: golangci-kube-api-linter | ||
| destination: ./bin | ||
| plugins: | ||
| - module: 'sigs.k8s.io/kube-api-linter' | ||
| version: 'v0.0.0-20250626111229-e719da12d840' # Replace with the latest version | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,25 +54,27 @@ | |
| // ClusterCatalog enables users to make File-Based Catalog (FBC) catalog data available to the cluster. | ||
| // For more information on FBC, see https://olm.operatorframework.io/docs/reference/file-based-catalogs/#docs | ||
| type ClusterCatalog struct { | ||
| // +optional | ||
| metav1.TypeMeta `json:",inline"` | ||
|
|
||
| // metadata is the standard object's metadata. | ||
| // More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata | ||
| // +optional | ||
| metav1.ObjectMeta `json:"metadata"` | ||
|
|
||
| // spec is the desired state of the ClusterCatalog. | ||
| // spec is required. | ||
| // The controller will work to ensure that the desired | ||
| // catalog is unpacked and served over the catalog content HTTP server. | ||
| // +required | ||
camilamacedo86 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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"` //nolint: kubeapilinter | ||
|
There was a problem hiding this comment. This nolint is because of a bug (I think) in kubeapilinter suggesting that status fields in top-level Kind objects should be pointers There was a problem hiding this comment. I think the linter is raising it right here. It should be a pointer because:
There was a problem hiding this comment. The linter is correct, optional structs, unless they have Otherwise the structured client will marshal There was a problem hiding this comment. 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. |
||
| } | ||
|
|
||
| //+kubebuilder:object:root=true | ||
|
|
@@ -87,7 +89,7 @@ | |
|
|
||
| // items is a list of ClusterCatalogs. | ||
| // items is required. | ||
| // +required | ||
| Items []ClusterCatalog `json:"items"` | ||
| } | ||
|
|
||
|
|
@@ -110,7 +112,7 @@ | |
| // image: | ||
| // ref: quay.io/operatorhubio/catalog:latest | ||
| // | ||
| // +required | ||
| Source CatalogSource `json:"source"` | ||
|
|
||
| // priority allows the user to define a priority for a ClusterCatalog. | ||
|
|
@@ -131,10 +133,10 @@ | |
| // The highest possible value is 2147483647. | ||
| // | ||
| // +kubebuilder:default:=0 | ||
| // +kubebuilder:validation:Minimum:=-2147483648 | ||
| // +kubebuilder:validation:Maximum:=2147483647 | ||
|
Comment on lines
-134
to
+137
There was a problem hiding this comment. 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:
There was a problem hiding this comment. IHMO, we should make this change. There was a problem hiding this comment. 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 There was a problem hiding this comment. 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. There was a problem hiding this comment. Whether or not this is a reasonably safe change is an entirely separate question. |
||
| // +optional | ||
| Priority int32 `json:"priority"` | ||
|
There was a problem hiding this comment. This should be a pointer with There was a problem hiding this comment. 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:
So, IHMO it should be fine as: // +optional pinging @JoelSpeed he might can help us to know more about There was a problem hiding this comment. 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 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. There was a problem hiding this comment. Thank you very much for the detailed and very comprehensive info There was a problem hiding this comment. Is Based on what you've said, it sounds to me like this field is actually required but allows the zero value. |
||
|
|
||
| // availabilityMode allows users to define how the ClusterCatalog is made available to clients on the cluster. | ||
| // availabilityMode is optional. | ||
|
|
@@ -179,6 +181,8 @@ | |
| // contents. This could occur when we've initially fetched the latest contents from the source for this catalog and when polling for changes | ||
| // to the contents we identify that there are updates to the contents. | ||
| // | ||
| // +patchMergeKey=type | ||
| // +patchStrategy=merge | ||
| // +listType=map | ||
| // +listMapKey=type | ||
| // +optional | ||
|
|
@@ -214,7 +218,7 @@ | |
| // | ||
| // As the needs of users and clients of the evolve, new endpoints may be added. | ||
| // | ||
| // +required | ||
| // +kubebuilder:validation:MaxLength:=525 | ||
| // +kubebuilder:validation:XValidation:rule="isURL(self)",message="must be a valid URL" | ||
| // +kubebuilder:validation:XValidation:rule="isURL(self) ? (url(self).getScheme() == \"http\" || url(self).getScheme() == \"https\") : true",message="scheme must be either http or https" | ||
|
|
@@ -236,7 +240,7 @@ | |
| // | ||
| // +unionDiscriminator | ||
| // +kubebuilder:validation:Enum:="Image" | ||
| // +required | ||
| Type SourceType `json:"type"` | ||
| // image is used to configure how catalog contents are sourced from an OCI image. | ||
| // This field is required when type is Image, and forbidden otherwise. | ||
|
|
@@ -258,19 +262,20 @@ | |
| // | ||
| // +unionDiscriminator | ||
| // +kubebuilder:validation:Enum:="Image" | ||
| // +required | ||
| 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. | ||
| // +optional | ||
| Image *ResolvedImageSource `json:"image,omitempty"` | ||
|
There was a problem hiding this comment. I believe adding the There was a problem hiding this comment. 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 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? There was a problem hiding this comment. Since, without You've also added the 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 There was a problem hiding this comment. @camilamacedo86 That sounds right to me. If there is no way for this to be set to |
||
| } | ||
|
|
||
| // ResolvedImageSource provides information about the resolved source of a Catalog sourced from an image. | ||
| type ResolvedImageSource struct { | ||
| // ref contains the resolved image digest-based reference. | ||
| // The digest format is used so users can use other tooling to fetch the exact | ||
| // OCI manifests that were used to extract the catalog contents. | ||
| // +required | ||
| // +kubebuilder:validation:MaxLength:=1000 | ||
| // +kubebuilder:validation:XValidation:rule="self.matches('^([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9])((\\\\.([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9]))+)?(:[0-9]+)?\\\\b')",message="must start with a valid domain. valid domains must be alphanumeric characters (lowercase and uppercase) separated by the \".\" character." | ||
| // +kubebuilder:validation:XValidation:rule="self.find('(\\\\/[a-z0-9]+((([._]|__|[-]*)[a-z0-9]+)+)?((\\\\/[a-z0-9]+((([._]|__|[-]*)[a-z0-9]+)+)?)+)?)') != \"\"",message="a valid name is required. valid names must contain lowercase alphanumeric characters separated only by the \".\", \"_\", \"__\", \"-\" characters." | ||
|
|
@@ -325,7 +330,7 @@ | |
| // An example of a valid digest-based image reference is "quay.io/operatorhubio/catalog@sha256:200d4ddb2a73594b91358fe6397424e975205bfbe44614f5846033cad64b3f05" | ||
| // An example of a valid tag-based image reference is "quay.io/operatorhubio/catalog:latest" | ||
| // | ||
| // +required | ||
| // +kubebuilder:validation:MaxLength:=1000 | ||
| // +kubebuilder:validation:XValidation:rule="self.matches('^([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9])((\\\\.([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9]))+)?(:[0-9]+)?\\\\b')",message="must start with a valid domain. valid domains must be alphanumeric characters (lowercase and uppercase) separated by the \".\" character." | ||
| // +kubebuilder:validation:XValidation:rule="self.find('(\\\\/[a-z0-9]+((([._]|__|[-]*)[a-z0-9]+)+)?((\\\\/[a-z0-9]+((([._]|__|[-]*)[a-z0-9]+)+)?)+)?)') != \"\"",message="a valid name is required. valid names must contain lowercase alphanumeric characters separated only by the \".\", \"_\", \"__\", \"-\" characters." | ||
|
|
@@ -344,7 +349,7 @@ | |
| // When omitted, the image will not be polled for new content. | ||
| // +kubebuilder:validation:Minimum:=1 | ||
| // +optional | ||
| PollIntervalMinutes *int `json:"pollIntervalMinutes,omitempty"` //nolint: kubeapilinter | ||
|
There was a problem hiding this comment. Nolint here because it wants this to be either There was a problem hiding this comment. So, in your POV, should we consider it a bug fix and move forward? @joelanford WDYT? |
||
| } | ||
|
|
||
| func init() { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,7 +59,7 @@ type ClusterExtensionSpec struct { | |
| // +kubebuilder:validation:MaxLength:=63 | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="namespace is immutable" | ||
| // +kubebuilder:validation:XValidation:rule="self.matches(\"^[a-z0-9]([-a-z0-9]*[a-z0-9])?$\")",message="namespace must be a valid DNS1123 label" | ||
| // +required | ||
| Namespace string `json:"namespace"` | ||
|
|
||
| // serviceAccount is a reference to a ServiceAccount used to perform all interactions | ||
|
|
@@ -68,7 +68,7 @@ type ClusterExtensionSpec struct { | |
| // The ServiceAccount must exist in the namespace referenced in the spec. | ||
| // serviceAccount is required. | ||
| // | ||
| // +required | ||
| ServiceAccount ServiceAccountReference `json:"serviceAccount"` | ||
|
|
||
| // source is a required field which selects the installation source of content | ||
|
|
@@ -84,7 +84,7 @@ type ClusterExtensionSpec struct { | |
| // catalog: | ||
| // packageName: example-package | ||
| // | ||
| // +required | ||
| Source SourceConfig `json:"source"` | ||
|
|
||
| // install is an optional field used to configure the installation options | ||
|
|
@@ -112,7 +112,7 @@ type SourceConfig struct { | |
| // | ||
| // +unionDiscriminator | ||
| // +kubebuilder:validation:Enum:="Catalog" | ||
| // +required | ||
| SourceType string `json:"sourceType"` | ||
|
|
||
| // catalog is used to configure how information is sourced from a catalog. | ||
|
|
@@ -162,11 +162,10 @@ type CatalogFilter struct { | |
| // | ||
| // [RFC 1123]: https://tools.ietf.org/html/rfc1123 | ||
| // | ||
| // +required | ||
| // +kubebuilder:validation:MaxLength:=253 | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="packageName is immutable" | ||
| // +kubebuilder:validation:XValidation:rule="self.matches(\"^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$\")",message="packageName must be a valid DNS1123 subdomain. It must contain only lowercase alphanumeric characters, hyphens (-) or periods (.), start and end with an alphanumeric character, and be no longer than 253 characters" | ||
| PackageName string `json:"packageName"` | ||
|
|
||
| // version is an optional semver constraint (a specific version or range of versions). When unspecified, the latest version available will be installed. | ||
|
|
@@ -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 | ||
|
There was a problem hiding this comment. Breaking change because explicitly setting Ratcheting should take effect here, but again - up to maintainer discretion. There was a problem hiding this comment. What happens if we set "version: """ today? There was a problem hiding this comment. From what I understand, this would be accepted as a valid input. |
||
| // +kubebuilder:validation:XValidation:rule="self.matches(\"^(\\\\s*(=||!=|>|<|>=|=>|<=|=<|~|~>|\\\\^)\\\\s*(v?(0|[1-9]\\\\d*|[x|X|\\\\*])(\\\\.(0|[1-9]\\\\d*|x|X|\\\\*]))?(\\\\.(0|[1-9]\\\\d*|x|X|\\\\*))?(-([0-9A-Za-z\\\\-]+(\\\\.[0-9A-Za-z\\\\-]+)*))?(\\\\+([0-9A-Za-z\\\\-]+(\\\\.[0-9A-Za-z\\\\-]+)*))?)\\\\s*)((?:\\\\s+|,\\\\s*|\\\\s*\\\\|\\\\|\\\\s*)(=||!=|>|<|>=|=>|<=|=<|~|~>|\\\\^)\\\\s*(v?(0|[1-9]\\\\d*|x|X|\\\\*])(\\\\.(0|[1-9]\\\\d*|x|X|\\\\*))?(\\\\.(0|[1-9]\\\\d*|x|X|\\\\*]))?(-([0-9A-Za-z\\\\-]+(\\\\.[0-9A-Za-z\\\\-]+)*))?(\\\\+([0-9A-Za-z\\\\-]+(\\\\.[0-9A-Za-z\\\\-]+)*))?)\\\\s*)*$\")",message="invalid version expression" | ||
| // +optional | ||
| Version string `json:"version,omitempty"` | ||
|
|
@@ -356,7 +356,7 @@ type ServiceAccountReference struct { | |
| // +kubebuilder:validation:MaxLength:=253 | ||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="name is immutable" | ||
| // +kubebuilder:validation:XValidation:rule="self.matches(\"^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$\")",message="name must be a valid DNS1123 subdomain. It must contain only lowercase alphanumeric characters, hyphens (-) or periods (.), start and end with an alphanumeric character, and be no longer than 253 characters" | ||
| // +required | ||
| Name string `json:"name"` | ||
| } | ||
|
|
||
|
|
@@ -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. | ||
| // +optional | ||
| CRDUpgradeSafety *CRDUpgradeSafetyPreflightConfig `json:"crdUpgradeSafety,omitempty"` | ||
|
There was a problem hiding this comment. I think that adding omitempty here is also a breaking change because it changes serialization (i.e 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 There was a problem hiding this comment. I think you are right. There was a problem hiding this comment. This field isn't nullable so an empty value would have marshalled to |
||
| } | ||
|
|
||
| // CRDUpgradeSafetyPreflightConfig is the configuration for CRD upgrade safety preflight check. | ||
|
|
@@ -386,7 +387,7 @@ type CRDUpgradeSafetyPreflightConfig struct { | |
| // performing an upgrade operation. | ||
| // | ||
| // +kubebuilder:validation:Enum:="None";"Strict" | ||
| // +required | ||
| Enforcement CRDUpgradeSafetyEnforcement `json:"enforcement"` | ||
| } | ||
|
|
||
|
|
@@ -411,20 +412,21 @@ type BundleMetadata struct { | |
| // hyphens (-) or periods (.), start and end with an alphanumeric character, | ||
| // and be no longer than 253 characters. | ||
| // | ||
| // +required | ||
| // +kubebuilder:validation:XValidation:rule="self.matches(\"^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$\")",message="packageName must be a valid DNS1123 subdomain. It must contain only lowercase alphanumeric characters, hyphens (-) or periods (.), start and end with an alphanumeric character, and be no longer than 253 characters" | ||
| Name string `json:"name"` | ||
|
|
||
| // version is a required field and is a reference to the version that this bundle represents | ||
| // version follows the semantic versioning standard as defined in https://semver.org/. | ||
| // | ||
| // +required | ||
| // +kubebuilder:validation:XValidation:rule="self.matches(\"^([0-9]+)(\\\\.[0-9]+)?(\\\\.[0-9]+)?(-([-0-9A-Za-z]+(\\\\.[-0-9A-Za-z]+)*))?(\\\\+([-0-9A-Za-z]+(-\\\\.[-0-9A-Za-z]+)*))?\")",message="version must be well-formed semver" | ||
| Version string `json:"version"` | ||
| } | ||
|
|
||
| // ClusterExtensionStatus defines the observed state of a ClusterExtension. | ||
| type ClusterExtensionStatus struct { | ||
| // conditions represents the observed state of the cluster extension. | ||
| // The set of condition types which apply to all spec.source variations are Installed and Progressing. | ||
| // | ||
| // The Installed condition represents whether or not the bundle has been installed for this ClusterExtension. | ||
|
|
@@ -463,7 +465,7 @@ type ClusterExtensionInstallStatus struct { | |
| // A "bundle" is a versioned set of content that represents the resources that | ||
| // need to be applied to a cluster to install a package. | ||
| // | ||
| // +required | ||
| Bundle BundleMetadata `json:"bundle"` | ||
| } | ||
|
|
||
|
|
@@ -478,16 +480,20 @@ type ClusterExtensionInstallStatus struct { | |
|
|
||
| // ClusterExtension is the Schema for the clusterextensions API | ||
| type ClusterExtension struct { | ||
| // +optional | ||
| metav1.TypeMeta `json:",inline"` | ||
|
|
||
| // metadata is the object metadata | ||
| // +optional | ||
| metav1.ObjectMeta `json:"metadata,omitempty"` | ||
|
|
||
| // spec is a required field that defines the desired state of the ClusterExtension. | ||
| // +required | ||
| Spec ClusterExtensionSpec `json:"spec"` | ||
|
|
||
| // status is an optional field that defines the observed state of the ClusterExtension. | ||
| // +optional | ||
| Status ClusterExtensionStatus `json:"status,omitempty"` //nolint: kubeapilinter | ||
| } | ||
|
|
||
| // +kubebuilder:object:root=true | ||
|
|
@@ -501,7 +507,7 @@ type ClusterExtensionList struct { | |
|
|
||
| // items is a required list of ClusterExtension objects. | ||
| // | ||
| // +required | ||
| Items []ClusterExtension `json:"items"` | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not using version: v1.64.8 any more
We need config it with 2x
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Golangci-lint version still shows as 1.64.8 on main to me:
operator-controller/.bingo/golangci-lint.mod
Line 5 in c322aaa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.