Allow patch removal of emptyDir {}

api/filters/nameref/nameref.go

@@ -1,7 +1,6 @@
 package nameref
 
 import (
-	"encoding/json"
 	"fmt"
 	"strings"
 
@@ -11,7 +10,6 @@ import (
 	"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/kustomize/kyaml/kio"
 	"sigs.k8s.io/kustomize/kyaml/yaml"
 )
@@ -186,11 +184,7 @@ func (f Filter) recordTheReferral(referral *resource.Resource) {
 
 // getRoleRefGvk returns a Gvk in the roleRef field. Return error
 // if the roleRef, roleRef/apiGroup or roleRef/kind is missing.
-func getRoleRefGvk(res json.Marshaler) (*resid.Gvk, error) {
-	n, err := filtersutil.GetRNode(res)
-	if err != nil {
-		return nil, err
-	}
+func getRoleRefGvk(n *yaml.RNode) (*resid.Gvk, error) {
 	roleRef, err := n.Pipe(yaml.Lookup("roleRef"))
 	if err != nil {
 		return nil, err
@@ -276,7 +270,7 @@ func (f Filter) roleRefFilter() sieveFunc {
 	if !strings.HasSuffix(f.NameFieldToUpdate.Path, "roleRef/name") {
 		return acceptAll
 	}
-	roleRefGvk, err := getRoleRefGvk(f.Referrer)
+	roleRefGvk, err := getRoleRefGvk(f.Referrer.AsRNode())
 	if err != nil {
 		return acceptAll
 	}

api/hasher/hasher_test.go

@@ -56,7 +56,7 @@ kind: ConfigMap`, "6ct58987ht", ""},
 		{"one key", `
 apiVersion: v1
 kind: ConfigMap
-data: 
+data:
   one: ""`, "9g67k2htb6", ""},
 		// three keys (tests sorting order)
 		{"three keys", `
@@ -224,7 +224,7 @@ kind: ConfigMap`, `{"data":"","kind":"ConfigMap","name":""}`, ""},
 		{"one key", `
 apiVersion: v1
 kind: ConfigMap
-data: 
+data:
   one: ""`, `{"data":{"one":""},"kind":"ConfigMap","name":""}`, ""},
 		// three keys (tests sorting order)
 		{"three keys", `

api/krusty/multiplepatch_test.go

@@ -235,6 +235,7 @@ metadata:
 `)
 }
 
+// Goal is to remove "  emptyDir: {}" with a patch.
 func TestRemoveEmptyDirWithPatchesAtSameLevel(t *testing.T) {
 	th := kusttest_test.MakeHarness(t)
 	th.WriteK("base", `
@@ -299,8 +300,6 @@ spec:
 	opts := th.MakeDefaultOptions()
 	m := th.Run("overlay", opts)
 	th.AssertActualEqualsExpected(
-		// TODO(#3394): Should be possible to delete emptyDir with a patch.
-		// TODO(#3304): DECISION - still a bug, emptyDir should be deleted.
 		m, `
 apiVersion: apps/v1
 kind: Deployment
@@ -318,8 +317,7 @@ spec:
       - image: sidecar:latest
         name: sidecar
       volumes:
-      - emptyDir: {}
-        gcePersistentDisk:
+      - gcePersistentDisk:
           pdName: nginx-persistent-storage
         name: nginx-persistent-storage
 `)
@@ -870,25 +868,23 @@ spec:
       - $patch: delete
         name: sidecar
 `
-	cases := []struct {
-		name        string
+	cases := map[string]struct {
 		patch1      string
 		patch2      string
 		expectError bool
 	}{
-		{
-			name:   "Patch with delete directive first",
+		"Patch with delete directive first": {
 			patch1: deletePatch,
 			patch2: additivePatch,
 		},
-		{
-			name:   "Patch with delete directive second",
+		"Patch with delete directive second": {
 			patch1: additivePatch,
 			patch2: deletePatch,
 		},
 	}
-	for _, c := range cases {
-		t.Run(c.name, func(t *testing.T) {
+	for name := range cases {
+		c := cases[name]
+		t.Run(name, func(t *testing.T) {
 			th := kusttest_test.MakeHarness(t)
 			makeCommonFilesForMultiplePatchTests(th)
 			th.WriteF("overlay/staging/deployment-patch1.yaml", c.patch1)

api/resource/resource.go

@@ -15,7 +15,6 @@ import (
 	"sigs.k8s.io/kustomize/api/konfig"
 	"sigs.k8s.io/kustomize/api/resid"
 	"sigs.k8s.io/kustomize/api/types"
-	"sigs.k8s.io/kustomize/kyaml/filtersutil"
 	"sigs.k8s.io/kustomize/kyaml/kio"
 	kyaml "sigs.k8s.io/kustomize/kyaml/yaml"
 	"sigs.k8s.io/yaml"
@@ -545,18 +544,13 @@ func (r *Resource) AppendRefVarName(variable types.Var) {
 
 // ApplySmPatch applies the provided strategic merge patch.
 func (r *Resource) ApplySmPatch(patch *Resource) error {
-	node, err := filtersutil.GetRNode(patch)
-	if err != nil {
-		return err
-	}
 	n, ns, k := r.GetName(), r.GetNamespace(), r.GetKind()
 	if patch.NameChangeAllowed() || patch.KindChangeAllowed() {
 		r.StorePreviousId()
 	}
-	err = r.ApplyFilter(patchstrategicmerge.Filter{
-		Patch: node,
-	})
-	if err != nil {
+	if err := r.ApplyFilter(patchstrategicmerge.Filter{
+		Patch: patch.node,
+	}); err != nil {
 		return err
 	}
 	if r.IsEmpty() {

plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer.go

@@ -32,45 +32,57 @@ func (p *plugin) Config(
 		return fmt.Errorf("empty file path and empty patch content")
 	}
 	if len(p.Paths) != 0 {
-		for _, onePath := range p.Paths {
-			// The following oddly attempts to interpret a path string as an
-			// actual patch (instead of as a path to a file containing a patch).
-			// All tests pass if this code is commented out.  This code should
-			// be deleted; the user should use the Patches field which
-			// exists for this purpose (inline patch declaration).
-			res, err := h.ResmapFactory().RF().SliceFromBytes([]byte(onePath))
-			if err == nil {
-				p.loadedPatches = append(p.loadedPatches, res...)
-				continue
-			}
-			res, err = h.ResmapFactory().RF().SliceFromPatches(
-				h.Loader(), []types.PatchStrategicMerge{onePath})
-			if err != nil {
-				return err
-			}
-			p.loadedPatches = append(p.loadedPatches, res...)
+		patches, err := loadFromPaths(h, p.Paths)
+		if err != nil {
+			return err
 		}
+		p.loadedPatches = append(p.loadedPatches, patches...)
 	}
 	if p.Patches != "" {
-		res, err := h.ResmapFactory().RF().SliceFromBytes([]byte(p.Patches))
+		patches, err := h.ResmapFactory().RF().SliceFromBytes([]byte(p.Patches))
 		if err != nil {
 			return err
 		}
-		p.loadedPatches = append(p.loadedPatches, res...)
+		p.loadedPatches = append(p.loadedPatches, patches...)
 	}
-
 	if len(p.loadedPatches) == 0 {
 		return fmt.Errorf(
 			"patch appears to be empty; files=%v, Patch=%s", p.Paths, p.Patches)
 	}
-	// Merge the patches, looking for conflicts.
-	_, err = h.ResmapFactory().ConflatePatches(p.loadedPatches)
-	if err != nil {
-		return err
-	}
+	// TODO(#3723): Delete conflict detection.
+	// Since #1500 closed, the conflict detector in use doesn't do
+	// anything useful.  The resmap returned by this method hasn't
+	// been used for many releases.  Leaving code as a comment to
+	// aid in deletion (fixing #3723).
+	// _, err = h.ResmapFactory().ConflatePatches(p.loadedPatches)
+	// if err != nil {
+	// 	return err
+	// }
 	return nil
 }
 
+func loadFromPaths(
+	h *resmap.PluginHelpers,
+	paths []types.PatchStrategicMerge) (
+	result []*resource.Resource, err error) {
+	var patches []*resource.Resource
+	for _, path := range paths {
+		// For legacy reasons, attempt to treat the path string as
+		// actual patch content.
+		patches, err = h.ResmapFactory().RF().SliceFromBytes([]byte(path))
+		if err != nil {
+			// Failing that, treat it as a file path.
+			patches, err = h.ResmapFactory().RF().SliceFromPatches(
+				h.Loader(), []types.PatchStrategicMerge{path})
+			if err != nil {
+				return
+			}
+		}
+		result = append(result, patches...)
+	}
+	return
+}
+
 func (p *plugin) Transform(m resmap.ResMap) error {
 	for _, patch := range p.loadedPatches {
 		target, err := m.GetById(patch.OrgId())

plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer_test.go

@@ -562,6 +562,7 @@ func TestStrategicMergeTransformerNoSchemaMultiPatchesNoConflict(t *testing.T) {
 	th := kusttest_test.MakeEnhancedHarness(t).
 		PrepBuiltin("PatchStrategicMergeTransformer")
 	defer th.Reset()
+	// This patch wants to delete "B".
 	th.WriteF("patch1.yaml", `
 apiVersion: example.com/v1
 kind: Foo
@@ -603,10 +604,6 @@ spec:
 `)
 	assert.NoError(t, err)
 	th.AssertActualEqualsExpectedNoIdAnnotations(
-		// In kyaml/yaml.merge2, the empty "B: " is dropped
-		// when patch1 and patch2 are merged, so the patch
-		// applied is effectively only patch2.yaml.
-		// So it cannot delete the "B: Y"
 		resMap, `
 apiVersion: example.com/v1
 kind: Foo
@@ -615,7 +612,6 @@ metadata:
 spec:
   bar:
     A: X
-    B: "Y"
     C: Z
     D: W
   baz:
@@ -639,8 +635,7 @@ spec:
 `)
 	assert.NoError(t, err)
 	th.AssertActualEqualsExpectedNoIdAnnotations(
-		// This time only patch2 was applied.  Same answer on the kyaml
-		// path, but different answer on apimachinery path (B becomes "true"?)
+		// This time only patch2 is applied.
 		resMap, `
 apiVersion: example.com/v1
 kind: Foo