✨ Enable ssatags KAL linter #12470

sbueringer · 2025-07-09T16:52:02Z

Signed-off-by: Stefan Büringer buringerst@vmware.com

What this PR does / why we need it:

This PR enables the ssatags linter and sets listType=atomic everywhere explicitly.

I think in all the affected cases we don't really have a use case for shared ownership of the fields.

Let's also keep in mind that:

transitioning from atomic to map is not trivial and leads to severe issues, e.g.: ClusterClass Upgrade Fails Due to ManagedFields Behavior Change in CAPI v1.6 → v1.8 #11857
listType map produces additional overhead in managedFields (more managedFields => more memory usage of all controllers caching the objects)

Huge Kudos to @sivchari & @JoelSpeed for the linter!!!

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Part of #10852
Fixes #6504

hack/tools/.custom-gcl.yaml

sbueringer · 2025-07-09T18:34:42Z

Ready for review

/assign @fabriziopandini @JoelSpeed @sivchari @mboersma

/hold
for #12470 (comment)

sbueringer · 2025-07-10T10:35:37Z

/hold cancel

sbueringer · 2025-07-10T10:42:41Z

Ready for review/merge now :)

.golangci-kal.yml

JoelSpeed · 2025-07-10T10:41:42Z

api/addons/v1beta2/clusterresourceset_types.go

@@ -64,6 +64,7 @@ type ClusterResourceSetSpec struct {

 	// resources is a list of Secrets/ConfigMaps where each contains 1 or more resources to be applied to remote clusters.
 	// +optional
+	// +listType=atomic


This field is a classic example of one that would probably be listTypeMap. Each name/kind pair within the list should be unique right?

It should be unique, yes. But I think that doesn't justify making this field a listType=map:

changing to listType=map produces additional overhead in managedFields (i.e. memory usage)

changing this to a map leads to issues through the atomic=>map transition (folks won't be able to delete array elements in all scenarios, see: ClusterClass Upgrade Fails Due to ManagedFields Behavior Change in CAPI v1.6 → v1.8 #11857)

If we want that validation we should achieve it through other means that don't lead to these issues.

In my opinion we should be very careful with listType=map and should only use it:

if we have to for compatibility reasons, e.g. when we changed extraArgs from map to array we had to use listType=map to ensure compatibility on the managedFields level (changing to atomic would have lead to issues)

if we have the use case for multiple owners for a field (otherwise it's not worth the additional memory overhead of managed fields which can be significant if the array elements are large, see conditions for an example: https://storage.googleapis.com/kubernetes-ci-logs/logs/periodic-cluster-api-e2e-main/1943237186371457024/artifacts/clusters/bootstrap/resources/quick-start-yfb57f/Cluster/quick-start-qsya01.yaml)

@JoelSpeed Let's discuss this here first. This has impact on almost all other findings. I'm really not in favor of making arrays maps if we can come up with a key for the reasons mentioned above.

I'm not opposed to adding additional validation through other means, but honestly I won't have the bandwith for it in this release cycle. If someone wants to open a PR to add CEL validation with appropriate envtest-based unit test coverage that's fine for me.

In general, when adding new fields I've always recommended, where there is an obvious map key (list of named objects style) to go map over atomic. In general those decisions have aged well so far to my knowledge and we haven't hit any issues with significant memory usage.

You are right about changing things day 2 though, the SSA migration from atomic to map has had issues in the past, but I was under the impression some fixes went in to resolve those migration issues?

When we've had issues in the past with changing the atomic->map post release, was that during a migration between API versions? Or within the same API version?

When we've had issues in the past with changing the atomic->map post release, was that during a migration between API versions? Or within the same API version?

It was within the same apiVersion. IIRC I also tested it at the time and the same thing happens even between apiVersions. (and yes there were some fixes in the structured merge library but they were years ago, or at least I'm not aware of others)

Please see this comment: #11857 (comment)

Specifically:

There was a PR in September 2020 that was improving support of schema changes from granular to atomic and vice versa: kubernetes-sigs/structured-merge-diff#170. The change to improve support for atomic to granular schema changes was later reverted in April 2021: kubernetes-sigs/structured-merge-diff#192.

The recommendation nowadays is to not "change a type from atomic to map/set/granular." (kubernetes.io/docs/reference/using-api/server-side-apply#compatibility-across-topology-changes).

Adding my two cents on why we should not rush in transforming atomic list on map lists:

+listType=atomic is making explicit the current behaviour, which seems ok to users

in some cases allowing co-ownership of fields might lead to undesired effects (e.g. KubeadmConfig, machines could be created before all the actors have added their data to mapLists, leading to unnecessary rollouts).

api/addons/v1beta2/clusterresourcesetbinding_types.go

JoelSpeed · 2025-07-10T10:42:41Z

api/addons/v1beta2/clusterresourcesetbinding_types.go

@@ -57,6 +57,7 @@ type ResourceSetBinding struct {

 	// resources is a list of resources that the ClusterResourceSet has.
 	// +optional
+	// +listType=atomic


[]ResourceBinding style could also be listType Map with each Name/Kind being the keys right?

JoelSpeed · 2025-07-10T11:17:45Z

api/core/v1beta2/machinepool_types.go

@@ -122,6 +124,7 @@ type MachinePoolStatus struct {

 	// nodeRefs will point to the corresponding Nodes if it they exist.
 	// +optional
+	// +listType=atomic


Could be a map

JoelSpeed · 2025-07-10T11:19:54Z

api/core/v1beta2/machinehealthcheck_types.go

@@ -65,6 +65,7 @@ type MachineHealthCheckSpec struct {
 	// logical OR, i.e. if any of the conditions is met, the node is unhealthy.
 	//
 	// +optional
+	// +listType=atomic


Map on type?

JoelSpeed · 2025-07-10T11:21:37Z

api/core/v1beta2/clusterclass_types.go

@@ -392,6 +394,7 @@ type MachineHealthCheckClass struct {
 	// logical OR, i.e. if any of the conditions is met, the node is unhealthy.
 	//
 	// +optional
+	// +listType=atomic


Map on type?

JoelSpeed · 2025-07-10T11:22:09Z

api/core/v1beta2/clusterclass_types.go

 	// +kubebuilder:validation:MaxItems=1000
 	Variables []ClusterClassVariable `json:"variables,omitempty"`

 	// patches defines the patches which are applied to customize
 	// referenced templates of a ClusterClass.
 	// Note: Patches will be applied in the order of the array.
 	// +optional
+	// +listType=atomic


Map on name?

JoelSpeed · 2025-07-10T11:22:33Z

api/core/v1beta2/clusterclass_types.go

@@ -119,13 +119,15 @@ type ClusterClassSpec struct {
 	// variables defines the variables which can be configured
 	// in the Cluster topology and are then used in patches.
 	// +optional
+	// +listType=atomic


Map on name?

sbueringer · 2025-07-10T13:17:53Z

/test pull-cluster-api-e2e-main

sbueringer · 2025-07-10T17:20:40Z

/override pull-cluster-api-e2e-main

k8s-ci-robot · 2025-07-10T17:20:44Z

@sbueringer: Overrode contexts on behalf of sbueringer: pull-cluster-api-e2e-main

In response to this:

/override pull-cluster-api-e2e-main

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.

Makefile

Signed-off-by: Stefan Büringer buringerst@vmware.com

…rray fields as atomic Signed-off-by: Stefan Büringer buringerst@vmware.com

Signed-off-by: Stefan Büringer buringerst@vmware.com

JoelSpeed · 2025-07-11T12:23:17Z

/lgtm
/approve

/hold any other reviewers we want to tag in?

k8s-ci-robot · 2025-07-11T12:23:23Z

LGTM label has been added.

Git tree hash: e7d9ee4cd78e2a658466064980b9ea51c8168af9

k8s-ci-robot · 2025-07-11T12:23:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

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~~ [JoelSpeed]

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

sbueringer · 2025-07-11T12:47:30Z

/hold any other reviewers we want to tag in?

I think Fabrizio once he's back

fabriziopandini · 2025-07-14T11:17:16Z

/lgtm

/hold cancel

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 9, 2025

k8s-ci-robot requested review from chrischdi and fabriziopandini July 9, 2025 16:52

k8s-ci-robot added do-not-merge/needs-area PR is missing an area label size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 9, 2025

sbueringer mentioned this pull request Jul 9, 2025

Umbrella issue: API changes #10852

Open

91 tasks

sbueringer added area/api Issues or PRs related to the APIs area/ci Issues or PRs related to ci labels Jul 9, 2025

k8s-ci-robot removed do-not-merge/needs-area PR is missing an area label labels Jul 9, 2025

sbueringer force-pushed the pr-enable-ssatags-linter branch from 7dcd23a to 2d68ae1 Compare July 9, 2025 18:19

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 9, 2025

sbueringer changed the title ~~[WIP] ✨ Enable ssatags KAL linter~~ ✨ Enable ssatags KAL linter Jul 9, 2025

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 9, 2025

sbueringer commented Jul 9, 2025

View reviewed changes

hack/tools/.custom-gcl.yaml Show resolved Hide resolved

sbueringer changed the title ~~✨ Enable ssatags KAL linter~~ [WIP] ✨ Enable ssatags KAL linter Jul 9, 2025

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 9, 2025

sbueringer force-pushed the pr-enable-ssatags-linter branch from 2d68ae1 to 8847e73 Compare July 9, 2025 18:34

k8s-ci-robot assigned fabriziopandini Jul 9, 2025

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 9, 2025

k8s-ci-robot assigned JoelSpeed, mboersma and sivchari Jul 9, 2025

sbueringer changed the title ~~[WIP] ✨ Enable ssatags KAL linter~~ ✨ Enable ssatags KAL linter Jul 9, 2025

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 9, 2025

sbueringer force-pushed the pr-enable-ssatags-linter branch from 8847e73 to 47e7680 Compare July 9, 2025 18:35

k8s-ci-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 9, 2025

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

sbueringer mentioned this pull request Jul 10, 2025

SSATags linter suggests SSA markers on field that already has listType on the type kubernetes-sigs/kube-api-linter#119

Closed

JoelSpeed reviewed Jul 10, 2025

View reviewed changes

sbueringer force-pushed the pr-enable-ssatags-linter branch from 42a8566 to 0ab5673 Compare July 10, 2025 13:16

JoelSpeed reviewed Jul 11, 2025

View reviewed changes

Makefile Outdated Show resolved Hide resolved

sbueringer added 7 commits July 11, 2025 14:02

Enable ssatags KAL linter

0463e8e

Signed-off-by: Stefan Büringer buringerst@vmware.com

Mark all []string array fields as atomic

f5ac2b2

Mark all remaining KubeadmConfig array fields as atomic

a90c31f

Signed-off-by: Stefan Büringer buringerst@vmware.com

Mark all remaining ClusterResourceSet and ClusterResourceSetBinding a…

18f7f63

…rray fields as atomic Signed-off-by: Stefan Büringer buringerst@vmware.com

Mark all remaining ClusterClass array fields as atomic

8fc263a

Signed-off-by: Stefan Büringer buringerst@vmware.com

Mark all remaining Machine, MHC and MachinePool array fields as atomic

19c90e8

Signed-off-by: Stefan Büringer buringerst@vmware.com

Regenerate manifests & OpenAPI

596994a

sbueringer force-pushed the pr-enable-ssatags-linter branch from 0ab5673 to 596994a Compare July 11, 2025 12:02

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jul 11, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 11, 2025

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

k8s-ci-robot merged commit 5974ef5 into kubernetes-sigs:main Jul 14, 2025
19 checks passed

k8s-ci-robot added this to the v1.11 milestone Jul 14, 2025

sbueringer deleted the pr-enable-ssatags-linter branch July 14, 2025 11:51

✨ Enable ssatags KAL linter #12470

✨ Enable ssatags KAL linter #12470

Conversation

sbueringer commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sbueringer commented Jul 9, 2025

Uh oh!

sbueringer commented Jul 10, 2025

Uh oh!

sbueringer commented Jul 10, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbueringer Jul 10, 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!

sbueringer Jul 10, 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!

sbueringer Jul 10, 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!

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!

sbueringer commented Jul 10, 2025

Uh oh!

sbueringer commented Jul 10, 2025

Uh oh!

k8s-ci-robot commented Jul 10, 2025

Uh oh!

Uh oh!

JoelSpeed commented Jul 11, 2025

Uh oh!

k8s-ci-robot commented Jul 11, 2025

Uh oh!

k8s-ci-robot commented Jul 11, 2025

Uh oh!

sbueringer commented Jul 11, 2025

Uh oh!

fabriziopandini commented Jul 14, 2025

Uh oh!

Uh oh!

Uh oh!

sbueringer commented Jul 9, 2025 •

edited

Loading

sbueringer Jul 10, 2025 •

edited

Loading

sbueringer Jul 10, 2025 •

edited

Loading

sbueringer Jul 10, 2025 •

edited

Loading