From 8df41544461be3ff4cf72aed6f79611ecfda94cc Mon Sep 17 00:00:00 2001 From: dprotaso Date: Fri, 15 Jul 2022 12:59:46 -0400 Subject: [PATCH 01/11] Markers can now indicate their priority when applying --- pkg/crd/markers/doc.go | 4 +-- pkg/crd/markers/priority.go | 37 ++++++++++++++++++++++++ pkg/crd/markers/topology.go | 4 ++- pkg/crd/markers/validation.go | 8 ++++-- pkg/crd/schema.go | 54 +++++++++++++++++++---------------- 5 files changed, 78 insertions(+), 29 deletions(-) create mode 100644 pkg/crd/markers/priority.go diff --git a/pkg/crd/markers/doc.go b/pkg/crd/markers/doc.go index 48736401c..8ce4a4266 100644 --- a/pkg/crd/markers/doc.go +++ b/pkg/crd/markers/doc.go @@ -25,8 +25,8 @@ limitations under the License. // (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. +// implement ApplyFirst or markers.ApplyPriority, but this is discouraged +// and may change in the future. // // All validation markers start with "+kubebuilder:validation", and // have the same name as their type name. diff --git a/pkg/crd/markers/priority.go b/pkg/crd/markers/priority.go new file mode 100644 index 000000000..3e4df7c44 --- /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 + +import "math" + +type ApplyPriority int64 + +const ( + // ApplyPriorityDefault is the default priority for markers + // that don't implement ApplyPriorityMarker + ApplyPriorityDefault ApplyPriority = math.MaxInt64 >> 2 + + // ApplyPriorityFirst is the priority value assigned to markers + // that implement the ApplyFirst() method + ApplyPriorityFirst ApplyPriority = ApplyPriorityDefault - 10 +) + +// ApplyPriorityMarker designates the order 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..580a93364 100644 --- a/pkg/crd/markers/topology.go +++ b/pkg/crd/markers/topology.go @@ -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..63d1f5bbe 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" @@ -126,39 +127,44 @@ func infoToSchema(ctx *schemaContext) *apiext.JSONSchemaProps { // applyMarkers applies schema markers to the given schema, respecting "apply first" markers. func applyMarkers(ctx *schemaContext, markerSet markers.MarkerValues, props *apiext.JSONSchemaProps, node ast.Node) { + markers := make([]SchemaMarker, 0, len(markerSet)) + // apply "apply first" markers first... 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)) } } } From 2a3e4d97b9e434171bbb29d08fbc484e3d909f2d Mon Sep 17 00:00:00 2001 From: dprotaso Date: Fri, 15 Jul 2022 14:44:48 -0400 Subject: [PATCH 02/11] add unit test to ensure marker invocation --- pkg/crd/schema_test.go | 57 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/pkg/crd/schema_test.go b/pkg/crd/schema_test.go index 5b8fa03fc..cb9dd9909 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,59 @@ 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: 1, callback: func() { + invocations = append(invocations, "1") + + }}, + &testapplyFirstMarker{callback: func() { + invocations = append(invocations, "applyFirst") + }}, + &testPriorityMarker{priority: crdmarkers.ApplyPriorityDefault, callback: func() { + invocations = append(invocations, "default") + + }}, + }}, props, nil) + + g.Expect(invocations).To(gomega.Equal([]string{"0", "1", "applyFirst", "default"})) +} + +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 { + priority crdmarkers.ApplyPriority + callback func() +} + +func (m *testapplyFirstMarker) ApplyFirst() {} +func (m *testapplyFirstMarker) ApplyToSchema(*apiext.JSONSchemaProps) error { + m.callback() + return nil +} From 4710a9238aa695e2caa78ba8423f10cc380692cd Mon Sep 17 00:00:00 2001 From: dprotaso Date: Mon, 5 Dec 2022 13:11:06 -0500 Subject: [PATCH 03/11] add example in godoc --- pkg/crd/markers/doc.go | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/pkg/crd/markers/doc.go b/pkg/crd/markers/doc.go index 8ce4a4266..2f238da63 100644 --- a/pkg/crd/markers/doc.go +++ b/pkg/crd/markers/doc.go @@ -25,13 +25,25 @@ limitations under the License. // (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 or markers.ApplyPriority, but this is discouraged -// and may change in the future. +// implement ApplyFirst, but this is discouraged and may change +// in the future. It is recommended to implement the ApplyPriority +// interface in combination with ApplyPriorityDefault and +// ApplyPriorityFirst constants. // +// 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 @@ -39,7 +51,7 @@ limitations under the License. // 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. From 43c8282e8e4a89bfe56ef0e88408a8297a422adb Mon Sep 17 00:00:00 2001 From: dprotaso Date: Mon, 5 Dec 2022 13:16:07 -0500 Subject: [PATCH 04/11] drop unused struct param --- pkg/crd/schema_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/crd/schema_test.go b/pkg/crd/schema_test.go index cb9dd9909..0ce209cb5 100644 --- a/pkg/crd/schema_test.go +++ b/pkg/crd/schema_test.go @@ -158,7 +158,6 @@ func (m *testPriorityMarker) ApplyToSchema(*apiext.JSONSchemaProps) error { } type testapplyFirstMarker struct { - priority crdmarkers.ApplyPriority callback func() } From bc48feb1d09147d44462dcbfaacb254b3694c4ba Mon Sep 17 00:00:00 2001 From: dprotaso Date: Tue, 6 Dec 2022 12:08:23 -0500 Subject: [PATCH 05/11] update godoc --- pkg/crd/markers/doc.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/crd/markers/doc.go b/pkg/crd/markers/doc.go index 2f238da63..5f1ecd05f 100644 --- a/pkg/crd/markers/doc.go +++ b/pkg/crd/markers/doc.go @@ -19,7 +19,7 @@ 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 @@ -28,7 +28,8 @@ limitations under the License. // implement ApplyFirst, but this is discouraged and may change // in the future. It is recommended to implement the ApplyPriority // interface in combination with ApplyPriorityDefault and -// ApplyPriorityFirst constants. +// ApplyPriorityFirst constants. Following is an example of how to +// implement such a marker: // // type MyCustomMarker string // @@ -39,7 +40,7 @@ limitations under the License. // func (m MyCustomMarker) ApplyToSchema(schema *apiext.JSONSchemaProps) error { // ... // } - +// // All validation markers start with "+kubebuilder:validation", and // have the same name as their type name. // From 11c6121dd5fa71d6766a6fb427a8f41f1a47e605 Mon Sep 17 00:00:00 2001 From: dprotaso Date: Tue, 6 Dec 2022 12:10:41 -0500 Subject: [PATCH 06/11] run goimports --- pkg/crd/markers/doc.go | 14 +++++++------- pkg/crd/markers/topology.go | 34 +++++++++++++++++----------------- 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/pkg/crd/markers/doc.go b/pkg/crd/markers/doc.go index 5f1ecd05f..14f36b41d 100644 --- a/pkg/crd/markers/doc.go +++ b/pkg/crd/markers/doc.go @@ -31,15 +31,15 @@ limitations under the License. // ApplyPriorityFirst constants. Following is an example of how to // implement such a marker: // -// type MyCustomMarker string +// type MyCustomMarker string // -// func (m MyCustomMarker) ApplyPriority() ApplyPriority { -// return ApplyPriorityFirst -// } +// func (m MyCustomMarker) ApplyPriority() ApplyPriority { +// return ApplyPriorityFirst +// } // -// func (m MyCustomMarker) ApplyToSchema(schema *apiext.JSONSchemaProps) error { -// ... -// } +// func (m MyCustomMarker) ApplyToSchema(schema *apiext.JSONSchemaProps) error { +// ... +// } // // All validation markers start with "+kubebuilder:validation", and // have the same name as their type name. diff --git a/pkg/crd/markers/topology.go b/pkg/crd/markers/topology.go index 580a93364..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 { From 0db0ef20f77c4300804b82e9a89ccdf5b3ddc4f5 Mon Sep 17 00:00:00 2001 From: dprotaso Date: Mon, 29 May 2023 09:43:38 -0400 Subject: [PATCH 07/11] change marker constants --- pkg/crd/markers/priority.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/pkg/crd/markers/priority.go b/pkg/crd/markers/priority.go index 3e4df7c44..f6b4fbaa6 100644 --- a/pkg/crd/markers/priority.go +++ b/pkg/crd/markers/priority.go @@ -16,18 +16,16 @@ limitations under the License. package markers -import "math" - type ApplyPriority int64 const ( // ApplyPriorityDefault is the default priority for markers // that don't implement ApplyPriorityMarker - ApplyPriorityDefault ApplyPriority = math.MaxInt64 >> 2 + ApplyPriorityDefault ApplyPriority = 10 // ApplyPriorityFirst is the priority value assigned to markers // that implement the ApplyFirst() method - ApplyPriorityFirst ApplyPriority = ApplyPriorityDefault - 10 + ApplyPriorityFirst ApplyPriority = 1 ) // ApplyPriorityMarker designates the order markers should be applied. From 08da28be3ee407d258e0aed52f2b8d23e26cc869 Mon Sep 17 00:00:00 2001 From: dprotaso Date: Mon, 29 May 2023 09:44:57 -0400 Subject: [PATCH 08/11] add godoc comment --- pkg/crd/markers/priority.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/crd/markers/priority.go b/pkg/crd/markers/priority.go index f6b4fbaa6..d334368bc 100644 --- a/pkg/crd/markers/priority.go +++ b/pkg/crd/markers/priority.go @@ -16,6 +16,8 @@ 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 ( From 852fc908966207b86f8ba8e62fdcf993c16ac86c Mon Sep 17 00:00:00 2001 From: dprotaso Date: Thu, 25 Jan 2024 14:57:17 -0500 Subject: [PATCH 09/11] address feedback --- pkg/crd/schema.go | 3 +-- pkg/crd/schema_test.go | 26 ++++++++++++++++++-------- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/pkg/crd/schema.go b/pkg/crd/schema.go index 63d1f5bbe..a5c2f28c9 100644 --- a/pkg/crd/schema.go +++ b/pkg/crd/schema.go @@ -125,11 +125,10 @@ 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) { markers := make([]SchemaMarker, 0, len(markerSet)) - // apply "apply first" markers first... for _, markerValues := range markerSet { for _, markerValue := range markerValues { if schemaMarker, isSchemaMarker := markerValue.(SchemaMarker); isSchemaMarker { diff --git a/pkg/crd/schema_test.go b/pkg/crd/schema_test.go index 0ce209cb5..9c0581d6f 100644 --- a/pkg/crd/schema_test.go +++ b/pkg/crd/schema_test.go @@ -127,20 +127,30 @@ func Test_Schema_ApplyMarkers(t *testing.T) { invocations = append(invocations, "0") }, }, - &testPriorityMarker{priority: 1, callback: func() { - invocations = append(invocations, "1") - + &testPriorityMarker{priority: 2, callback: func() { + invocations = append(invocations, "2") }}, - &testapplyFirstMarker{callback: func() { - invocations = append(invocations, "applyFirst") + &testPriorityMarker{priority: 11, callback: func() { + invocations = append(invocations, "11") }}, - &testPriorityMarker{priority: crdmarkers.ApplyPriorityDefault, callback: func() { + &defaultPriorityMarker{callback: func() { invocations = append(invocations, "default") - + }}, + &testapplyFirstMarker{callback: func() { + invocations = append(invocations, "applyFirst") }}, }}, props, nil) - g.Expect(invocations).To(gomega.Equal([]string{"0", "1", "applyFirst", "default"})) + 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 { From f9cffe33a9ddc9b595847323ddd84ca5c177f314 Mon Sep 17 00:00:00 2001 From: dprotaso Date: Wed, 31 Jan 2024 13:01:52 -0500 Subject: [PATCH 10/11] add comment that ordering is for validation markers for now --- pkg/crd/markers/priority.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/crd/markers/priority.go b/pkg/crd/markers/priority.go index d334368bc..1b4482521 100644 --- a/pkg/crd/markers/priority.go +++ b/pkg/crd/markers/priority.go @@ -30,7 +30,7 @@ const ( ApplyPriorityFirst ApplyPriority = 1 ) -// ApplyPriorityMarker designates the order markers should be applied. +// ApplyPriorityMarker designates the order validation markers should be applied. // Lower priority indicates it should be applied first type ApplyPriorityMarker interface { ApplyPriority() ApplyPriority From 239ff67a447356eb4be275ec7feb2ecb3e695271 Mon Sep 17 00:00:00 2001 From: dprotaso Date: Wed, 31 Jan 2024 13:02:24 -0500 Subject: [PATCH 11/11] fix godoc comment --- pkg/crd/markers/doc.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/crd/markers/doc.go b/pkg/crd/markers/doc.go index 14f36b41d..f01e9f1b3 100644 --- a/pkg/crd/markers/doc.go +++ b/pkg/crd/markers/doc.go @@ -47,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.