Skip to content

✨ +enum validation marker #1179

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 6 commits into
base: main
Choose a base branch
from
Open

✨ +enum validation marker #1179

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1bf22cb
feature: +enum validation marker
MishimaPorte Mar 25, 2025
b338e3f
Update pkg/crd/schema.go
MishimaPorte Mar 25, 2025
bb6e2b6
added a godoc
MishimaPorte Mar 25, 2025
ccd6c36
added a testcase for ensuring correct different enum type precedence
MishimaPorte Mar 25, 2025
410a208
the linter is happy now
MishimaPorte Apr 12, 2025
040149f
the kubebuilder:validation:items:Enum marker takes precedence over +enum
MishimaPorte Apr 12, 2025
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
6 changes: 6 additions & 0 deletions pkg/crd/markers/crd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
7 changes: 7 additions & 0 deletions pkg/crd/markers/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -468,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
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any other markers that do this kind of thing? Is there precedence?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any other markers that do this kind of thing? Is there precedence?

Copy link
Author

Choose a reason for hiding this comment

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

I don't think so. But enum markers are kind of the only ones that do the same thing differently so some decision on precedence here is needed in any case.

Copy link
Contributor

Choose a reason for hiding this comment

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

CC @alvaroaleman @sbueringer do you have an opinion on this?

At present, when you have the kubebuilder:validation:enum on the field and a type alias, I believe you get a struct like

allOf:
- enum: []
- enum: []

I wonder if, to be consistent, this case should also do the same? It would encourage developers to deduplicate their markers which I think is probably a positive

Copy link
Author

Choose a reason for hiding this comment

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

It would encourage developers to deduplicate their markers

I believe this is less of a usecase here since both markers (+enum and +kubebuilder:validation:enum) are at the same place - on the type alias declaration, not in distant places (one on the field and the other on the type). So it is close to impossible to have accidental duplicates, like with the aforementioned example. I personally would prefer this to be an error right away, but it can probably harm backward compatibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we want to make the preference configurable. The default behaviour though I think needs to be that kubebuilder:validation:enum takes precedence over the naked enum though. That way we won't break existing users. I'm worried that built in types that already have the enum will get broken when this change rolls out

Copy link
Author

Choose a reason for hiding this comment

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

I think configurable precedence would complicate things too much - we can just make the kubebuilder:validation:enum take precedence over +enum and call it a day.

For known core types it can be made configurable, but, honestly, the now-generated enum block should have always been there in the schema. I can think of zero example where such a change would complicate something - on the contrary, probably additional validation would make the generated crds less error-prone.

Copy link
Contributor

Choose a reason for hiding this comment

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

should have always been there in the schema

Yes, but depending on the use case, this is likely a breaking change for the people using the CRDs built by this tooling

we can just make the kubebuilder:validation:enum take precedence over +enum and call it a day.

I would lean towards this for now to be safe

Copy link
Author

@MishimaPorte MishimaPorte Apr 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

this is likely a breaking change for the people using the CRDs built by this tooling

so, a configuration flag? but an opt-in or opt-out one is what we need? I personally would argue for the latter since the change seems to be beneficial in most of the cases (at least when something like PodTemplate is used)

// TODO(directxman12): this is a bit hacky -- we should
// probably support AnyType better + using the schema structure
vals := make([]apiext.JSON, len(m))
Expand Down
11 changes: 11 additions & 0 deletions pkg/crd/markers/zz_generated.markerhelp.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 17 additions & 0 deletions pkg/crd/parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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{}{}
}
Expand Down
27 changes: 27 additions & 0 deletions pkg/crd/schema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,14 @@
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"
Expand Down Expand Up @@ -128,7 +130,7 @@
case implements(obj.Type(), textMarshaler):
schema := &apiext.JSONSchemaProps{Type: "string"}
applyMarkers(ctx, ctx.info.Markers, schema, ctx.info.RawSpec.Type)
if schema.Type != "string" {

Check failure on line 133 in pkg/crd/schema.go

View workflow job for this annotation

GitHub Actions / lint 

string `string` has 3 occurrences, make it a constant (goconst)
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))
}
Expand Down Expand Up @@ -274,6 +276,29 @@
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 strings"), 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.
Expand All @@ -284,12 +309,14 @@
link := TypeRefLink("", ident.Name)
return &apiext.JSONSchemaProps{
Type: typ,
Enum: enumMembers,
Format: fmt,
Ref: &link,
}
}
return &apiext.JSONSchemaProps{
Type: typ,
Enum: enumMembers,
Format: fmt,
}
}
Expand Down
26 changes: 26 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 @@ -42,6 +42,29 @@ import (

const DefaultRefValue = "defaultRefValue"

// +enum
type EnumType string

const (
EnumType_One EnumType = "one"
EnumType_Two EnumType = "two"
EnumType_Three EnumType = "three"
)

// This enum type tests for whether when both "+enum" and
// "+kubebuilder:validation:Enum" are defined the former takes precedence.
Copy link
Contributor

Choose a reason for hiding this comment

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

The latter should be taking precedence no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. The tests show - Allow - Forbid - Replace.

Copy link
Contributor

Choose a reason for hiding this comment

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

@MishimaPorte Can we get this comment updated please

// It should.
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 understand this last sentence.

//
// +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 {
Expand Down Expand Up @@ -332,6 +355,9 @@ type CronJobSpec struct {
// +kubebuilder:validation:items:Enum=0;1;3
EnumSlice []int `json:"enumSlice,omitempty"`

EnumValue EnumType `json:"enumValue,omitempty"`
AnotherEnumValue AnotherEnumType `json:"anotherEnumValue,omitempty"`

HostsAlias Hosts `json:"hostsAlias,omitempty"`

// This tests that alias imported from a package is handled correctly. The
Expand Down
Loading
Loading