From 5932e263c933a5791196fe9cc238d68b72fd0c64 Mon Sep 17 00:00:00 2001
From: lou <alex1988@outlook.com>
Date: Tue, 10 Oct 2023 16:00:46 +0800
Subject: [PATCH] update after review

Signed-off-by: lou <alex1988@outlook.com>
---
 .../resourcemodifiers/resource_modifiers.go   | 20 +++++++++-----
 .../resource_modifiers_test.go                |  4 +--
 .../resource_modifiers_validator.go           | 15 +++++++++++
 .../resource_modifiers_validator_test.go      | 26 +++++++++++++++++++
 4 files changed, 56 insertions(+), 9 deletions(-)

diff --git a/internal/resourcemodifiers/resource_modifiers.go b/internal/resourcemodifiers/resource_modifiers.go
index ff10300d3..d9a2baefa 100644
--- a/internal/resourcemodifiers/resource_modifiers.go
+++ b/internal/resourcemodifiers/resource_modifiers.go
@@ -23,17 +23,21 @@ const (
 	ResourceModifierSupportedVersionV1 = "v1"
 )
 
+type MatchRule struct {
+	Path  string `json:"path,omitempty"`
+	Value string `json:"value,omitempty"`
+}
+
 type Conditions struct {
 	Namespaces        []string              `json:"namespaces,omitempty"`
 	GroupResource     string                `json:"groupResource"`
 	ResourceNameRegex string                `json:"resourceNameRegex,omitempty"`
 	LabelSelector     *metav1.LabelSelector `json:"labelSelector,omitempty"`
-	Matches           []JSONPatch           `json:"matches,omitempty"`
+	Matches           []MatchRule           `json:"matches,omitempty"`
 }
 
 type ResourceModifierRule struct {
 	Conditions       Conditions            `json:"conditions"`
-	PatchData        string                `json:"patchData,omitempty"`
 	Patches          []JSONPatch           `json:"patches,omitempty"`
 	MergePatches     []JSONMergePatch      `json:"mergePatches,omitempty"`
 	StrategicPatches []StrategicMergePatch `json:"strategicPatches,omitempty"`
@@ -85,7 +89,7 @@ func (r *ResourceModifierRule) apply(obj *unstructured.Unstructured, groupResour
 
 	g, err := glob.Compile(r.Conditions.GroupResource)
 	if err != nil {
-		log.Errorf("bad glob pattern %s, err: %s", r.Conditions.GroupResource, err)
+		log.Errorf("Bad glob pattern of groupResource in condition, groupResource: %s, err: %s", r.Conditions.GroupResource, err)
 		return err
 	}
 
@@ -129,16 +133,18 @@ func (r *ResourceModifierRule) apply(obj *unstructured.Unstructured, groupResour
 	return nil
 }
 
-func matchConditions(u *unstructured.Unstructured, patches []JSONPatch, _ logrus.FieldLogger) (bool, error) {
+func matchConditions(u *unstructured.Unstructured, patches []MatchRule, _ logrus.FieldLogger) (bool, error) {
 	if len(patches) == 0 {
 		return true, nil
 	}
 
 	var fixed []JSONPatch
 	for _, patch := range patches {
-		patch.From = ""
-		patch.Operation = "test"
-		fixed = append(fixed, patch)
+		fixed = append(fixed, JSONPatch{
+			Operation: "test",
+			Path:      patch.Path,
+			Value:     patch.Value,
+		})
 	}
 
 	p := &JSONPatcher{patches: fixed}
diff --git a/internal/resourcemodifiers/resource_modifiers_test.go b/internal/resourcemodifiers/resource_modifiers_test.go
index 594a33255..150078583 100644
--- a/internal/resourcemodifiers/resource_modifiers_test.go
+++ b/internal/resourcemodifiers/resource_modifiers_test.go
@@ -1282,7 +1282,7 @@ func TestResourceModifiers_conditional_patches(t *testing.T) {
 						Conditions: Conditions{
 							GroupResource: "*",
 							Namespaces:    []string{"fake"},
-							Matches: []JSONPatch{
+							Matches: []MatchRule{
 								{
 									Path:  "/metadata/labels/a",
 									Value: "b",
@@ -1311,7 +1311,7 @@ func TestResourceModifiers_conditional_patches(t *testing.T) {
 						Conditions: Conditions{
 							GroupResource: "*",
 							Namespaces:    []string{"fake"},
-							Matches: []JSONPatch{
+							Matches: []MatchRule{
 								{
 									Path:  "/metadata/labels/a",
 									Value: "c",
diff --git a/internal/resourcemodifiers/resource_modifiers_validator.go b/internal/resourcemodifiers/resource_modifiers_validator.go
index 24fe0dbb2..9ad817eca 100644
--- a/internal/resourcemodifiers/resource_modifiers_validator.go
+++ b/internal/resourcemodifiers/resource_modifiers_validator.go
@@ -9,6 +9,21 @@ func (r *ResourceModifierRule) Validate() error {
 	if err := r.Conditions.Validate(); err != nil {
 		return err
 	}
+
+	count := 0
+	for _, size := range []int{
+		len(r.Patches),
+		len(r.MergePatches),
+		len(r.StrategicPatches),
+	} {
+		if size != 0 {
+			count++
+		}
+		if count >= 2 {
+			return fmt.Errorf("only one of patches, mergePatches, strategicPatches can be specified")
+		}
+	}
+
 	for _, patch := range r.Patches {
 		if err := patch.Validate(); err != nil {
 			return err
diff --git a/internal/resourcemodifiers/resource_modifiers_validator_test.go b/internal/resourcemodifiers/resource_modifiers_validator_test.go
index a46c46530..626c72a18 100644
--- a/internal/resourcemodifiers/resource_modifiers_validator_test.go
+++ b/internal/resourcemodifiers/resource_modifiers_validator_test.go
@@ -114,6 +114,32 @@ func TestResourceModifiers_Validate(t *testing.T) {
 			},
 			wantErr: true,
 		},
+		{
+			name: "More than one patch type in a rule",
+			fields: fields{
+				Version: "v1",
+				ResourceModifierRules: []ResourceModifierRule{
+					{
+						Conditions: Conditions{
+							GroupResource: "*",
+						},
+						Patches: []JSONPatch{
+							{
+								Operation: "test",
+								Path:      "/spec/storageClassName",
+								Value:     "premium",
+							},
+						},
+						MergePatches: []JSONMergePatch{
+							{
+								PatchBytes: []byte(`{"metadata":{"labels":{"a":null}}}`),
+							},
+						},
+					},
+				},
+			},
+			wantErr: true,
+		},
 	}
 
 	for _, tt := range tests {