Skip to content

✨ Enable ssatags KAL linter #12470

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 19 additions & 6 deletions .golangci-kal.yml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ linters:
# having the `omitempty` value in their `json` tag where appropriate.
- "optionalorrequired" # Every field should be marked as `+optional` or `+required`.
- "requiredfields" # Required fields should not be pointers, and should not have `omitempty`.
- "ssatags" # Ensure array fields have the appropriate listType markers
- "statusoptional" # Ensure all first children within status should be optional.
- "statussubresource" # All root objects that have a `status` field should have a status subresource.
- "uniquemarkers" # Ensure that types and fields do not contain more than a single definition of a marker that should only be present once.
Expand All @@ -44,19 +45,21 @@ linters:
isFirstField: Warn # Require conditions to be the first field in the status struct.
usePatchStrategy: Forbid # Require conditions to be the first field in the status struct.
useProtobuf: Forbid # We don't use protobuf, so protobuf tags are not required.
optionalFields:
optionalfields:
pointers:
preference: WhenRequired # Always | WhenRequired # Whether to always require pointers, or only when required. Defaults to `Always`.
policy: SuggestFix # SuggestFix | Warn # The policy for pointers in optional fields. Defaults to `SuggestFix`.
omitempty:
policy: SuggestFix # SuggestFix | Warn | Ignore # The policy for omitempty in optional fields. Defaults to `SuggestFix`.
# jsonTags:
# jsontags:
# jsonTagRegex: "^[a-z][a-z0-9]*(?:[A-Z][a-z0-9]*)*$" # The default regex is appropriate for our use case.
# optionalOrRequired:
# optionalorrequired:
# preferredOptionalMarker: optional | kubebuilder:validation:Optional # The preferred optional marker to use, fixes will suggest to use this marker. Defaults to `optional`.
# preferredRequiredMarker: required | kubebuilder:validation:Required # The preferred required marker to use, fixes will suggest to use this marker. Defaults to `required`.
# requiredFields:
# requiredfields:
# pointerPolicy: Warn | SuggestFix # Defaults to `SuggestFix`. We want our required fields to not be pointers.
# ssatags:
# listTypeSetUsage: Warn | Ignore # The policy for listType=set usage on object arrays. Defaults to `Warn`.

exclusions:
generated: strict
Expand All @@ -83,23 +86,33 @@ linters:
text: "Conditions field must be a slice of metav1.Condition"
linters:
- kubeapilinter
- path: "api/addons/v1beta2|api/bootstrap/kubeadm/v1beta2|api/controlplane/kubeadm/v1beta2|api/core/v1beta2|api/ipam/v1beta2|api/runtime/v1beta2"
text: "ssatags: Conditions should have a listType marker for proper Server-Side Apply behavior"
linters:
- kubeapilinter
- path: "api/core/v1beta2"
text: "field Conditions type Conditions must have a maximum items, add kubebuilder:validation:MaxItems marker"
linters:
- kubeapilinter

## Excludes for current clusterctl v1alpha3 and Runtime Hooks v1alpha1 apiVersions (can be fixed once we bump their apiVersion).
# Note: The types in api/runtime/hooks/v1alpha1 are not CRDs, so e.g. SSA markers don't make sense there.
- path: "cmd/clusterctl/api/v1alpha3|api/runtime/hooks/v1alpha1"
text: "optionalfields|maxlength"
text: "optionalfields|maxlength|ssatags"
linters:
- kubeapilinter

## Excludes for JSONSchemaProps
# controller-gen does not allow to add MaxItems to Schemaless fields
# controller-gen does not allow to add MaxItems to Schemaless fields: https://github.com/kubernetes-sigs/kube-api-linter/issues/120
- path: "api/core/v1beta2/clusterclass_types.go"
text: "maxlength: field (AllOf|OneOf|AnyOf) must have a maximum items, add kubebuilder:validation:MaxItems marker"
linters:
- kubeapilinter
# controller-gen does not allow to add listType to Schemaless fields: https://github.com/kubernetes-sigs/kube-api-linter/issues/120
- path: "api/core/v1beta2/clusterclass_types.go"
text: "ssatags: (AllOf|OneOf|AnyOf) should have a listType marker for proper Server-Side Apply behavior"
linters:
- kubeapilinter
# We want to align to the JSON tags of the CustomResourceDefinition fields.
- path: "api/core/v1beta2/clusterclass_types"
text: "field (XPreserveUnknownFields|XPreserveUnknownFields|XValidations|XMetadata|XIntOrString) json tag does not match pattern"
Expand Down
1 change: 1 addition & 0 deletions api/addons/v1beta2/clusterresourceset_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@sbueringer sbueringer Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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:

Copy link
Member Author

@sbueringer sbueringer Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@sbueringer sbueringer Jul 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

// +kubebuilder:validation:MaxItems=100
Resources []ResourceRef `json:"resources,omitempty"`

Expand Down
2 changes: 2 additions & 0 deletions api/addons/v1beta2/clusterresourcesetbinding_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ type ResourceSetBinding struct {

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// +kubebuilder:validation:MaxItems=100
Resources []ResourceBinding `json:"resources,omitempty"`
}
Expand Down Expand Up @@ -137,6 +138,7 @@ type ClusterResourceSetBinding struct {
type ClusterResourceSetBindingSpec struct {
// bindings is a list of ClusterResourceSets and their resources.
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=100
Bindings []*ResourceSetBinding `json:"bindings,omitempty"`

Expand Down
25 changes: 25 additions & 0 deletions api/bootstrap/kubeadm/v1beta2/kubeadm_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ type InitConfiguration struct {
// bootstrapTokens is respected at `kubeadm init` time and describes a set of Bootstrap Tokens to create.
// This information IS NOT uploaded to the kubeadm cluster configmap, partly because of its sensitive nature
// +optional
// +listType=atomic
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each .token probably wants to be unique in this list? So could be a map with token as the key?

// +kubebuilder:validation:MaxItems=100
BootstrapTokens []BootstrapToken `json:"bootstrapTokens,omitempty"`

Expand All @@ -99,6 +100,7 @@ type InitConfiguration struct {
// The list of phases can be obtained with the "kubeadm init --help" command.
// This option takes effect only on Kubernetes >=1.22.0.
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=50
// +kubebuilder:validation:items:MinLength=1
// +kubebuilder:validation:items:MaxLength=256
Expand Down Expand Up @@ -199,18 +201,21 @@ type APIServer struct {

// extraVolumes is an extra set of host volumes, mounted to the control plane component.
// +optional
// +listType=atomic
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a clear map key on this apart from name, looking at the type, we probably want some validation to make sure the mountPath is unique for entries in this list

// +kubebuilder:validation:MaxItems=100
ExtraVolumes []HostPathMount `json:"extraVolumes,omitempty"`

// extraEnvs is an extra set of environment variables to pass to the control plane component.
// Environment variables passed using ExtraEnvs will override any existing environment variables, or *_proxy environment variables that kubeadm adds by default.
// This option takes effect only on Kubernetes >=1.31.0.
// +optional
// +listType=atomic
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Map with the env var name as the key?

// +kubebuilder:validation:MaxItems=100
ExtraEnvs []EnvVar `json:"extraEnvs,omitempty"`

// certSANs sets extra Subject Alternative Names for the API Server signing cert.
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=100
// +kubebuilder:validation:items:MinLength=1
// +kubebuilder:validation:items:MaxLength=253
Expand All @@ -234,13 +239,15 @@ type ControllerManager struct {

// extraVolumes is an extra set of host volumes, mounted to the control plane component.
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=100
ExtraVolumes []HostPathMount `json:"extraVolumes,omitempty"`

// extraEnvs is an extra set of environment variables to pass to the control plane component.
// Environment variables passed using ExtraEnvs will override any existing environment variables, or *_proxy environment variables that kubeadm adds by default.
// This option takes effect only on Kubernetes >=1.31.0.
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=100
ExtraEnvs []EnvVar `json:"extraEnvs,omitempty"`
}
Expand All @@ -262,13 +269,15 @@ type Scheduler struct {

// extraVolumes is an extra set of host volumes, mounted to the control plane component.
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=100
ExtraVolumes []HostPathMount `json:"extraVolumes,omitempty"`

// extraEnvs is an extra set of environment variables to pass to the control plane component.
// Environment variables passed using ExtraEnvs will override any existing environment variables, or *_proxy environment variables that kubeadm adds by default.
// This option takes effect only on Kubernetes >=1.31.0.
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=100
ExtraEnvs []EnvVar `json:"extraEnvs,omitempty"`
}
Expand Down Expand Up @@ -357,6 +366,7 @@ type NodeRegistrationOptions struct {
// ignorePreflightErrors provides a slice of pre-flight errors to be ignored when the current node is registered, e.g. 'IsPrivilegedUser,Swap'.
// Value 'all' ignores errors from all checks.
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=50
// +kubebuilder:validation:items:MinLength=1
// +kubebuilder:validation:items:MaxLength=512
Expand All @@ -383,31 +393,38 @@ type BootstrapToken struct {
// Used for joining nodes in the cluster.
// +required
Token *BootstrapTokenString `json:"token"`

// description sets a human-friendly message why this token exists and what it's used
// for, so other administrators can know its purpose.
// +optional
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=512
Description string `json:"description,omitempty"`

// ttlSeconds defines the time to live for this token. Defaults to 24h.
// Expires and ttlSeconds are mutually exclusive.
// +optional
// +kubebuilder:validation:Minimum=0
TTLSeconds *int32 `json:"ttlSeconds,omitempty"`

// expires specifies the timestamp when this token expires. Defaults to being set
// dynamically at runtime based on the ttlSeconds. Expires and ttlSeconds are mutually exclusive.
// +optional
Expires *metav1.Time `json:"expires,omitempty"`

// usages describes the ways in which this token can be used. Can by default be used
// for establishing bidirectional trust, but that can be changed here.
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=100
// +kubebuilder:validation:items:MinLength=1
// +kubebuilder:validation:items:MaxLength=256
Usages []string `json:"usages,omitempty"`

// groups specifies the extra groups that this token will authenticate as when/if
// used for authentication
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=100
// +kubebuilder:validation:items:MinLength=1
// +kubebuilder:validation:items:MaxLength=256
Expand Down Expand Up @@ -456,18 +473,21 @@ type LocalEtcd struct {
// Environment variables passed using ExtraEnvs will override any existing environment variables, or *_proxy environment variables that kubeadm adds by default.
// This option takes effect only on Kubernetes >=1.31.0.
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=100
ExtraEnvs []EnvVar `json:"extraEnvs,omitempty"`

// serverCertSANs sets extra Subject Alternative Names for the etcd server signing cert.
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=100
// +kubebuilder:validation:items:MinLength=1
// +kubebuilder:validation:items:MaxLength=253
ServerCertSANs []string `json:"serverCertSANs,omitempty"`

// peerCertSANs sets extra Subject Alternative Names for the etcd peer signing cert.
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=100
// +kubebuilder:validation:items:MinLength=1
// +kubebuilder:validation:items:MaxLength=253
Expand All @@ -479,6 +499,7 @@ type LocalEtcd struct {
type ExternalEtcd struct {
// endpoints of etcd members. Required for ExternalEtcd.
// +required
// +listType=atomic
// +kubebuilder:validation:MinItems=1
// +kubebuilder:validation:MaxItems=50
// +kubebuilder:validation:items:MinLength=1
Expand Down Expand Up @@ -538,6 +559,7 @@ type JoinConfiguration struct {
// The list of phases can be obtained with the "kubeadm init --help" command.
// This option takes effect only on Kubernetes >=1.22.0.
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=50
// +kubebuilder:validation:items:MinLength=1
// +kubebuilder:validation:items:MaxLength=256
Expand Down Expand Up @@ -606,6 +628,7 @@ type BootstrapTokenDiscovery struct {
// ASN.1. These hashes can be calculated using, for example, OpenSSL:
// openssl x509 -pubkey -in ca.crt openssl rsa -pubin -outform der 2>&/dev/null | openssl dgst -sha256 -hex
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=100
// +kubebuilder:validation:items:MinLength=1
// +kubebuilder:validation:items:MaxLength=512
Expand Down Expand Up @@ -743,6 +766,7 @@ type KubeConfigAuthExec struct {

// args is the arguments to pass to the command when executing it.
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=100
// +kubebuilder:validation:items:MinLength=1
// +kubebuilder:validation:items:MaxLength=512
Expand All @@ -752,6 +776,7 @@ type KubeConfigAuthExec struct {
// are unioned with the host's environment, as well as variables client-go uses
// to pass argument to the plugin.
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=100
Env []KubeConfigAuthExecEnv `json:"env,omitempty"`

Expand Down
11 changes: 11 additions & 0 deletions api/bootstrap/kubeadm/v1beta2/kubeadmconfig_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ type KubeadmConfigSpec struct {

// files specifies extra files to be passed to user_data upon creation.
// +optional
// +listType=atomic
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be map on the path?

// +kubebuilder:validation:MaxItems=200
Files []File `json:"files,omitempty"`

Expand All @@ -75,13 +76,15 @@ type KubeadmConfigSpec struct {

// mounts specifies a list of mount points to be setup.
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=100
Mounts []MountPoints `json:"mounts,omitempty"`

// bootCommands specifies extra commands to run very early in the boot process via the cloud-init bootcmd
// module. bootcmd will run on every boot, 'cloud-init-per' command can be used to make bootcmd run exactly
// once. This is typically run in the cloud-init.service systemd unit. This has no effect in Ignition.
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=1000
// +kubebuilder:validation:items:MinLength=1
// +kubebuilder:validation:items:MaxLength=10240
Expand All @@ -91,6 +94,7 @@ type KubeadmConfigSpec struct {
// With cloud-init, this is prepended to the runcmd module configuration, and is typically executed in
// the cloud-final.service systemd unit. In Ignition, this is prepended to /etc/kubeadm.sh.
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=1000
// +kubebuilder:validation:items:MinLength=1
// +kubebuilder:validation:items:MaxLength=10240
Expand All @@ -100,13 +104,15 @@ type KubeadmConfigSpec struct {
// With cloud-init, this is appended to the runcmd module configuration, and is typically executed in
// the cloud-final.service systemd unit. In Ignition, this is appended to /etc/kubeadm.sh.
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=1000
// +kubebuilder:validation:items:MinLength=1
// +kubebuilder:validation:items:MaxLength=10240
PostKubeadmCommands []string `json:"postKubeadmCommands,omitempty"`

// users specifies extra users to add
// +optional
// +listType=atomic
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Map on name?

// +kubebuilder:validation:MaxItems=100
Users []User `json:"users,omitempty"`

Expand Down Expand Up @@ -766,6 +772,7 @@ type User struct {

// sshAuthorizedKeys specifies a list of ssh authorized keys for the user
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=100
// +kubebuilder:validation:items:MinLength=1
// +kubebuilder:validation:items:MaxLength=2048
Expand All @@ -776,6 +783,7 @@ type User struct {
type NTP struct {
// servers specifies which NTP servers to use
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=100
// +kubebuilder:validation:items:MinLength=1
// +kubebuilder:validation:items:MaxLength=512
Expand All @@ -790,11 +798,13 @@ type NTP struct {
type DiskSetup struct {
// partitions specifies the list of the partitions to setup.
// +optional
// +listType=atomic
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Map on device?

// +kubebuilder:validation:MaxItems=100
Partitions []Partition `json:"partitions,omitempty"`

// filesystems specifies the list of file systems to setup.
// +optional
// +listType=atomic
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Map on device?

// +kubebuilder:validation:MaxItems=100
Filesystems []Filesystem `json:"filesystems,omitempty"`
}
Expand Down Expand Up @@ -863,6 +873,7 @@ type Filesystem struct {

// extraOpts defined extra options to add to the command for creating the file system.
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=100
// +kubebuilder:validation:items:MinLength=1
// +kubebuilder:validation:items:MaxLength=256
Expand Down
2 changes: 2 additions & 0 deletions api/core/v1beta2/cluster_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -813,6 +813,7 @@ type MachinePoolTopology struct {
// failureDomains is the list of failure domains the machine pool will be created in.
// Must match a key in the FailureDomains map stored on the cluster object.
// +optional
// +listType=atomic
// +kubebuilder:validation:MaxItems=100
// +kubebuilder:validation:items:MinLength=1
// +kubebuilder:validation:items:MaxLength=256
Expand Down Expand Up @@ -943,6 +944,7 @@ type ClusterNetwork struct {
type NetworkRanges struct {
// cidrBlocks is a list of CIDR blocks.
// +required
// +listType=atomic
// +kubebuilder:validation:MaxItems=100
// +kubebuilder:validation:items:MinLength=1
// +kubebuilder:validation:items:MaxLength=43
Expand Down
Loading