Tolerate only the two allowed field changes

Signed-off-by: Mike Spreitzer <mspreitz@us.ibm.com>

pkg/registry/flowcontrol/ensurer/prioritylevelconfiguration.go

@@ -26,7 +26,7 @@ import (
 
 func NewPriorityLevelConfigurationOps(client flowcontrolclient.PriorityLevelConfigurationInterface, lister flowcontrollisters.PriorityLevelConfigurationLister) ObjectOps[*flowcontrolv1beta3.PriorityLevelConfiguration] {
 	return NewObjectOps[*flowcontrolv1beta3.PriorityLevelConfiguration](client, lister, (*flowcontrolv1beta3.PriorityLevelConfiguration).DeepCopy,
-		plcReplaceSpec, plcSpecEqual)
+		plcReplaceSpec, plcSpecEqualish)
 }
 
 func plcReplaceSpec(into, from *flowcontrolv1beta3.PriorityLevelConfiguration) *flowcontrolv1beta3.PriorityLevelConfiguration {
@@ -35,8 +35,15 @@ func plcReplaceSpec(into, from *flowcontrolv1beta3.PriorityLevelConfiguration) *
 	return copy
 }
 
-func plcSpecEqual(expected, actual *flowcontrolv1beta3.PriorityLevelConfiguration) bool {
+func plcSpecEqualish(expected, actual *flowcontrolv1beta3.PriorityLevelConfiguration) bool {
 	copiedExpected := expected.DeepCopy()
 	flowcontrolapisv1beta3.SetObjectDefaults_PriorityLevelConfiguration(copiedExpected)
+	if expected.Name == flowcontrolv1beta3.PriorityLevelConfigurationNameExempt {
+		if actual.Spec.Exempt == nil {
+			return false
+		}
+		copiedExpected.Spec.Exempt.NominalConcurrencyShares = actual.Spec.Exempt.NominalConcurrencyShares
+		copiedExpected.Spec.Exempt.LendablePercent = actual.Spec.Exempt.LendablePercent
+	}
 	return equality.Semantic.DeepEqual(copiedExpected.Spec, actual.Spec)
 }

pkg/registry/flowcontrol/ensurer/prioritylevelconfiguration_test.go

@@ -37,7 +37,7 @@ import (
 func TestEnsurePriorityLevel(t *testing.T) {
 	validExemptPL := func() *flowcontrolv1beta3.PriorityLevelConfiguration {
 		copy := bootstrap.MandatoryPriorityLevelConfigurationExempt.DeepCopy()
-		copy.Annotations[flowcontrolv1beta3.AutoUpdateAnnotationKey] = "false"
+		copy.Annotations[flowcontrolv1beta3.AutoUpdateAnnotationKey] = "true"
 		copy.Spec.Exempt.NominalConcurrencyShares = pointer.Int32(10)
 		copy.Spec.Exempt.LendablePercent = pointer.Int32(50)
 		return copy
@@ -281,6 +281,41 @@ func TestPriorityLevelSpecChanged(t *testing.T) {
 			},
 		},
 	}
+	ple1 := &flowcontrolv1beta3.PriorityLevelConfiguration{
+		ObjectMeta: metav1.ObjectMeta{Name: "exempt"},
+		Spec: flowcontrolv1beta3.PriorityLevelConfigurationSpec{
+			Type: flowcontrolv1beta3.PriorityLevelEnablementExempt,
+			Exempt: &flowcontrolv1beta3.ExemptPriorityLevelConfiguration{
+				NominalConcurrencyShares: pointer.Int32(42),
+				LendablePercent:          pointer.Int32(33),
+			},
+		},
+	}
+	ple2 := &flowcontrolv1beta3.PriorityLevelConfiguration{
+		ObjectMeta: metav1.ObjectMeta{Name: "exempt"},
+		Spec: flowcontrolv1beta3.PriorityLevelConfigurationSpec{
+			Type: flowcontrolv1beta3.PriorityLevelEnablementExempt,
+			Exempt: &flowcontrolv1beta3.ExemptPriorityLevelConfiguration{
+				NominalConcurrencyShares: pointer.Int32(24),
+				LendablePercent:          pointer.Int32(86),
+			},
+		},
+	}
+	pleWrong := &flowcontrolv1beta3.PriorityLevelConfiguration{
+		ObjectMeta: metav1.ObjectMeta{Name: "exempt"},
+		Spec: flowcontrolv1beta3.PriorityLevelConfigurationSpec{
+			Type: flowcontrolv1beta3.PriorityLevelEnablementLimited,
+			Limited: &flowcontrolv1beta3.LimitedPriorityLevelConfiguration{
+				NominalConcurrencyShares: 1,
+			},
+		},
+	}
+	pleInvalid := &flowcontrolv1beta3.PriorityLevelConfiguration{
+		ObjectMeta: metav1.ObjectMeta{Name: "exempt"},
+		Spec: flowcontrolv1beta3.PriorityLevelConfigurationSpec{
+			Type: "widget",
+		},
+	}
 	testCases := []struct {
 		name        string
 		expected    *flowcontrolv1beta3.PriorityLevelConfiguration
@@ -305,10 +340,28 @@ func TestPriorityLevelSpecChanged(t *testing.T) {
 			actual:      pl2,
 			specChanged: true,
 		},
+		{
+			name:        "tweaked exempt config",
+			expected:    ple1,
+			actual:      ple2,
+			specChanged: false,
+		},
+		{
+			name:        "exempt with wrong tag",
+			expected:    ple1,
+			actual:      pleWrong,
+			specChanged: true,
+		},
+		{
+			name:        "exempt with invalid tag",
+			expected:    ple1,
+			actual:      pleInvalid,
+			specChanged: true,
+		},
 	}
 	for _, testCase := range testCases {
 		t.Run(testCase.name, func(t *testing.T) {
-			w := !plcSpecEqual(testCase.expected, testCase.actual)
+			w := !plcSpecEqualish(testCase.expected, testCase.actual)
 			if testCase.specChanged != w {
 				t.Errorf("Expected priorityLevelSpecChanged to return %t, but got: %t - diff: %s", testCase.specChanged, w,
 					cmp.Diff(testCase.expected, testCase.actual))

pkg/registry/flowcontrol/ensurer/strategy.go

@@ -69,8 +69,10 @@ type objectLocalOps[ObjectType configurationObject] interface {
 	// replaceSpec returns a deep copy of `into` except that the spec is a deep copy of `from`
 	ReplaceSpec(into, from ObjectType) ObjectType
 
-	// specEqual says whether applying defaulting to `expected` makes its spec equal that of `actual`
+	// SpecEqualish says whether applying defaulting to `expected`
-	SpecEqual(expected, actual ObjectType) bool
+	// makes its spec more or less equal (as appropriate for the
+	// object at hand) that of `actual`.
+	SpecEqualish(expected, actual ObjectType) bool
 }
 
 // ObjectOps is the needed operations, both as a receiver from a server and server-independent, on configurationObjects
@@ -109,21 +111,21 @@ type configurationObjectType interface {
 type objectOps[ObjectType configurationObjectType] struct {
 	client[ObjectType]
 	cache[ObjectType]
-	deepCopy    func(ObjectType) ObjectType
+	deepCopy     func(ObjectType) ObjectType
-	replaceSpec func(ObjectType, ObjectType) ObjectType
+	replaceSpec  func(ObjectType, ObjectType) ObjectType
-	specEqual   func(expected, actual ObjectType) bool
+	specEqualish func(expected, actual ObjectType) bool
 }
 
 func NewObjectOps[ObjectType configurationObjectType](client client[ObjectType], cache cache[ObjectType],
 	deepCopy func(ObjectType) ObjectType,
 	replaceSpec func(ObjectType, ObjectType) ObjectType,
-	specEqual func(expected, actual ObjectType) bool,
+	specEqualish func(expected, actual ObjectType) bool,
 ) ObjectOps[ObjectType] {
 	return objectOps[ObjectType]{client: client,
-		cache:       cache,
+		cache:        cache,
-		deepCopy:    deepCopy,
+		deepCopy:     deepCopy,
-		replaceSpec: replaceSpec,
+		replaceSpec:  replaceSpec,
-		specEqual:   specEqual}
+		specEqualish: specEqualish}
 }
 
 func (oo objectOps[ObjectType]) DeepCopy(obj ObjectType) ObjectType { return oo.deepCopy(obj) }
@@ -132,39 +134,30 @@ func (oo objectOps[ObjectType]) ReplaceSpec(into, from ObjectType) ObjectType {
 	return oo.replaceSpec(into, from)
 }
 
-func (oo objectOps[ObjectType]) SpecEqual(expected, actual ObjectType) bool {
+func (oo objectOps[ObjectType]) SpecEqualish(expected, actual ObjectType) bool {
-	return oo.specEqual(expected, actual)
+	return oo.specEqualish(expected, actual)
 }
 
 // NewSuggestedEnsureStrategy returns an EnsureStrategy for suggested config objects
 func NewSuggestedEnsureStrategy[ObjectType configurationObjectType]() EnsureStrategy[ObjectType] {
 	return &strategy[ObjectType]{
-		alwaysAutoUpdateSpecFn: func(want, have ObjectType) bool {
+		alwaysAutoUpdateSpec: false,
-			return false
+		name:                 "suggested",
-		},
-		name: "suggested",
 	}
 }
 
 // NewMandatoryEnsureStrategy returns an EnsureStrategy for mandatory config objects
 func NewMandatoryEnsureStrategy[ObjectType configurationObjectType]() EnsureStrategy[ObjectType] {
 	return &strategy[ObjectType]{
-		alwaysAutoUpdateSpecFn: func(want, have ObjectType) bool {
+		alwaysAutoUpdateSpec: true,
-			if want.GetName() == flowcontrolv1beta3.PriorityLevelConfigurationNameExempt {
+		name:                 "mandatory",
-				// we allow the cluster operator to update the spec of the
-				// singleton 'exempt' prioritylevelconfiguration object.
-				return false
-			}
-			return true
-		},
-		name: "mandatory",
 	}
 }
 
 // auto-update strategy for the configuration objects
 type strategy[ObjectType configurationObjectType] struct {
-	alwaysAutoUpdateSpecFn func(want, have ObjectType) bool
+	alwaysAutoUpdateSpec bool
-	name                   string
+	name                 string
 }
 
 func (s *strategy[ObjectType]) Name() string {
@@ -177,14 +170,13 @@ func (s *strategy[ObjectType]) ReviseIfNeeded(objectOps objectLocalOps[ObjectTyp
 		return zero, false, nil
 	}
 
-	autoUpdateSpec := s.alwaysAutoUpdateSpecFn(bootstrap, current)
+	autoUpdateSpec := s.alwaysAutoUpdateSpec
 	if !autoUpdateSpec {
 		autoUpdateSpec = shouldUpdateSpec(current)
 	}
 	updateAnnotation := shouldUpdateAnnotation(current, autoUpdateSpec)
 
-	// specChanged := autoUpdateSpec && wah.specsDiffer()
+	specChanged := autoUpdateSpec && !objectOps.SpecEqualish(bootstrap, current)
-	specChanged := autoUpdateSpec && !objectOps.SpecEqual(bootstrap, current)
 
 	if !(updateAnnotation || specChanged) {
 		// the annotation key is up to date and the spec has not changed, no update is necessary