Modify the wrong ConfigMap name in v1.13 node-agent-concurrency docum…

…ent. (#7715) Fix condition matching in resource modifier when there are multiple rules Signed-off-by: lou <[email protected]> Co-authored-by: Xun Jiang <[email protected]>
vmware-tanzu · May 14, 2024 · 6c2b66b · 6c2b66b
1 parent 27392d3
commit 6c2b66b
Show file tree

Hide file tree

Showing 3 changed files with 145 additions and 17 deletions.
diff --git a/changelogs/unreleased/7715-27149chen b/changelogs/unreleased/7715-27149chen
@@ -0,0 +1 @@
+Fix condition matching in resource modifier when there are multiple rules
diff --git a/internal/resourcemodifiers/resource_modifiers.go b/internal/resourcemodifiers/resource_modifiers.go
@@ -86,8 +86,22 @@ func GetResourceModifiersFromConfig(cm *v1.ConfigMap) (*ResourceModifiers, error
 
 func (p *ResourceModifiers) ApplyResourceModifierRules(obj *unstructured.Unstructured, groupResource string, scheme *runtime.Scheme, log logrus.FieldLogger) []error {
 	var errs []error
+	origin := obj
+	// If there are more than one rules, we need to keep the original object for condition matching
+	if len(p.ResourceModifierRules) > 1 {
+		origin = obj.DeepCopy()
+	}
 	for _, rule := range p.ResourceModifierRules {
-		err := rule.apply(obj, groupResource, scheme, log)
+		matched, err := rule.match(origin, groupResource, log)
+		if err != nil {
+			errs = append(errs, err)
+			continue
+		} else if !matched {
+			continue
+		}
+
+		log.Infof("Applying resource modifier patch on %s/%s", origin.GetNamespace(), origin.GetName())
+		err = rule.applyPatch(obj, scheme, log)
 		if err != nil {
 			errs = append(errs, err)
 		}
@@ -96,59 +110,54 @@ func (p *ResourceModifiers) ApplyResourceModifierRules(obj *unstructured.Unstruc
 	return errs
 }
 
-func (r *ResourceModifierRule) apply(obj *unstructured.Unstructured, groupResource string, scheme *runtime.Scheme, log logrus.FieldLogger) error {
+func (r *ResourceModifierRule) match(obj *unstructured.Unstructured, groupResource string, log logrus.FieldLogger) (bool, error) {
 	ns := obj.GetNamespace()
 	if ns != "" {
 		namespaceInclusion := collections.NewIncludesExcludes().Includes(r.Conditions.Namespaces...)
 		if !namespaceInclusion.ShouldInclude(ns) {
-			return nil
+			return false, nil
 		}
 	}
 
 	g, err := glob.Compile(r.Conditions.GroupResource, '.')
 	if err != nil {
 		log.Errorf("Bad glob pattern of groupResource in condition, groupResource: %s, err: %s", r.Conditions.GroupResource, err)
-		return err
+		return false, err
 	}
 
 	if !g.Match(groupResource) {
-		return nil
+		return false, nil
 	}
 
 	if r.Conditions.ResourceNameRegex != "" {
 		match, err := regexp.MatchString(r.Conditions.ResourceNameRegex, obj.GetName())
 		if err != nil {
-			return errors.Errorf("error in matching regex %s", err.Error())
+			return false, errors.Errorf("error in matching regex %s", err.Error())
 		}
 		if !match {
-			return nil
+			return false, nil
 		}
 	}
 
 	if r.Conditions.LabelSelector != nil {
 		selector, err := metav1.LabelSelectorAsSelector(r.Conditions.LabelSelector)
 		if err != nil {
-			return errors.Errorf("error in creating label selector %s", err.Error())
+			return false, errors.Errorf("error in creating label selector %s", err.Error())
 		}
 		if !selector.Matches(labels.Set(obj.GetLabels())) {
-			return nil
+			return false, nil
 		}
 	}
 
 	match, err := matchConditions(obj, r.Conditions.Matches, log)
 	if err != nil {
-		return err
+		return false, err
 	} else if !match {
 		log.Info("Conditions do not match, skip it")
-		return nil
+		return false, nil
 	}
 
-	log.Infof("Applying resource modifier patch on %s/%s", obj.GetNamespace(), obj.GetName())
-	err = r.applyPatch(obj, scheme, log)
-	if err != nil {
-		return err
-	}
-	return nil
+	return true, nil
 }
 
 func matchConditions(u *unstructured.Unstructured, rules []MatchRule, _ logrus.FieldLogger) (bool, error) {

diff --git a/internal/resourcemodifiers/resource_modifiers_test.go b/internal/resourcemodifiers/resource_modifiers_test.go
@@ -456,6 +456,20 @@ func TestResourceModifiers_ApplyResourceModifierRules(t *testing.T) {
 		},
 	}
 
+	pvcGoldSc := &unstructured.Unstructured{
+		Object: map[string]interface{}{
+			"apiVersion": "v1",
+			"kind":       "PersistentVolumeClaim",
+			"metadata": map[string]interface{}{
+				"name":      "test-pvc",
+				"namespace": "foo",
+			},
+			"spec": map[string]interface{}{
+				"storageClassName": "gold",
+			},
+		},
+	}
+
 	deployNginxOneReplica := &unstructured.Unstructured{
 		Object: map[string]interface{}{
 			"apiVersion": "apps/v1",
@@ -679,6 +693,110 @@ func TestResourceModifiers_ApplyResourceModifierRules(t *testing.T) {
 			wantErr: false,
 			wantObj: pvcPremiumSc.DeepCopy(),
 		},
+		{
+			name: "pvc with standard storage class should be patched to premium, even when rules are [standard => premium, premium => gold]",
+			fields: fields{
+				Version: "v1",
+				ResourceModifierRules: []ResourceModifierRule{
+					{
+						Conditions: Conditions{
+							GroupResource:     "persistentvolumeclaims",
+							ResourceNameRegex: ".*",
+							Matches: []MatchRule{
+								{
+									Path:  "/spec/storageClassName",
+									Value: "standard",
+								},
+							},
+						},
+						Patches: []JSONPatch{
+							{
+								Operation: "replace",
+								Path:      "/spec/storageClassName",
+								Value:     "premium",
+							},
+						},
+					},
+					{
+						Conditions: Conditions{
+							GroupResource:     "persistentvolumeclaims",
+							ResourceNameRegex: ".*",
+							Matches: []MatchRule{
+								{
+									Path:  "/spec/storageClassName",
+									Value: "premium",
+								},
+							},
+						},
+						Patches: []JSONPatch{
+							{
+								Operation: "replace",
+								Path:      "/spec/storageClassName",
+								Value:     "gold",
+							},
+						},
+					},
+				},
+			},
+			args: args{
+				obj:           pvcStandardSc.DeepCopy(),
+				groupResource: "persistentvolumeclaims",
+			},
+			wantErr: false,
+			wantObj: pvcPremiumSc.DeepCopy(),
+		},
+		{
+			name: "pvc with standard storage class should be patched to gold, even when rules are [standard => premium, standard => gold]",
+			fields: fields{
+				Version: "v1",
+				ResourceModifierRules: []ResourceModifierRule{
+					{
+						Conditions: Conditions{
+							GroupResource:     "persistentvolumeclaims",
+							ResourceNameRegex: ".*",
+							Matches: []MatchRule{
+								{
+									Path:  "/spec/storageClassName",
+									Value: "standard",
+								},
+							},
+						},
+						Patches: []JSONPatch{
+							{
+								Operation: "replace",
+								Path:      "/spec/storageClassName",
+								Value:     "premium",
+							},
+						},
+					},
+					{
+						Conditions: Conditions{
+							GroupResource:     "persistentvolumeclaims",
+							ResourceNameRegex: ".*",
+							Matches: []MatchRule{
+								{
+									Path:  "/spec/storageClassName",
+									Value: "standard",
+								},
+							},
+						},
+						Patches: []JSONPatch{
+							{
+								Operation: "replace",
+								Path:      "/spec/storageClassName",
+								Value:     "gold",
+							},
+						},
+					},
+				},
+			},
+			args: args{
+				obj:           pvcStandardSc.DeepCopy(),
+				groupResource: "persistentvolumeclaims",
+			},
+			wantErr: false,
+			wantObj: pvcGoldSc.DeepCopy(),
+		},
 		{
 			name: "nginx deployment: 1 -> 2 replicas",
 			fields: fields{