diff --git a/design/merge-patch-and-strategic-in-resource-modifier.md b/design/merge-patch-and-strategic-in-resource-modifier.md index aa082abea..a8127bc49 100644 --- 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`. diff --git a/internal/resourcemodifiers/json_merge_patch.go b/internal/resourcemodifiers/json_merge_patch.go index bffaeef70..5082e2cf5 100644 --- 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) } diff --git a/internal/resourcemodifiers/json_merge_patch_test.go b/internal/resourcemodifiers/json_merge_patch_test.go index 53142c559..b7323fc61 100644 --- 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 { diff --git a/internal/resourcemodifiers/resource_modifiers_test.go b/internal/resourcemodifiers/resource_modifiers_test.go index 9aedec20f..7b4b87e5b 100644 --- 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"}}}`, }, }, }, diff --git a/internal/resourcemodifiers/resource_modifiers_validator_test.go b/internal/resourcemodifiers/resource_modifiers_validator_test.go index 6b6ce449a..4c31e929a 100644 --- 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}}}`, }, }, }, diff --git a/internal/resourcemodifiers/strategic_merge_patch.go b/internal/resourcemodifiers/strategic_merge_patch.go index eaa1e2986..8d3fe4c51 100644 --- 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) } diff --git a/internal/resourcemodifiers/strategic_merge_patch_test.go b/internal/resourcemodifiers/strategic_merge_patch_test.go index 967ef3e66..6c3c700f5 100644 --- 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", }, }