🐛 Fix normalize {} to [] for slice defaults in CRDs

[kvaps]

What this fixes

Right now if you set a default with an inline object that contains a slice, and use {} as the value, the generator ends up treating it as an object instead of an empty list.

For example, if I use:

// +kubebuilder:default:={"md0":{"ephemeralStorage":"20Gi","gpus":{},"instanceType":"u1.medium","maxReplicas":10,"minReplicas":0,"resources":{},"roles":{"ingress-nginx"}}}

the generated CRD gives me:

default:
  md0:
    ephemeralStorage: 20Gi
    gpus: {}
    instanceType: u1.medium
    maxReplicas: 10
    minReplicas: 0
    resources: {}
    roles:
    - ingress-nginx

but since gpus is a slice, what I actually want (and what users would expect) is:

default:
  md0:
    ephemeralStorage: 20Gi
    gpus: []
    instanceType: u1.medium
    maxReplicas: 10
    minReplicas: 0
    resources: {}
    roles:
    - ingress-nginx

What changed

• Added a normalization step in ApplyToSchema that looks at the target schema type and coerces {} → [] when the field is an array (and vice versa for objects).
• This runs recursively, so nested defaults inside maps/objects are also fixed.
• Extended the testdata with cases for nested slices inside maps/objects to make sure the behavior is covered.

Why

It’s a small thing, but it avoids confusion when writing defaults for slices. Before this patch, the CRD schema would show gpus: {}, which is wrong and makes clients unhappy. Now it consistently produces [].

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kvaps
Once this PR has been reviewed and has the lgtm label, please assign vincepri 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

[alvaroaleman]

Why would we translate {} to []? One is an object, the other is a list - We should likely just error out, as that is invalid.

I also am not sure if I like the idea of nesting defaulting expressions like this - I would prefer it if we would only default the map entry here and do the defaulting of the map key in its own type efinition, i.E. on the Gpus field of the NodeProfile struct.

cc @JoelSpeed @sbueringer

[sbueringer]

Why would we translate {} to []? One is an object, the other is a list - We should likely just error out, as that is invalid.

Agree

I also am not sure if I like the idea of nesting defaulting expressions like this - I would prefer it if we would only default the map entry here and do the defaulting of the map key in its own type efinition, i.E. on the Gpus field of the NodeProfile struct.

I would prefer to keep supporting both as we do today. This e.g. allows having different defaults if some part of the nested object structure is used in different places. I think there also might be different behaviors possible based on how the kube-apiserver does defaulting ("top-down"). I think we should leave that choice to our users.

[kvaps]

I have a case when I need to specify defaults for map[string]Object, for example:

type ConfigSpec struct {
	// Cluster addons configuration
	Addons Addons `json:"addons"`
	// Control Plane Configuration
	ControlPlane ControlPlane `json:"controlPlane"`
	// Hostname used to access the Kubernetes cluster externally. Defaults to `<cluster-name>.<tenant-host>` when empty.
	Host string `json:"host"`
	// Worker nodes configuration
	// +kubebuilder:default:={"md0":{"ephemeralStorage":"20Gi","gpus":{},"instanceType":"u1.medium","maxReplicas":10,"minReplicas":0,"resources":{},"roles":{"ingress-nginx"}}}
	NodeGroups map[string]NodeGroup `json:"nodeGroups,omitempty"`
	// StorageClass used to store the data
	StorageClass string `json:"storageClass"`
	// Kubernetes version given as vMAJOR.MINOR. Available are versions from 1.28 to 1.33.
	Version string `json:"version"`
}

type NodeGroup struct {
	// Ephemeral storage size
	EphemeralStorage resource.Quantity `json:"ephemeralStorage"`
	// List of GPUs to attach (WARN: NVIDIA driver requires at least 4 GiB of RAM)
	Gpus []Gpu `json:"gpus,omitempty"`
	// Virtual machine instance type
	InstanceType string `json:"instanceType"`
	// Maximum amount of replicas
	MaxReplicas int `json:"maxReplicas"`
	// Minimum amount of replicas
	MinReplicas int `json:"minReplicas"`
	// Resources available to each worker node
	Resources Resources `json:"resources"`
	// List of node's roles
	Roles []string `json:"roles,omitempty"`
}

I don't need to have defaults for every NodeGroup added, but I need to add one NodeGroup with example defaults if NodeGroups is not specified

[kvaps]

Maybe this is a corner case but is up to discussion

[kvaps]

Why would we translate {} to []? One is an object, the other is a list - We should likely just error out, as that is invalid.

According to the kubebuilder notaion logic:

• {} will be converted to {}

• [] will be converted to "[]"

• {aaa} will be converted to ["aaa"]

• {aaa:bbb} will be converted to {"aaa":"bbb"}

• {} can be converted to [] but only on a top level, nested values have not checked (before this PR)

[kvaps]

// +kubebuilder:default:={"md0":{"gpus":[]}}

yelds to string:

default:
  md0:
    gpus: "[]"

not to array:

default:
  md0:
    gpus: []

Kubebuilder annotations do not support square brackets notation.

[alvaroaleman]

Kubebuilder annotations do not support square brackets notation.

Okay, that does seem like a bug. Will fixing this break anyone?

[kvaps]

Personally I would be super glad of using standard json arrays []. On the other hand, I did a some research and found out that kubebuilder uses its own annotation format.

For example, if you want to default list, you'd need to specify:

// +kubebuilder:default:={1,2,3}

not

// +kubebuilder:default:=[1,2,3]

and default empty list works already as:

// +kubebuilder:default:={}

not

// +kubebuilder:default:=[]

this was introduced by
#863

this way I prepared my PR which fixes already existing notation to make it working for nested lists, instead of introducing a new format.

[sbueringer]

@JoelSpeed Do you know what the syntax of existing and potentially upcoming k/k default markers is?

Can we maybe use that instead? As we want to support these anyway (We already support +default, but I don't remember what format it supports and if that's identical to how k/k is interpreting the +default marker)

[JoelSpeed]

I don't see a default marker implemented yet, see the tag catalog, I'll raise this and see what the plans are for the new defaulting behaviour

[k8s-ci-robot]

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.