Skip to content

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

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+112 −2
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
72 changes: 70 additions & 2 deletions pkg/crd/markers/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,15 +552,83 @@ func (m Nullable) ApplyToSchema(schema *apiext.JSONSchemaProps) error {
return nil
}

func coerceDefaultValueToSchema(val interface{}, schema *apiext.JSONSchemaProps) interface{} {
switch schema.Type {
case string(Array):
switch v := val.(type) {
case string:
s := strings.TrimSpace(v)
if s == "[]" || s == "{}" || s == "" {
return []interface{}{}
}
return v
case map[string]interface{}:
if len(v) == 0 {
return []interface{}{}
}
return v
case []interface{}:
if schema.Items != nil {
if schema.Items.Schema != nil {
for i := range v {
v[i] = coerceDefaultValueToSchema(v[i], schema.Items.Schema)
}
} else if len(schema.Items.JSONSchemas) > 0 {
for i := range v {
if i < len(schema.Items.JSONSchemas) {
v[i] = coerceDefaultValueToSchema(v[i], &schema.Items.JSONSchemas[i])
}
}
}
}
return v
default:
return val
}
case string(Object):
switch v := val.(type) {
case string:
if strings.TrimSpace(v) == "{}" {
return map[string]interface{}{}
}
return v
case map[string]interface{}:
for name, p := range schema.Properties {
if child, ok := v[name]; ok {
v[name] = coerceDefaultValueToSchema(child, &p)
}
}
if schema.AdditionalProperties != nil && schema.AdditionalProperties.Schema != nil {
for k, child := range v {
if _, known := schema.Properties[k]; known {
continue
}
v[k] = coerceDefaultValueToSchema(child, schema.AdditionalProperties.Schema)
}
}
return v
default:
return val
}
default:
return val
}
}

// Defaults are only valid CRDs created with the v1 API
func (m Default) ApplyToSchema(schema *apiext.JSONSchemaProps) error {
marshalledDefault, err := json.Marshal(m.Value)
val := m.Value
val = coerceDefaultValueToSchema(val, schema)

marshalledDefault, err := json.Marshal(val)
if err != nil {
return err
}
if schema.Type == "array" && string(marshalledDefault) == "{}" {

if schema.Type == string(Array) && string(marshalledDefault) == "{}" {
marshalledDefault = []byte("[]")
}

schema.Default = &apiext.JSON{Raw: marshalledDefault}
return nil
}
Expand Down
17 changes: 17 additions & 0 deletions pkg/crd/testdata/cronjob_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,14 @@ type CronJobSpec struct {
// +kubebuilder:title=DefaultedSlice
DefaultedSlice []string `json:"defaultedSlice"`

// +kubebuilder:default:={"md0":{"gpus":{}}}
Copy link
Member

@alvaroaleman alvaroaleman Sep 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

@sbueringer sbueringer Sep 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe this is a corner case but is up to discussion

Copy link
Member Author

@kvaps kvaps Sep 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

@kvaps kvaps Sep 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

yelds to string:

default:
  md0:
    gpus: "[]"

not to array:

default:
  md0:
    gpus: []

Kubebuilder annotations do not support square brackets notation.

Copy link
Member

Choose a reason for hiding this comment

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

Kubebuilder annotations do not support square brackets notation.

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

Copy link
Member Author

@kvaps kvaps Sep 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@sbueringer sbueringer Sep 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

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 default marker implemented yet, see the tag catalog, I'll raise this and see what the plans are for the new defaulting behaviour

// Expect: default.md0.gpus == []
DefaultedNestedProfiles Profiles `json:"defaultedNestedProfiles,omitempty"`

// +kubebuilder:default:={"md0":{}}
// Expect: default.md0 == []
DefaultedNestedSliceMap map[string][]string `json:"defaultedNestedSliceMap,omitempty"`

// This tests that slice and object defaulting can be performed.
// +kubebuilder:default={{nested: {foo: "baz", bar: true}},{nested: {foo: "qux", bar: false}}}
// +kubebuilder:example={{nested: {foo: "baz", bar: true}},{nested: {foo: "qux", bar: false}}}
Expand Down Expand Up @@ -420,6 +428,15 @@ type CronJobSpec struct {
FieldLevelLocalDeclarationOverride LongerString `json:"fieldLevelLocalDeclarationOverride,omitempty"`
}

// NodeProfile is used to verify nested array defaulting inside an object.
// gpus is a slice; when default supplies {}, it must become [].
type NodeProfile struct {
Gpus []string `json:"gpus,omitempty"`
}

// Profiles map verifies nested coercion under properties.
type Profiles map[string]NodeProfile

type InlineAlias = EmbeddedStruct

// EmbeddedStruct is for testing that embedded struct is handled correctly when it is used through an alias type.
Expand Down
25 changes: 25 additions & 0 deletions pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,31 @@ spec:
type: string
title: '{}'
type: array
defaultedNestedProfiles:
additionalProperties:
description: |-
NodeProfile is used to verify nested array defaulting inside an object.
gpus is a slice; when default supplies {}, it must become [].
properties:
gpus:
items:
type: string
type: array
type: object
default:
md0:
gpus: []
description: 'Expect: default.md0.gpus == []'
type: object
defaultedNestedSliceMap:
additionalProperties:
items:
type: string
type: array
default:
md0: []
description: 'Expect: default.md0 == []'
type: object
defaultedObject:
default:
- nested:
Expand Down
Loading