diff --git a/docs/fields.md b/docs/fields.md index 919752851..20344c798 100644 --- a/docs/fields.md +++ b/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 diff --git a/docs/glossary.md b/docs/glossary.md index 03b3abcc6..349f78b60 100644 --- a/docs/glossary.md +++ b/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]. diff --git a/k8sdeps/transformer/patch/conflictdetector.go b/k8sdeps/transformer/patch/conflictdetector.go index 7ba142a8b..42a55928d 100644 --- a/k8sdeps/transformer/patch/conflictdetector.go +++ b/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 +} diff --git a/pkg/target/multiplepatch_test.go b/pkg/target/multiplepatch_test.go index 772b9f16d..47c92391f 100644 --- a/pkg/target/multiplepatch_test.go +++ b/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 }