From 1bf22cba0a986705db151d978094b70120534f80 Mon Sep 17 00:00:00 2001 From: MishimaPorte Date: Tue, 25 Mar 2025 15:40:07 +0300 Subject: [PATCH 1/6] feature: +enum validation marker --- pkg/crd/markers/crd.go | 6 + pkg/crd/markers/validation.go | 2 + pkg/crd/markers/zz_generated.markerhelp.go | 11 + pkg/crd/parser.go | 17 ++ pkg/crd/schema.go | 27 ++ pkg/crd/testdata/cronjob_types.go | 11 + .../testdata.kubebuilder.io_cronjobs.yaml | 250 ++++++++++++++++++ pkg/loader/visit.go | 29 ++ pkg/markers/zip.go | 8 + 9 files changed, 361 insertions(+) diff --git a/pkg/crd/markers/crd.go b/pkg/crd/markers/crd.go index bd3cef563..bd0856aed 100644 --- a/pkg/crd/markers/crd.go +++ b/pkg/crd/markers/crd.go @@ -399,6 +399,12 @@ type SelectableField struct { JSONPath string `marker:"JSONPath"` } +// +controllertools:marker:generateHelp:category="CRD validation" + +// Enum marker marks a string alias type as an enum. +// It infers the members from constants declared of that type. +type InferredEnum struct{} + func (s SelectableField) ApplyToCRD(crd *apiext.CustomResourceDefinitionSpec, version string) error { var selectableFields *[]apiext.SelectableField for i := range crd.Versions { diff --git a/pkg/crd/markers/validation.go b/pkg/crd/markers/validation.go index ccd52e53c..e1d72c970 100644 --- a/pkg/crd/markers/validation.go +++ b/pkg/crd/markers/validation.go @@ -113,6 +113,8 @@ var ValidationIshMarkers = []*definitionWithHelp{ WithHelp(XPreserveUnknownFields{}.Help()), must(markers.MakeDefinition("kubebuilder:pruning:PreserveUnknownFields", markers.DescribesType, XPreserveUnknownFields{})). WithHelp(XPreserveUnknownFields{}.Help()), + must(markers.MakeDefinition("enum", markers.DescribesType, InferredEnum{})). + WithHelp(InferredEnum{}.Help()), } func init() { diff --git a/pkg/crd/markers/zz_generated.markerhelp.go b/pkg/crd/markers/zz_generated.markerhelp.go index a9ee23311..16004d0ac 100644 --- a/pkg/crd/markers/zz_generated.markerhelp.go +++ b/pkg/crd/markers/zz_generated.markerhelp.go @@ -67,6 +67,17 @@ func (Enum) Help() *markers.DefinitionHelp { } } +func (InferredEnum) Help() *markers.DefinitionHelp { + return &markers.DefinitionHelp{ + Category: "CRD validation", + DetailedHelp: markers.DetailedHelp{ + Summary: "enum marks a string type as an enum.", + Details: "It infers the members from constants declared of that type.", + }, + FieldHelp: map[string]markers.DetailedHelp{}, + } +} + func (Example) Help() *markers.DefinitionHelp { return &markers.DefinitionHelp{ Category: "CRD validation", diff --git a/pkg/crd/parser.go b/pkg/crd/parser.go index 4b96ee96f..8bdc0c9f1 100644 --- a/pkg/crd/parser.go +++ b/pkg/crd/parser.go @@ -18,6 +18,7 @@ package crd import ( "fmt" + "go/ast" apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/runtime/schema" @@ -122,6 +123,21 @@ func (p *Parser) init() { } } +func (p *Parser) indexEnumMemberConstantDeclarations(pkg *loader.Package) { + loader.EachConstDecl(pkg, func(spec *ast.ValueSpec) { + if id, ok := spec.Type.(*ast.Ident); ok { + if typeinfo, ok := p.Types[TypeIdent{ + pkg, id.Name, + }]; ok { + typeinfo.EnumValues = append(typeinfo.EnumValues, markers.EnumMemberInfo{ + Name: spec.Names[0].Name, + ValueSpec: spec, + }) + } + } + }) +} + // indexTypes loads all types in the package into Types. func (p *Parser) indexTypes(pkg *loader.Package) { // autodetect @@ -220,6 +236,7 @@ func (p *Parser) AddPackage(pkg *loader.Package) { return } p.indexTypes(pkg) + p.indexEnumMemberConstantDeclarations(pkg) p.Checker.Check(pkg) p.packages[pkg] = struct{}{} } diff --git a/pkg/crd/schema.go b/pkg/crd/schema.go index 616141cb1..d96c3cec3 100644 --- a/pkg/crd/schema.go +++ b/pkg/crd/schema.go @@ -17,12 +17,14 @@ limitations under the License. package crd import ( + "encoding/json" "errors" "fmt" "go/ast" "go/token" "go/types" "sort" + "strconv" "strings" apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" @@ -274,6 +276,29 @@ func localNamedToSchema(ctx *schemaContext, ident *ast.Ident) *apiext.JSONSchema if err != nil { ctx.pkg.AddError(loader.ErrFromNode(err, ident)) } + var enumMembers []apiext.JSON + if ctx.info.Markers.Get("enum") != nil && typ == "string" { + enumMembers = make([]apiext.JSON, 0, len(ctx.info.EnumValues)) + var ok bool + for i := range ctx.info.EnumValues { + var member = &ctx.info.EnumValues[i] + var v *ast.BasicLit + if v, ok = member.Values[0].(*ast.BasicLit); !ok { + ctx.pkg.AddError(loader.ErrFromNode(errors.New("constants for a +enum decorated type should be stirngs"), ident)) + } + var value string + if value, err = strconv.Unquote(v.Value); err != nil { + ctx.pkg.AddError(loader.ErrFromNode(err, ident)) + continue + } + var j apiext.JSON + if j.Raw, err = json.Marshal(value); err != nil { + ctx.pkg.AddError(loader.ErrFromNode(err, ident)) + continue + } + enumMembers = append(enumMembers, j) + } + } // Check for type aliasing to a basic type for gotypesalias=0. See more // in documentation https://pkg.go.dev/go/types#Alias: // > For gotypesalias=1, alias declarations produce an Alias type. @@ -284,12 +309,14 @@ func localNamedToSchema(ctx *schemaContext, ident *ast.Ident) *apiext.JSONSchema link := TypeRefLink("", ident.Name) return &apiext.JSONSchemaProps{ Type: typ, + Enum: enumMembers, Format: fmt, Ref: &link, } } return &apiext.JSONSchemaProps{ Type: typ, + Enum: enumMembers, Format: fmt, } } diff --git a/pkg/crd/testdata/cronjob_types.go b/pkg/crd/testdata/cronjob_types.go index c7c96103e..5c5bf304f 100644 --- a/pkg/crd/testdata/cronjob_types.go +++ b/pkg/crd/testdata/cronjob_types.go @@ -42,6 +42,15 @@ import ( const DefaultRefValue = "defaultRefValue" +// +enum +type EnumType string + +const ( + EnumType_One EnumType = "one" + EnumType_Two EnumType = "two" + EnumType_Three EnumType = "three" +) + // CronJobSpec defines the desired state of CronJob // +kubebuilder:validation:XValidation:rule="has(oldSelf.forbiddenInt) || !has(self.forbiddenInt)",message="forbiddenInt is not allowed",fieldPath=".forbiddenInt",reason="FieldValueForbidden" type CronJobSpec struct { @@ -332,6 +341,8 @@ type CronJobSpec struct { // +kubebuilder:validation:items:Enum=0;1;3 EnumSlice []int `json:"enumSlice,omitempty"` + EnumValue EnumType `json:"enumValue,omitempty"` + HostsAlias Hosts `json:"hostsAlias,omitempty"` // This tests that alias imported from a package is handled correctly. The diff --git a/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml b/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml index ab81550fc..0dd3d1845 100644 --- a/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml +++ b/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml @@ -46,6 +46,10 @@ spec: This tests that alias imported from a package is handled correctly. The corev1.IPFamilyPolicyType is just reused since it's available from imported package. We can create our own in a separate package if needed. + enum: + - SingleStack + - PreferDualStack + - RequireDualStack type: string array: description: Checks that fixed-length arrays work @@ -197,6 +201,12 @@ spec: - 3 type: integer type: array + enumValue: + enum: + - one + - two + - three + type: string explicitlyOptionalKubebuilder: description: This tests explicitly optional kubebuilder fields type: string @@ -360,6 +370,9 @@ spec: If the Job controller observes a mode that it doesn't recognize, which is possible during upgrades due to version skew, the controller skips updates for the Job. + enum: + - NonIndexed + - Indexed type: string completions: description: |- @@ -461,6 +474,11 @@ spec: counter towards the .backoffLimit is incremented. Additional values are considered to be added in the future. Clients should react to an unknown action by skipping the rule. + enum: + - FailJob + - FailIndex + - Ignore + - Count type: string onExitCodes: description: Represents the requirement on the container @@ -487,6 +505,9 @@ spec: by the 'containerName' field) is not in the set of specified values. Additional values are considered to be added in the future. Clients should react to an unknown operator by assuming the requirement is not satisfied. + enum: + - In + - NotIn type: string values: description: |- @@ -552,6 +573,9 @@ spec: TerminatingOrFailed and Failed are allowed values when podFailurePolicy is not in use. This is an beta field. To use this, enable the JobPodReplacementPolicy feature toggle. This is on by default. + enum: + - TerminatingOrFailed + - Failed type: string selector: description: |- @@ -737,6 +761,13 @@ spec: description: |- Represents a key's relationship to a set of values. Valid operators are In, NotIn, Exists, DoesNotExist. Gt, and Lt. + enum: + - In + - NotIn + - Exists + - DoesNotExist + - Gt + - Lt type: string values: description: |- @@ -772,6 +803,13 @@ spec: description: |- Represents a key's relationship to a set of values. Valid operators are In, NotIn, Exists, DoesNotExist. Gt, and Lt. + enum: + - In + - NotIn + - Exists + - DoesNotExist + - Gt + - Lt type: string values: description: |- @@ -838,6 +876,13 @@ spec: description: |- Represents a key's relationship to a set of values. Valid operators are In, NotIn, Exists, DoesNotExist. Gt, and Lt. + enum: + - In + - NotIn + - Exists + - DoesNotExist + - Gt + - Lt type: string values: description: |- @@ -873,6 +918,13 @@ spec: description: |- Represents a key's relationship to a set of values. Valid operators are In, NotIn, Exists, DoesNotExist. Gt, and Lt. + enum: + - In + - NotIn + - Exists + - DoesNotExist + - Gt + - Lt type: string values: description: |- @@ -1885,6 +1937,10 @@ spec: Defaults to Always if :latest tag is specified, or IfNotPresent otherwise. Cannot be updated. More info: https://kubernetes.io/docs/concepts/containers/images#updating-images + enum: + - Always + - Never + - IfNotPresent type: string lifecycle: description: |- @@ -1964,6 +2020,9 @@ spec: description: |- Scheme to use for connecting to the host. Defaults to HTTP. + enum: + - HTTP + - HTTPS type: string required: - port @@ -2083,6 +2142,9 @@ spec: description: |- Scheme to use for connecting to the host. Defaults to HTTP. + enum: + - HTTP + - HTTPS type: string required: - port @@ -2225,6 +2287,9 @@ spec: description: |- Scheme to use for connecting to the host. Defaults to HTTP. + enum: + - HTTP + - HTTPS type: string required: - port @@ -2337,6 +2402,10 @@ spec: description: |- Protocol for port. Must be UDP, TCP, or SCTP. Defaults to "TCP". + enum: + - TCP + - UDP + - SCTP type: string required: - containerPort @@ -2446,6 +2515,9 @@ spec: description: |- Scheme to use for connecting to the host. Defaults to HTTP. + enum: + - HTTP + - HTTPS type: string required: - port @@ -2650,6 +2722,10 @@ spec: Localhost - a profile pre-loaded on the node. RuntimeDefault - the container runtime's default profile. Unconfined - no AppArmor enforcement. + enum: + - Unconfined + - RuntimeDefault + - Localhost type: string required: - type @@ -2691,6 +2767,9 @@ spec: readonly paths and masked paths. This requires the ProcMountType feature flag to be enabled. Note that this field cannot be set when spec.os.name is windows. + enum: + - Default + - Unmasked type: string readOnlyRootFilesystem: description: |- @@ -2772,6 +2851,10 @@ spec: Localhost - a profile defined in a file on the node should be used. RuntimeDefault - the container runtime default profile should be used. Unconfined - no profile should be applied. + enum: + - Unconfined + - RuntimeDefault + - Localhost type: string required: - type @@ -2913,6 +2996,9 @@ spec: description: |- Scheme to use for connecting to the host. Defaults to HTTP. + enum: + - HTTP + - HTTPS type: string required: - port @@ -3012,6 +3098,9 @@ spec: The log output is limited to 2048 bytes or 80 lines, whichever is smaller. Defaults to File. Cannot be updated. + enum: + - File + - FallbackToLogsOnError type: string tty: description: |- @@ -3063,6 +3152,10 @@ spec: This field is beta in 1.10. When RecursiveReadOnly is set to IfPossible or to Enabled, MountPropagation must be None or unspecified (which defaults to None). + enum: + - None + - HostToContainer + - Bidirectional type: string name: description: This must match the Name @@ -3177,6 +3270,11 @@ spec: DNS parameters given in DNSConfig will be merged with the policy selected with DNSPolicy. To have DNS options set along with hostNetwork, you have to specify DNS policy explicitly to 'ClusterFirstWithHostNet'. + enum: + - ClusterFirstWithHostNet + - ClusterFirst + - Default + - None type: string enableServiceLinks: description: |- @@ -3430,6 +3528,10 @@ spec: Defaults to Always if :latest tag is specified, or IfNotPresent otherwise. Cannot be updated. More info: https://kubernetes.io/docs/concepts/containers/images#updating-images + enum: + - Always + - Never + - IfNotPresent type: string lifecycle: description: Lifecycle is not allowed for ephemeral @@ -3508,6 +3610,9 @@ spec: description: |- Scheme to use for connecting to the host. Defaults to HTTP. + enum: + - HTTP + - HTTPS type: string required: - port @@ -3627,6 +3732,9 @@ spec: description: |- Scheme to use for connecting to the host. Defaults to HTTP. + enum: + - HTTP + - HTTPS type: string required: - port @@ -3766,6 +3874,9 @@ spec: description: |- Scheme to use for connecting to the host. Defaults to HTTP. + enum: + - HTTP + - HTTPS type: string required: - port @@ -3871,6 +3982,10 @@ spec: description: |- Protocol for port. Must be UDP, TCP, or SCTP. Defaults to "TCP". + enum: + - TCP + - UDP + - SCTP type: string required: - containerPort @@ -3977,6 +4092,9 @@ spec: description: |- Scheme to use for connecting to the host. Defaults to HTTP. + enum: + - HTTP + - HTTPS type: string required: - port @@ -4168,6 +4286,10 @@ spec: Localhost - a profile pre-loaded on the node. RuntimeDefault - the container runtime's default profile. Unconfined - no AppArmor enforcement. + enum: + - Unconfined + - RuntimeDefault + - Localhost type: string required: - type @@ -4209,6 +4331,9 @@ spec: readonly paths and masked paths. This requires the ProcMountType feature flag to be enabled. Note that this field cannot be set when spec.os.name is windows. + enum: + - Default + - Unmasked type: string readOnlyRootFilesystem: description: |- @@ -4290,6 +4415,10 @@ spec: Localhost - a profile defined in a file on the node should be used. RuntimeDefault - the container runtime default profile should be used. Unconfined - no profile should be applied. + enum: + - Unconfined + - RuntimeDefault + - Localhost type: string required: - type @@ -4425,6 +4554,9 @@ spec: description: |- Scheme to use for connecting to the host. Defaults to HTTP. + enum: + - HTTP + - HTTPS type: string required: - port @@ -4533,6 +4665,9 @@ spec: The log output is limited to 2048 bytes or 80 lines, whichever is smaller. Defaults to File. Cannot be updated. + enum: + - File + - FallbackToLogsOnError type: string tty: description: |- @@ -4584,6 +4719,10 @@ spec: This field is beta in 1.10. When RecursiveReadOnly is set to IfPossible or to Enabled, MountPropagation must be None or unspecified (which defaults to None). + enum: + - None + - HostToContainer + - Bidirectional type: string name: description: This must match the Name @@ -4980,6 +5119,10 @@ spec: Defaults to Always if :latest tag is specified, or IfNotPresent otherwise. Cannot be updated. More info: https://kubernetes.io/docs/concepts/containers/images#updating-images + enum: + - Always + - Never + - IfNotPresent type: string lifecycle: description: |- @@ -5059,6 +5202,9 @@ spec: description: |- Scheme to use for connecting to the host. Defaults to HTTP. + enum: + - HTTP + - HTTPS type: string required: - port @@ -5178,6 +5324,9 @@ spec: description: |- Scheme to use for connecting to the host. Defaults to HTTP. + enum: + - HTTP + - HTTPS type: string required: - port @@ -5320,6 +5469,9 @@ spec: description: |- Scheme to use for connecting to the host. Defaults to HTTP. + enum: + - HTTP + - HTTPS type: string required: - port @@ -5432,6 +5584,10 @@ spec: description: |- Protocol for port. Must be UDP, TCP, or SCTP. Defaults to "TCP". + enum: + - TCP + - UDP + - SCTP type: string required: - containerPort @@ -5541,6 +5697,9 @@ spec: description: |- Scheme to use for connecting to the host. Defaults to HTTP. + enum: + - HTTP + - HTTPS type: string required: - port @@ -5745,6 +5904,10 @@ spec: Localhost - a profile pre-loaded on the node. RuntimeDefault - the container runtime's default profile. Unconfined - no AppArmor enforcement. + enum: + - Unconfined + - RuntimeDefault + - Localhost type: string required: - type @@ -5786,6 +5949,9 @@ spec: readonly paths and masked paths. This requires the ProcMountType feature flag to be enabled. Note that this field cannot be set when spec.os.name is windows. + enum: + - Default + - Unmasked type: string readOnlyRootFilesystem: description: |- @@ -5867,6 +6033,10 @@ spec: Localhost - a profile defined in a file on the node should be used. RuntimeDefault - the container runtime default profile should be used. Unconfined - no profile should be applied. + enum: + - Unconfined + - RuntimeDefault + - Localhost type: string required: - type @@ -6008,6 +6178,9 @@ spec: description: |- Scheme to use for connecting to the host. Defaults to HTTP. + enum: + - HTTP + - HTTPS type: string required: - port @@ -6107,6 +6280,9 @@ spec: The log output is limited to 2048 bytes or 80 lines, whichever is smaller. Defaults to File. Cannot be updated. + enum: + - File + - FallbackToLogsOnError type: string tty: description: |- @@ -6158,6 +6334,10 @@ spec: This field is beta in 1.10. When RecursiveReadOnly is set to IfPossible or to Enabled, MountPropagation must be None or unspecified (which defaults to None). + enum: + - None + - HostToContainer + - Bidirectional type: string name: description: This must match the Name @@ -6303,6 +6483,9 @@ spec: PreemptionPolicy is the Policy for preempting pods with lower priority. One of Never, PreemptLowerPriority. Defaults to PreemptLowerPriority if unset. + enum: + - PreemptLowerPriority + - Never type: string priority: description: |- @@ -6406,6 +6589,10 @@ spec: One of Always, OnFailure, Never. In some contexts, only a subset of those values may be permitted. Default to Always. More info: https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/#restart-policy + enum: + - Always + - OnFailure + - Never type: string runtimeClassName: description: |- @@ -6467,6 +6654,10 @@ spec: Localhost - a profile pre-loaded on the node. RuntimeDefault - the container runtime's default profile. Unconfined - no AppArmor enforcement. + enum: + - Unconfined + - RuntimeDefault + - Localhost type: string required: - type @@ -6494,6 +6685,9 @@ spec: and emptydir. Valid values are "OnRootMismatch" and "Always". If not specified, "Always" is used. Note that this field cannot be set when spec.os.name is windows. + enum: + - OnRootMismatch + - Always type: string runAsGroup: description: |- @@ -6570,6 +6764,10 @@ spec: Localhost - a profile defined in a file on the node should be used. RuntimeDefault - the container runtime default profile should be used. Unconfined - no profile should be applied. + enum: + - Unconfined + - RuntimeDefault + - Localhost type: string required: - type @@ -6597,6 +6795,9 @@ spec: (Alpha) Using the field requires the SupplementalGroupsPolicy feature gate to be enabled and the container runtime must implement support for this feature. Note that this field cannot be set when spec.os.name is windows. + enum: + - Merge + - Strict type: string sysctls: description: |- @@ -6706,6 +6907,10 @@ spec: description: |- Effect indicates the taint effect to match. Empty means match all taint effects. When specified, allowed values are NoSchedule, PreferNoSchedule and NoExecute. + enum: + - NoSchedule + - PreferNoSchedule + - NoExecute type: string key: description: |- @@ -6718,6 +6923,9 @@ spec: Valid operators are Exists and Equal. Defaults to Equal. Exists is equivalent to wildcard for value, so that a pod can tolerate all taints of a particular category. + enum: + - Exists + - Equal type: string tolerationSeconds: description: |- @@ -6863,6 +7071,9 @@ spec: If this value is nil, the behavior is equivalent to the Honor policy. This is a beta-level feature default enabled by the NodeInclusionPolicyInPodTopologySpread feature flag. + enum: + - Ignore + - Honor type: string nodeTaintsPolicy: description: |- @@ -6874,6 +7085,9 @@ spec: If this value is nil, the behavior is equivalent to the Ignore policy. This is a beta-level feature default enabled by the NodeInclusionPolicyInPodTopologySpread feature flag. + enum: + - Ignore + - Honor type: string topologyKey: description: |- @@ -6908,6 +7122,9 @@ spec: MaxSkew(1). In other words, the cluster can still be imbalanced, but scheduler won't make it *more* imbalanced. It's a required field. + enum: + - DoNotSchedule + - ScheduleAnyway type: string required: - maxSkew @@ -6970,6 +7187,10 @@ spec: cachingMode: description: 'cachingMode is the Host Caching mode: None, Read Only, Read Write.' + enum: + - None + - ReadOnly + - ReadWrite type: string diskName: description: diskName is the Name of the @@ -6992,6 +7213,10 @@ spec: single blob disk per storage account Managed: azure managed data disk (only in managed availability set). defaults to shared' + enum: + - Shared + - Dedicated + - Managed type: string readOnly: default: false @@ -7431,6 +7656,11 @@ spec: accessModes contains the desired access modes the volume should have. More info: https://kubernetes.io/docs/concepts/storage/persistent-volumes#access-modes-1 items: + enum: + - ReadWriteOnce + - ReadOnlyMany + - ReadWriteMany + - ReadWriteOncePod type: string type: array x-kubernetes-list-type: atomic @@ -7622,6 +7852,9 @@ spec: description: |- volumeMode defines what type of volume is required by the claim. Value of Filesystem is implied when not included in claim spec. + enum: + - Block + - Filesystem type: string volumeName: description: volumeName is the binding @@ -7839,6 +8072,15 @@ spec: type for HostPath Volume Defaults to "" More info: https://kubernetes.io/docs/concepts/storage/volumes#hostpath + enum: + - "" + - DirectoryOrCreate + - Directory + - FileOrCreate + - File + - Socket + - CharDevice + - BlockDevice type: string required: - path @@ -7867,6 +8109,10 @@ spec: Never: the kubelet never pulls the reference and only uses a local image or artifact. Container creation will fail if the reference isn't present. IfNotPresent: the kubelet pulls if the reference isn't already present on disk. Container creation will fail if the reference isn't present and the pull fails. Defaults to Always if :latest tag is specified, or IfNotPresent otherwise. + enum: + - Always + - Never + - IfNotPresent type: string reference: description: |- @@ -8987,6 +9233,10 @@ spec: This tests that we can embed protocol correctly (without ending up with allOf). Context: https://github.com/kubernetes-sigs/controller-tools/issues/1027 Defaults to "TCP". + enum: + - TCP + - UDP + - SCTP type: string ptrData: additionalProperties: diff --git a/pkg/loader/visit.go b/pkg/loader/visit.go index b5646fde1..3adb6b025 100644 --- a/pkg/loader/visit.go +++ b/pkg/loader/visit.go @@ -18,6 +18,7 @@ package loader import ( "go/ast" + "go/token" "reflect" "strconv" ) @@ -25,6 +26,10 @@ import ( // TypeCallback is a callback called for each raw AST (gendecl, typespec) combo. type TypeCallback func(file *ast.File, decl *ast.GenDecl, spec *ast.TypeSpec) +// A ConstCallback is used to iterate over constant definitions. It provides +// the value spec only since it is the only thing needed yet. +type ConstCallback func(spec *ast.ValueSpec) + // EachType calls the given callback for each (gendecl, typespec) combo in the // given package. Generally, using markers.EachType is better when working // with marker data, and has a more convinient representation. @@ -39,6 +44,30 @@ func EachType(pkg *Package, cb TypeCallback) { } } +func EachConstDecl(pkg *Package, cb ConstCallback) { + pkg.NeedSyntax() + for _, file := range pkg.Syntax { + ast.Walk(cb, file) + } +} + +// Visit visits all top-level constant declarations. +func (v ConstCallback) Visit(node ast.Node) ast.Visitor { + if node == nil { + return v + } + var typedNode, ok = node.(*ast.GenDecl) + if !ok { + return v + } + if typedNode.Tok == token.CONST { + for i := range typedNode.Specs { + v(typedNode.Specs[i].(*ast.ValueSpec)) + } + } + return nil +} + // typeVisitor visits all TypeSpecs, calling the given callback for each. type typeVisitor struct { callback TypeCallback diff --git a/pkg/markers/zip.go b/pkg/markers/zip.go index 0ad44e610..02ef2c644 100644 --- a/pkg/markers/zip.go +++ b/pkg/markers/zip.go @@ -153,6 +153,11 @@ type FieldInfo struct { RawField *ast.Field } +type EnumMemberInfo struct { + Name string + *ast.ValueSpec +} + // TypeInfo contains marker values and commonly used information for a type declaration. type TypeInfo struct { // Name is the name of the type. @@ -168,6 +173,9 @@ type TypeInfo struct { // (if not, Fields will be nil). Fields []FieldInfo + // Enum value list (nil if the field is not an enum). + EnumValues []EnumMemberInfo + // RawDecl contains the raw GenDecl that the type was declared as part of. RawDecl *ast.GenDecl // RawSpec contains the raw Spec that declared this type. From b338e3fa4d4e80bb5b4b40f144a6c97672c585a2 Mon Sep 17 00:00:00 2001 From: MishimaPorte <76715147+MishimaPorte@users.noreply.github.com> Date: Tue, 25 Mar 2025 16:30:25 +0300 Subject: [PATCH 2/6] Update pkg/crd/schema.go Co-authored-by: Joel Speed --- pkg/crd/schema.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/crd/schema.go b/pkg/crd/schema.go index d96c3cec3..eff44701a 100644 --- a/pkg/crd/schema.go +++ b/pkg/crd/schema.go @@ -284,7 +284,7 @@ func localNamedToSchema(ctx *schemaContext, ident *ast.Ident) *apiext.JSONSchema var member = &ctx.info.EnumValues[i] var v *ast.BasicLit if v, ok = member.Values[0].(*ast.BasicLit); !ok { - ctx.pkg.AddError(loader.ErrFromNode(errors.New("constants for a +enum decorated type should be stirngs"), ident)) + ctx.pkg.AddError(loader.ErrFromNode(errors.New("constants for a +enum decorated type should be strings"), ident)) } var value string if value, err = strconv.Unquote(v.Value); err != nil { From bb6e2b65b64942d9cef51351e37d9845f9ed7854 Mon Sep 17 00:00:00 2001 From: MishimaPorte Date: Tue, 25 Mar 2025 16:32:44 +0300 Subject: [PATCH 3/6] added a godoc --- pkg/loader/visit.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/loader/visit.go b/pkg/loader/visit.go index 3adb6b025..c1a459818 100644 --- a/pkg/loader/visit.go +++ b/pkg/loader/visit.go @@ -44,6 +44,8 @@ func EachType(pkg *Package, cb TypeCallback) { } } +// EachConstDecl calls the given callback for each +// *ast.ValueSpec associated with constant declarations. func EachConstDecl(pkg *Package, cb ConstCallback) { pkg.NeedSyntax() for _, file := range pkg.Syntax { From ccd6c3679fdb4f212adb21cf51805054cb4133c9 Mon Sep 17 00:00:00 2001 From: MishimaPorte Date: Tue, 25 Mar 2025 17:58:59 +0300 Subject: [PATCH 4/6] added a testcase for ensuring correct different enum type precedence --- pkg/crd/markers/validation.go | 5 +++++ pkg/crd/markers/zz_generated.markerhelp.go | 22 +++++++++---------- pkg/crd/testdata/cronjob_types.go | 17 +++++++++++++- .../testdata.kubebuilder.io_cronjobs.yaml | 10 +++++++++ 4 files changed, 42 insertions(+), 12 deletions(-) diff --git a/pkg/crd/markers/validation.go b/pkg/crd/markers/validation.go index e1d72c970..86ac3d1cc 100644 --- a/pkg/crd/markers/validation.go +++ b/pkg/crd/markers/validation.go @@ -470,6 +470,11 @@ func (m MaxProperties) ApplyToSchema(schema *apiext.JSONSchemaProps) error { } func (m Enum) ApplyToSchema(schema *apiext.JSONSchemaProps) error { + // apply this enum only if there were no other enum values specified + // (e.g. via a "+enum" marker) + if len(schema.Enum) != 0 { + return nil + } // TODO(directxman12): this is a bit hacky -- we should // probably support AnyType better + using the schema structure vals := make([]apiext.JSON, len(m)) diff --git a/pkg/crd/markers/zz_generated.markerhelp.go b/pkg/crd/markers/zz_generated.markerhelp.go index 16004d0ac..711778140 100644 --- a/pkg/crd/markers/zz_generated.markerhelp.go +++ b/pkg/crd/markers/zz_generated.markerhelp.go @@ -67,17 +67,6 @@ func (Enum) Help() *markers.DefinitionHelp { } } -func (InferredEnum) Help() *markers.DefinitionHelp { - return &markers.DefinitionHelp{ - Category: "CRD validation", - DetailedHelp: markers.DetailedHelp{ - Summary: "enum marks a string type as an enum.", - Details: "It infers the members from constants declared of that type.", - }, - FieldHelp: map[string]markers.DetailedHelp{}, - } -} - func (Example) Help() *markers.DefinitionHelp { return &markers.DefinitionHelp{ Category: "CRD validation", @@ -127,6 +116,17 @@ func (Format) Help() *markers.DefinitionHelp { } } +func (InferredEnum) Help() *markers.DefinitionHelp { + return &markers.DefinitionHelp{ + Category: "CRD validation", + DetailedHelp: markers.DetailedHelp{ + Summary: "Enum marker marks a string alias type as an enum.", + Details: "It infers the members from constants declared of that type.", + }, + FieldHelp: map[string]markers.DetailedHelp{}, + } +} + func (KubernetesDefault) Help() *markers.DefinitionHelp { return &markers.DefinitionHelp{ Category: "CRD validation", diff --git a/pkg/crd/testdata/cronjob_types.go b/pkg/crd/testdata/cronjob_types.go index 5c5bf304f..e15f884d0 100644 --- a/pkg/crd/testdata/cronjob_types.go +++ b/pkg/crd/testdata/cronjob_types.go @@ -51,6 +51,20 @@ const ( EnumType_Three EnumType = "three" ) +// This enum type tests for whether when both "+enum" and +// "+kubebuilder:validation:Enum" are defined the former takes precedence. +// It should. +// +// +enum +// +kubebuilder:validation:Enum=Allow;Forbid;Replace +type AnotherEnumType string + +const ( + AnotherEnumType_One AnotherEnumType = "another_one" + AnotherEnumType_Two AnotherEnumType = "another_two" + AnotherEnumType_Three AnotherEnumType = "another_three" +) + // CronJobSpec defines the desired state of CronJob // +kubebuilder:validation:XValidation:rule="has(oldSelf.forbiddenInt) || !has(self.forbiddenInt)",message="forbiddenInt is not allowed",fieldPath=".forbiddenInt",reason="FieldValueForbidden" type CronJobSpec struct { @@ -341,7 +355,8 @@ type CronJobSpec struct { // +kubebuilder:validation:items:Enum=0;1;3 EnumSlice []int `json:"enumSlice,omitempty"` - EnumValue EnumType `json:"enumValue,omitempty"` + EnumValue EnumType `json:"enumValue,omitempty"` + AnotherEnumValue AnotherEnumType `json:"anotherEnumValue,omitempty"` HostsAlias Hosts `json:"hostsAlias,omitempty"` diff --git a/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml b/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml index 0dd3d1845..4f1e2a7c7 100644 --- a/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml +++ b/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml @@ -51,6 +51,16 @@ spec: - PreferDualStack - RequireDualStack type: string + anotherEnumValue: + description: |- + This enum type tests for whether when both "+enum" and + "+kubebuilder:validation:Enum" are defined the former takes precedence. + It should. + enum: + - another_one + - another_two + - another_three + type: string array: description: Checks that fixed-length arrays work items: From 410a208167186e06ce8b619bb97b929eb345e507 Mon Sep 17 00:00:00 2001 From: MishimaPorte Date: Sat, 12 Apr 2025 15:13:08 +0300 Subject: [PATCH 5/6] the linter is happy now --- pkg/crd/schema.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/pkg/crd/schema.go b/pkg/crd/schema.go index eff44701a..71d2b2ccd 100644 --- a/pkg/crd/schema.go +++ b/pkg/crd/schema.go @@ -38,7 +38,8 @@ import ( const ( // defPrefix is the prefix used to link to definitions in the OpenAPI schema. - defPrefix = "#/definitions/" + defPrefix = "#/definitions/" + typeString = "string" ) // byteType is the types.Type for byte (see the types documention @@ -128,9 +129,9 @@ func infoToSchema(ctx *schemaContext) *apiext.JSONSchemaProps { // If the obj implements a text marshaler, encode it as a string. case implements(obj.Type(), textMarshaler): - schema := &apiext.JSONSchemaProps{Type: "string"} + schema := &apiext.JSONSchemaProps{Type: typeString} applyMarkers(ctx, ctx.info.Markers, schema, ctx.info.RawSpec.Type) - if schema.Type != "string" { + if schema.Type != typeString { err := fmt.Errorf("%q implements encoding.TextMarshaler but schema type is not string: %q", ctx.info.RawSpec.Name, schema.Type) ctx.pkg.AddError(loader.ErrFromNode(err, ctx.info.RawSpec.Type)) } @@ -277,7 +278,7 @@ func localNamedToSchema(ctx *schemaContext, ident *ast.Ident) *apiext.JSONSchema ctx.pkg.AddError(loader.ErrFromNode(err, ident)) } var enumMembers []apiext.JSON - if ctx.info.Markers.Get("enum") != nil && typ == "string" { + if ctx.info.Markers.Get("enum") != nil && typ == typeString { enumMembers = make([]apiext.JSON, 0, len(ctx.info.EnumValues)) var ok bool for i := range ctx.info.EnumValues { @@ -361,7 +362,7 @@ func arrayToSchema(ctx *schemaContext, array *ast.ArrayType) *apiext.JSONSchemaP // byte slices are represented as base64-encoded strings // (the format is defined in OpenAPI v3, but not JSON Schema) return &apiext.JSONSchemaProps{ - Type: "string", + Type: typeString, Format: "byte", } } @@ -529,7 +530,7 @@ func builtinToType(basic *types.Basic, allowDangerousTypes bool) (typ string, fo case basicInfo&types.IsBoolean != 0: typ = "boolean" case basicInfo&types.IsString != 0: - typ = "string" + typ = typeString case basicInfo&types.IsInteger != 0: typ = "integer" case basicInfo&types.IsFloat != 0: From 040149faf20b9b0a24f493cffa6d63b8beb4f6c4 Mon Sep 17 00:00:00 2001 From: MishimaPorte Date: Sat, 12 Apr 2025 15:23:59 +0300 Subject: [PATCH 6/6] the kubebuilder:validation:items:Enum marker takes precedence over +enum --- pkg/crd/markers/validation.go | 5 ----- pkg/crd/testdata/cronjob_types.go | 2 ++ .../testdata.kubebuilder.io_cronjobs.yaml | 17 ++++++++++++++--- 3 files changed, 16 insertions(+), 8 deletions(-) diff --git a/pkg/crd/markers/validation.go b/pkg/crd/markers/validation.go index 86ac3d1cc..e1d72c970 100644 --- a/pkg/crd/markers/validation.go +++ b/pkg/crd/markers/validation.go @@ -470,11 +470,6 @@ func (m MaxProperties) ApplyToSchema(schema *apiext.JSONSchemaProps) error { } func (m Enum) ApplyToSchema(schema *apiext.JSONSchemaProps) error { - // apply this enum only if there were no other enum values specified - // (e.g. via a "+enum" marker) - if len(schema.Enum) != 0 { - return nil - } // TODO(directxman12): this is a bit hacky -- we should // probably support AnyType better + using the schema structure vals := make([]apiext.JSON, len(m)) diff --git a/pkg/crd/testdata/cronjob_types.go b/pkg/crd/testdata/cronjob_types.go index e15f884d0..d56aeaa11 100644 --- a/pkg/crd/testdata/cronjob_types.go +++ b/pkg/crd/testdata/cronjob_types.go @@ -357,6 +357,8 @@ type CronJobSpec struct { EnumValue EnumType `json:"enumValue,omitempty"` AnotherEnumValue AnotherEnumType `json:"anotherEnumValue,omitempty"` + // +kubebuilder:validation:Enum=Allow;Forbid;Replace + OneMoreEnumValue EnumType `json:"oneMoreEnumValue,omitempty"` HostsAlias Hosts `json:"hostsAlias,omitempty"` diff --git a/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml b/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml index 4f1e2a7c7..5623891a8 100644 --- a/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml +++ b/pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml @@ -57,9 +57,9 @@ spec: "+kubebuilder:validation:Enum" are defined the former takes precedence. It should. enum: - - another_one - - another_two - - another_three + - Allow + - Forbid + - Replace type: string array: description: Checks that fixed-length arrays work @@ -9223,6 +9223,17 @@ spec: This flag is like suspend, but for when you really mean it. It helps test the +kubebuilder:validation:Type marker. type: string + oneMoreEnumValue: + allOf: + - enum: + - one + - two + - three + - enum: + - Allow + - Forbid + - Replace + type: string onlyAllowSettingOnUpdate: description: Test that we can add a field that can only be set to a non-default value on updates using XValidation OptionalOldSelf.