diff --git a/pkg/crd/markers/doc.go b/pkg/crd/markers/doc.go index 48736401c..f01e9f1b3 100644 --- a/pkg/crd/markers/doc.go +++ b/pkg/crd/markers/doc.go @@ -19,27 +19,40 @@ limitations under the License. // // All markers related to CRD generation live in AllDefinitions. // -// Validation Markers +// # Validation Markers // // Validation markers have values that implement ApplyToSchema // (crd.SchemaMarker). Any marker implementing this will automatically // be run after the rest of a given schema node has been generated. // Markers that need to be run before any other markers can also // implement ApplyFirst, but this is discouraged and may change -// in the future. +// in the future. It is recommended to implement the ApplyPriority +// interface in combination with ApplyPriorityDefault and +// ApplyPriorityFirst constants. Following is an example of how to +// implement such a marker: +// +// type MyCustomMarker string +// +// func (m MyCustomMarker) ApplyPriority() ApplyPriority { +// return ApplyPriorityFirst +// } +// +// func (m MyCustomMarker) ApplyToSchema(schema *apiext.JSONSchemaProps) error { +// ... +// } // // All validation markers start with "+kubebuilder:validation", and // have the same name as their type name. // -// CRD Markers +// # CRD Markers // // Markers that modify anything in the CRD itself *except* for the schema -// implement ApplyToCRD (crd.CRDMarker). They are expected to detect whether +// implement ApplyToCRD (crd.SpecMarker). They are expected to detect whether // they should apply themselves to a specific version in the CRD (as passed to // them), or to the root-level CRD for legacy cases. They are applied *after* // the rest of the CRD is computed. // -// Misc +// # Misc // // This package also defines the "+groupName" and "+versionName" package-level // markers, for defining package<->group-version mappings. diff --git a/pkg/crd/markers/priority.go b/pkg/crd/markers/priority.go new file mode 100644 index 000000000..1b4482521 --- /dev/null +++ b/pkg/crd/markers/priority.go @@ -0,0 +1,37 @@ +/* +Copyright 2022 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package markers + +// ApplyPriority designates the order markers should be applied. +// Lower priority indicates it should be applied first +type ApplyPriority int64 + +const ( + // ApplyPriorityDefault is the default priority for markers + // that don't implement ApplyPriorityMarker + ApplyPriorityDefault ApplyPriority = 10 + + // ApplyPriorityFirst is the priority value assigned to markers + // that implement the ApplyFirst() method + ApplyPriorityFirst ApplyPriority = 1 +) + +// ApplyPriorityMarker designates the order validation markers should be applied. +// Lower priority indicates it should be applied first +type ApplyPriorityMarker interface { + ApplyPriority() ApplyPriority +} diff --git a/pkg/crd/markers/topology.go b/pkg/crd/markers/topology.go index 0f4a94b18..d8f2a6ea2 100644 --- a/pkg/crd/markers/topology.go +++ b/pkg/crd/markers/topology.go @@ -47,15 +47,15 @@ func init() { // // Possible data-structure types of a list are: // -// - "map": it needs to have a key field, which will be used to build an -// associative list. A typical example is a the pod container list, -// which is indexed by the container name. +// - "map": it needs to have a key field, which will be used to build an +// associative list. A typical example is a the pod container list, +// which is indexed by the container name. // -// - "set": Fields need to be "scalar", and there can be only one -// occurrence of each. +// - "set": Fields need to be "scalar", and there can be only one +// occurrence of each. // -// - "atomic": All the fields in the list are treated as a single value, -// are typically manipulated together by the same actor. +// - "atomic": All the fields in the list are treated as a single value, +// are typically manipulated together by the same actor. type ListType string // +controllertools:marker:generateHelp:category="CRD processing" @@ -75,12 +75,12 @@ type ListMapKey string // // Possible values: // -// - "granular": items in the map are independent of each other, -// and can be manipulated by different actors. -// This is the default behavior. +// - "granular": items in the map are independent of each other, +// and can be manipulated by different actors. +// This is the default behavior. // -// - "atomic": all fields are treated as one unit. -// Any changes have to replace the entire map. +// - "atomic": all fields are treated as one unit. +// Any changes have to replace the entire map. type MapType string // +controllertools:marker:generateHelp:category="CRD processing" @@ -91,12 +91,12 @@ type MapType string // // Possible values: // -// - "granular": fields in the struct are independent of each other, -// and can be manipulated by different actors. -// This is the default behavior. +// - "granular": fields in the struct are independent of each other, +// and can be manipulated by different actors. +// This is the default behavior. // -// - "atomic": all fields are treated as one unit. -// Any changes have to replace the entire struct. +// - "atomic": all fields are treated as one unit. +// Any changes have to replace the entire struct. type StructType string func (l ListType) ApplyToSchema(schema *apiext.JSONSchemaProps) error { @@ -111,7 +111,9 @@ func (l ListType) ApplyToSchema(schema *apiext.JSONSchemaProps) error { return nil } -func (l ListType) ApplyFirst() {} +func (l ListType) ApplyPriority() ApplyPriority { + return ApplyPriorityDefault - 1 +} func (l ListMapKey) ApplyToSchema(schema *apiext.JSONSchemaProps) error { if schema.Type != "array" { diff --git a/pkg/crd/markers/validation.go b/pkg/crd/markers/validation.go index 5d1496189..5d6df5838 100644 --- a/pkg/crd/markers/validation.go +++ b/pkg/crd/markers/validation.go @@ -448,7 +448,9 @@ func (m Type) ApplyToSchema(schema *apiext.JSONSchemaProps) error { return nil } -func (m Type) ApplyFirst() {} +func (m Type) ApplyPriority() ApplyPriority { + return ApplyPriorityDefault - 1 +} func (m Nullable) ApplyToSchema(schema *apiext.JSONSchemaProps) error { schema.Nullable = true @@ -484,7 +486,9 @@ func (m XIntOrString) ApplyToSchema(schema *apiext.JSONSchemaProps) error { return nil } -func (m XIntOrString) ApplyFirst() {} +func (m XIntOrString) ApplyPriority() ApplyPriority { + return ApplyPriorityDefault - 1 +} func (m XValidation) ApplyToSchema(schema *apiext.JSONSchemaProps) error { schema.XValidations = append(schema.XValidations, apiext.ValidationRule{ diff --git a/pkg/crd/schema.go b/pkg/crd/schema.go index e76d3ea88..a5c2f28c9 100644 --- a/pkg/crd/schema.go +++ b/pkg/crd/schema.go @@ -22,6 +22,7 @@ import ( "go/ast" "go/token" "go/types" + "sort" "strings" apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" @@ -124,41 +125,45 @@ func infoToSchema(ctx *schemaContext) *apiext.JSONSchemaProps { return typeToSchema(ctx, ctx.info.RawSpec.Type) } -// applyMarkers applies schema markers to the given schema, respecting "apply first" markers. +// applyMarkers applies schema markers given their priority to the given schema func applyMarkers(ctx *schemaContext, markerSet markers.MarkerValues, props *apiext.JSONSchemaProps, node ast.Node) { - // apply "apply first" markers first... + markers := make([]SchemaMarker, 0, len(markerSet)) + for _, markerValues := range markerSet { for _, markerValue := range markerValues { - if _, isApplyFirst := markerValue.(applyFirstMarker); !isApplyFirst { - continue + if schemaMarker, isSchemaMarker := markerValue.(SchemaMarker); isSchemaMarker { + markers = append(markers, schemaMarker) } + } + } - schemaMarker, isSchemaMarker := markerValue.(SchemaMarker) - if !isSchemaMarker { - continue - } + sort.Slice(markers, func(i, j int) bool { + var iPriority, jPriority crdmarkers.ApplyPriority - if err := schemaMarker.ApplyToSchema(props); err != nil { - ctx.pkg.AddError(loader.ErrFromNode(err /* an okay guess */, node)) - } + switch m := markers[i].(type) { + case crdmarkers.ApplyPriorityMarker: + iPriority = m.ApplyPriority() + case applyFirstMarker: + iPriority = crdmarkers.ApplyPriorityFirst + default: + iPriority = crdmarkers.ApplyPriorityDefault } - } - // ...then the rest of the markers - for _, markerValues := range markerSet { - for _, markerValue := range markerValues { - if _, isApplyFirst := markerValue.(applyFirstMarker); isApplyFirst { - // skip apply-first markers, which were already applied - continue - } + switch m := markers[j].(type) { + case crdmarkers.ApplyPriorityMarker: + jPriority = m.ApplyPriority() + case applyFirstMarker: + jPriority = crdmarkers.ApplyPriorityFirst + default: + jPriority = crdmarkers.ApplyPriorityDefault + } - schemaMarker, isSchemaMarker := markerValue.(SchemaMarker) - if !isSchemaMarker { - continue - } - if err := schemaMarker.ApplyToSchema(props); err != nil { - ctx.pkg.AddError(loader.ErrFromNode(err /* an okay guess */, node)) - } + return iPriority < jPriority + }) + + for _, schemaMarker := range markers { + if err := schemaMarker.ApplyToSchema(props); err != nil { + ctx.pkg.AddError(loader.ErrFromNode(err /* an okay guess */, node)) } } } diff --git a/pkg/crd/schema_test.go b/pkg/crd/schema_test.go index 5b8fa03fc..9c0581d6f 100644 --- a/pkg/crd/schema_test.go +++ b/pkg/crd/schema_test.go @@ -25,6 +25,7 @@ import ( "golang.org/x/tools/go/packages" pkgstest "golang.org/x/tools/go/packages/packagestest" apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + crdmarkers "sigs.k8s.io/controller-tools/pkg/crd/markers" testloader "sigs.k8s.io/controller-tools/pkg/loader/testutils" "sigs.k8s.io/controller-tools/pkg/markers" ) @@ -110,3 +111,68 @@ func Test_Schema_MapOfStringToArrayOfFloat32(t *testing.T) { }, })) } + +func Test_Schema_ApplyMarkers(t *testing.T) { + g := gomega.NewWithT(t) + + props := &apiext.JSONSchemaProps{} + ctx := &schemaContext{} + + var invocations []string + + applyMarkers(ctx, markers.MarkerValues{ + "blah": []interface{}{ + &testPriorityMarker{ + priority: 0, callback: func() { + invocations = append(invocations, "0") + }, + }, + &testPriorityMarker{priority: 2, callback: func() { + invocations = append(invocations, "2") + }}, + &testPriorityMarker{priority: 11, callback: func() { + invocations = append(invocations, "11") + }}, + &defaultPriorityMarker{callback: func() { + invocations = append(invocations, "default") + }}, + &testapplyFirstMarker{callback: func() { + invocations = append(invocations, "applyFirst") + }}, + }}, props, nil) + + g.Expect(invocations).To(gomega.Equal([]string{"0", "applyFirst", "2", "default", "11"})) +} + +type defaultPriorityMarker struct { + callback func() +} + +func (m *defaultPriorityMarker) ApplyToSchema(*apiext.JSONSchemaProps) error { + m.callback() + return nil +} + +type testPriorityMarker struct { + priority crdmarkers.ApplyPriority + callback func() +} + +func (m *testPriorityMarker) ApplyPriority() crdmarkers.ApplyPriority { + return m.priority +} + +func (m *testPriorityMarker) ApplyToSchema(*apiext.JSONSchemaProps) error { + m.callback() + return nil +} + +type testapplyFirstMarker struct { + callback func() +} + +func (m *testapplyFirstMarker) ApplyFirst() {} +func (m *testapplyFirstMarker) ApplyToSchema(*apiext.JSONSchemaProps) error { + m.callback() + return nil +}