From b67d9438233b0666bc09de32e4ca5a580ed28d26 Mon Sep 17 00:00:00 2001 From: Dave Protasowski Date: Mon, 5 Feb 2024 05:35:45 -0500 Subject: [PATCH] :sparkles: Markers can now indicate their priority when applying (#706) * Markers can now indicate their priority when applying * add unit test to ensure marker invocation * add example in godoc * drop unused struct param * update godoc * run goimports * change marker constants * add godoc comment * address feedback * add comment that ordering is for validation markers for now * fix godoc comment --- pkg/crd/markers/doc.go | 17 +++++++-- pkg/crd/markers/priority.go | 37 ++++++++++++++++++++ pkg/crd/markers/topology.go | 4 ++- pkg/crd/markers/validation.go | 8 +++-- pkg/crd/schema.go | 57 ++++++++++++++++-------------- pkg/crd/schema_test.go | 66 +++++++++++++++++++++++++++++++++++ 6 files changed, 158 insertions(+), 31 deletions(-) create mode 100644 pkg/crd/markers/priority.go diff --git a/pkg/crd/markers/doc.go b/pkg/crd/markers/doc.go index 995af44b3..f01e9f1b3 100644 --- a/pkg/crd/markers/doc.go +++ b/pkg/crd/markers/doc.go @@ -26,7 +26,20 @@ limitations under the License. // 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. @@ -34,7 +47,7 @@ limitations under the License. // # 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. 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 a92995c80..97dbc47c5 100644 --- a/pkg/crd/markers/topology.go +++ b/pkg/crd/markers/topology.go @@ -119,7 +119,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 40aec5f94..48c89e6d0 100644 --- a/pkg/crd/markers/validation.go +++ b/pkg/crd/markers/validation.go @@ -464,7 +464,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 @@ -512,7 +514,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 +}