Support +enum tags in k8s APIs

[tenzen-y]

What do you want to happen?

Kubernetes APIs include an enum tag such as this one:

https://github.com/kubernetes/kubernetes/blob/be4b7176dc131ea842cab6882cd4a06dbfeed12a/staging/src/k8s.io/api/core/v1/types.go#L3506-L3507

which translates to the following OpenAPIv3:

          "effect": {
            "description": "Required. The effect of the taint on pods that do not tolerate the taint. Valid effects are NoSchedule, PreferNoSc>
            "type": "string",
            "default": "",
            "enum": [
              "NoExecute",
              "NoSchedule",
              "PreferNoSchedule"
            ]

However, kubebuilder doesn't recognize them when using these types in another project.

Example in Kueue, for the code in https://github.com/kubernetes-sigs/kueue/blob/34bfbe0f359b8439b737f07c3f3e5da92c7d0d67/apis/kueue/v1beta1/resourceflavor_types.go#L68 The rendered CRD lacks the enum information: https://github.com/kubernetes-sigs/kueue/blob/34bfbe0f359b8439b737f07c3f3e5da92c7d0d67/config/components/crd/bases/kueue.x-k8s.io_resourceflavors.yaml#L87-L89

Extra Labels

No response

Originally posted by @alculquicondor in https://github.com/kubernetes-sigs/kubebuilder/issues/3861

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[tenzen-y]

/remove-lifecycle stale

[sbueringer]

Sounds good to me! (we also already support e.g. +default)

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[tenzen-y]

/remove-lifecycle stale

[pmalek]

I've taken a look at this one and it seems that there are 2 things (at least for me) that need answering:

• when we add support for +enum (without parameters like in the linked TaintEffect case) then we need to somehow know all this particular type's values (if the code marker is on a type as it is in this case). Looking at the https://github.com/kubernetes-sigs/controller-tools/blob/5e5bc88a01cab8ebff8c9fc4d1e90bb858139186/pkg/markers/parse.go it doesn't seem we do it yet.

• Adding diff shown below would make the parser also take the in-tree types into account which requires adding schema for all the types that were ignored up until now (e.g. TaintEffect). Without this we get the following errors:

  testdata.kubebuilder.io/cronjob:-: unable to locate schema for type "k8s.io/api/core/v1".ObjectReference
  testdata.kubebuilder.io/cronjob:-: unable to locate schema for type "k8s.io/api/core/v1".IPFamilyPolicy
  testdata.kubebuilder.io/cronjob:-: unable to locate schema for type "k8s.io/api/core/v1".Protocol
  k8s.io/api/batch/v1beta1:-: unable to locate schema for type "k8s.io/api/batch/v1".JobSpec
  

  I'm not sure yet but

  diff --git a/pkg/crd/known_types.go b/pkg/crd/known_types.go
  index ab939328..1d029097 100644
  --- a/pkg/crd/known_types.go
  +++ b/pkg/crd/known_types.go
  @@ -25,6 +25,25 @@ import (
   // KnownPackages overrides types in some comment packages that have custom validation
   // but don't have validation markers on them (since they're from core Kubernetes).
   var KnownPackages = map[string]PackageOverride{
  +       "k8s.io/api/core/v1": func(p *Parser, pkg *loader.Package) {
  +               p.Schemata[TypeIdent{Name: "Protocol", Package: pkg}] = apiext.JSONSchemaProps{
  +                       Type: "object",
  +               }
  +               p.Schemata[TypeIdent{Name: "IPFamilyPolicy", Package: pkg}] = apiext.JSONSchemaProps{
  +                       Type: "object",
  +               }
  +               p.Schemata[TypeIdent{Name: "ObjectReference", Package: pkg}] = apiext.JSONSchemaProps{
  +                       Type: "object",
  +               }
  +       },
  +       "k8s.io/api/batch/v1": func(p *Parser, pkg *loader.Package) {
  +               p.Schemata[TypeIdent{Name: "Job", Package: pkg}] = apiext.JSONSchemaProps{
  +                       Type: "object",
  +               }
  +               p.Schemata[TypeIdent{Name: "JobSpec", Package: pkg}] = apiext.JSONSchemaProps{
  +                       Type: "object",
  +               }
  +       },
  

  does seem to make the errors go away. This is not scalable as in-tree types might get new fields with +enum so this would require manual maintenance. Not sure how to do this "automagically" though.

Diff adding support for `+enum=` on types

diff --git a/pkg/crd/markers/validation.go b/pkg/crd/markers/validation.go
index 08d28612..3e064252 100644
--- a/pkg/crd/markers/validation.go
+++ b/pkg/crd/markers/validation.go
@@ -34,14 +34,14 @@ const (
 	ValidationItemsPrefix = validationPrefix + "items:"
 )
 
-// ValidationMarkers lists all available markers that affect CRD schema generation,
+// ValidationWithPrefixMarkers lists all available markers that affect CRD schema generation,
 // except for the few that don't make sense as type-level markers (see FieldOnlyMarkers).
 // All markers start with `+kubebuilder:validation:`, and continue with their type name.
 // A copy is produced of all markers that describes types as well, for making types
 // reusable and writing complex validations on slice items.
 // At last a copy of all markers with the prefix `+kubebuilder:validation:items:` is
 // produced for marking slice fields and types.
-var ValidationMarkers = mustMakeAllWithPrefix(validationPrefix, markers.DescribesField,
+var ValidationWithPrefixMarkers = mustMakeAllWithPrefix(validationPrefix, markers.DescribesField,
 
 	// numeric markers
 
@@ -76,6 +76,14 @@ var ValidationMarkers = mustMakeAllWithPrefix(validationPrefix, markers.Describe
 	XValidation{},
 )
 
+var ValidationMarkers = []*definitionWithHelp{
+	// general markers
+
+	// Enum(nil),
+	must(markers.MakeDefinition("enum", markers.DescribesType, Enum{})).
+		WithHelp(Enum{}.Help()),
+}
+
 // FieldOnlyMarkers list field-specific validation markers (i.e. those markers that don't make
 // sense on a type, and thus aren't in ValidationMarkers).
 var FieldOnlyMarkers = []*definitionWithHelp{
@@ -117,9 +125,8 @@ var ValidationIshMarkers = []*definitionWithHelp{
 }
 
 func init() {
-	AllDefinitions = append(AllDefinitions, ValidationMarkers...)
-
-	for _, def := range ValidationMarkers {
+	AllDefinitions = append(AllDefinitions, ValidationWithPrefixMarkers...)
+	for _, def := range ValidationWithPrefixMarkers {
 		typDef := def.clone()
 		typDef.Target = markers.DescribesType
 		AllDefinitions = append(AllDefinitions, typDef)
@@ -138,6 +145,8 @@ func init() {
 		AllDefinitions = append(AllDefinitions, itemsTypDef)
 	}
 
+	AllDefinitions = append(AllDefinitions, ValidationMarkers...)
+
 	AllDefinitions = append(AllDefinitions, FieldOnlyMarkers...)
 	AllDefinitions = append(AllDefinitions, ValidationIshMarkers...)
 }
@@ -210,6 +219,10 @@ type MinProperties int
 // Enum specifies that this (scalar) field is restricted to the *exact* values specified here.
 type Enum []interface{}
 
+// +controllertools:marker:generateHelp:category="CRD validation"
+// KubernetesEnum specifies that this (scalar) field is restricted to the *exact* values specified here.
+type KubernetesEnum []interface{}
+
 // +controllertools:marker:generateHelp:category="CRD validation"
 // Format specifies additional "complex" formatting for this field.
 //
@@ -466,11 +479,13 @@ func (m MaxProperties) ApplyToSchema(schema *apiext.JSONSchemaProps) error {
 	return nil
 }
 
-func (m Enum) ApplyToSchema(schema *apiext.JSONSchemaProps) error {
+func enumApply[
+	T ~[]any,
+](enum T, schema *apiext.JSONSchemaProps) error {
 	// TODO(directxman12): this is a bit hacky -- we should
 	// probably support AnyType better + using the schema structure
-	vals := make([]apiext.JSON, len(m))
-	for i, val := range m {
+	vals := make([]apiext.JSON, len(enum))
+	for i, val := range enum {
 		// TODO(directxman12): check actual type with schema type?
 		// if we're expecting a string, marshal the string properly...
 		// NB(directxman12): we use json.Marshal to ensure we handle JSON escaping properly
@@ -484,6 +499,14 @@ func (m Enum) ApplyToSchema(schema *apiext.JSONSchemaProps) error {
 	return nil
 }
 
+func (m Enum) ApplyToSchema(schema *apiext.JSONSchemaProps) error {
+	return enumApply(m, schema)
+}
+
+func (m KubernetesEnum) ApplyToSchema(schema *apiext.JSONSchemaProps) error {
+	return enumApply(m, schema)
+}
+
 func (m Format) ApplyToSchema(schema *apiext.JSONSchemaProps) error {
 	schema.Format = string(m)
 	return nil
diff --git a/pkg/crd/markers/zz_generated.markerhelp.go b/pkg/crd/markers/zz_generated.markerhelp.go
index e2db2b99..4089b503 100644
--- a/pkg/crd/markers/zz_generated.markerhelp.go
+++ b/pkg/crd/markers/zz_generated.markerhelp.go
@@ -132,6 +132,17 @@ func (KubernetesDefault) Help() *markers.DefinitionHelp {
 	}
 }
 
+func (KubernetesEnum) Help() *markers.DefinitionHelp {
+	return &markers.DefinitionHelp{
+		Category: "CRD validation",
+		DetailedHelp: markers.DetailedHelp{
+			Summary: "specifies that this (scalar) field is restricted to the *exact* values specified here.",
+			Details: "",
+		},
+		FieldHelp: map[string]markers.DetailedHelp{},
+	}
+}
+
 func (ListMapKey) Help() *markers.DefinitionHelp {
 	return &markers.DefinitionHelp{
 		Category: "CRD processing",
diff --git a/pkg/crd/testdata/cronjob_types.go b/pkg/crd/testdata/cronjob_types.go
index e81a28ac..8a891cbe 100644
--- a/pkg/crd/testdata/cronjob_types.go
+++ b/pkg/crd/testdata/cronjob_types.go
@@ -325,9 +325,12 @@ type CronJobSpec struct {
 	// +listType=set
 	Hosts []string `json:"hosts,omitempty"`
 
-	// This tests slice item validation with enum
-	// +kubebuilder:validation:items:Enum=0;1;3
-	EnumSlice []int `json:"enumSlice,omitempty"`
+	// This tests slice item validation with kubebuilder:validation:items:Enum
+	// +kubebuilder:validation:items:Enum=dummyA;dummyB;dummyC
+	EnumKubernetesSlice []string `json:"enumKubernetesSlice,omitempty"`
+
+	// This tests support for struct types with enum validation.
+	EnumPolicy EnumPolicy `json:"enumPolicy,omitempty"`
 
 	HostsAlias Hosts `json:"hostsAlias,omitempty"`
 
@@ -613,6 +616,10 @@ var _ json.Marshaler = Duration{}
 // +kubebuilder:validation:Enum=Allow;Forbid;Replace
 type ConcurrencyPolicy string
 
+// EnumPolicy is a type that includes an enum validation.
+// +enum=dummyA;dummyB;dummyC
+type EnumPolicy string
+
 const (
 	// AllowConcurrent allows CronJobs to run concurrently.
 	AllowConcurrent ConcurrencyPolicy = "Allow"
diff --git a/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml b/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml
index cdc5190b..f9c3a989 100644
--- a/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml
+++ b/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml
@@ -182,15 +182,23 @@ spec:
                 type: object
                 x-kubernetes-embedded-resource: true
                 x-kubernetes-preserve-unknown-fields: true
-              enumSlice:
-                description: This tests slice item validation with enum
+              enumKubernetesSlice:
+                description: This tests slice item validation with kubebuilder:validation:items:Enum
                 type: array
                 items:
-                  type: integer
+                  type: string
                   enum:
-                  - 0
-                  - 1
-                  - 3
+                  - "dummyA"
+                  - "dummyB"
+                  - "dummyC"
+              enumPolicy:
+                description: This tests support for struct types with enum validation.
+                type: string
+                items:
+                enum:
+                - "dummyA"
+                - "dummyB"
+                - "dummyC"
               explicitlyOptionalKubebuilder:
                 description: This tests explicitly optional kubebuilder fields
                 type: string

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[tenzen-y]

/remove-lifecycle stale

[MishimaPorte]

Maybe adding some code to parse constant definitions as well as type definitions would work? Something like indexTypes function here (a naive implementation would look like this). This makes it possible to properly parse constant declarations and render a proper enum block in the schema.

Though it seems that only string-based enums could be supported since it would probably be too much logic to properly handle all the possible iota problems (though it certainly would be nice to have numeric ones too).

[JoelSpeed]

I believe that's approximately how the upstream marker works, so we probably want to take inspiration from their implementation and see if they handle iota

[MishimaPorte]

It seems that upstream kube-openapi package accounts only for string-based enums. If there are no other questions, i could start preparing a PR for this change.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[tenzen-y]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[sbueringer]

/remove-lifecycle stale