pkg/crd: support validating internal list items on list types

For kubernetes-sigs#342

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>

pkg/crd/markers/register.go

@@ -42,6 +42,15 @@ func (d *definitionWithHelp) Register(reg *markers.Registry) error {
 	return nil
 }
 
+func (d *definitionWithHelp) clone() *definitionWithHelp {
+	newDef := *d.Definition
+	// copy both parts so we don't change the definition
+	return &definitionWithHelp{
+		Definition: &newDef,
+		Help:       d.Help,
+	}
+}
+
 func must(def *markers.Definition, err error) *definitionWithHelp {
 	return &definitionWithHelp{
 		Definition: markers.Must(def, err),
@@ -60,7 +69,7 @@ type hasHelp interface {
 func mustMakeAllWithPrefix(prefix string, target markers.TargetType, objs ...interface{}) []*definitionWithHelp {
 	defs := make([]*definitionWithHelp, len(objs))
 	for i, obj := range objs {
-		name := prefix + ":" + reflect.TypeOf(obj).Name()
+		name := prefix + reflect.TypeOf(obj).Name()
 		def, err := markers.MakeDefinition(name, target, obj)
 		if err != nil {
 			panic(err)

pkg/crd/markers/validation.go

@@ -20,22 +20,28 @@ import (
 	"encoding/json"
 	"fmt"
 	"math"
+	"strings"
 
 	apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 
 	"sigs.k8s.io/controller-tools/pkg/markers"
 )
 
 const (
-	SchemalessName = "kubebuilder:validation:Schemaless"
+	validationPrefix = "kubebuilder:validation:"
+
+	SchemalessName        = "kubebuilder:validation:Schemaless"
+	ValidationItemsPrefix = validationPrefix + "items:"
 )
 
 // ValidationMarkers 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.
-var ValidationMarkers = mustMakeAllWithPrefix("kubebuilder:validation", markers.DescribesField,
+// 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,
 
 	// numeric markers
 
@@ -110,14 +116,20 @@ func init() {
 	AllDefinitions = append(AllDefinitions, ValidationMarkers...)
 
 	for _, def := range ValidationMarkers {
-		newDef := *def.Definition
-		// copy both parts so we don't change the definition
-		typDef := definitionWithHelp{
-			Definition: &newDef,
-			Help:       def.Help,
-		}
+		typDef := def.clone()
 		typDef.Target = markers.DescribesType
-		AllDefinitions = append(AllDefinitions, &typDef)
+		AllDefinitions = append(AllDefinitions, typDef)
+
+		itemsName := ValidationItemsPrefix + strings.TrimPrefix(def.Name, validationPrefix)
+
+		itemsFieldDef := def.clone()
+		itemsFieldDef.Name = itemsName
+		AllDefinitions = append(AllDefinitions, itemsFieldDef)
+
+		itemsTypDef := def.clone()
+		itemsTypDef.Name = itemsName
+		itemsTypDef.Target = markers.DescribesType
+		AllDefinitions = append(AllDefinitions, itemsTypDef)
 	}
 
 	AllDefinitions = append(AllDefinitions, FieldOnlyMarkers...)

pkg/crd/schema.go

@@ -128,16 +128,23 @@ func infoToSchema(ctx *schemaContext) *apiext.JSONSchemaProps {
 // applyMarkers applies schema markers given their priority to the given schema
 func applyMarkers(ctx *schemaContext, markerSet markers.MarkerValues, props *apiext.JSONSchemaProps, node ast.Node) {
 	markers := make([]SchemaMarker, 0, len(markerSet))
+	itemsMarkers := make([]SchemaMarker, 0, len(markerSet))
+	itemsMarkerNames := make(map[SchemaMarker]string)
 
-	for _, markerValues := range markerSet {
+	for markerName, markerValues := range markerSet {
 		for _, markerValue := range markerValues {
 			if schemaMarker, isSchemaMarker := markerValue.(SchemaMarker); isSchemaMarker {
-				markers = append(markers, schemaMarker)
+				if strings.HasPrefix(markerName, crdmarkers.ValidationItemsPrefix) {
+					itemsMarkers = append(itemsMarkers, schemaMarker)
+					itemsMarkerNames[schemaMarker] = markerName
+				} else {
+					markers = append(markers, schemaMarker)
+				}
 			}
 		}
 	}
 
-	sort.Slice(markers, func(i, j int) bool {
+	cmpPriority := func(markers []SchemaMarker, i, j int) bool {
 		var iPriority, jPriority crdmarkers.ApplyPriority
 
 		switch m := markers[i].(type) {
@@ -159,13 +166,27 @@ func applyMarkers(ctx *schemaContext, markerSet markers.MarkerValues, props *api
 		}
 
 		return iPriority < jPriority
-	})
+	}
+	sort.Slice(markers, func(i, j int) bool { return cmpPriority(markers, i, j) })
+	sort.Slice(itemsMarkers, func(i, j int) bool { return cmpPriority(itemsMarkers, i, j) })
 
 	for _, schemaMarker := range markers {
 		if err := schemaMarker.ApplyToSchema(props); err != nil {
 			ctx.pkg.AddError(loader.ErrFromNode(err /* an okay guess */, node))
 		}
 	}
+
+	for _, schemaMarker := range itemsMarkers {
+		if props.Type != "array" || props.Items == nil || props.Items.Schema == nil {
+			err := fmt.Errorf("must apply %s to an array value, found %s", itemsMarkerNames[schemaMarker], props.Type)
+			ctx.pkg.AddError(loader.ErrFromNode(err, node))
+		} else {
+			itemsSchema := props.Items.Schema
+			if err := schemaMarker.ApplyToSchema(itemsSchema); err != nil {
+				ctx.pkg.AddError(loader.ErrFromNode(err /* an okay guess */, node))
+			}
+		}
+	}
 }
 
 // typeToSchema creates a schema for the given AST type.

pkg/crd/testdata/cronjob_types.go

@@ -197,7 +197,7 @@ type CronJobSpec struct {
 
 	// This tests that an IntOrString can also have a pattern attached
 	// to it.
-	// This can be useful if you want to limit the string to a perecentage or integer.
+	// This can be useful if you want to limit the string to a percentage or integer.
 	// The XIntOrString marker is a requirement for having a pattern on this type.
 	// +kubebuilder:validation:XIntOrString
 	// +kubebuilder:validation:Pattern="^((100|[0-9]{1,2})%|[0-9]+)$"
@@ -252,6 +252,32 @@ type CronJobSpec struct {
 
 	// Checks that arrays work when the type contains a composite literal
 	ArrayUsingCompositeLiteral [len(struct{ X [3]int }{}.X)]string `json:"arrayUsingCompositeLiteral,omitempty"`
+
+	// This tests string slice item validation.
+	// +kubebuilder:validation:MinItems=1
+	// +kubebuilder:validation:items:MinLength=1
+	// +kubebuilder:validation:items:MaxLength=255
+	// +kubebuilder:validation:items:Pattern=^[a-z0-9]([-a-z0-9]*[a-z0-9])?([.][a-z0-9]([-a-z0-9]*[a-z0-9])?)*$
+	// +listType=set
+	Hosts []string `json:"hosts,omitempty"`
+
+	HostsAlias Hosts `json:"hostsAlias,omitempty"`
+
+	// This tests string slice validation.
+	// +kubebuilder:validation:MinItems=2
+	// +kubebuilder:validation:MaxItems=2
+	StringPair []string `json:"stringPair"`
+
+	// This tests string alias slice item validation.
+	// +kubebuilder:validation:MinItems=3
+	LongerStringArray []LongerString `json:"longerStringArray,omitempty"`
+
+	// This tests that a slice of IntOrString can also have a pattern attached to it.
+	// This can be useful if you want to limit the string to a percentage or integer.
+	// The XIntOrString marker is a requirement for having a pattern on this type.
+	// +kubebuilder:validation:items:XIntOrString
+	// +kubebuilder:validation:items:Pattern="^((100|[0-9]{1,2})%|[0-9]+)$"
+	IntOrStringArrayWithAPattern []*intstr.IntOrString `json:"intOrStringArrayWithAPattern,omitempty"`
 }
 
 type ContainsNestedMap struct {
@@ -360,6 +386,14 @@ type LongerString string
 // TotallyABool is a bool that serializes as a string.
 type TotallyABool bool
 
+// This tests string slice item validation.
+// +kubebuilder:validation:MinItems=1
+// +kubebuilder:validation:items:MinLength=1
+// +kubebuilder:validation:items:MaxLength=255
+// +kubebuilder:validation:items:Pattern=^[a-z0-9]([-a-z0-9]*[a-z0-9])?([.][a-z0-9]([-a-z0-9]*[a-z0-9])?)*$
+// +listType=set
+type Hosts []string
+
 func (t TotallyABool) MarshalJSON() ([]byte, error) {
 	if t {
 		return []byte(`"true"`), nil

pkg/crd/testdata/testdata.kubebuilder.io_cronjobs.yaml

@@ -196,20 +196,52 @@ spec:
                 description: This tests that exported fields are not skipped in the
                   schema generation
                 type: string
+              hosts:
+                description: This tests string slice item validation.
+                items:
+                  maxLength: 255
+                  minLength: 1
+                  pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?([.][a-z0-9]([-a-z0-9]*[a-z0-9])?)*$
+                  type: string
+                minItems: 1
+                type: array
+                x-kubernetes-list-type: set
+              hostsAlias:
+                description: This tests string slice item validation.
+                items:
+                  maxLength: 255
+                  minLength: 1
+                  pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?([.][a-z0-9]([-a-z0-9]*[a-z0-9])?)*$
+                  type: string
+                minItems: 1
+                type: array
+                x-kubernetes-list-type: set
               int32WithValidations:
                 format: int32
                 maximum: 2
                 minimum: -2
                 multipleOf: 2
                 type: integer
+              intOrStringArrayWithAPattern:
+                description: |-
+                  This tests that a slice of IntOrString can also have a pattern attached to it.
+                  This can be useful if you want to limit the string to a percentage or integer.
+                  The XIntOrString marker is a requirement for having a pattern on this type.
+                items:
+                  anyOf:
+                  - type: integer
+                  - type: string
+                  pattern: ^((100|[0-9]{1,2})%|[0-9]+)$
+                  x-kubernetes-int-or-string: true
+                type: array
               intOrStringWithAPattern:
                 anyOf:
                 - type: integer
                 - type: string
                 description: |-
                   This tests that an IntOrString can also have a pattern attached
                   to it.
-                  This can be useful if you want to limit the string to a perecentage or integer.
+                  This can be useful if you want to limit the string to a percentage or integer.
                   The XIntOrString marker is a requirement for having a pattern on this type.
                 pattern: ^((100|[0-9]{1,2})%|[0-9]+)$
                 x-kubernetes-int-or-string: true
@@ -6609,6 +6641,15 @@ spec:
                 - bar
                 - foo
                 type: object
+              longerStringArray:
+                description: This tests string alias slice item validation.
+                items:
+                  description: This tests that markers that are allowed on both fields
+                    and types are applied to types
+                  minLength: 4
+                  type: string
+                minItems: 3
+                type: array
               mapOfArraysOfFloats:
                 additionalProperties:
                   items:
@@ -6742,6 +6783,13 @@ spec:
                   time for any reason.  Missed jobs executions will be counted as failed ones.
                 format: int64
                 type: integer
+              stringPair:
+                description: This tests string slice validation.
+                items:
+                  type: string
+                maxItems: 2
+                minItems: 2
+                type: array
               stringSliceData:
                 additionalProperties:
                   items:
@@ -6839,6 +6887,7 @@ spec:
             - nestedassociativeList
             - patternObject
             - schedule
+            - stringPair
             - structWithSeveralFields
             - twoOfAKindPart0
             - twoOfAKindPart1