✨ Markers can now indicate their priority when applying

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
 // 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.

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
+}

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" {

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{

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))
 		}
 	}
 }

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,58 @@ 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 {
+	callback func()
+}
+
+func (m *testapplyFirstMarker) ApplyFirst() {}
+func (m *testapplyFirstMarker) ApplyToSchema(*apiext.JSONSchemaProps) error {
+	m.callback()
+	return nil
+}