From d8b9328310e0e309285a664b16056608b0344601 Mon Sep 17 00:00:00 2001 From: lou Date: Mon, 25 Sep 2023 18:00:18 +0800 Subject: [PATCH 01/12] support JSON Merge Patch and Strategic Merge Patch in Resource Modifiers Signed-off-by: lou --- .../resourcemodifiers/json_merge_patch.go | 46 ++ internal/resourcemodifiers/json_patch.go | 96 +++ .../resourcemodifiers/resource_modifiers.go | 130 ++-- .../resource_modifiers_test.go | 633 +++++++++++++++++- .../strategic_merge_patch.go | 157 +++++ pkg/restore/restore.go | 2 +- 6 files changed, 993 insertions(+), 71 deletions(-) create mode 100644 internal/resourcemodifiers/json_merge_patch.go create mode 100644 internal/resourcemodifiers/json_patch.go create mode 100644 internal/resourcemodifiers/strategic_merge_patch.go diff --git a/internal/resourcemodifiers/json_merge_patch.go b/internal/resourcemodifiers/json_merge_patch.go new file mode 100644 index 000000000..6201e75c7 --- /dev/null +++ b/internal/resourcemodifiers/json_merge_patch.go @@ -0,0 +1,46 @@ +package resourcemodifiers + +import ( + "fmt" + + jsonpatch "github.com/evanphx/json-patch" + "github.com/sirupsen/logrus" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "sigs.k8s.io/yaml" +) + +type JSONMergePatch struct { + PatchBytes []byte `json:"patchBytes,omitempty"` +} + +type JSONMergePatcher struct { + patches []JSONMergePatch +} + +func (p *JSONMergePatcher) Patch(u *unstructured.Unstructured, _ logrus.FieldLogger) (*unstructured.Unstructured, error) { + objBytes, err := u.MarshalJSON() + if err != nil { + return nil, fmt.Errorf("error in marshaling object %s", err) + } + + var modifiedObjBytes []byte + for _, patch := range p.patches { + patchBytes, err := yaml.YAMLToJSON(patch.PatchBytes) + if err != nil { + return nil, fmt.Errorf("error in converting YAML to JSON %s", err) + } + + modifiedObjBytes, err = jsonpatch.MergePatch(objBytes, patchBytes) + if err != nil { + return nil, fmt.Errorf("error in applying JSON Patch %s", err.Error()) + } + } + + updated := &unstructured.Unstructured{} + err = updated.UnmarshalJSON(modifiedObjBytes) + if err != nil { + return nil, fmt.Errorf("error in unmarshalling modified object %s", err.Error()) + } + + return updated, nil +} diff --git a/internal/resourcemodifiers/json_patch.go b/internal/resourcemodifiers/json_patch.go new file mode 100644 index 000000000..b5af7c362 --- /dev/null +++ b/internal/resourcemodifiers/json_patch.go @@ -0,0 +1,96 @@ +package resourcemodifiers + +import ( + "errors" + "fmt" + "strconv" + "strings" + + jsonpatch "github.com/evanphx/json-patch" + "github.com/sirupsen/logrus" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +type JSONPatch struct { + Operation string `json:"operation"` + From string `json:"from,omitempty"` + Path string `json:"path"` + Value string `json:"value,omitempty"` +} + +func (p *JSONPatch) ToString() string { + if addQuotes(p.Value) { + return fmt.Sprintf(`{"op": "%s", "from": "%s", "path": "%s", "value": "%s"}`, p.Operation, p.From, p.Path, p.Value) + } + return fmt.Sprintf(`{"op": "%s", "from": "%s", "path": "%s", "value": %s}`, p.Operation, p.From, p.Path, p.Value) +} + +func addQuotes(value string) bool { + if value == "" { + return true + } + // if value is null, then don't add quotes + if value == "null" { + return false + } + // if value is a boolean, then don't add quotes + if _, err := strconv.ParseBool(value); err == nil { + return false + } + // if value is a json object or array, then don't add quotes. + if strings.HasPrefix(value, "{") || strings.HasPrefix(value, "[") { + return false + } + // if value is a number, then don't add quotes + if _, err := strconv.ParseFloat(value, 64); err == nil { + return false + } + return true +} + +type JSONPatcher struct { + patches []JSONPatch `yaml:"patches"` +} + +func (p *JSONPatcher) Patch(u *unstructured.Unstructured, logger logrus.FieldLogger) (*unstructured.Unstructured, error) { + modifiedObjBytes, err := p.applyPatch(u) + if err != nil { + if errors.Is(err, jsonpatch.ErrTestFailed) { + logger.Infof("Test operation failed for JSON Patch %s", err.Error()) + return u.DeepCopy(), nil + } + return nil, fmt.Errorf("error in applying JSON Patch %s", err.Error()) + } + + updated := &unstructured.Unstructured{} + err = updated.UnmarshalJSON(modifiedObjBytes) + if err != nil { + return nil, fmt.Errorf("error in unmarshalling modified object %s", err.Error()) + } + + return updated, nil +} + +func (p *JSONPatcher) applyPatch(u *unstructured.Unstructured) ([]byte, error) { + patchBytes := p.patchArrayToByteArray() + jsonPatch, err := jsonpatch.DecodePatch(patchBytes) + if err != nil { + return nil, fmt.Errorf("error in decoding json patch %s", err.Error()) + } + + objBytes, err := u.MarshalJSON() + if err != nil { + return nil, fmt.Errorf("error in marshaling object %s", err.Error()) + } + + return jsonPatch.Apply(objBytes) +} + +func (p *JSONPatcher) patchArrayToByteArray() []byte { + var patches []string + for _, patch := range p.patches { + patches = append(patches, patch.ToString()) + } + patchesStr := strings.Join(patches, ",\n\t") + return []byte(fmt.Sprintf(`[%s]`, patchesStr)) +} diff --git a/internal/resourcemodifiers/resource_modifiers.go b/internal/resourcemodifiers/resource_modifiers.go index dbcd8e7ba..419c3c6df 100644 --- a/internal/resourcemodifiers/resource_modifiers.go +++ b/internal/resourcemodifiers/resource_modifiers.go @@ -3,16 +3,16 @@ package resourcemodifiers import ( "fmt" "regexp" - "strconv" - "strings" jsonpatch "github.com/evanphx/json-patch" + "github.com/gobwas/glob" "github.com/pkg/errors" "github.com/sirupsen/logrus" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/yaml" "github.com/vmware-tanzu/velero/pkg/util/collections" @@ -23,23 +23,20 @@ const ( ResourceModifierSupportedVersionV1 = "v1" ) -type JSONPatch struct { - Operation string `json:"operation"` - From string `json:"from,omitempty"` - Path string `json:"path"` - 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"` } type ResourceModifierRule struct { - Conditions Conditions `json:"conditions"` - Patches []JSONPatch `json:"patches"` + Conditions Conditions `json:"conditions"` + PatchData string `json:"patchData,omitempty"` + Patches []JSONPatch `json:"patches,omitempty"` + MergePatches []JSONMergePatch `json:"mergePatches,omitempty"` + StrategicPatches []StrategicMergePatch `json:"strategicPatches,omitempty"` } type ResourceModifiers struct { @@ -68,10 +65,10 @@ func GetResourceModifiersFromConfig(cm *v1.ConfigMap) (*ResourceModifiers, error return resModifiers, nil } -func (p *ResourceModifiers) ApplyResourceModifierRules(obj *unstructured.Unstructured, groupResource string, log logrus.FieldLogger) []error { +func (p *ResourceModifiers) ApplyResourceModifierRules(obj *unstructured.Unstructured, groupResource string, scheme *runtime.Scheme, log logrus.FieldLogger) []error { var errs []error for _, rule := range p.ResourceModifierRules { - err := rule.Apply(obj, groupResource, log) + err := rule.apply(obj, groupResource, scheme, log) if err != nil { errs = append(errs, err) } @@ -80,13 +77,19 @@ func (p *ResourceModifiers) ApplyResourceModifierRules(obj *unstructured.Unstruc return errs } -func (r *ResourceModifierRule) Apply(obj *unstructured.Unstructured, groupResource string, log logrus.FieldLogger) error { +func (r *ResourceModifierRule) apply(obj *unstructured.Unstructured, groupResource string, scheme *runtime.Scheme, log logrus.FieldLogger) error { namespaceInclusion := collections.NewIncludesExcludes().Includes(r.Conditions.Namespaces...) if !namespaceInclusion.ShouldInclude(obj.GetNamespace()) { return nil } - if r.Conditions.GroupResource != groupResource { + g, err := glob.Compile(r.Conditions.GroupResource) + if err != nil { + log.Errorf("bad glob pattern %s, err: %s", r.Conditions.GroupResource, err) + return err + } + + if !g.Match(groupResource) { return nil } @@ -110,57 +113,44 @@ func (r *ResourceModifierRule) Apply(obj *unstructured.Unstructured, groupResour } } - patches, err := r.PatchArrayToByteArray() + match, err := matchConditions(obj, r.Conditions.Matches, log) if err != nil { return err + } else if !match { + log.Info("Conditions do not match, skip it") + return nil } + log.Infof("Applying resource modifier patch on %s/%s", obj.GetNamespace(), obj.GetName()) - err = ApplyPatch(patches, obj, log) + err = r.applyPatch(obj, scheme, log) if err != nil { return err } return nil } -// PatchArrayToByteArray converts all JsonPatch to string array with the format of jsonpatch.Patch and then convert it to byte array -func (r *ResourceModifierRule) PatchArrayToByteArray() ([]byte, error) { - var patches []string - for _, patch := range r.Patches { - patches = append(patches, patch.ToString()) +func matchConditions(u *unstructured.Unstructured, patches []JSONPatch, logger logrus.FieldLogger) (bool, error) { + if len(patches) == 0 { + return true, nil } - patchesStr := strings.Join(patches, ",\n\t") - return []byte(fmt.Sprintf(`[%s]`, patchesStr)), nil -} -func (p *JSONPatch) ToString() string { - if addQuotes(p.Value) { - return fmt.Sprintf(`{"op": "%s", "from": "%s", "path": "%s", "value": "%s"}`, p.Operation, p.From, p.Path, p.Value) + var fixed []JSONPatch + for _, patch := range patches { + patch.From = "" + patch.Operation = "test" + fixed = append(fixed, patch) } - return fmt.Sprintf(`{"op": "%s", "from": "%s", "path": "%s", "value": %s}`, p.Operation, p.From, p.Path, p.Value) -} -func ApplyPatch(patch []byte, obj *unstructured.Unstructured, log logrus.FieldLogger) error { - jsonPatch, err := jsonpatch.DecodePatch(patch) - if err != nil { - return fmt.Errorf("error in decoding json patch %s", err.Error()) - } - objBytes, err := obj.MarshalJSON() - if err != nil { - return fmt.Errorf("error in marshaling object %s", err.Error()) - } - modifiedObjBytes, err := jsonPatch.Apply(objBytes) + p := &JSONPatcher{patches: fixed} + _, err := p.applyPatch(u) if err != nil { if errors.Is(err, jsonpatch.ErrTestFailed) { - log.Infof("Test operation failed for JSON Patch %s", err.Error()) - return nil + return false, nil } - return fmt.Errorf("error in applying JSON Patch %s", err.Error()) + return false, err } - err = obj.UnmarshalJSON(modifiedObjBytes) - if err != nil { - return fmt.Errorf("error in unmarshalling modified object %s", err.Error()) - } - return nil + + return true, nil } func unmarshalResourceModifiers(yamlData []byte) (*ResourceModifiers, error) { @@ -172,25 +162,27 @@ func unmarshalResourceModifiers(yamlData []byte) (*ResourceModifiers, error) { return resModifiers, nil } -func addQuotes(value string) bool { - if value == "" { - return true - } - // if value is null, then don't add quotes - if value == "null" { - return false - } - // if value is a boolean, then don't add quotes - if _, err := strconv.ParseBool(value); err == nil { - return false - } - // if value is a json object or array, then don't add quotes. - if strings.HasPrefix(value, "{") || strings.HasPrefix(value, "[") { - return false - } - // if value is a number, then don't add quotes - if _, err := strconv.ParseFloat(value, 64); err == nil { - return false - } - return true +type patcher interface { + Patch(u *unstructured.Unstructured, logger logrus.FieldLogger) (*unstructured.Unstructured, error) +} + +func (r *ResourceModifierRule) applyPatch(u *unstructured.Unstructured, scheme *runtime.Scheme, logger logrus.FieldLogger) error { + var p patcher + if len(r.Patches) > 0 { + p = &JSONPatcher{patches: r.Patches} + } else if len(r.MergePatches) > 0 { + p = &JSONMergePatcher{patches: r.MergePatches} + } else if len(r.StrategicPatches) > 0 { + p = &StrategicMergePatcher{patches: r.StrategicPatches, scheme: scheme} + } else { + return fmt.Errorf("no patch data found") + } + + updated, err := p.Patch(u, logger) + if err != nil { + return fmt.Errorf("error in applying patch %s", err) + } + + u.SetUnstructuredContent(updated.Object) + return nil } diff --git a/internal/resourcemodifiers/resource_modifiers_test.go b/internal/resourcemodifiers/resource_modifiers_test.go index c2150bbc8..594a33255 100644 --- a/internal/resourcemodifiers/resource_modifiers_test.go +++ b/internal/resourcemodifiers/resource_modifiers_test.go @@ -9,6 +9,10 @@ import ( v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/serializer/yaml" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" ) func TestGetResourceModifiersFromConfig(t *testing.T) { @@ -704,7 +708,7 @@ func TestResourceModifiers_ApplyResourceModifierRules(t *testing.T) { Version: tt.fields.Version, ResourceModifierRules: tt.fields.ResourceModifierRules, } - got := p.ApplyResourceModifierRules(tt.args.obj, tt.args.groupResource, logrus.New()) + got := p.ApplyResourceModifierRules(tt.args.obj, tt.args.groupResource, nil, logrus.New()) assert.Equal(t, tt.wantErr, len(got) > 0) assert.Equal(t, *tt.wantObj, *tt.args.obj) @@ -712,6 +716,633 @@ func TestResourceModifiers_ApplyResourceModifierRules(t *testing.T) { } } +var podYAMLWithNginxImage = ` +apiVersion: v1 +kind: Pod +metadata: + name: pod1 + namespace: fake +spec: + containers: + - image: nginx + name: nginx +` + +var podYAMLWithNginx1Image = ` +apiVersion: v1 +kind: Pod +metadata: + name: pod1 + namespace: fake +spec: + containers: + - image: nginx1 + name: nginx +` + +var podYAMLWithNFSVolume = ` +apiVersion: v1 +kind: Pod +metadata: + name: pod1 + namespace: fake +spec: + containers: + - image: fake + name: fake + volumeMounts: + - mountPath: /fake1 + name: vol1 + - mountPath: /fake2 + name: vol2 + volumes: + - name: vol1 + nfs: + path: /fake2 + - name: vol2 + emptyDir: {} +` + +var podYAMLWithPVCVolume = ` +apiVersion: v1 +kind: Pod +metadata: + name: pod1 + namespace: fake +spec: + containers: + - image: fake + name: fake + volumeMounts: + - mountPath: /fake1 + name: vol1 + - mountPath: /fake2 + name: vol2 + volumes: + - name: vol1 + persistentVolumeClaim: + claimName: pvc1 + - name: vol2 + emptyDir: {} +` + +var svcYAMLWithPort8000 = ` +apiVersion: v1 +kind: Service +metadata: + name: svc1 + namespace: fake +spec: + ports: + - name: fake1 + port: 8001 + protocol: TCP + targetPort: 8001 + - name: fake + port: 8000 + protocol: TCP + targetPort: 8000 + - name: fake2 + port: 8002 + protocol: TCP + targetPort: 8002 +` + +var svcYAMLWithPort9000 = ` +apiVersion: v1 +kind: Service +metadata: + name: svc1 + namespace: fake +spec: + ports: + - name: fake1 + port: 8001 + protocol: TCP + targetPort: 8001 + - name: fake + port: 9000 + protocol: TCP + targetPort: 9000 + - name: fake2 + port: 8002 + protocol: TCP + targetPort: 8002 +` + +var cmYAMLWithLabelAToB = ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm1 + namespace: fake + labels: + a: b + c: d +` + +var cmYAMLWithLabelAToC = ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm1 + namespace: fake + labels: + a: c + c: d +` + +var cmYAMLWithoutLabelA = ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm1 + namespace: fake + labels: + c: d +` + +func TestResourceModifiers_ApplyResourceModifierRules_StrategicMergePatch(t *testing.T) { + scheme := runtime.NewScheme() + utilruntime.Must(clientgoscheme.AddToScheme(scheme)) + unstructuredSerializer := yaml.NewDecodingSerializer(unstructured.UnstructuredJSONScheme) + o1, _, err := unstructuredSerializer.Decode([]byte(podYAMLWithNFSVolume), nil, nil) + assert.NoError(t, err) + podWithNFSVolume := o1.(*unstructured.Unstructured) + + o2, _, err := unstructuredSerializer.Decode([]byte(podYAMLWithPVCVolume), nil, nil) + assert.NoError(t, err) + podWithPVCVolume := o2.(*unstructured.Unstructured) + + o3, _, err := unstructuredSerializer.Decode([]byte(svcYAMLWithPort8000), nil, nil) + assert.NoError(t, err) + svcWithPort8000 := o3.(*unstructured.Unstructured) + + o4, _, err := unstructuredSerializer.Decode([]byte(svcYAMLWithPort9000), nil, nil) + assert.NoError(t, err) + svcWithPort9000 := o4.(*unstructured.Unstructured) + + o5, _, err := unstructuredSerializer.Decode([]byte(podYAMLWithNginxImage), nil, nil) + assert.NoError(t, err) + podWithNginxImage := o5.(*unstructured.Unstructured) + + o6, _, err := unstructuredSerializer.Decode([]byte(podYAMLWithNginx1Image), nil, nil) + assert.NoError(t, err) + podWithNginx1Image := o6.(*unstructured.Unstructured) + + tests := []struct { + name string + rm *ResourceModifiers + obj *unstructured.Unstructured + groupResource string + wantErr bool + wantObj *unstructured.Unstructured + }{ + { + name: "update image", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "pods", + Namespaces: []string{"fake"}, + }, + StrategicPatches: []StrategicMergePatch{ + { + PatchBytes: []byte(`{"spec":{"containers":[{"name":"nginx","image":"nginx1"}]}}`), + }, + }, + }, + }, + }, + obj: podWithNginxImage.DeepCopy(), + groupResource: "pods", + wantErr: false, + wantObj: podWithNginx1Image.DeepCopy(), + }, + { + name: "update image with yaml format", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "pods", + Namespaces: []string{"fake"}, + }, + StrategicPatches: []StrategicMergePatch{ + { + PatchBytes: []byte(`spec: + containers: + - name: nginx + image: nginx1`), + }, + }, + }, + }, + }, + obj: podWithNginxImage.DeepCopy(), + groupResource: "pods", + wantErr: false, + wantObj: podWithNginx1Image.DeepCopy(), + }, + { + name: "replace nfs with pvc in volume", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "pods", + Namespaces: []string{"fake"}, + }, + StrategicPatches: []StrategicMergePatch{ + { + PatchBytes: []byte(`{"spec":{"volumes":[{"nfs":null,"name":"vol1","persistentVolumeClaim":{"claimName":"pvc1"}}]}}`), + }, + }, + }, + }, + }, + obj: podWithNFSVolume.DeepCopy(), + groupResource: "pods", + wantErr: false, + wantObj: podWithPVCVolume.DeepCopy(), + }, + { + name: "replace any other volume source with pvc in volume", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "pods", + Namespaces: []string{"fake"}, + }, + StrategicPatches: []StrategicMergePatch{ + { + PatchBytes: []byte(`{"spec":{"volumes":[{"$retainKeys":["name","persistentVolumeClaim"],"name":"vol1","persistentVolumeClaim":{"claimName":"pvc1"}}]}}`), + }, + }, + }, + }, + }, + obj: podWithNFSVolume.DeepCopy(), + groupResource: "pods", + wantErr: false, + wantObj: podWithPVCVolume.DeepCopy(), + }, + { + name: "update a service port", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "services", + Namespaces: []string{"fake"}, + }, + StrategicPatches: []StrategicMergePatch{ + { + PatchBytes: []byte(`{"spec":{"$setElementOrder/ports":[{"port":8001},{"port":9000},{"port":8002}],"ports":[{"name":"fake","port":9000,"protocol":"TCP","targetPort":9000},{"$patch":"delete","port":8000}]}}`), + }, + }, + }, + }, + }, + obj: svcWithPort8000.DeepCopy(), + groupResource: "services", + wantErr: false, + wantObj: svcWithPort9000.DeepCopy(), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.rm.ApplyResourceModifierRules(tt.obj, tt.groupResource, scheme, logrus.New()) + + assert.Equal(t, tt.wantErr, len(got) > 0) + assert.Equal(t, *tt.wantObj, *tt.obj) + }) + } +} + +func TestResourceModifiers_ApplyResourceModifierRules_JSONMergePatch(t *testing.T) { + unstructuredSerializer := yaml.NewDecodingSerializer(unstructured.UnstructuredJSONScheme) + o1, _, err := unstructuredSerializer.Decode([]byte(cmYAMLWithLabelAToB), nil, nil) + assert.NoError(t, err) + cmWithLabelAToB := o1.(*unstructured.Unstructured) + + o2, _, err := unstructuredSerializer.Decode([]byte(cmYAMLWithLabelAToC), nil, nil) + assert.NoError(t, err) + cmWithLabelAToC := o2.(*unstructured.Unstructured) + + o3, _, err := unstructuredSerializer.Decode([]byte(cmYAMLWithoutLabelA), nil, nil) + assert.NoError(t, err) + cmWithoutLabelA := o3.(*unstructured.Unstructured) + + tests := []struct { + name string + rm *ResourceModifiers + obj *unstructured.Unstructured + groupResource string + wantErr bool + wantObj *unstructured.Unstructured + }{ + { + name: "update labels", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "configmaps", + Namespaces: []string{"fake"}, + }, + MergePatches: []JSONMergePatch{ + { + PatchBytes: []byte(`{"metadata":{"labels":{"a":"c"}}}`), + }, + }, + }, + }, + }, + obj: cmWithLabelAToB.DeepCopy(), + groupResource: "configmaps", + wantErr: false, + wantObj: cmWithLabelAToC.DeepCopy(), + }, + { + name: "update labels in yaml format", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "configmaps", + Namespaces: []string{"fake"}, + }, + MergePatches: []JSONMergePatch{ + { + PatchBytes: []byte(`metadata: + labels: + a: c`), + }, + }, + }, + }, + }, + obj: cmWithLabelAToB.DeepCopy(), + groupResource: "configmaps", + wantErr: false, + wantObj: cmWithLabelAToC.DeepCopy(), + }, + { + name: "delete labels", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "configmaps", + Namespaces: []string{"fake"}, + }, + MergePatches: []JSONMergePatch{ + { + PatchBytes: []byte(`{"metadata":{"labels":{"a":null}}}`), + }, + }, + }, + }, + }, + obj: cmWithLabelAToB.DeepCopy(), + groupResource: "configmaps", + wantErr: false, + wantObj: cmWithoutLabelA.DeepCopy(), + }, + { + name: "add labels", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "configmaps", + Namespaces: []string{"fake"}, + }, + MergePatches: []JSONMergePatch{ + { + PatchBytes: []byte(`{"metadata":{"labels":{"a":"b"}}}`), + }, + }, + }, + }, + }, + obj: cmWithoutLabelA.DeepCopy(), + groupResource: "configmaps", + wantErr: false, + wantObj: cmWithLabelAToB.DeepCopy(), + }, + { + name: "delete non-existing labels", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "configmaps", + Namespaces: []string{"fake"}, + }, + MergePatches: []JSONMergePatch{ + { + PatchBytes: []byte(`{"metadata":{"labels":{"a":null}}}`), + }, + }, + }, + }, + }, + obj: cmWithoutLabelA.DeepCopy(), + groupResource: "configmaps", + wantErr: false, + wantObj: cmWithoutLabelA.DeepCopy(), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.rm.ApplyResourceModifierRules(tt.obj, tt.groupResource, nil, logrus.New()) + + assert.Equal(t, tt.wantErr, len(got) > 0) + assert.Equal(t, *tt.wantObj, *tt.obj) + }) + } +} + +func TestResourceModifiers_wildcard_in_GroupResource(t *testing.T) { + unstructuredSerializer := yaml.NewDecodingSerializer(unstructured.UnstructuredJSONScheme) + o1, _, err := unstructuredSerializer.Decode([]byte(cmYAMLWithLabelAToB), nil, nil) + assert.NoError(t, err) + cmWithLabelAToB := o1.(*unstructured.Unstructured) + + o2, _, err := unstructuredSerializer.Decode([]byte(cmYAMLWithLabelAToC), nil, nil) + assert.NoError(t, err) + cmWithLabelAToC := o2.(*unstructured.Unstructured) + + tests := []struct { + name string + rm *ResourceModifiers + obj *unstructured.Unstructured + groupResource string + wantErr bool + wantObj *unstructured.Unstructured + }{ + { + name: "match all groups and resources", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "*", + Namespaces: []string{"fake"}, + }, + MergePatches: []JSONMergePatch{ + { + PatchBytes: []byte(`{"metadata":{"labels":{"a":"c"}}}`), + }, + }, + }, + }, + }, + obj: cmWithLabelAToB.DeepCopy(), + groupResource: "configmaps", + wantErr: false, + wantObj: cmWithLabelAToC.DeepCopy(), + }, + { + name: "match all resources in group apps", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "*.apps", + Namespaces: []string{"fake"}, + }, + MergePatches: []JSONMergePatch{ + { + PatchBytes: []byte(`{"metadata":{"labels":{"a":"c"}}}`), + }, + }, + }, + }, + }, + obj: cmWithLabelAToB.DeepCopy(), + groupResource: "fake.apps", + wantErr: false, + wantObj: cmWithLabelAToC.DeepCopy(), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.rm.ApplyResourceModifierRules(tt.obj, tt.groupResource, nil, logrus.New()) + + assert.Equal(t, tt.wantErr, len(got) > 0) + assert.Equal(t, *tt.wantObj, *tt.obj) + }) + } +} + +func TestResourceModifiers_conditional_patches(t *testing.T) { + unstructuredSerializer := yaml.NewDecodingSerializer(unstructured.UnstructuredJSONScheme) + o1, _, err := unstructuredSerializer.Decode([]byte(cmYAMLWithLabelAToB), nil, nil) + assert.NoError(t, err) + cmWithLabelAToB := o1.(*unstructured.Unstructured) + + o2, _, err := unstructuredSerializer.Decode([]byte(cmYAMLWithLabelAToC), nil, nil) + assert.NoError(t, err) + cmWithLabelAToC := o2.(*unstructured.Unstructured) + + tests := []struct { + name string + rm *ResourceModifiers + obj *unstructured.Unstructured + groupResource string + wantErr bool + wantObj *unstructured.Unstructured + }{ + { + name: "match conditions and apply patches", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "*", + Namespaces: []string{"fake"}, + Matches: []JSONPatch{ + { + Path: "/metadata/labels/a", + Value: "b", + }, + }, + }, + MergePatches: []JSONMergePatch{ + { + PatchBytes: []byte(`{"metadata":{"labels":{"a":"c"}}}`), + }, + }, + }, + }, + }, + obj: cmWithLabelAToB.DeepCopy(), + groupResource: "configmaps", + wantErr: false, + wantObj: cmWithLabelAToC.DeepCopy(), + }, + { + name: "mismatch conditions and skip patches", + rm: &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "*", + Namespaces: []string{"fake"}, + Matches: []JSONPatch{ + { + Path: "/metadata/labels/a", + Value: "c", + }, + }, + }, + MergePatches: []JSONMergePatch{ + { + PatchBytes: []byte(`{"metadata":{"labels":{"a":"c"}}}`), + }, + }, + }, + }, + }, + obj: cmWithLabelAToB.DeepCopy(), + groupResource: "configmaps", + wantErr: false, + wantObj: cmWithLabelAToB.DeepCopy(), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.rm.ApplyResourceModifierRules(tt.obj, tt.groupResource, nil, logrus.New()) + + assert.Equal(t, tt.wantErr, len(got) > 0) + assert.Equal(t, *tt.wantObj, *tt.obj) + }) + } +} + func TestJSONPatch_ToString(t *testing.T) { type fields struct { Operation string diff --git a/internal/resourcemodifiers/strategic_merge_patch.go b/internal/resourcemodifiers/strategic_merge_patch.go new file mode 100644 index 000000000..936043eca --- /dev/null +++ b/internal/resourcemodifiers/strategic_merge_patch.go @@ -0,0 +1,157 @@ +package resourcemodifiers + +import ( + "fmt" + "net/http" + + "github.com/sirupsen/logrus" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/mergepatch" + "k8s.io/apimachinery/pkg/util/strategicpatch" + "k8s.io/apimachinery/pkg/util/validation/field" + "sigs.k8s.io/json" + "sigs.k8s.io/yaml" +) + +type StrategicMergePatch struct { + PatchBytes []byte `json:"patchBytes,omitempty"` +} + +type StrategicMergePatcher struct { + patches []StrategicMergePatch + scheme *runtime.Scheme +} + +func (p *StrategicMergePatcher) Patch(u *unstructured.Unstructured, _ logrus.FieldLogger) (*unstructured.Unstructured, error) { + gvk := u.GetObjectKind().GroupVersionKind() + schemaReferenceObj, err := p.scheme.New(gvk) + if err != nil { + return nil, err + } + + if err != nil { + return nil, fmt.Errorf("error in unmarshaling object %s", err.Error()) + } + + origin := u.DeepCopy() + updated := u.DeepCopy() + for _, patch := range p.patches { + patchBytes, err := yaml.YAMLToJSON(patch.PatchBytes) + if err != nil { + return nil, fmt.Errorf("error in converting YAML to JSON %s", err) + } + + err = strategicPatchObject(origin, patchBytes, updated, schemaReferenceObj, metav1.FieldValidationStrict) + if err != nil { + return nil, fmt.Errorf("error in applying JSON Patch %s", err.Error()) + } + + origin = updated.DeepCopy() + } + + return updated, nil +} + +// strategicPatchObject applies a strategic merge patch of `patchBytes` to +// `originalObject` and stores the result in `objToUpdate`. +// It additionally returns the map[string]interface{} representation of the +// `originalObject` and `patchBytes`. +// NOTE: Both `originalObject` and `objToUpdate` are supposed to be versioned. +func strategicPatchObject( + originalObject runtime.Object, + patchBytes []byte, + objToUpdate runtime.Object, + schemaReferenceObj runtime.Object, + validationDirective string, +) error { + originalObjMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(originalObject) + if err != nil { + return err + } + + patchMap := make(map[string]interface{}) + var strictErrs []error + if validationDirective == metav1.FieldValidationWarn || validationDirective == metav1.FieldValidationStrict { + strictErrs, err = json.UnmarshalStrict(patchBytes, &patchMap) + if err != nil { + return apierrors.NewBadRequest(err.Error()) + } + } else { + if err = json.UnmarshalCaseSensitivePreserveInts(patchBytes, &patchMap); err != nil { + return apierrors.NewBadRequest(err.Error()) + } + } + + if err := applyPatchToObject(originalObjMap, patchMap, objToUpdate, schemaReferenceObj, strictErrs, validationDirective); err != nil { + return err + } + return nil +} + +// applyPatchToObject applies a strategic merge patch of to +// and stores the result in . +// NOTE: must be a versioned object. +func applyPatchToObject( + originalMap map[string]interface{}, + patchMap map[string]interface{}, + objToUpdate runtime.Object, + schemaReferenceObj runtime.Object, + strictErrs []error, + validationDirective string, +) error { + patchedObjMap, err := strategicpatch.StrategicMergeMapPatch(originalMap, patchMap, schemaReferenceObj) + if err != nil { + return interpretStrategicMergePatchError(err) + } + + // Rather than serialize the patched map to JSON, then decode it to an object, we go directly from a map to an object + converter := runtime.DefaultUnstructuredConverter + returnUnknownFields := validationDirective == metav1.FieldValidationWarn || validationDirective == metav1.FieldValidationStrict + if err := converter.FromUnstructuredWithValidation(patchedObjMap, objToUpdate, returnUnknownFields); err != nil { + strictError, isStrictError := runtime.AsStrictDecodingError(err) + switch { + case !isStrictError: + // disregard any sttrictErrs, because it's an incomplete + // list of strict errors given that we don't know what fields were + // unknown because StrategicMergeMapPatch failed. + // Non-strict errors trump in this case. + return apierrors.NewInvalid(schema.GroupKind{}, "", field.ErrorList{ + field.Invalid(field.NewPath("patch"), fmt.Sprintf("%+v", patchMap), err.Error()), + }) + case validationDirective == metav1.FieldValidationWarn: + //addStrictDecodingWarnings(requestContext, append(strictErrs, strictError.Errors()...)) + default: + strictDecodingError := runtime.NewStrictDecodingError(append(strictErrs, strictError.Errors()...)) + return apierrors.NewInvalid(schema.GroupKind{}, "", field.ErrorList{ + field.Invalid(field.NewPath("patch"), fmt.Sprintf("%+v", patchMap), strictDecodingError.Error()), + }) + } + } else if len(strictErrs) > 0 { + switch { + case validationDirective == metav1.FieldValidationWarn: + //addStrictDecodingWarnings(requestContext, strictErrs) + default: + return apierrors.NewInvalid(schema.GroupKind{}, "", field.ErrorList{ + field.Invalid(field.NewPath("patch"), fmt.Sprintf("%+v", patchMap), runtime.NewStrictDecodingError(strictErrs).Error()), + }) + } + } + + return nil +} + +// interpretStrategicMergePatchError interprets the error type and returns an error with appropriate HTTP code. +func interpretStrategicMergePatchError(err error) error { + switch err { + case mergepatch.ErrBadJSONDoc, mergepatch.ErrBadPatchFormatForPrimitiveList, mergepatch.ErrBadPatchFormatForRetainKeys, mergepatch.ErrBadPatchFormatForSetElementOrderList, mergepatch.ErrUnsupportedStrategicMergePatchFormat: + return apierrors.NewBadRequest(err.Error()) + case mergepatch.ErrNoListOfLists, mergepatch.ErrPatchContentNotMatchRetainKeys: + return apierrors.NewGenericServerResponse(http.StatusUnprocessableEntity, "", schema.GroupResource{}, "", err.Error(), 0, false) + default: + return err + } +} diff --git a/pkg/restore/restore.go b/pkg/restore/restore.go index 6d0d814ca..0f1e89c85 100644 --- a/pkg/restore/restore.go +++ b/pkg/restore/restore.go @@ -1353,7 +1353,7 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso } if ctx.resourceModifiers != nil { - if errList := ctx.resourceModifiers.ApplyResourceModifierRules(obj, groupResource.String(), ctx.log); errList != nil { + if errList := ctx.resourceModifiers.ApplyResourceModifierRules(obj, groupResource.String(), ctx.kbClient.Scheme(), ctx.log); errList != nil { for _, err := range errList { errs.Add(namespace, err) } From 06ed9dcc71b3912e24b41e98f7a4ed865e7f2164 Mon Sep 17 00:00:00 2001 From: lou Date: Wed, 4 Oct 2023 16:02:23 +0800 Subject: [PATCH 02/12] add changelog Signed-off-by: lou --- changelogs/unreleased/6917-27149chen | 1 + go.mod | 2 +- internal/resourcemodifiers/json_merge_patch.go | 5 ++--- internal/resourcemodifiers/strategic_merge_patch.go | 6 +----- 4 files changed, 5 insertions(+), 9 deletions(-) create mode 100644 changelogs/unreleased/6917-27149chen diff --git a/changelogs/unreleased/6917-27149chen b/changelogs/unreleased/6917-27149chen new file mode 100644 index 000000000..94648eaa4 --- /dev/null +++ b/changelogs/unreleased/6917-27149chen @@ -0,0 +1 @@ +Support JSON Merge Patch and Strategic Merge Patch in Resource Modifiers \ No newline at end of file diff --git a/go.mod b/go.mod index 51cd71913..9ae6506a5 100644 --- a/go.mod +++ b/go.mod @@ -58,6 +58,7 @@ require ( k8s.io/metrics v0.25.6 k8s.io/utils v0.0.0-20220728103510-ee6ede2d64ed sigs.k8s.io/controller-runtime v0.12.2 + sigs.k8s.io/json v0.0.0-20220713155537-f223a00ba0e2 sigs.k8s.io/yaml v1.3.0 ) @@ -163,7 +164,6 @@ require ( gopkg.in/yaml.v2 v2.4.0 // indirect k8s.io/component-base v0.24.2 // indirect k8s.io/kube-openapi v0.0.0-20220803162953-67bda5d908f1 // indirect - sigs.k8s.io/json v0.0.0-20220713155537-f223a00ba0e2 // indirect sigs.k8s.io/structured-merge-diff/v4 v4.2.3 // indirect ) diff --git a/internal/resourcemodifiers/json_merge_patch.go b/internal/resourcemodifiers/json_merge_patch.go index 6201e75c7..18fe564f4 100644 --- a/internal/resourcemodifiers/json_merge_patch.go +++ b/internal/resourcemodifiers/json_merge_patch.go @@ -23,21 +23,20 @@ func (p *JSONMergePatcher) Patch(u *unstructured.Unstructured, _ logrus.FieldLog return nil, fmt.Errorf("error in marshaling object %s", err) } - var modifiedObjBytes []byte for _, patch := range p.patches { patchBytes, err := yaml.YAMLToJSON(patch.PatchBytes) if err != nil { return nil, fmt.Errorf("error in converting YAML to JSON %s", err) } - modifiedObjBytes, err = jsonpatch.MergePatch(objBytes, patchBytes) + objBytes, err = jsonpatch.MergePatch(objBytes, patchBytes) if err != nil { return nil, fmt.Errorf("error in applying JSON Patch %s", err.Error()) } } updated := &unstructured.Unstructured{} - err = updated.UnmarshalJSON(modifiedObjBytes) + err = updated.UnmarshalJSON(objBytes) if err != nil { return nil, fmt.Errorf("error in unmarshalling modified object %s", err.Error()) } diff --git a/internal/resourcemodifiers/strategic_merge_patch.go b/internal/resourcemodifiers/strategic_merge_patch.go index 936043eca..4f7945cc5 100644 --- a/internal/resourcemodifiers/strategic_merge_patch.go +++ b/internal/resourcemodifiers/strategic_merge_patch.go @@ -33,10 +33,6 @@ func (p *StrategicMergePatcher) Patch(u *unstructured.Unstructured, _ logrus.Fie return nil, err } - if err != nil { - return nil, fmt.Errorf("error in unmarshaling object %s", err.Error()) - } - origin := u.DeepCopy() updated := u.DeepCopy() for _, patch := range p.patches { @@ -47,7 +43,7 @@ func (p *StrategicMergePatcher) Patch(u *unstructured.Unstructured, _ logrus.Fie err = strategicPatchObject(origin, patchBytes, updated, schemaReferenceObj, metav1.FieldValidationStrict) if err != nil { - return nil, fmt.Errorf("error in applying JSON Patch %s", err.Error()) + return nil, fmt.Errorf("error in applying Strategic Patch %s", err.Error()) } origin = updated.DeepCopy() From 58d842595200e2909463177b9b31708e5dd0ea7b Mon Sep 17 00:00:00 2001 From: lou Date: Thu, 5 Oct 2023 01:19:05 +0800 Subject: [PATCH 03/12] fix lint Signed-off-by: lou --- internal/resourcemodifiers/resource_modifiers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/resourcemodifiers/resource_modifiers.go b/internal/resourcemodifiers/resource_modifiers.go index 419c3c6df..ff10300d3 100644 --- a/internal/resourcemodifiers/resource_modifiers.go +++ b/internal/resourcemodifiers/resource_modifiers.go @@ -129,7 +129,7 @@ func (r *ResourceModifierRule) apply(obj *unstructured.Unstructured, groupResour return nil } -func matchConditions(u *unstructured.Unstructured, patches []JSONPatch, logger logrus.FieldLogger) (bool, error) { +func matchConditions(u *unstructured.Unstructured, patches []JSONPatch, _ logrus.FieldLogger) (bool, error) { if len(patches) == 0 { return true, nil } From e880c0d01b5deece00bfafb0596cbf7f6440d574 Mon Sep 17 00:00:00 2001 From: lou Date: Sat, 7 Oct 2023 16:33:33 +0800 Subject: [PATCH 04/12] update after review Signed-off-by: lou --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 9ae6506a5..41e062b71 100644 --- a/go.mod +++ b/go.mod @@ -58,7 +58,7 @@ require ( k8s.io/metrics v0.25.6 k8s.io/utils v0.0.0-20220728103510-ee6ede2d64ed sigs.k8s.io/controller-runtime v0.12.2 - sigs.k8s.io/json v0.0.0-20220713155537-f223a00ba0e2 + sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd sigs.k8s.io/yaml v1.3.0 ) diff --git a/go.sum b/go.sum index bc4c0ed7c..7372efd57 100644 --- a/go.sum +++ b/go.sum @@ -1392,8 +1392,8 @@ sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.0.30/go.mod h1:fEO7lR sigs.k8s.io/controller-runtime v0.12.2 h1:nqV02cvhbAj7tbt21bpPpTByrXGn2INHRsi39lXy9sE= sigs.k8s.io/controller-runtime v0.12.2/go.mod h1:qKsk4WE6zW2Hfj0G4v10EnNB2jMG1C+NTb8h+DwCoU0= sigs.k8s.io/json v0.0.0-20211208200746-9f7c6b3444d2/go.mod h1:B+TnT182UBxE84DiCz4CVE26eOSDAeYCpfDnC2kdKMY= -sigs.k8s.io/json v0.0.0-20220713155537-f223a00ba0e2 h1:iXTIw73aPyC+oRdyqqvVJuloN1p0AC/kzH07hu3NE+k= -sigs.k8s.io/json v0.0.0-20220713155537-f223a00ba0e2/go.mod h1:B8JuhiUyNFVKdsE8h686QcCxMaH6HrOAZj4vswFpcB0= +sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd h1:EDPBXCAspyGV4jQlpZSudPeMmr1bNJefnuqLsRAsHZo= +sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd/go.mod h1:B8JuhiUyNFVKdsE8h686QcCxMaH6HrOAZj4vswFpcB0= sigs.k8s.io/kustomize/api v0.8.11/go.mod h1:a77Ls36JdfCWojpUqR6m60pdGY1AYFix4AH83nJtY1g= sigs.k8s.io/kustomize/api v0.11.4/go.mod h1:k+8RsqYbgpkIrJ4p9jcdPqe8DprLxFUUO0yNOq8C+xI= sigs.k8s.io/kustomize/kyaml v0.11.0/go.mod h1:GNMwjim4Ypgp/MueD3zXHLRJEjz7RvtPae0AwlvEMFM= From 5932e263c933a5791196fe9cc238d68b72fd0c64 Mon Sep 17 00:00:00 2001 From: lou Date: Tue, 10 Oct 2023 16:00:46 +0800 Subject: [PATCH 05/12] update after review Signed-off-by: lou --- .../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 { From 65082f33a451d0fbfc75f2858d555ab884f72f5f Mon Sep 17 00:00:00 2001 From: lou Date: Tue, 10 Oct 2023 18:59:45 +0800 Subject: [PATCH 06/12] add deserialization tests Signed-off-by: lou --- .../resourcemodifiers/json_merge_patch.go | 7 +- .../resourcemodifiers/resource_modifiers.go | 2 +- .../resource_modifiers_test.go | 104 +++++++++++++++--- .../resource_modifiers_validator_test.go | 2 +- .../strategic_merge_patch.go | 11 +- 5 files changed, 101 insertions(+), 25 deletions(-) diff --git a/internal/resourcemodifiers/json_merge_patch.go b/internal/resourcemodifiers/json_merge_patch.go index 18fe564f4..bffaeef70 100644 --- a/internal/resourcemodifiers/json_merge_patch.go +++ b/internal/resourcemodifiers/json_merge_patch.go @@ -1,6 +1,7 @@ package resourcemodifiers import ( + "encoding/json" "fmt" jsonpatch "github.com/evanphx/json-patch" @@ -10,7 +11,7 @@ import ( ) type JSONMergePatch struct { - PatchBytes []byte `json:"patchBytes,omitempty"` + PatchData json.RawMessage `json:"patchData,omitempty"` } type JSONMergePatcher struct { @@ -24,14 +25,14 @@ func (p *JSONMergePatcher) Patch(u *unstructured.Unstructured, _ logrus.FieldLog } for _, patch := range p.patches { - patchBytes, err := yaml.YAMLToJSON(patch.PatchBytes) + patchBytes, err := yaml.YAMLToJSON(patch.PatchData) if err != nil { return nil, fmt.Errorf("error in converting YAML to JSON %s", err) } objBytes, err = jsonpatch.MergePatch(objBytes, patchBytes) if err != nil { - return nil, fmt.Errorf("error in applying JSON Patch %s", err.Error()) + return nil, fmt.Errorf("error in applying JSON Patch: %s", err.Error()) } } diff --git a/internal/resourcemodifiers/resource_modifiers.go b/internal/resourcemodifiers/resource_modifiers.go index d9a2baefa..2f19ee39c 100644 --- a/internal/resourcemodifiers/resource_modifiers.go +++ b/internal/resourcemodifiers/resource_modifiers.go @@ -163,7 +163,7 @@ func unmarshalResourceModifiers(yamlData []byte) (*ResourceModifiers, error) { resModifiers := &ResourceModifiers{} err := yaml.UnmarshalStrict(yamlData, resModifiers) if err != nil { - return nil, fmt.Errorf("failed to decode yaml data into resource modifiers %v", err) + return nil, fmt.Errorf("failed to decode yaml data into resource modifiers, err: %s", err) } return resModifiers, nil } diff --git a/internal/resourcemodifiers/resource_modifiers_test.go b/internal/resourcemodifiers/resource_modifiers_test.go index 150078583..5c232d887 100644 --- a/internal/resourcemodifiers/resource_modifiers_test.go +++ b/internal/resourcemodifiers/resource_modifiers_test.go @@ -120,6 +120,64 @@ func TestGetResourceModifiersFromConfig(t *testing.T) { }, } + cm5 := &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:\n annotations:\n foo: null", + }, + } + + rules5 := &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "pods", + Namespaces: []string{ + "ns1", + }, + }, + MergePatches: []JSONMergePatch{ + { + PatchData: []byte(`{"metadata":{"annotations":{"foo":null}}}`), + }, + }, + }, + }, + } + + cm6 := &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:\n containers:\n - name: nginx\n image: repo2/nginx", + }, + } + + rules6 := &ResourceModifiers{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "pods", + Namespaces: []string{ + "ns1", + }, + }, + StrategicPatches: []StrategicMergePatch{ + { + PatchData: []byte(`{"spec":{"containers":[{"image":"repo2/nginx","name":"nginx"}]}}`), + }, + }, + }, + }, + } + type args struct { cm *v1.ConfigMap } @@ -169,6 +227,22 @@ func TestGetResourceModifiersFromConfig(t *testing.T) { want: nil, wantErr: true, }, + { + name: "complex payload with json merge patch", + args: args{ + cm: cm5, + }, + want: rules5, + wantErr: false, + }, + { + name: "complex payload with strategic merge patch", + args: args{ + cm: cm6, + }, + want: rules6, + wantErr: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -178,7 +252,7 @@ func TestGetResourceModifiersFromConfig(t *testing.T) { return } if !reflect.DeepEqual(got, tt.want) { - t.Errorf("GetResourceModifiersFromConfig() = %v, want %v", got, tt.want) + t.Errorf("GetResourceModifiersFromConfig() = %#v, want %#v", got, tt.want) } }) } @@ -910,7 +984,7 @@ func TestResourceModifiers_ApplyResourceModifierRules_StrategicMergePatch(t *tes }, StrategicPatches: []StrategicMergePatch{ { - PatchBytes: []byte(`{"spec":{"containers":[{"name":"nginx","image":"nginx1"}]}}`), + PatchData: []byte(`{"spec":{"containers":[{"name":"nginx","image":"nginx1"}]}}`), }, }, }, @@ -933,7 +1007,7 @@ func TestResourceModifiers_ApplyResourceModifierRules_StrategicMergePatch(t *tes }, StrategicPatches: []StrategicMergePatch{ { - PatchBytes: []byte(`spec: + PatchData: []byte(`spec: containers: - name: nginx image: nginx1`), @@ -959,7 +1033,7 @@ func TestResourceModifiers_ApplyResourceModifierRules_StrategicMergePatch(t *tes }, StrategicPatches: []StrategicMergePatch{ { - PatchBytes: []byte(`{"spec":{"volumes":[{"nfs":null,"name":"vol1","persistentVolumeClaim":{"claimName":"pvc1"}}]}}`), + PatchData: []byte(`{"spec":{"volumes":[{"nfs":null,"name":"vol1","persistentVolumeClaim":{"claimName":"pvc1"}}]}}`), }, }, }, @@ -982,7 +1056,7 @@ func TestResourceModifiers_ApplyResourceModifierRules_StrategicMergePatch(t *tes }, StrategicPatches: []StrategicMergePatch{ { - PatchBytes: []byte(`{"spec":{"volumes":[{"$retainKeys":["name","persistentVolumeClaim"],"name":"vol1","persistentVolumeClaim":{"claimName":"pvc1"}}]}}`), + PatchData: []byte(`{"spec":{"volumes":[{"$retainKeys":["name","persistentVolumeClaim"],"name":"vol1","persistentVolumeClaim":{"claimName":"pvc1"}}]}}`), }, }, }, @@ -1005,7 +1079,7 @@ func TestResourceModifiers_ApplyResourceModifierRules_StrategicMergePatch(t *tes }, StrategicPatches: []StrategicMergePatch{ { - PatchBytes: []byte(`{"spec":{"$setElementOrder/ports":[{"port":8001},{"port":9000},{"port":8002}],"ports":[{"name":"fake","port":9000,"protocol":"TCP","targetPort":9000},{"$patch":"delete","port":8000}]}}`), + PatchData: []byte(`{"spec":{"$setElementOrder/ports":[{"port":8001},{"port":9000},{"port":8002}],"ports":[{"name":"fake","port":9000,"protocol":"TCP","targetPort":9000},{"$patch":"delete","port":8000}]}}`), }, }, }, @@ -1062,7 +1136,7 @@ func TestResourceModifiers_ApplyResourceModifierRules_JSONMergePatch(t *testing. }, MergePatches: []JSONMergePatch{ { - PatchBytes: []byte(`{"metadata":{"labels":{"a":"c"}}}`), + PatchData: []byte(`{"metadata":{"labels":{"a":"c"}}}`), }, }, }, @@ -1085,7 +1159,7 @@ func TestResourceModifiers_ApplyResourceModifierRules_JSONMergePatch(t *testing. }, MergePatches: []JSONMergePatch{ { - PatchBytes: []byte(`metadata: + PatchData: []byte(`metadata: labels: a: c`), }, @@ -1110,7 +1184,7 @@ func TestResourceModifiers_ApplyResourceModifierRules_JSONMergePatch(t *testing. }, MergePatches: []JSONMergePatch{ { - PatchBytes: []byte(`{"metadata":{"labels":{"a":null}}}`), + PatchData: []byte(`{"metadata":{"labels":{"a":null}}}`), }, }, }, @@ -1133,7 +1207,7 @@ func TestResourceModifiers_ApplyResourceModifierRules_JSONMergePatch(t *testing. }, MergePatches: []JSONMergePatch{ { - PatchBytes: []byte(`{"metadata":{"labels":{"a":"b"}}}`), + PatchData: []byte(`{"metadata":{"labels":{"a":"b"}}}`), }, }, }, @@ -1156,7 +1230,7 @@ func TestResourceModifiers_ApplyResourceModifierRules_JSONMergePatch(t *testing. }, MergePatches: []JSONMergePatch{ { - PatchBytes: []byte(`{"metadata":{"labels":{"a":null}}}`), + PatchData: []byte(`{"metadata":{"labels":{"a":null}}}`), }, }, }, @@ -1209,7 +1283,7 @@ func TestResourceModifiers_wildcard_in_GroupResource(t *testing.T) { }, MergePatches: []JSONMergePatch{ { - PatchBytes: []byte(`{"metadata":{"labels":{"a":"c"}}}`), + PatchData: []byte(`{"metadata":{"labels":{"a":"c"}}}`), }, }, }, @@ -1232,7 +1306,7 @@ func TestResourceModifiers_wildcard_in_GroupResource(t *testing.T) { }, MergePatches: []JSONMergePatch{ { - PatchBytes: []byte(`{"metadata":{"labels":{"a":"c"}}}`), + PatchData: []byte(`{"metadata":{"labels":{"a":"c"}}}`), }, }, }, @@ -1291,7 +1365,7 @@ func TestResourceModifiers_conditional_patches(t *testing.T) { }, MergePatches: []JSONMergePatch{ { - PatchBytes: []byte(`{"metadata":{"labels":{"a":"c"}}}`), + PatchData: []byte(`{"metadata":{"labels":{"a":"c"}}}`), }, }, }, @@ -1320,7 +1394,7 @@ func TestResourceModifiers_conditional_patches(t *testing.T) { }, MergePatches: []JSONMergePatch{ { - PatchBytes: []byte(`{"metadata":{"labels":{"a":"c"}}}`), + PatchData: []byte(`{"metadata":{"labels":{"a":"c"}}}`), }, }, }, diff --git a/internal/resourcemodifiers/resource_modifiers_validator_test.go b/internal/resourcemodifiers/resource_modifiers_validator_test.go index 626c72a18..32feb6e79 100644 --- a/internal/resourcemodifiers/resource_modifiers_validator_test.go +++ b/internal/resourcemodifiers/resource_modifiers_validator_test.go @@ -132,7 +132,7 @@ func TestResourceModifiers_Validate(t *testing.T) { }, MergePatches: []JSONMergePatch{ { - PatchBytes: []byte(`{"metadata":{"labels":{"a":null}}}`), + PatchData: []byte(`{"metadata":{"labels":{"a":null}}}`), }, }, }, diff --git a/internal/resourcemodifiers/strategic_merge_patch.go b/internal/resourcemodifiers/strategic_merge_patch.go index 4f7945cc5..e18cbdbb4 100644 --- a/internal/resourcemodifiers/strategic_merge_patch.go +++ b/internal/resourcemodifiers/strategic_merge_patch.go @@ -1,6 +1,7 @@ package resourcemodifiers import ( + "encoding/json" "fmt" "net/http" @@ -13,12 +14,12 @@ import ( "k8s.io/apimachinery/pkg/util/mergepatch" "k8s.io/apimachinery/pkg/util/strategicpatch" "k8s.io/apimachinery/pkg/util/validation/field" - "sigs.k8s.io/json" + kubejson "sigs.k8s.io/json" "sigs.k8s.io/yaml" ) type StrategicMergePatch struct { - PatchBytes []byte `json:"patchBytes,omitempty"` + PatchData json.RawMessage `json:"patchData,omitempty"` } type StrategicMergePatcher struct { @@ -36,7 +37,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.PatchBytes) + patchBytes, err := yaml.YAMLToJSON(patch.PatchData) if err != nil { return nil, fmt.Errorf("error in converting YAML to JSON %s", err) } @@ -72,12 +73,12 @@ func strategicPatchObject( patchMap := make(map[string]interface{}) var strictErrs []error if validationDirective == metav1.FieldValidationWarn || validationDirective == metav1.FieldValidationStrict { - strictErrs, err = json.UnmarshalStrict(patchBytes, &patchMap) + strictErrs, err = kubejson.UnmarshalStrict(patchBytes, &patchMap) if err != nil { return apierrors.NewBadRequest(err.Error()) } } else { - if err = json.UnmarshalCaseSensitivePreserveInts(patchBytes, &patchMap); err != nil { + if err = kubejson.UnmarshalCaseSensitivePreserveInts(patchBytes, &patchMap); err != nil { return apierrors.NewBadRequest(err.Error()) } } From a607810b1386196cf2f0ca8501fe0b1f4c810131 Mon Sep 17 00:00:00 2001 From: lou Date: Tue, 10 Oct 2023 19:11:43 +0800 Subject: [PATCH 07/12] update design Signed-off-by: lou --- ...atch-and-strategic-in-resource-modifier.md | 38 +++++++------------ 1 file changed, 13 insertions(+), 25 deletions(-) diff --git a/design/merge-patch-and-strategic-in-resource-modifier.md b/design/merge-patch-and-strategic-in-resource-modifier.md index 0259ca642..aa082abea 100644 --- a/design/merge-patch-and-strategic-in-resource-modifier.md +++ b/design/merge-patch-and-strategic-in-resource-modifier.md @@ -68,17 +68,12 @@ 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,25 +83,18 @@ Example of StrategicPatches in ResourceModifierRule version: v1 resourceModifierRules: - conditions: - groupResource: pods - resourceNameRegex: "^my-pod$" - namespaces: - - ns1 + groupResource: pods + resourceNameRegex: "^my-pod$" + 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`. From 6d89780fb201cfd4d1ebf101be3c42d9dfee25b9 Mon Sep 17 00:00:00 2001 From: lou Date: Tue, 10 Oct 2023 22:33:35 +0800 Subject: [PATCH 08/12] add more tests Signed-off-by: lou --- .../json_merge_patch_test.go | 41 +++++++++++++++ .../strategic_merge_patch.go | 30 ++++------- .../strategic_merge_patch_test.go | 52 +++++++++++++++++++ 3 files changed, 103 insertions(+), 20 deletions(-) create mode 100644 internal/resourcemodifiers/json_merge_patch_test.go create mode 100644 internal/resourcemodifiers/strategic_merge_patch_test.go diff --git a/internal/resourcemodifiers/json_merge_patch_test.go b/internal/resourcemodifiers/json_merge_patch_test.go new file mode 100644 index 000000000..53142c559 --- /dev/null +++ b/internal/resourcemodifiers/json_merge_patch_test.go @@ -0,0 +1,41 @@ +package resourcemodifiers + +import ( + "testing" + + "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" +) + +func TestJsonMergePatchFailure(t *testing.T) { + tests := []struct { + name string + data []byte + }{ + { + name: "patch with bad yaml", + data: []byte("a: b:"), + }, + { + name: "patch with bad json", + data: []byte(`{"a"::1}`), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + scheme := runtime.NewScheme() + err := clientgoscheme.AddToScheme(scheme) + assert.NoError(t, err) + pt := &JSONMergePatcher{ + patches: []JSONMergePatch{{PatchData: tt.data}}, + } + + u := &unstructured.Unstructured{} + _, err = pt.Patch(u, logrus.New()) + assert.Error(t, err) + }) + } +} diff --git a/internal/resourcemodifiers/strategic_merge_patch.go b/internal/resourcemodifiers/strategic_merge_patch.go index e18cbdbb4..eaa1e2986 100644 --- a/internal/resourcemodifiers/strategic_merge_patch.go +++ b/internal/resourcemodifiers/strategic_merge_patch.go @@ -7,7 +7,6 @@ import ( "github.com/sirupsen/logrus" apierrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -42,7 +41,7 @@ func (p *StrategicMergePatcher) Patch(u *unstructured.Unstructured, _ logrus.Fie return nil, fmt.Errorf("error in converting YAML to JSON %s", err) } - err = strategicPatchObject(origin, patchBytes, updated, schemaReferenceObj, metav1.FieldValidationStrict) + err = strategicPatchObject(origin, patchBytes, updated, schemaReferenceObj) if err != nil { return nil, fmt.Errorf("error in applying Strategic Patch %s", err.Error()) } @@ -63,7 +62,6 @@ func strategicPatchObject( patchBytes []byte, objToUpdate runtime.Object, schemaReferenceObj runtime.Object, - validationDirective string, ) error { originalObjMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(originalObject) if err != nil { @@ -72,18 +70,12 @@ func strategicPatchObject( patchMap := make(map[string]interface{}) var strictErrs []error - if validationDirective == metav1.FieldValidationWarn || validationDirective == metav1.FieldValidationStrict { - strictErrs, err = kubejson.UnmarshalStrict(patchBytes, &patchMap) - if err != nil { - return apierrors.NewBadRequest(err.Error()) - } - } else { - if err = kubejson.UnmarshalCaseSensitivePreserveInts(patchBytes, &patchMap); err != nil { - return apierrors.NewBadRequest(err.Error()) - } + strictErrs, err = kubejson.UnmarshalStrict(patchBytes, &patchMap) + if err != nil { + return apierrors.NewBadRequest(err.Error()) } - if err := applyPatchToObject(originalObjMap, patchMap, objToUpdate, schemaReferenceObj, strictErrs, validationDirective); err != nil { + if err := applyPatchToObject(originalObjMap, patchMap, objToUpdate, schemaReferenceObj, strictErrs); err != nil { return err } return nil @@ -98,7 +90,6 @@ func applyPatchToObject( objToUpdate runtime.Object, schemaReferenceObj runtime.Object, strictErrs []error, - validationDirective string, ) error { patchedObjMap, err := strategicpatch.StrategicMergeMapPatch(originalMap, patchMap, schemaReferenceObj) if err != nil { @@ -107,8 +98,7 @@ func applyPatchToObject( // Rather than serialize the patched map to JSON, then decode it to an object, we go directly from a map to an object converter := runtime.DefaultUnstructuredConverter - returnUnknownFields := validationDirective == metav1.FieldValidationWarn || validationDirective == metav1.FieldValidationStrict - if err := converter.FromUnstructuredWithValidation(patchedObjMap, objToUpdate, returnUnknownFields); err != nil { + if err := converter.FromUnstructuredWithValidation(patchedObjMap, objToUpdate, true); err != nil { strictError, isStrictError := runtime.AsStrictDecodingError(err) switch { case !isStrictError: @@ -119,8 +109,8 @@ func applyPatchToObject( return apierrors.NewInvalid(schema.GroupKind{}, "", field.ErrorList{ field.Invalid(field.NewPath("patch"), fmt.Sprintf("%+v", patchMap), err.Error()), }) - case validationDirective == metav1.FieldValidationWarn: - //addStrictDecodingWarnings(requestContext, append(strictErrs, strictError.Errors()...)) + //case validationDirective == metav1.FieldValidationWarn: + // addStrictDecodingWarnings(requestContext, append(strictErrs, strictError.Errors()...)) default: strictDecodingError := runtime.NewStrictDecodingError(append(strictErrs, strictError.Errors()...)) return apierrors.NewInvalid(schema.GroupKind{}, "", field.ErrorList{ @@ -129,8 +119,8 @@ func applyPatchToObject( } } else if len(strictErrs) > 0 { switch { - case validationDirective == metav1.FieldValidationWarn: - //addStrictDecodingWarnings(requestContext, strictErrs) + //case validationDirective == metav1.FieldValidationWarn: + // addStrictDecodingWarnings(requestContext, strictErrs) default: return apierrors.NewInvalid(schema.GroupKind{}, "", field.ErrorList{ field.Invalid(field.NewPath("patch"), fmt.Sprintf("%+v", patchMap), runtime.NewStrictDecodingError(strictErrs).Error()), diff --git a/internal/resourcemodifiers/strategic_merge_patch_test.go b/internal/resourcemodifiers/strategic_merge_patch_test.go new file mode 100644 index 000000000..967ef3e66 --- /dev/null +++ b/internal/resourcemodifiers/strategic_merge_patch_test.go @@ -0,0 +1,52 @@ +package resourcemodifiers + +import ( + "testing" + + "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" +) + +func TestStrategicMergePatchFailure(t *testing.T) { + tests := []struct { + name string + data []byte + kind string + }{ + { + name: "patch with unknown kind", + data: []byte("{}"), + kind: "BadKind", + }, + { + name: "patch with bad yaml", + data: []byte("a: b:"), + kind: "Pod", + }, + { + name: "patch with bad json", + data: []byte(`{"a"::1}`), + kind: "Pod", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + scheme := runtime.NewScheme() + err := clientgoscheme.AddToScheme(scheme) + assert.NoError(t, err) + pt := &StrategicMergePatcher{ + patches: []StrategicMergePatch{{PatchData: tt.data}}, + scheme: scheme, + } + + u := &unstructured.Unstructured{} + u.SetGroupVersionKind(schema.GroupVersionKind{Version: "v1", Kind: tt.kind}) + _, err = pt.Patch(u, logrus.New()) + assert.Error(t, err) + }) + } +} From d1f5219cbbb7a577fa815cb0edb2264c0b0bebc2 Mon Sep 17 00:00:00 2001 From: lou Date: Wed, 18 Oct 2023 17:05:00 +0800 Subject: [PATCH 09/12] update after review Signed-off-by: lou --- ...atch-and-strategic-in-resource-modifier.md | 30 +++-- .../resourcemodifiers/json_merge_patch.go | 5 +- .../json_merge_patch_test.go | 6 +- .../resource_modifiers_test.go | 118 ++++++++++++++---- .../resource_modifiers_validator_test.go | 2 +- .../strategic_merge_patch.go | 5 +- .../strategic_merge_patch_test.go | 8 +- 7 files changed, 129 insertions(+), 45 deletions(-) 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", }, } From 4ead4d6976f28315b1fc52c0908eca3174160538 Mon Sep 17 00:00:00 2001 From: lou Date: Tue, 24 Oct 2023 21:44:14 +0800 Subject: [PATCH 10/12] update after review Signed-off-by: lou --- .../resourcemodifiers/resource_modifiers.go | 23 ++++-- .../resource_modifiers_test.go | 42 +++++++++- .../docs/main/restore-resource-modifiers.md | 78 +++++++++++++++++++ 3 files changed, 133 insertions(+), 10 deletions(-) diff --git a/internal/resourcemodifiers/resource_modifiers.go b/internal/resourcemodifiers/resource_modifiers.go index ee631eac1..46f4aa859 100644 --- a/internal/resourcemodifiers/resource_modifiers.go +++ b/internal/resourcemodifiers/resource_modifiers.go @@ -97,9 +97,12 @@ func (p *ResourceModifiers) ApplyResourceModifierRules(obj *unstructured.Unstruc } func (r *ResourceModifierRule) apply(obj *unstructured.Unstructured, groupResource string, scheme *runtime.Scheme, log logrus.FieldLogger) error { - namespaceInclusion := collections.NewIncludesExcludes().Includes(r.Conditions.Namespaces...) - if !namespaceInclusion.ShouldInclude(obj.GetNamespace()) { - return nil + ns := obj.GetNamespace() + if ns != "" { + namespaceInclusion := collections.NewIncludesExcludes().Includes(r.Conditions.Namespaces...) + if !namespaceInclusion.ShouldInclude(ns) { + return nil + } } g, err := glob.Compile(r.Conditions.GroupResource) @@ -148,17 +151,21 @@ func (r *ResourceModifierRule) apply(obj *unstructured.Unstructured, groupResour return nil } -func matchConditions(u *unstructured.Unstructured, patches []MatchRule, _ logrus.FieldLogger) (bool, error) { - if len(patches) == 0 { +func matchConditions(u *unstructured.Unstructured, rules []MatchRule, _ logrus.FieldLogger) (bool, error) { + if len(rules) == 0 { return true, nil } var fixed []JSONPatch - for _, patch := range patches { + for _, rule := range rules { + if rule.Path == "" { + return false, fmt.Errorf("path is required for match rule") + } + fixed = append(fixed, JSONPatch{ Operation: "test", - Path: patch.Path, - Value: patch.Value, + Path: rule.Path, + Value: rule.Value, }) } diff --git a/internal/resourcemodifiers/resource_modifiers_test.go b/internal/resourcemodifiers/resource_modifiers_test.go index 7b4b87e5b..b09b473d9 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 matches:\n - path: /metadata/annotations/foo\n value: bar\n mergePatches:\n - patchData: |\n metadata:\n annotations:\n foo: null", }, } @@ -154,6 +154,12 @@ func TestGetResourceModifiersFromConfig(t *testing.T) { Namespaces: []string{ "ns1", }, + Matches: []MatchRule{ + { + Path: "/metadata/annotations/foo", + Value: "bar", + }, + }, }, MergePatches: []JSONMergePatch{ { @@ -341,7 +347,7 @@ func TestGetResourceModifiersFromConfig(t *testing.T) { return } if !reflect.DeepEqual(got, tt.want) { - t.Errorf("GetResourceModifiersFromConfig() = %#v, want %#v", got, tt.want) + t.Errorf("GetResourceModifiersFromConfig() = %v, want %v", got, tt.want) } }) } @@ -654,6 +660,38 @@ func TestResourceModifiers_ApplyResourceModifierRules(t *testing.T) { wantErr: false, wantObj: deployNginxTwoReplica.DeepCopy(), }, + { + name: "nginx deployment: Empty Resource Regex", + fields: fields{ + Version: "v1", + ResourceModifierRules: []ResourceModifierRule{ + { + Conditions: Conditions{ + GroupResource: "deployments.apps", + Namespaces: []string{"foo"}, + }, + Patches: []JSONPatch{ + { + Operation: "test", + Path: "/spec/replicas", + Value: "1", + }, + { + Operation: "replace", + Path: "/spec/replicas", + Value: "2", + }, + }, + }, + }, + }, + args: args{ + obj: deployNginxOneReplica.DeepCopy(), + groupResource: "deployments.apps", + }, + wantErr: false, + wantObj: deployNginxTwoReplica.DeepCopy(), + }, { name: "nginx deployment: Empty Resource Regex and namespaces list", fields: fields{ diff --git a/site/content/docs/main/restore-resource-modifiers.md b/site/content/docs/main/restore-resource-modifiers.md index 0d59fad5a..0e12d7c53 100644 --- a/site/content/docs/main/restore-resource-modifiers.md +++ b/site/content/docs/main/restore-resource-modifiers.md @@ -105,3 +105,81 @@ resourceModifierRules: - Update a container's image using a json patch with positional arrays kubectl patch pod valid-pod -type='json' -p='[{"op": "replace", "path": "/spec/containers/0/image", "value":"new image"}]' - Before creating the resource modifier yaml, you can try it out using kubectl patch command. The same commands should work as it is. + +### New features introduced in 1.13 + +#### JSON Merge Patch +you can modify a resource using JSON Merge Patch +```yaml +version: v1 +resourceModifierRules: +- conditions: + groupResource: pods + namespaces: + - ns1 + mergePatches: + - 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. + +#### Strategic Merge Patch +you can modify a resource using Strategic Merge Patch +```yaml +version: v1 +resourceModifierRules: +- conditions: + groupResource: pods + resourceNameRegex: "^my-pod$" + namespaces: + - ns1 + strategicPatches: + - 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 use the `test` operation of JSON Patch to support conditional patches in all patch types by adding it to `Conditions` struct in `ResourceModifierRule`. + +Example of test in conditions +```yaml +version: v1 +resourceModifierRules: +- conditions: + groupResource: persistentvolumeclaims.storage.k8s.io + matches: + - path: "/spec/storageClassName" + value: "premium" + mergePatches: + - patchData: | + { + "metadata": { + "annotations": { + "foo": null + } + } + } +``` +- The above configmap will apply the Merge Patch to all the PVCs in all namespaces with storageClassName premium and remove the annotation `foo` from the PVCs. +- You can specify multiple rules in the `matches` list. The patch will be applied only if all the matches are satisfied. + +### Wildcard Support for GroupResource +The user can specify a wildcard for groupResource in the conditions' struct. This will allow the user to apply the patches for all the resources of a particular group or all resources in all groups. For example, `*.apps` will apply to all the resources in the `apps` group, `*` will apply to all the resources in all groups. From f66016d416a9494ad609b80c1493b8db0bfbcb9e Mon Sep 17 00:00:00 2001 From: lou Date: Wed, 25 Oct 2023 17:54:20 +0800 Subject: [PATCH 11/12] update docs Signed-off-by: lou --- .../content/docs/main/restore-resource-modifiers.md | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/site/content/docs/main/restore-resource-modifiers.md b/site/content/docs/main/restore-resource-modifiers.md index 0e12d7c53..241d6bc99 100644 --- a/site/content/docs/main/restore-resource-modifiers.md +++ b/site/content/docs/main/restore-resource-modifiers.md @@ -106,10 +106,8 @@ resourceModifierRules: kubectl patch pod valid-pod -type='json' -p='[{"op": "replace", "path": "/spec/containers/0/image", "value":"new image"}]' - Before creating the resource modifier yaml, you can try it out using kubectl patch command. The same commands should work as it is. -### New features introduced in 1.13 - #### JSON Merge Patch -you can modify a resource using JSON Merge Patch +You can modify a resource using JSON Merge Patch ```yaml version: v1 resourceModifierRules: @@ -129,9 +127,10 @@ resourceModifierRules: ``` - 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. +- For more details, please refer to [this doc](https://kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch/) #### Strategic Merge Patch -you can modify a resource using Strategic Merge Patch +You can modify a resource using Strategic Merge Patch ```yaml version: v1 resourceModifierRules: @@ -155,11 +154,13 @@ resourceModifierRules: ``` - 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. +- For more details, please refer to [this doc](https://kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch/) + ### Conditional Patches in ALL Patch Types -Since JSON Merge Patch and Strategic Merge Patch do not support conditional patches, we use the `test` operation of JSON Patch to support conditional patches in all patch types by adding it to `Conditions` struct in `ResourceModifierRule`. +A new field `matches` is added in conditions to support conditional patches. -Example of test in conditions +Example of matches in conditions ```yaml version: v1 resourceModifierRules: From e30937550e6bf981bea11e0d67ff76014c2523b2 Mon Sep 17 00:00:00 2001 From: lou Date: Wed, 1 Nov 2023 21:53:30 +0800 Subject: [PATCH 12/12] update after review Signed-off-by: lou --- internal/resourcemodifiers/resource_modifiers.go | 2 +- site/content/docs/main/restore-resource-modifiers.md | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/resourcemodifiers/resource_modifiers.go b/internal/resourcemodifiers/resource_modifiers.go index 46f4aa859..d4ae93e71 100644 --- a/internal/resourcemodifiers/resource_modifiers.go +++ b/internal/resourcemodifiers/resource_modifiers.go @@ -105,7 +105,7 @@ func (r *ResourceModifierRule) apply(obj *unstructured.Unstructured, groupResour } } - g, err := glob.Compile(r.Conditions.GroupResource) + 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 diff --git a/site/content/docs/main/restore-resource-modifiers.md b/site/content/docs/main/restore-resource-modifiers.md index 241d6bc99..0c1f2f217 100644 --- a/site/content/docs/main/restore-resource-modifiers.md +++ b/site/content/docs/main/restore-resource-modifiers.md @@ -183,4 +183,5 @@ resourceModifierRules: - You can specify multiple rules in the `matches` list. The patch will be applied only if all the matches are satisfied. ### Wildcard Support for GroupResource -The user can specify a wildcard for groupResource in the conditions' struct. This will allow the user to apply the patches for all the resources of a particular group or all resources in all groups. For example, `*.apps` will apply to all the resources in the `apps` group, `*` will apply to all the resources in all groups. +The user can specify a wildcard for groupResource in the conditions' struct. This will allow the user to apply the patches for all the resources of a particular group or all resources in all groups. For example, `*.apps` will apply to all the resources in the `apps` group, `*` will apply to all the resources in core group, `*.*` will apply to all the resources in all groups. +- If both `*.groupName` and `namespaces` are specified, the patches will be applied to all the namespaced resources in this group in the specified namespaces and all the cluster resources in this group. \ No newline at end of file