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 kubernetes-sigs/kubebuilder#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