update after review

Signed-off-by: lou <alex1988@outlook.com>
2026-01-03 11:45:20 +00:00 · 2023-10-18 17:05:00 +08:00
parent 6d89780fb2
commit d1f5219cbb
7 changed files with 129 additions and 45 deletions
--- a/design/merge-patch-and-strategic-in-resource-modifier.md
+++ b/design/merge-patch-and-strategic-in-resource-modifier.md
@@ -68,12 +68,17 @@ resourceModifierRules:
    namespaces:
    - ns1
  mergePatches:
-  - patchData:
-      metadata:
-        annotations:
-          foo: null
+  - patchData: |
+      {
+        "metadata": {
+          "annotations": {
+            "foo": null
+          }
+        }
+      }
 ```
 - The above configmap will apply the Merge Patch to all the pods in namespace ns1 and remove the annotation `foo` from the pods.
+- Both json and yaml format are supported for the patchData.

 ### New Field StrategicPatches
 StrategicPatches is a list to specify the strategic merge patches to be applied on the resource. The strategic merge patches will be applied in the order specified in the configmap. A subsequent patch is applied in order and if multiple patches are specified for the same path, the last patch will override the previous patches.
@@ -88,13 +93,20 @@ resourceModifierRules:
    namespaces:
    - ns1
  strategicPatches:
-  - patchData:
-      spec:
-        containers:
-        - name: nginx
-          image: repo2/nginx
+  - patchData: |
+      {
+        "spec": {
+          "containers": [
+            {
+              "name": "nginx",
+              "image": "repo2/nginx"
+            }
+          ]
+        }
+      }
 ```
 - The above configmap will apply the Strategic Merge Patch to the pod with name my-pod in namespace ns1 and update the image of container nginx to `repo2/nginx`.
+- Both json and yaml format are supported for the patchData.

 ### Conditional Patches in ALL Patch Types
 Since JSON Merge Patch and Strategic Merge Patch do not support conditional patches, we will use the `test` operation of JSON Patch to support conditional patches in all patch types by adding it to `Conditions` struct in `ResourceModifierRule`.
--- a/internal/resourcemodifiers/json_merge_patch.go
+++ b/internal/resourcemodifiers/json_merge_patch.go
@@ -1,7 +1,6 @@
 package resourcemodifiers

 import (
-	"encoding/json"
 	"fmt"

 	jsonpatch "github.com/evanphx/json-patch"
@@ -11,7 +10,7 @@ import (
 )

 type JSONMergePatch struct {
-	PatchData json.RawMessage `json:"patchData,omitempty"`
+	PatchData string `json:"patchData,omitempty"`
 }

 type JSONMergePatcher struct {
@@ -25,7 +24,7 @@ func (p *JSONMergePatcher) Patch(u *unstructured.Unstructured, _ logrus.FieldLog
 	}

 	for _, patch := range p.patches {
-		patchBytes, err := yaml.YAMLToJSON(patch.PatchData)
+		patchBytes, err := yaml.YAMLToJSON([]byte(patch.PatchData))
 		if err != nil {
 			return nil, fmt.Errorf("error in converting YAML to JSON %s", err)
 		}
--- a/internal/resourcemodifiers/json_merge_patch_test.go
+++ b/internal/resourcemodifiers/json_merge_patch_test.go
@@ -13,15 +13,15 @@ import (
 func TestJsonMergePatchFailure(t *testing.T) {
 	tests := []struct {
 		name string
-		data []byte
+		data string
 	}{
 		{
 			name: "patch with bad yaml",
-			data: []byte("a: b:"),
+			data: "a: b:",
 		},
 		{
 			name: "patch with bad json",
-			data: []byte(`{"a"::1}`),
+			data: `{"a"::1}`,
 		},
 	}
 	for _, tt := range tests {
--- a/internal/resourcemodifiers/resource_modifiers_test.go
+++ b/internal/resourcemodifiers/resource_modifiers_test.go
@@ -141,7 +141,7 @@ func TestGetResourceModifiersFromConfig(t *testing.T) {
 			Namespace: "test-namespace",
 		},
 		Data: map[string]string{
-			"sub.yml": "version: v1\nresourceModifierRules:\n- conditions:\n    groupResource: pods\n    namespaces:\n    - ns1\n  mergePatches:\n  - patchData:\n      metadata:\n        annotations:\n          foo: null",
+			"sub.yml": "version: v1\nresourceModifierRules:\n- conditions:\n    groupResource: pods\n    namespaces:\n    - ns1\n  mergePatches:\n  - patchData: |\n      metadata:\n        annotations:\n          foo: null",
 		},
 	}

@@ -157,7 +157,7 @@ func TestGetResourceModifiersFromConfig(t *testing.T) {
 				},
 				MergePatches: []JSONMergePatch{
 					{
-						PatchData: []byte(`{"metadata":{"annotations":{"foo":null}}}`),
+						PatchData: "metadata:\n  annotations:\n    foo: null",
 					},
 				},
 			},
@@ -170,7 +170,7 @@ func TestGetResourceModifiersFromConfig(t *testing.T) {
 			Namespace: "test-namespace",
 		},
 		Data: map[string]string{
-			"sub.yml": "version: v1\nresourceModifierRules:\n- conditions:\n    groupResource: pods\n    namespaces:\n    - ns1\n  strategicPatches:\n  - patchData:\n      spec:\n        containers:\n        - name: nginx\n          image: repo2/nginx",
+			"sub.yml": "version: v1\nresourceModifierRules:\n- conditions:\n    groupResource: pods\n    namespaces:\n    - ns1\n  strategicPatches:\n  - patchData: |\n      spec:\n        containers:\n        - name: nginx\n          image: repo2/nginx",
 		},
 	}

@@ -186,7 +186,65 @@ func TestGetResourceModifiersFromConfig(t *testing.T) {
 				},
 				StrategicPatches: []StrategicMergePatch{
 					{
-						PatchData: []byte(`{"spec":{"containers":[{"image":"repo2/nginx","name":"nginx"}]}}`),
+						PatchData: "spec:\n  containers:\n  - name: nginx\n    image: repo2/nginx",
+					},
+				},
+			},
+		},
+	}
+
+	cm7 := &v1.ConfigMap{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-configmap",
+			Namespace: "test-namespace",
+		},
+		Data: map[string]string{
+			"sub.yml": "version: v1\nresourceModifierRules:\n- conditions:\n    groupResource: pods\n    namespaces:\n    - ns1\n  mergePatches:\n  - patchData: |\n      {\"metadata\":{\"annotations\":{\"foo\":null}}}",
+		},
+	}
+
+	rules7 := &ResourceModifiers{
+		Version: "v1",
+		ResourceModifierRules: []ResourceModifierRule{
+			{
+				Conditions: Conditions{
+					GroupResource: "pods",
+					Namespaces: []string{
+						"ns1",
+					},
+				},
+				MergePatches: []JSONMergePatch{
+					{
+						PatchData: `{"metadata":{"annotations":{"foo":null}}}`,
+					},
+				},
+			},
+		},
+	}
+
+	cm8 := &v1.ConfigMap{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-configmap",
+			Namespace: "test-namespace",
+		},
+		Data: map[string]string{
+			"sub.yml": "version: v1\nresourceModifierRules:\n- conditions:\n    groupResource: pods\n    namespaces:\n    - ns1\n  strategicPatches:\n  - patchData: |\n      {\"spec\":{\"containers\":[{\"name\": \"nginx\",\"image\": \"repo2/nginx\"}]}}",
+		},
+	}
+
+	rules8 := &ResourceModifiers{
+		Version: "v1",
+		ResourceModifierRules: []ResourceModifierRule{
+			{
+				Conditions: Conditions{
+					GroupResource: "pods",
+					Namespaces: []string{
+						"ns1",
+					},
+				},
+				StrategicPatches: []StrategicMergePatch{
+					{
+						PatchData: `{"spec":{"containers":[{"name": "nginx","image": "repo2/nginx"}]}}`,
 					},
 				},
 			},
@@ -243,7 +301,7 @@ func TestGetResourceModifiersFromConfig(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name: "complex payload with json merge patch",
+			name: "complex yaml data with json merge patch",
 			args: args{
 				cm: cm5,
 			},
@@ -251,13 +309,29 @@ func TestGetResourceModifiersFromConfig(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "complex payload with strategic merge patch",
+			name: "complex yaml data with strategic merge patch",
 			args: args{
 				cm: cm6,
 			},
 			want:    rules6,
 			wantErr: false,
 		},
+		{
+			name: "complex json data with json merge patch",
+			args: args{
+				cm: cm7,
+			},
+			want:    rules7,
+			wantErr: false,
+		},
+		{
+			name: "complex json data with strategic merge patch",
+			args: args{
+				cm: cm8,
+			},
+			want:    rules8,
+			wantErr: false,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -999,7 +1073,7 @@ func TestResourceModifiers_ApplyResourceModifierRules_StrategicMergePatch(t *tes
 						},
 						StrategicPatches: []StrategicMergePatch{
 							{
-								PatchData: []byte(`{"spec":{"containers":[{"name":"nginx","image":"nginx1"}]}}`),
+								PatchData: `{"spec":{"containers":[{"name":"nginx","image":"nginx1"}]}}`,
 							},
 						},
 					},
@@ -1022,10 +1096,10 @@ func TestResourceModifiers_ApplyResourceModifierRules_StrategicMergePatch(t *tes
 						},
 						StrategicPatches: []StrategicMergePatch{
 							{
-								PatchData: []byte(`spec:
+								PatchData: `spec:
  containers:
  - name: nginx
-    image: nginx1`),
+    image: nginx1`,
 							},
 						},
 					},
@@ -1048,7 +1122,7 @@ func TestResourceModifiers_ApplyResourceModifierRules_StrategicMergePatch(t *tes
 						},
 						StrategicPatches: []StrategicMergePatch{
 							{
-								PatchData: []byte(`{"spec":{"volumes":[{"nfs":null,"name":"vol1","persistentVolumeClaim":{"claimName":"pvc1"}}]}}`),
+								PatchData: `{"spec":{"volumes":[{"nfs":null,"name":"vol1","persistentVolumeClaim":{"claimName":"pvc1"}}]}}`,
 							},
 						},
 					},
@@ -1071,7 +1145,7 @@ func TestResourceModifiers_ApplyResourceModifierRules_StrategicMergePatch(t *tes
 						},
 						StrategicPatches: []StrategicMergePatch{
 							{
-								PatchData: []byte(`{"spec":{"volumes":[{"$retainKeys":["name","persistentVolumeClaim"],"name":"vol1","persistentVolumeClaim":{"claimName":"pvc1"}}]}}`),
+								PatchData: `{"spec":{"volumes":[{"$retainKeys":["name","persistentVolumeClaim"],"name":"vol1","persistentVolumeClaim":{"claimName":"pvc1"}}]}}`,
 							},
 						},
 					},
@@ -1094,7 +1168,7 @@ func TestResourceModifiers_ApplyResourceModifierRules_StrategicMergePatch(t *tes
 						},
 						StrategicPatches: []StrategicMergePatch{
 							{
-								PatchData: []byte(`{"spec":{"$setElementOrder/ports":[{"port":8001},{"port":9000},{"port":8002}],"ports":[{"name":"fake","port":9000,"protocol":"TCP","targetPort":9000},{"$patch":"delete","port":8000}]}}`),
+								PatchData: `{"spec":{"$setElementOrder/ports":[{"port":8001},{"port":9000},{"port":8002}],"ports":[{"name":"fake","port":9000,"protocol":"TCP","targetPort":9000},{"$patch":"delete","port":8000}]}}`,
 							},
 						},
 					},
@@ -1151,7 +1225,7 @@ func TestResourceModifiers_ApplyResourceModifierRules_JSONMergePatch(t *testing.
 						},
 						MergePatches: []JSONMergePatch{
 							{
-								PatchData: []byte(`{"metadata":{"labels":{"a":"c"}}}`),
+								PatchData: `{"metadata":{"labels":{"a":"c"}}}`,
 							},
 						},
 					},
@@ -1174,9 +1248,9 @@ func TestResourceModifiers_ApplyResourceModifierRules_JSONMergePatch(t *testing.
 						},
 						MergePatches: []JSONMergePatch{
 							{
-								PatchData: []byte(`metadata:
+								PatchData: `metadata:
  labels:
-    a: c`),
+    a: c`,
 							},
 						},
 					},
@@ -1199,7 +1273,7 @@ func TestResourceModifiers_ApplyResourceModifierRules_JSONMergePatch(t *testing.
 						},
 						MergePatches: []JSONMergePatch{
 							{
-								PatchData: []byte(`{"metadata":{"labels":{"a":null}}}`),
+								PatchData: `{"metadata":{"labels":{"a":null}}}`,
 							},
 						},
 					},
@@ -1222,7 +1296,7 @@ func TestResourceModifiers_ApplyResourceModifierRules_JSONMergePatch(t *testing.
 						},
 						MergePatches: []JSONMergePatch{
 							{
-								PatchData: []byte(`{"metadata":{"labels":{"a":"b"}}}`),
+								PatchData: `{"metadata":{"labels":{"a":"b"}}}`,
 							},
 						},
 					},
@@ -1245,7 +1319,7 @@ func TestResourceModifiers_ApplyResourceModifierRules_JSONMergePatch(t *testing.
 						},
 						MergePatches: []JSONMergePatch{
 							{
-								PatchData: []byte(`{"metadata":{"labels":{"a":null}}}`),
+								PatchData: `{"metadata":{"labels":{"a":null}}}`,
 							},
 						},
 					},
@@ -1298,7 +1372,7 @@ func TestResourceModifiers_wildcard_in_GroupResource(t *testing.T) {
 						},
 						MergePatches: []JSONMergePatch{
 							{
-								PatchData: []byte(`{"metadata":{"labels":{"a":"c"}}}`),
+								PatchData: `{"metadata":{"labels":{"a":"c"}}}`,
 							},
 						},
 					},
@@ -1321,7 +1395,7 @@ func TestResourceModifiers_wildcard_in_GroupResource(t *testing.T) {
 						},
 						MergePatches: []JSONMergePatch{
 							{
-								PatchData: []byte(`{"metadata":{"labels":{"a":"c"}}}`),
+								PatchData: `{"metadata":{"labels":{"a":"c"}}}`,
 							},
 						},
 					},
@@ -1380,7 +1454,7 @@ func TestResourceModifiers_conditional_patches(t *testing.T) {
 						},
 						MergePatches: []JSONMergePatch{
 							{
-								PatchData: []byte(`{"metadata":{"labels":{"a":"c"}}}`),
+								PatchData: `{"metadata":{"labels":{"a":"c"}}}`,
 							},
 						},
 					},
@@ -1409,7 +1483,7 @@ func TestResourceModifiers_conditional_patches(t *testing.T) {
 						},
 						MergePatches: []JSONMergePatch{
 							{
-								PatchData: []byte(`{"metadata":{"labels":{"a":"c"}}}`),
+								PatchData: `{"metadata":{"labels":{"a":"c"}}}`,
 							},
 						},
 					},
--- a/internal/resourcemodifiers/resource_modifiers_validator_test.go
+++ b/internal/resourcemodifiers/resource_modifiers_validator_test.go
@@ -147,7 +147,7 @@ func TestResourceModifiers_Validate(t *testing.T) {
 						},
 						MergePatches: []JSONMergePatch{
 							{
-								PatchData: []byte(`{"metadata":{"labels":{"a":null}}}`),
+								PatchData: `{"metadata":{"labels":{"a":null}}}`,
 							},
 						},
 					},
--- a/internal/resourcemodifiers/strategic_merge_patch.go
+++ b/internal/resourcemodifiers/strategic_merge_patch.go
@@ -1,7 +1,6 @@
 package resourcemodifiers

 import (
-	"encoding/json"
 	"fmt"
 	"net/http"

@@ -18,7 +17,7 @@ import (
 )

 type StrategicMergePatch struct {
-	PatchData json.RawMessage `json:"patchData,omitempty"`
+	PatchData string `json:"patchData,omitempty"`
 }

 type StrategicMergePatcher struct {
@@ -36,7 +35,7 @@ func (p *StrategicMergePatcher) Patch(u *unstructured.Unstructured, _ logrus.Fie
 	origin := u.DeepCopy()
 	updated := u.DeepCopy()
 	for _, patch := range p.patches {
-		patchBytes, err := yaml.YAMLToJSON(patch.PatchData)
+		patchBytes, err := yaml.YAMLToJSON([]byte(patch.PatchData))
 		if err != nil {
 			return nil, fmt.Errorf("error in converting YAML to JSON %s", err)
 		}
--- a/internal/resourcemodifiers/strategic_merge_patch_test.go
+++ b/internal/resourcemodifiers/strategic_merge_patch_test.go
@@ -14,22 +14,22 @@ import (
 func TestStrategicMergePatchFailure(t *testing.T) {
 	tests := []struct {
 		name string
-		data []byte
+		data string
 		kind string
 	}{
 		{
 			name: "patch with unknown kind",
-			data: []byte("{}"),
+			data: "{}",
 			kind: "BadKind",
 		},
 		{
 			name: "patch with bad yaml",
-			data: []byte("a: b:"),
+			data: "a: b:",
 			kind: "Pod",
 		},
 		{
 			name: "patch with bad json",
-			data: []byte(`{"a"::1}`),
+			data: `{"a"::1}`,
 			kind: "Pod",
 		},
 	}