Merge pull request #2668 from monopole/doesItWork

Switch namespace and patch transformers to kyaml.

Makefile

@@ -15,7 +15,7 @@ verify-kustomize: \
 	lint-kustomize \
 	test-unit-kustomize-all \
 	test-examples-kustomize-against-HEAD \
-	test-examples-kustomize-against-3.6.1
+	test-examples-kustomize-against-3.7.0
 
 # The following target referenced by a file in
 # https://github.com/kubernetes/test-infra/tree/master/config/jobs/kubernetes-sigs/kustomize
@@ -24,7 +24,7 @@ prow-presubmit-check: \
 	lint-kustomize \
 	test-unit-kustomize-all \
 	test-examples-kustomize-against-HEAD \
-	test-examples-kustomize-against-3.6.1 \
+	test-examples-kustomize-against-3.7.0 \
 	test-unit-cmd-all \
 	test-go-mod
 
@@ -233,10 +233,10 @@ test-examples-kustomize-against-HEAD: $(MYGOBIN)/kustomize $(MYGOBIN)/mdrip
 	./hack/testExamplesAgainstKustomize.sh HEAD
 
 .PHONY:
-test-examples-kustomize-against-3.6.1: $(MYGOBIN)/mdrip
+test-examples-kustomize-against-3.7.0: $(MYGOBIN)/mdrip
 	( \
 		set -e; \
-		tag=v3.6.1; \
+		tag=v3.7.0; \
 		/bin/rm -f $(MYGOBIN)/kustomize; \
 		echo "Installing kustomize $$tag."; \
 		GO111MODULE=on go get sigs.k8s.io/kustomize/kustomize/v3@$${tag}; \

api/builtins/NamespaceTransformer.go

@@ -5,6 +5,7 @@ package builtins
 
 import (
 	"fmt"
+	"strings"
 
 	"sigs.k8s.io/kustomize/api/filters/namespace"
 	"sigs.k8s.io/kustomize/api/resid"
@@ -31,6 +32,11 @@ func (p *NamespaceTransformerPlugin) Config(
 	_ *resmap.PluginHelpers, c []byte) (err error) {
 	p.Namespace = ""
 	p.FieldSpecs = nil
+	if !strings.Contains(string(c), "yamlSupport") {
+		// If not explicitly denied,
+		// activate kyaml-based transformation.
+		p.YAMLSupport = true
+	}
 	return yaml.Unmarshal(c, p)
 }
 

api/builtins/PatchJson6902Transformer.go

@@ -5,6 +5,7 @@ package builtins
 
 import (
 	"fmt"
+	"strings"
 
 	jsonpatch "github.com/evanphx/json-patch"
 	"github.com/pkg/errors"
@@ -34,6 +35,11 @@ func (p *PatchJson6902TransformerPlugin) Config(
 	if err != nil {
 		return err
 	}
+	if !strings.Contains(string(c), "yamlSupport") {
+		// If not explicitly denied,
+		// activate kyaml-based transformation.
+		p.YAMLSupport = true
+	}
 	if p.Target.Name == "" {
 		return fmt.Errorf("must specify the target name")
 	}

api/builtins/PatchStrategicMergeTransformer.go

@@ -5,6 +5,7 @@ package builtins
 
 import (
 	"fmt"
+	"strings"
 
 	"sigs.k8s.io/kustomize/api/filters/patchstrategicmerge"
 	"sigs.k8s.io/kustomize/api/resmap"
@@ -30,6 +31,11 @@ func (p *PatchStrategicMergeTransformerPlugin) Config(
 	if err != nil {
 		return err
 	}
+	if !strings.Contains(string(c), "yamlSupport") {
+		// If not explicitly denied,
+		// activate kyaml-based transformation.
+		p.YAMLSupport = true
+	}
 	if len(p.Paths) == 0 && p.Patches == "" {
 		return fmt.Errorf("empty file path and empty patch content")
 	}
@@ -75,17 +81,6 @@ func (p *PatchStrategicMergeTransformerPlugin) Transform(m resmap.ResMap) error
 		}
 		if !p.YAMLSupport {
 			err = target.Patch(patch.Copy())
-			if err != nil {
-				return err
-			}
-			// remove the resource from resmap
-			// when the patch is to $patch: delete that target
-			if len(target.Map()) == 0 {
-				err = m.Remove(target.CurId())
-				if err != nil {
-					return err
-				}
-			}
 		} else {
 			patchCopy := patch.DeepCopy()
 			patchCopy.SetName(target.GetName())
@@ -99,6 +94,19 @@ func (p *PatchStrategicMergeTransformerPlugin) Transform(m resmap.ResMap) error
 				Patch: node,
 			}, target)
 		}
+		if err != nil {
+			return err
+		}
+		if len(target.Map()) == 0 {
+			// This means all fields have been removed from the object.
+			// This can happen if a patch required deletion of the
+			// entire resource (not just a part of it).  This means
+			// the overall resmap must shrink by one.
+			err = m.Remove(target.CurId())
+			if err != nil {
+				return err
+			}
+		}
 	}
 	return nil
 }

api/builtins/PatchTransformer.go

@@ -34,6 +34,11 @@ func (p *PatchTransformerPlugin) Config(
 	if err != nil {
 		return err
 	}
+	if !strings.Contains(string(c), "yamlSupport") {
+		// If not explicitly denied,
+		// activate kyaml-based transformation.
+		p.YAMLSupport = true
+	}
 	p.Patch = strings.TrimSpace(p.Patch)
 	if p.Patch == "" && p.Path == "" {
 		return fmt.Errorf(

api/filters/patchstrategicmerge/patchstrategicmerge.go

@@ -1,3 +1,6 @@
+// Copyright 2019 The Kubernetes Authors.
+// SPDX-License-Identifier: Apache-2.0
+
 package patchstrategicmerge
 
 import (
@@ -13,9 +16,13 @@ type Filter struct {
 var _ kio.Filter = Filter{}
 
 func (pf Filter) Filter(nodes []*yaml.RNode) ([]*yaml.RNode, error) {
-	return kio.FilterAll(yaml.FilterFunc(pf.run)).Filter(nodes)
+	var result []*yaml.RNode
-}
+	for i := range nodes {
-
+		r, err := merge2.Merge(pf.Patch, nodes[i])
-func (pf Filter) run(node *yaml.RNode) (*yaml.RNode, error) {
+		if err != nil {
-	return merge2.Merge(pf.Patch, node)
+			return nil, err
+		}
+		result = append(result, r)
+	}
+	return result, nil
 }

api/krusty/baseandoverlaymedium_test.go

@@ -225,13 +225,13 @@ spec:
     spec:
       containers:
       - env:
+        - name: foo
+          value: bar
         - name: FOO
           valueFrom:
             configMapKeyRef:
               key: somekey
               name: test-infra-app-env-ffmd9b969m
-        - name: foo
-          value: bar
         image: nginx:1.8.0
         name: nginx
         ports:

api/krusty/customconfig_test.go

@@ -59,6 +59,7 @@ spec:
   location: SW
 `)
 	th.WriteF("/app/base/animalPark.yaml", `
+apiVersion: foo
 kind: AnimalPark
 metadata:
   name: sandiego
@@ -93,6 +94,7 @@ varReference:
 `)
 	m := th.Run("/app/base", th.MakeDefaultOptions())
 	th.AssertActualEqualsExpected(m, `
+apiVersion: foo
 kind: AnimalPark
 metadata:
   labels:
@@ -161,6 +163,7 @@ varReference:
 `)
 	m := th.Run("/app/base", th.MakeDefaultOptions())
 	th.AssertActualEqualsExpected(m, `
+apiVersion: foo
 kind: AnimalPark
 metadata:
   labels:
@@ -212,14 +215,17 @@ func TestFixedBug605_BaseCustomizationAvailableInOverlay(t *testing.T) {
 nameReference:
 - kind: Gorilla
   fieldSpecs:
-  - kind: AnimalPark
+  - apiVersion: foo
+    kind: AnimalPark
     path: spec/gorillaRef/name
 - kind: Giraffe
   fieldSpecs:
-  - kind: AnimalPark
+  - apiVersion: foo
+    kind: AnimalPark
     path: spec/giraffeRef/name
 varReference:
 - path: spec/food
+  apiVersion: foo
   kind: AnimalPark
 `)
 	th.WriteK("/app/overlay", `
@@ -242,6 +248,7 @@ spec:
 `)
 	// The following replaces the gorillaRef in the AnimalPark.
 	th.WriteF("/app/overlay/animalPark.yaml", `
+apiVersion: foo
 kind: AnimalPark
 metadata:
   name: sandiego
@@ -251,6 +258,7 @@ spec:
 `)
 	m := th.Run("/app/overlay", th.MakeDefaultOptions())
 	th.AssertActualEqualsExpected(m, `
+apiVersion: foo
 kind: AnimalPark
 metadata:
   labels:

api/krusty/extendedpatch_test.go

@@ -156,8 +156,7 @@ spec:
         - mountPath: /tmp/ps
           name: busybox-persistent-storage
       volumes:
-      - emptyDir: {}
+      - name: busybox-persistent-storage
-        name: busybox-persistent-storage
       - configMap:
           name: configmap-in-base
         name: configmap-in-base
@@ -233,8 +232,7 @@ spec:
         - mountPath: /tmp/ps
           name: nginx-persistent-storage
       volumes:
-      - emptyDir: {}
+      - name: nginx-persistent-storage
-        name: nginx-persistent-storage
       - configMap:
           name: configmap-in-base
         name: configmap-in-base
@@ -260,8 +258,7 @@ spec:
         - mountPath: /tmp/ps
           name: busybox-persistent-storage
       volumes:
-      - emptyDir: {}
+      - name: busybox-persistent-storage
-        name: busybox-persistent-storage
       - configMap:
           name: configmap-in-base
         name: configmap-in-base
@@ -335,8 +332,7 @@ spec:
         - mountPath: /tmp/ps
           name: nginx-persistent-storage
       volumes:
-      - emptyDir: {}
+      - name: nginx-persistent-storage
-        name: nginx-persistent-storage
       - configMap:
           name: configmap-in-base
         name: configmap-in-base
@@ -463,8 +459,7 @@ spec:
         - mountPath: /tmp/ps
           name: busybox-persistent-storage
       volumes:
-      - emptyDir: {}
+      - name: busybox-persistent-storage
-        name: busybox-persistent-storage
       - configMap:
           name: configmap-in-base
         name: configmap-in-base
@@ -564,8 +559,7 @@ spec:
         - mountPath: /tmp/ps
           name: busybox-persistent-storage
       volumes:
-      - emptyDir: {}
+      - name: busybox-persistent-storage
-        name: busybox-persistent-storage
       - configMap:
           name: configmap-in-base
         name: configmap-in-base
@@ -667,8 +661,7 @@ spec:
         - mountPath: /tmp/ps
           name: busybox-persistent-storage
       volumes:
-      - emptyDir: {}
+      - name: busybox-persistent-storage
-        name: busybox-persistent-storage
       - configMap:
           name: configmap-in-base
         name: configmap-in-base
@@ -769,8 +762,7 @@ spec:
         - mountPath: /tmp/ps
           name: busybox-persistent-storage
       volumes:
-      - emptyDir: {}
+      - name: busybox-persistent-storage
-        name: busybox-persistent-storage
       - configMap:
           name: configmap-in-base
         name: configmap-in-base
@@ -965,8 +957,7 @@ spec:
         - mountPath: /tmp/ps
           name: busybox-persistent-storage
       volumes:
-      - emptyDir: {}
+      - name: busybox-persistent-storage
-        name: busybox-persistent-storage
       - configMap:
           name: configmap-in-base
         name: configmap-in-base
@@ -1180,8 +1171,7 @@ spec:
         - mountPath: /tmp/ps
           name: busybox-persistent-storage
       volumes:
-      - emptyDir: {}
+      - name: busybox-persistent-storage
-        name: busybox-persistent-storage
       - configMap:
           name: configmap-in-base
         name: configmap-in-base

api/krusty/generatormergeandreplace_test.go

@@ -150,8 +150,6 @@ spec:
 
 func makeBaseWithGenerators(th kusttest_test.Harness) {
 	th.WriteK("/app", `
-apiVersion: kustomize.config.k8s.io/v1beta1
-kind: Kustomization
 namePrefix: team-foo-
 commonLabels:
   app: mynginx
@@ -325,8 +323,6 @@ spec:
         name: configmap-in-overlay
 `)
 	th.WriteK("/overlay", `
-apiVersion: kustomize.config.k8s.io/v1beta1
-kind: Kustomization
 namePrefix: staging-
 commonLabels:
   env: staging
@@ -389,12 +385,12 @@ spec:
       - gcePersistentDisk:
           pdName: nginx-persistent-storage
         name: nginx-persistent-storage
-      - configMap:
-          name: staging-configmap-in-overlay-k7cbc75tg8
-        name: configmap-in-overlay
       - configMap:
           name: staging-team-foo-configmap-in-base-gh9d7t85gb
         name: configmap-in-base
+      - configMap:
+          name: staging-configmap-in-overlay-k7cbc75tg8
+        name: configmap-in-overlay
 ---
 apiVersion: v1
 kind: Service

api/krusty/inlinepatch_test.go

@@ -79,8 +79,7 @@ spec:
         - mountPath: /tmp/ps
           name: nginx-persistent-storage
       volumes:
-      - emptyDir: {}
+      - name: nginx-persistent-storage
-        name: nginx-persistent-storage
       - configMap:
           name: configmap-in-base
         name: configmap-in-base
@@ -223,8 +222,7 @@ spec:
         - mountPath: /tmp/ps
           name: nginx-persistent-storage
       volumes:
-      - emptyDir: {}
+      - name: nginx-persistent-storage
-        name: nginx-persistent-storage
       - configMap:
           name: configmap-in-base
         name: configmap-in-base

api/krusty/multiplepatch_test.go

@@ -146,12 +146,12 @@ spec:
       - gcePersistentDisk:
           pdName: nginx-persistent-storage
         name: nginx-persistent-storage
-      - configMap:
-          name: a-configmap-in-overlay-ffm9hf78mc
-        name: configmap-in-overlay
       - configMap:
           name: a-b-configmap-in-base-fm96mhk4dt
         name: configmap-in-base
+      - configMap:
+          name: a-configmap-in-overlay-ffm9hf78mc
+        name: configmap-in-overlay
 ---
 apiVersion: v1
 kind: Service
@@ -190,8 +190,6 @@ metadata:
 
 func makeCommonFileForMultiplePatchTest(th kusttest_test.Harness) {
 	th.WriteK("/app/base", `
-apiVersion: kustomize.config.k8s.io/v1beta1
-kind: Kustomization
 namePrefix: team-foo-
 commonLabels:
   app: mynginx
@@ -249,8 +247,6 @@ spec:
     app: nginx
 `)
 	th.WriteK("/app/overlay/staging", `
-apiVersion: kustomize.config.k8s.io/v1beta1
-kind: Kustomization
 namePrefix: staging-
 commonLabels:
   env: staging
@@ -355,12 +351,12 @@ spec:
       - gcePersistentDisk:
           pdName: nginx-persistent-storage
         name: nginx-persistent-storage
-      - configMap:
-          name: staging-configmap-in-overlay-k7cbc75tg8
-        name: configmap-in-overlay
       - configMap:
           name: staging-team-foo-configmap-in-base-g7k6gt2889
         name: configmap-in-base
+      - configMap:
+          name: staging-configmap-in-overlay-k7cbc75tg8
+        name: configmap-in-overlay
 ---
 apiVersion: v1
 kind: Service
@@ -544,8 +540,7 @@ spec:
         - mountPath: /tmp/ps
           name: nginx-persistent-storage
       volumes:
-      - emptyDir: {}
+      - name: nginx-persistent-storage
-        name: nginx-persistent-storage
       - configMap:
           name: staging-team-foo-configmap-in-base-g7k6gt2889
         name: configmap-in-base

kyaml/yaml/merge2/map_test.go

@@ -63,6 +63,34 @@ spec:
 `,
 	},
 
+	{description: `strategic merge patch delete 4`,
+		source: `
+apiVersion: apps/v1
+metadata:
+  name: myDeploy
+kind: Deployment
+$patch: delete
+`,
+		dest: `
+apiVersion: apps/v1
+metadata:
+  name: myDeploy
+kind: Deployment
+spec:
+  replica: 2
+  template:
+    metadata:
+      labels:
+        old-label: old-value
+    spec:
+      containers:
+      - name: nginx
+        image: nginx
+`,
+		expected: `
+`,
+	},
+
 	{description: `strategic merge patch replace 1`,
 		source: `
 kind: Deployment

plugin/builtin/namespacetransformer/NamespaceTransformer.go

@@ -6,6 +6,7 @@ package main
 
 import (
 	"fmt"
+	"strings"
 
 	"sigs.k8s.io/kustomize/api/filters/namespace"
 	"sigs.k8s.io/kustomize/api/resid"
@@ -35,6 +36,11 @@ func (p *plugin) Config(
 	_ *resmap.PluginHelpers, c []byte) (err error) {
 	p.Namespace = ""
 	p.FieldSpecs = nil
+	if !strings.Contains(string(c), "yamlSupport") {
+		// If not explicitly denied,
+		// activate kyaml-based transformation.
+		p.YAMLSupport = true
+	}
 	return yaml.Unmarshal(c, p)
 }
 

plugin/builtin/patchjson6902transformer/PatchJson6902Transformer.go

@@ -6,6 +6,7 @@ package main
 
 import (
 	"fmt"
+	"strings"
 
 	jsonpatch "github.com/evanphx/json-patch"
 	"github.com/pkg/errors"
@@ -38,6 +39,11 @@ func (p *plugin) Config(
 	if err != nil {
 		return err
 	}
+	if !strings.Contains(string(c), "yamlSupport") {
+		// If not explicitly denied,
+		// activate kyaml-based transformation.
+		p.YAMLSupport = true
+	}
 	if p.Target.Name == "" {
 		return fmt.Errorf("must specify the target name")
 	}

plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer.go

@@ -6,6 +6,7 @@ package main
 
 import (
 	"fmt"
+	"strings"
 
 	"sigs.k8s.io/kustomize/api/filters/patchstrategicmerge"
 	"sigs.k8s.io/kustomize/api/resmap"
@@ -34,6 +35,11 @@ func (p *plugin) Config(
 	if err != nil {
 		return err
 	}
+	if !strings.Contains(string(c), "yamlSupport") {
+		// If not explicitly denied,
+		// activate kyaml-based transformation.
+		p.YAMLSupport = true
+	}
 	if len(p.Paths) == 0 && p.Patches == "" {
 		return fmt.Errorf("empty file path and empty patch content")
 	}
@@ -79,17 +85,6 @@ func (p *plugin) Transform(m resmap.ResMap) error {
 		}
 		if !p.YAMLSupport {
 			err = target.Patch(patch.Copy())
-			if err != nil {
-				return err
-			}
-			// remove the resource from resmap
-			// when the patch is to $patch: delete that target
-			if len(target.Map()) == 0 {
-				err = m.Remove(target.CurId())
-				if err != nil {
-					return err
-				}
-			}
 		} else {
 			patchCopy := patch.DeepCopy()
 			patchCopy.SetName(target.GetName())
@@ -103,6 +98,19 @@ func (p *plugin) Transform(m resmap.ResMap) error {
 				Patch: node,
 			}, target)
 		}
+		if err != nil {
+			return err
+		}
+		if len(target.Map()) == 0 {
+			// This means all fields have been removed from the object.
+			// This can happen if a patch required deletion of the
+			// entire resource (not just a part of it).  This means
+			// the overall resmap must shrink by one.
+			err = m.Remove(target.CurId())
+			if err != nil {
+				return err
+			}
+		}
 	}
 	return nil
 }

plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer_test.go

@@ -1208,9 +1208,89 @@ func TestMultipleNamespaces(t *testing.T) {
 	}
 }
 
-// We don't run this for the kyaml implementation since we don't support
+func TestPatchStrategicMergeTransformerPatchDelete1(t *testing.T) {
-// the strategic merge patch directives (yet).
+	th := kusttest_test.MakeEnhancedHarness(t).
-func TestPatchStrategicMergeTransformerPatchDelete(t *testing.T) {
+		PrepBuiltin("PatchStrategicMergeTransformer")
+	defer th.Reset()
+
+	th.WriteF("patch.yaml", `
+apiVersion: apps/v1
+metadata:
+  name: myDeploy
+kind: Deployment
+spec:
+  replica: 2
+  template:
+    $patch: delete
+    metadata:
+      labels:
+        old-label: old-value
+    spec:
+      containers:
+      - name: nginx
+        image: nginx
+`)
+
+	rm := th.LoadAndRunTransformer(`
+apiVersion: builtin
+kind: PatchStrategicMergeTransformer
+metadata:
+  name: notImportantHere
+paths:
+- patch.yaml
+`, target)
+
+	th.AssertActualEqualsExpected(rm, `
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: myDeploy
+spec:
+  replica: 2
+`)
+}
+
+func TestPatchStrategicMergeTransformerPatchDelete2(t *testing.T) {
+	th := kusttest_test.MakeEnhancedHarness(t).
+		PrepBuiltin("PatchStrategicMergeTransformer")
+	defer th.Reset()
+
+	th.WriteF("patch.yaml", `
+apiVersion: apps/v1
+metadata:
+  name: myDeploy
+kind: Deployment
+spec:
+  $patch: delete
+  replica: 2
+  template:
+    metadata:
+      labels:
+        old-label: old-value
+    spec:
+      containers:
+      - name: nginx
+        image: nginx
+`)
+
+	rm := th.LoadAndRunTransformer(`
+apiVersion: builtin
+kind: PatchStrategicMergeTransformer
+metadata:
+  name: notImportantHere
+paths:
+- patch.yaml
+`, target)
+
+	th.AssertActualEqualsExpected(rm, `
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: myDeploy
+`)
+}
+
+func TestPatchStrategicMergeTransformerPatchDelete3(t *testing.T) {
 	th := kusttest_test.MakeEnhancedHarness(t).
 		PrepBuiltin("PatchStrategicMergeTransformer")
 	defer th.Reset()
@@ -1234,3 +1314,4 @@ paths:
 
 	th.AssertActualEqualsExpected(rm, ``)
 }
+

plugin/builtin/patchtransformer/PatchTransformer.go

@@ -38,6 +38,11 @@ func (p *plugin) Config(
 	if err != nil {
 		return err
 	}
+	if !strings.Contains(string(c), "yamlSupport") {
+		// If not explicitly denied,
+		// activate kyaml-based transformation.
+		p.YAMLSupport = true
+	}
 	p.Patch = strings.TrimSpace(p.Patch)
 	if p.Patch == "" && p.Path == "" {
 		return fmt.Errorf(