Merge pull request #2359 from mortent/PatchTransformerFilter

Update PatchTransformer to use kyaml filters

api/builtins/PatchTransformer.go

@@ -8,9 +8,12 @@ import (
 
 	jsonpatch "github.com/evanphx/json-patch"
 	"github.com/pkg/errors"
+	"sigs.k8s.io/kustomize/api/filters/patchjson6902"
+	"sigs.k8s.io/kustomize/api/filters/patchstrategicmerge"
 	"sigs.k8s.io/kustomize/api/resmap"
 	"sigs.k8s.io/kustomize/api/resource"
 	"sigs.k8s.io/kustomize/api/types"
+	"sigs.k8s.io/kustomize/kyaml/filtersutil"
 	"sigs.k8s.io/yaml"
 )
 
@@ -20,6 +23,8 @@ type PatchTransformerPlugin struct {
 	Path         string          `json:"path,omitempty" yaml:"path,omitempty"`
 	Patch        string          `json:"patch,omitempty" yaml:"patch,omitempty"`
 	Target       *types.Selector `json:"target,omitempty" yaml:"target,omitempty"`
+
+	YAMLSupport bool `json:"yamlSupport,omitempty" yaml:"yamlSupport,omitempty"`
 }
 
 func (p *PatchTransformerPlugin) Config(
@@ -38,19 +43,17 @@ func (p *PatchTransformerPlugin) Config(
 			"patch and path can't be set at the same time\n%s", string(c))
 		return
 	}
-	var in []byte
+
 	if p.Path != "" {
-		in, err = h.Loader().Load(p.Path)
+		loaded, loadErr := h.Loader().Load(p.Path)
-		if err != nil {
+		if loadErr != nil {
-			return
+			return loadErr
 		}
-	}
+		p.Patch = string(loaded)
-	if p.Patch != "" {
-		in = []byte(p.Patch)
 	}
 
-	patchSM, errSM := h.ResmapFactory().RF().FromBytes(in)
+	patchSM, errSM := h.ResmapFactory().RF().FromBytes([]byte(p.Patch))
-	patchJson, errJson := jsonPatchFromBytes(in)
+	patchJson, errJson := jsonPatchFromBytes([]byte(p.Patch))
 	if errSM != nil && errJson != nil {
 		err = fmt.Errorf(
 			"unable to get either a Strategic Merge Patch or JSON patch 6902 from %s", p.Patch)
@@ -71,18 +74,63 @@ func (p *PatchTransformerPlugin) Config(
 }
 
 func (p *PatchTransformerPlugin) Transform(m resmap.ResMap) error {
-	if p.loadedPatch != nil && p.Target == nil {
+	if p.loadedPatch != nil {
-		target, err := m.GetById(p.loadedPatch.OrgId())
+		// The patch was a strategic merge patch
+		return p.transformStrategicMerge(m, p.loadedPatch)
+	} else {
+		return p.transformJson6902(m, p.decodedPatch)
+	}
+}
+
+// transformStrategicMerge applies the provided strategic merge patch
+// to all the resources in the ResMap that match either the Target or
+// the identifier of the patch.
+func (p *PatchTransformerPlugin) transformStrategicMerge(m resmap.ResMap, patch *resource.Resource) error {
+	if p.Target == nil {
+		target, err := m.GetById(patch.OrgId())
 		if err != nil {
 			return err
 		}
-		err = target.Patch(p.loadedPatch.Kunstructured)
+		return p.applySMPatch(target, patch)
-		if err != nil {
-			return err
-		}
-		return nil
 	}
 
+	resources, err := m.Select(*p.Target)
+	if err != nil {
+		return err
+	}
+	for _, res := range resources {
+		patchCopy := patch.DeepCopy()
+		patchCopy.SetName(res.GetName())
+		patchCopy.SetNamespace(res.GetNamespace())
+		patchCopy.SetGvk(res.GetGvk())
+		err := p.applySMPatch(res, patchCopy)
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+// applySMPatch applies the provided strategic merge patch to the
+// given resource. Depending on the value of YAMLSupport, it will either
+// use the legacy implementation or the kyaml-based solution.
+func (p *PatchTransformerPlugin) applySMPatch(resource, patch *resource.Resource) error {
+	if !p.YAMLSupport {
+		return resource.Patch(patch.Kunstructured)
+	} else {
+		node, err := filtersutil.GetRNode(patch)
+		if err != nil {
+			return err
+		}
+		return filtersutil.ApplyToJSON(patchstrategicmerge.Filter{
+			Patch: node,
+		}, resource.Kunstructured)
+	}
+}
+
+// transformJson6902 applies the provided json6902 patch
+// to all the resources in the ResMap that match the Target.
+func (p *PatchTransformerPlugin) transformJson6902(m resmap.ResMap, patch jsonpatch.Patch) error {
 	if p.Target == nil {
 		return fmt.Errorf("must specify a target for patch %s", p.Patch)
 	}
@@ -92,35 +140,36 @@ func (p *PatchTransformerPlugin) Transform(m resmap.ResMap) error {
 		return err
 	}
 	for _, res := range resources {
-		if p.decodedPatch != nil {
+		err = p.applyJson6902Patch(res, patch)
-			rawObj, err := res.MarshalJSON()
+		if err != nil {
-			if err != nil {
+			return err
-				return err
-			}
-			modifiedObj, err := p.decodedPatch.Apply(rawObj)
-			if err != nil {
-				return errors.Wrapf(
-					err, "failed to apply json patch '%s'", p.Patch)
-			}
-			err = res.UnmarshalJSON(modifiedObj)
-			if err != nil {
-				return err
-			}
-		}
-		if p.loadedPatch != nil {
-			patchCopy := p.loadedPatch.DeepCopy()
-			patchCopy.SetName(res.GetName())
-			patchCopy.SetNamespace(res.GetNamespace())
-			patchCopy.SetGvk(res.GetGvk())
-			err = res.Patch(patchCopy.Kunstructured)
-			if err != nil {
-				return err
-			}
 		}
 	}
 	return nil
 }
 
+// applyJson6902Patch applies the provided patch to the given resource.
+// Depending on the value of YAMLSupport, it will either
+// use the legacy implementation or the kyaml-based solution.
+func (p *PatchTransformerPlugin) applyJson6902Patch(resource *resource.Resource, patch jsonpatch.Patch) error {
+	if !p.YAMLSupport {
+		rawObj, err := resource.MarshalJSON()
+		if err != nil {
+			return err
+		}
+		modifiedObj, err := patch.Apply(rawObj)
+		if err != nil {
+			return errors.Wrapf(
+				err, "failed to apply json patch '%s'", p.Patch)
+		}
+		return resource.UnmarshalJSON(modifiedObj)
+	} else {
+		return filtersutil.ApplyToJSON(patchjson6902.Filter{
+			Patch: p.Patch,
+		}, resource.Kunstructured)
+	}
+}
+
 // jsonPatchFromBytes loads a Json 6902 patch from
 // a bytes input
 func jsonPatchFromBytes(

plugin/builtin/patchtransformer/PatchTransformer.go

@@ -7,11 +7,14 @@ package main
 import (
 	"fmt"
 
-	"github.com/evanphx/json-patch"
+	jsonpatch "github.com/evanphx/json-patch"
 	"github.com/pkg/errors"
+	"sigs.k8s.io/kustomize/api/filters/patchjson6902"
+	"sigs.k8s.io/kustomize/api/filters/patchstrategicmerge"
 	"sigs.k8s.io/kustomize/api/resmap"
 	"sigs.k8s.io/kustomize/api/resource"
 	"sigs.k8s.io/kustomize/api/types"
+	"sigs.k8s.io/kustomize/kyaml/filtersutil"
 	"sigs.k8s.io/yaml"
 )
 
@@ -21,6 +24,8 @@ type plugin struct {
 	Path         string          `json:"path,omitempty" yaml:"path,omitempty"`
 	Patch        string          `json:"patch,omitempty" yaml:"patch,omitempty"`
 	Target       *types.Selector `json:"target,omitempty" yaml:"target,omitempty"`
+
+	YAMLSupport bool `json:"yamlSupport,omitempty" yaml:"yamlSupport,omitempty"`
 }
 
 //noinspection GoUnusedGlobalVariable
@@ -42,19 +47,17 @@ func (p *plugin) Config(
 			"patch and path can't be set at the same time\n%s", string(c))
 		return
 	}
-	var in []byte
+
 	if p.Path != "" {
-		in, err = h.Loader().Load(p.Path)
+		loaded, loadErr := h.Loader().Load(p.Path)
-		if err != nil {
+		if loadErr != nil {
-			return
+			return loadErr
 		}
-	}
+		p.Patch = string(loaded)
-	if p.Patch != "" {
-		in = []byte(p.Patch)
 	}
 
-	patchSM, errSM := h.ResmapFactory().RF().FromBytes(in)
+	patchSM, errSM := h.ResmapFactory().RF().FromBytes([]byte(p.Patch))
-	patchJson, errJson := jsonPatchFromBytes(in)
+	patchJson, errJson := jsonPatchFromBytes([]byte(p.Patch))
 	if errSM != nil && errJson != nil {
 		err = fmt.Errorf(
 			"unable to get either a Strategic Merge Patch or JSON patch 6902 from %s", p.Patch)
@@ -75,18 +78,63 @@ func (p *plugin) Config(
 }
 
 func (p *plugin) Transform(m resmap.ResMap) error {
-	if p.loadedPatch != nil && p.Target == nil {
+	if p.loadedPatch != nil {
-		target, err := m.GetById(p.loadedPatch.OrgId())
+		// The patch was a strategic merge patch
+		return p.transformStrategicMerge(m, p.loadedPatch)
+	} else {
+		return p.transformJson6902(m, p.decodedPatch)
+	}
+}
+
+// transformStrategicMerge applies the provided strategic merge patch
+// to all the resources in the ResMap that match either the Target or
+// the identifier of the patch.
+func (p *plugin) transformStrategicMerge(m resmap.ResMap, patch *resource.Resource) error {
+	if p.Target == nil {
+		target, err := m.GetById(patch.OrgId())
 		if err != nil {
 			return err
 		}
-		err = target.Patch(p.loadedPatch.Kunstructured)
+		return p.applySMPatch(target, patch)
-		if err != nil {
-			return err
-		}
-		return nil
 	}
 
+	resources, err := m.Select(*p.Target)
+	if err != nil {
+		return err
+	}
+	for _, res := range resources {
+		patchCopy := patch.DeepCopy()
+		patchCopy.SetName(res.GetName())
+		patchCopy.SetNamespace(res.GetNamespace())
+		patchCopy.SetGvk(res.GetGvk())
+		err := p.applySMPatch(res, patchCopy)
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+// applySMPatch applies the provided strategic merge patch to the
+// given resource. Depending on the value of YAMLSupport, it will either
+// use the legacy implementation or the kyaml-based solution.
+func (p *plugin) applySMPatch(resource, patch *resource.Resource) error {
+	if !p.YAMLSupport {
+		return resource.Patch(patch.Kunstructured)
+	} else {
+		node, err := filtersutil.GetRNode(patch)
+		if err != nil {
+			return err
+		}
+		return filtersutil.ApplyToJSON(patchstrategicmerge.Filter{
+			Patch: node,
+		}, resource.Kunstructured)
+	}
+}
+
+// transformJson6902 applies the provided json6902 patch
+// to all the resources in the ResMap that match the Target.
+func (p *plugin) transformJson6902(m resmap.ResMap, patch jsonpatch.Patch) error {
 	if p.Target == nil {
 		return fmt.Errorf("must specify a target for patch %s", p.Patch)
 	}
@@ -96,35 +144,36 @@ func (p *plugin) Transform(m resmap.ResMap) error {
 		return err
 	}
 	for _, res := range resources {
-		if p.decodedPatch != nil {
+		err = p.applyJson6902Patch(res, patch)
-			rawObj, err := res.MarshalJSON()
+		if err != nil {
-			if err != nil {
+			return err
-				return err
-			}
-			modifiedObj, err := p.decodedPatch.Apply(rawObj)
-			if err != nil {
-				return errors.Wrapf(
-					err, "failed to apply json patch '%s'", p.Patch)
-			}
-			err = res.UnmarshalJSON(modifiedObj)
-			if err != nil {
-				return err
-			}
-		}
-		if p.loadedPatch != nil {
-			patchCopy := p.loadedPatch.DeepCopy()
-			patchCopy.SetName(res.GetName())
-			patchCopy.SetNamespace(res.GetNamespace())
-			patchCopy.SetGvk(res.GetGvk())
-			err = res.Patch(patchCopy.Kunstructured)
-			if err != nil {
-				return err
-			}
 		}
 	}
 	return nil
 }
 
+// applyJson6902Patch applies the provided patch to the given resource.
+// Depending on the value of YAMLSupport, it will either
+// use the legacy implementation or the kyaml-based solution.
+func (p *plugin) applyJson6902Patch(resource *resource.Resource, patch jsonpatch.Patch) error {
+	if !p.YAMLSupport {
+		rawObj, err := resource.MarshalJSON()
+		if err != nil {
+			return err
+		}
+		modifiedObj, err := patch.Apply(rawObj)
+		if err != nil {
+			return errors.Wrapf(
+				err, "failed to apply json patch '%s'", p.Patch)
+		}
+		return resource.UnmarshalJSON(modifiedObj)
+	} else {
+		return filtersutil.ApplyToJSON(patchjson6902.Filter{
+			Patch: p.Patch,
+		}, resource.Kunstructured)
+	}
+}
+
 // jsonPatchFromBytes loads a Json 6902 patch from
 // a bytes input
 func jsonPatchFromBytes(

plugin/builtin/patchtransformer/PatchTransformer_test.go

@@ -4,6 +4,7 @@
 package main_test
 
 import (
+	"fmt"
 	"strings"
 	"testing"
 
@@ -251,11 +252,7 @@ spec:
 }
 
 func TestPatchTransformerSmpSidecars(t *testing.T) {
-	th := kusttest_test.MakeEnhancedHarness(t).
+	patch := `
-		PrepBuiltin("PatchTransformer")
-	defer th.Reset()
-
-	th.WriteF("patch.yaml", `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -269,17 +266,32 @@ spec:
           args:
           - proxy
           - sidecar
-`)
+`
 
-	th.RunTransformerAndCheckResult(`
+	config := `
 apiVersion: builtin
 kind: PatchTransformer
 metadata:
   name: notImportantHere
+yamlSupport: %t
 path: patch.yaml
 target:
   name: myDeploy
-`, target, `
+`
+
+	// The expected results with and without yamlSupport is
+	// slightly different for this test. This is because
+	// the two different implementations order the results
+	// differently.
+	testCases := []struct {
+		testName       string
+		yamlSupport    bool
+		expectedOutput string
+	}{
+		{
+			testName:    "yaml=false",
+			yamlSupport: false,
+			expectedOutput: `
 apiVersion: apps/v1
 kind: Deployment
 metadata:
@@ -337,7 +349,87 @@ spec:
         - sidecar
         image: docker.io/istio/proxyv2
         name: istio-proxy
-`)
+`,
+		},
+		{
+			testName:    "yaml=true",
+			yamlSupport: true,
+			expectedOutput: `
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  labels:
+    old-label: old-value
+  name: myDeploy
+spec:
+  replica: 2
+  template:
+    metadata:
+      labels:
+        old-label: old-value
+    spec:
+      containers:
+      - image: nginx
+        name: nginx
+      - args:
+        - proxy
+        - sidecar
+        image: docker.io/istio/proxyv2
+        name: istio-proxy
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  labels:
+    new-label: new-value
+  name: yourDeploy
+spec:
+  replica: 1
+  template:
+    metadata:
+      labels:
+        new-label: new-value
+    spec:
+      containers:
+      - image: nginx:1.7.9
+        name: nginx
+---
+apiVersion: apps/v1
+kind: MyKind
+metadata:
+  label:
+    old-label: old-value
+  name: myDeploy
+spec:
+  template:
+    metadata:
+      labels:
+        old-label: old-value
+    spec:
+      containers:
+      - args:
+        - proxy
+        - sidecar
+        image: docker.io/istio/proxyv2
+        name: istio-proxy
+`,
+		},
+	}
+
+	for i := range testCases {
+		tc := testCases[i]
+		t.Run(tc.testName, func(t *testing.T) {
+			th := kusttest_test.MakeEnhancedHarness(t).
+				PrepBuiltin("PatchTransformer")
+			defer th.Reset()
+
+			th.WriteF("patch.yaml", patch)
+
+			c := fmt.Sprintf(config, tc.yamlSupport)
+			rm := th.LoadAndRunTransformer(c, target)
+			th.AssertActualEqualsExpected(rm, tc.expectedOutput)
+		})
+	}
 }
 
 func TestPatchTransformerWithInlineJson(t *testing.T) {

plugin/builtin/patchtransformer/go.mod

@@ -6,6 +6,7 @@ require (
 	github.com/evanphx/json-patch v4.5.0+incompatible
 	github.com/pkg/errors v0.8.1
 	sigs.k8s.io/kustomize/api v0.3.1
+	sigs.k8s.io/kustomize/kyaml v0.1.5
 	sigs.k8s.io/yaml v1.1.0
 )