✨ 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

@@ -26,15 +26,28 @@ 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.
 //
 // # 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.

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

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

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{

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

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