Skip to content
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

Support +enum tags in k8s APIs #933

Open
tenzen-y opened this issue Apr 26, 2024 · 6 comments
Open

Support +enum tags in k8s APIs #933

tenzen-y opened this issue Apr 26, 2024 · 6 comments

Comments

@tenzen-y
Copy link
Member

tenzen-y commented Apr 26, 2024

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
Copy link

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

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 25, 2024
@tenzen-y
Copy link
Member Author

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 25, 2024
@sbueringer
Copy link
Member

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

@k8s-triage-robot
Copy link

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

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 23, 2024
@tenzen-y
Copy link
Member Author

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 23, 2024
@pmalek
Copy link
Contributor

pmalek commented Nov 10, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants