Merge pull request #1383 from richardmarshall/smp_patch_directive

Handle ordering patch with SMP delete directives

docs/fields.md

@@ -339,6 +339,11 @@ patchesStrategicMerge:
             image: nignx:latest
 ```
 
+Note that kustomize does not support more than one patch
+for the same object that contain a _delete_ directive. To remove
+several fields / slice elements from an object create a single
+patch that performs all the needed deletions.
+
 ### patchesJson6902
 
 Each entry in this list should resolve to

docs/glossary.md

@@ -376,9 +376,8 @@ value is a list.
 
 To change this
 default behavior, add a _directive_.  Recognized
-directives include _replace_ (the default), _merge_
-(avoid replacing a list), _delete_ and a few more
-(see [these notes][strategic-merge]).
+directives in YAML patches are _replace_ (the default)
+and _delete_ (see [these notes][strategic-merge]).
 
 Note that for custom resources, SMPs are treated as
 [json merge patches][JSONMergePatch].

k8sdeps/transformer/patch/conflictdetector.go

@@ -6,12 +6,13 @@ package patch
 import (
 	"encoding/json"
 	"fmt"
+
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/client-go/kubernetes/scheme"
 	"sigs.k8s.io/kustomize/v3/pkg/gvk"
 	"sigs.k8s.io/kustomize/v3/pkg/resmap"
 
-	"github.com/evanphx/json-patch"
+	jsonpatch "github.com/evanphx/json-patch"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/mergepatch"
 	"k8s.io/apimachinery/pkg/util/strategicpatch"
@@ -123,12 +124,18 @@ func (smp *strategicMergePatch) findConflict(
 }
 
 func (smp *strategicMergePatch) mergePatches(patch1, patch2 *resource.Resource) (*resource.Resource, error) {
+	if hasDeleteDirectiveMarker(patch2.Map()) {
+		if hasDeleteDirectiveMarker(patch1.Map()) {
+			return nil, fmt.Errorf("cannot merge patches both containing '$patch: delete' directives")
+		}
+		patch1, patch2 = patch2, patch1
+	}
 	mergeJSONMap, err := strategicpatch.MergeStrategicMergeMapPatchUsingLookupPatchMeta(
 		smp.lookupPatchMeta, patch1.Map(), patch2.Map())
 	return smp.rf.FromMap(mergeJSONMap), err
 }
 
-// mergePatches merge and index patches by OrgId.
+// MergePatches merge and index patches by OrgId.
 // It errors out if there is conflict between patches.
 func MergePatches(patches []*resource.Resource,
 	rf *resource.Factory) (resmap.ResMap, error) {
@@ -188,3 +195,28 @@ func toSchemaGvk(x gvk.Gvk) schema.GroupVersionKind {
 		Kind:    x.Kind,
 	}
 }
+
+func hasDeleteDirectiveMarker(patch map[string]interface{}) bool {
+	if v, ok := patch["$patch"]; ok && v == "delete" {
+		return true
+	}
+	for _, v := range patch {
+		switch typedV := v.(type) {
+		case map[string]interface{}:
+			if hasDeleteDirectiveMarker(typedV) {
+				return true
+			}
+		case []interface{}:
+			for _, sv := range typedV {
+				typedE, ok := sv.(map[string]interface{})
+				if !ok {
+					break
+				}
+				if hasDeleteDirectiveMarker(typedE) {
+					return true
+				}
+			}
+		}
+	}
+	return false
+}

pkg/target/multiplepatch_test.go

@@ -20,7 +20,7 @@ import (
 	"strings"
 	"testing"
 
-	"sigs.k8s.io/kustomize/v3/pkg/kusttest"
+	kusttest_test "sigs.k8s.io/kustomize/v3/pkg/kusttest"
 )
 
 func makeCommonFileForMultiplePatchTest(th *kusttest_test.KustTestHarness) {
@@ -294,16 +294,8 @@ spec:
 	}
 }
 
-// TestMultiplePatchesWithPatchDeleteIgnored demonstrates that if the
-// patch containing the $patch:delete directive is second in the list,
-// the behavior of kustomize is incorrect, and the sidecar container is
-// not removed from the final output. No conflict, nor error is reported
-// even so the patch is ignored. See issue #1354
-func TestMultiplePatchesWithPatchDeleteIgnored(t *testing.T) {
-	th := kusttest_test.NewKustTestHarness(t, "/app/overlay/staging")
-	makeCommonFileForMultiplePatchTest(th)
-	th.WriteF("/app/overlay/staging/deployment-patch1.yaml", `
-apiVersion: apps/v1beta2
+func TestMultiplePatchesWithOnePatchDeleteDirective(t *testing.T) {
+	additivePatch := `apiVersion: apps/v1beta2
 kind: Deployment
 metadata:
   name: nginx
@@ -315,9 +307,8 @@ spec:
         env:
         - name: SOME_NAME
           value: somevalue
-`)
-	th.WriteF("/app/overlay/staging/deployment-patch2.yaml", `
-apiVersion: apps/v1beta2
+`
+	deletePatch := `apiVersion: apps/v1beta2
 kind: Deployment
 metadata:
   name: nginx
@@ -327,12 +318,36 @@ spec:
       containers:
       - $patch: delete
         name: sidecar
-`)
-	m, err := th.MakeKustTarget().MakeCustomizedResMap()
-	if err != nil {
-		t.Fatalf("Err: %v", err)
+`
+	cases := []struct {
+		name        string
+		patch1      string
+		patch2      string
+		expectError bool
+	}{
+		{
+			name:   "Patch with delete directive first",
+			patch1: deletePatch,
+			patch2: additivePatch,
+		},
+		{
+			name:   "Patch with delete directive second",
+			patch1: additivePatch,
+			patch2: deletePatch,
+		},
 	}
-	th.AssertActualEqualsExpected(m, `apiVersion: apps/v1beta2
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			th := kusttest_test.NewKustTestHarness(t, "/app/overlay/staging")
+
+			makeCommonFileForMultiplePatchTest(th)
+			th.WriteF("/app/overlay/staging/deployment-patch1.yaml", c.patch1)
+			th.WriteF("/app/overlay/staging/deployment-patch2.yaml", c.patch2)
+			m, err := th.MakeKustTarget().MakeCustomizedResMap()
+			if err != nil {
+				t.Fatalf("Err: %v", err)
+			}
+			th.AssertActualEqualsExpected(m, `apiVersion: apps/v1beta2
 kind: Deployment
 metadata:
   annotations:
@@ -369,8 +384,6 @@ spec:
         volumeMounts:
         - mountPath: /tmp/ps
           name: nginx-persistent-storage
-      - image: sidecar:latest
-        name: sidecar
       volumes:
       - emptyDir: {}
         name: nginx-persistent-storage
@@ -421,13 +434,11 @@ metadata:
     env: staging
   name: staging-configmap-in-overlay-k7cbc75tg8
 `)
+		})
+	}
 }
 
-// TestMultiplePatchesWithPatchDeleteApplied demonstrates that if the
-// patch containing the $patch:delete directive is first in the list,
-// the behavior of kustomize is correct, and the sidecar container
-// is removed from the final output. See issue #1354
-func TestMultiplePatchesWithPatchDeleteApplied(t *testing.T) {
+func TestMultiplePatchesBothWithPatchDeleteDirective(t *testing.T) {
 	th := kusttest_test.NewKustTestHarness(t, "/app/overlay/staging")
 	makeCommonFileForMultiplePatchTest(th)
 	th.WriteF("/app/overlay/staging/deployment-patch1.yaml", `
@@ -451,100 +462,16 @@ spec:
   template:
     spec:
       containers:
-      - name: nginx
-        env:
-        - name: SOME_NAME
-          value: somevalue
-`)
-	m, err := th.MakeKustTarget().MakeCustomizedResMap()
-	if err != nil {
-		t.Fatalf("Err: %v", err)
-	}
-	th.AssertActualEqualsExpected(m, `apiVersion: apps/v1beta2
-kind: Deployment
-metadata:
-  annotations:
-    note: This is a test annotation
-  labels:
-    app: mynginx
-    env: staging
-    org: example.com
-    team: foo
-  name: staging-team-foo-nginx
-spec:
-  selector:
-    matchLabels:
-      app: mynginx
-      env: staging
-      org: example.com
-      team: foo
-  template:
-    metadata:
-      annotations:
-        note: This is a test annotation
-      labels:
-        app: mynginx
-        env: staging
-        org: example.com
-        team: foo
-    spec:
-      containers:
-      - env:
-        - name: SOME_NAME
-          value: somevalue
-        image: nginx
+      - $patch: delete
         name: nginx
-        volumeMounts:
-        - mountPath: /tmp/ps
-          name: nginx-persistent-storage
-      volumes:
-      - emptyDir: {}
-        name: nginx-persistent-storage
-      - configMap:
-          name: staging-team-foo-configmap-in-base-g7k6gt2889
-        name: configmap-in-base
----
-apiVersion: v1
-kind: Service
-metadata:
-  annotations:
-    note: This is a test annotation
-  labels:
-    app: mynginx
-    env: staging
-    org: example.com
-    team: foo
-  name: staging-team-foo-nginx
-spec:
-  ports:
-  - port: 80
-  selector:
-    app: mynginx
-    env: staging
-    org: example.com
-    team: foo
----
-apiVersion: v1
-data:
-  foo: bar
-kind: ConfigMap
-metadata:
-  annotations:
-    note: This is a test annotation
-  labels:
-    app: mynginx
-    env: staging
-    org: example.com
-    team: foo
-  name: staging-team-foo-configmap-in-base-g7k6gt2889
----
-apiVersion: v1
-data:
-  hello: world
-kind: ConfigMap
-metadata:
-  labels:
-    env: staging
-  name: staging-configmap-in-overlay-k7cbc75tg8
 `)
+	_, err := th.MakeKustTarget().MakeCustomizedResMap()
+	if err == nil {
+		t.Fatalf("Expected error")
+	}
+	if !strings.Contains(
+		err.Error(), "both containing ") {
+		t.Fatalf("Unexpected err: %v", err)
+	}
+	return
 }