diff --git a/api/builtins/PatchStrategicMergeTransformer.go b/api/builtins/PatchStrategicMergeTransformer.go index e2f61238f..e94b8c4a2 100644 --- a/api/builtins/PatchStrategicMergeTransformer.go +++ b/api/builtins/PatchStrategicMergeTransformer.go @@ -13,7 +13,6 @@ import ( ) type PatchStrategicMergeTransformerPlugin struct { - h *resmap.PluginHelpers loadedPatches []*resource.Resource Paths []types.PatchStrategicMerge `json:"paths,omitempty" yaml:"paths,omitempty"` Patches string `json:"patches,omitempty" yaml:"patches,omitempty"` @@ -21,7 +20,6 @@ type PatchStrategicMergeTransformerPlugin struct { func (p *PatchStrategicMergeTransformerPlugin) Config( h *resmap.PluginHelpers, c []byte) (err error) { - p.h = h err = yaml.Unmarshal(c, p) if err != nil { return err @@ -36,13 +34,13 @@ func (p *PatchStrategicMergeTransformerPlugin) Config( // 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 := p.h.ResmapFactory().RF().SliceFromBytes([]byte(onePath)) + res, err := h.ResmapFactory().RF().SliceFromBytes([]byte(onePath)) if err == nil { p.loadedPatches = append(p.loadedPatches, res...) continue } - res, err = p.h.ResmapFactory().RF().SliceFromPatches( - p.h.Loader(), []types.PatchStrategicMerge{onePath}) + res, err = h.ResmapFactory().RF().SliceFromPatches( + h.Loader(), []types.PatchStrategicMerge{onePath}) if err != nil { return err } @@ -50,7 +48,7 @@ func (p *PatchStrategicMergeTransformerPlugin) Config( } } if p.Patches != "" { - res, err := p.h.ResmapFactory().RF().SliceFromBytes([]byte(p.Patches)) + res, err := h.ResmapFactory().RF().SliceFromBytes([]byte(p.Patches)) if err != nil { return err } @@ -61,15 +59,17 @@ func (p *PatchStrategicMergeTransformerPlugin) Config( return fmt.Errorf( "patch appears to be empty; files=%v, Patch=%s", p.Paths, p.Patches) } - return err -} - -func (p *PatchStrategicMergeTransformerPlugin) Transform(m resmap.ResMap) error { - patches, err := p.h.ResmapFactory().Merge(p.loadedPatches) + // Merge the patches, looking for conflicts. + m, err := h.ResmapFactory().ConflatePatches(p.loadedPatches) if err != nil { return err } - for _, patch := range patches.Resources() { + p.loadedPatches = m.Resources() + return nil +} + +func (p *PatchStrategicMergeTransformerPlugin) Transform(m resmap.ResMap) error { + for _, patch := range p.loadedPatches { target, err := m.GetById(patch.OrgId()) if err != nil { return err diff --git a/api/internal/conflict/smpatchmergeonlydetector.go b/api/internal/conflict/smpatchmergeonlydetector.go index 4dda0a4cf..a33fdc3c3 100644 --- a/api/internal/conflict/smpatchmergeonlydetector.go +++ b/api/internal/conflict/smpatchmergeonlydetector.go @@ -21,6 +21,11 @@ func (c *smPatchMergeOnlyDetector) HasConflict( return false, nil } +// There's at least one case that doesn't work. Suppose one has a +// Deployment with a volume with the bizarre "emptyDir: {}" entry. +// If you want to get rid of this entry via a patch containing +// the entry "emptyDir: null", then the following won't work, +// because null entries are eliminated. func (c *smPatchMergeOnlyDetector) MergePatches( r, patch *resource.Resource) (*resource.Resource, error) { err := r.ApplySmPatch(patch) diff --git a/api/krusty/multiplepatch_test.go b/api/krusty/multiplepatch_test.go index 4df226adb..713951e44 100644 --- a/api/krusty/multiplepatch_test.go +++ b/api/krusty/multiplepatch_test.go @@ -10,6 +10,441 @@ import ( kusttest_test "sigs.k8s.io/kustomize/api/testutils/kusttest" ) +func TestVolumePatch1(t *testing.T) { + th := kusttest_test.MakeHarness(t) + th.WriteK(".", ` +resources: +- deployment.yaml +patchesStrategicMerge: +- patch.yaml +`) + th.WriteF("deployment.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx +spec: + template: + spec: + volumes: + - name: fancyDisk + emptyDir: {} +`) + th.WriteF("patch.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx +spec: + template: + spec: + volumes: + - name: fancyDisk + emptyDir: null + gcePersistentDisk: + pdName: fancyDisk +`) + m := th.Run(".", th.MakeDefaultOptions()) + th.AssertActualEqualsExpected(m, ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx +spec: + template: + spec: + volumes: + - gcePersistentDisk: + pdName: fancyDisk + name: fancyDisk +`) +} + +func TestVolumePatch2(t *testing.T) { + th := kusttest_test.MakeHarness(t) + th.WriteK("base", ` +resources: +- deployment.yaml +configMapGenerator: +- name: baseCm + literals: + - foo=bar +`) + th.WriteF("base/deployment.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx +spec: + template: + spec: + containers: + - name: nginx + image: nginx + volumeMounts: + - name: fancyDisk + mountPath: /tmp/ps + volumes: + - name: fancyDisk + emptyDir: {} + - configMap: + name: baseCm + name: baseCm +`) + m := th.Run("base", th.MakeDefaultOptions()) + th.AssertActualEqualsExpected(m, ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx +spec: + template: + spec: + containers: + - image: nginx + name: nginx + volumeMounts: + - mountPath: /tmp/ps + name: fancyDisk + volumes: + - emptyDir: {} + name: fancyDisk + - configMap: + name: baseCm-798k5k7g9f + name: baseCm +--- +apiVersion: v1 +data: + foo: bar +kind: ConfigMap +metadata: + name: baseCm-798k5k7g9f +`) + + th.WriteK("overlay", ` +patchesStrategicMerge: +- patch.yaml +resources: +- ../base +configMapGenerator: +- name: overlayCm + literals: + - hello=world +`) + th.WriteF("overlay/patch.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx +spec: + template: + spec: + volumes: + - name: fancyDisk + emptyDir: null + gcePersistentDisk: + pdName: fancyDisk + - configMap: + name: overlayCm + name: overlayCm +`) + m = th.Run("overlay", th.MakeDefaultOptions()) + th.AssertActualEqualsExpected(m, ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx +spec: + template: + spec: + containers: + - image: nginx + name: nginx + volumeMounts: + - mountPath: /tmp/ps + name: fancyDisk + volumes: + - gcePersistentDisk: + pdName: fancyDisk + name: fancyDisk + - configMap: + name: overlayCm-dc6fm46dhm + name: overlayCm + - configMap: + name: baseCm-798k5k7g9f + name: baseCm +--- +apiVersion: v1 +data: + foo: bar +kind: ConfigMap +metadata: + name: baseCm-798k5k7g9f +--- +apiVersion: v1 +data: + hello: world +kind: ConfigMap +metadata: + name: overlayCm-dc6fm46dhm +`) +} + +func TestVolumePatch3(t *testing.T) { + th := kusttest_test.MakeHarness(t) + th.WriteK("base", ` +commonLabels: + team: foo +resources: +- deployment.yaml +configMapGenerator: +- name: configmap-in-base + literals: + - foo=bar +`) + th.WriteF("base/deployment.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx +spec: + template: + spec: + containers: + - name: nginx + image: nginx + - name: sidecar + image: sidecar:latest + volumes: + - name: nginx-persistent-storage + emptyDir: {} + - configMap: + name: configmap-in-base + name: configmap-in-base +`) + th.WriteK("overlay", ` +commonLabels: + env: staging +patchesStrategicMerge: +- deployment-patch1.yaml +- deployment-patch2.yaml +resources: +- ../base +`) + th.WriteF("overlay/deployment-patch1.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx +spec: + template: + spec: + volumes: + - name: nginx-persistent-storage + emptyDir: null + gcePersistentDisk: + pdName: nginx-persistent-storage +`) + th.WriteF("overlay/deployment-patch2.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx +spec: + template: + spec: + containers: + - image: nginx + name: nginx + env: + - name: ANOTHERENV + value: FOO + volumes: + - name: nginx-persistent-storage +`) + m := th.Run("overlay", th.MakeDefaultOptions()) + th.AssertActualEqualsExpected(m, ` +apiVersion: apps/v1 +kind: Deployment +metadata: + labels: + env: staging + team: foo + name: nginx +spec: + selector: + matchLabels: + env: staging + team: foo + template: + metadata: + labels: + env: staging + team: foo + spec: + containers: + - env: + - name: ANOTHERENV + value: FOO + image: nginx + name: nginx + - image: sidecar:latest + name: sidecar + volumes: + - gcePersistentDisk: + pdName: nginx-persistent-storage + name: nginx-persistent-storage + - configMap: + name: configmap-in-base-798k5k7g9f + name: configmap-in-base +--- +apiVersion: v1 +data: + foo: bar +kind: ConfigMap +metadata: + labels: + env: staging + team: foo + name: configmap-in-base-798k5k7g9f +`) +} + +func TestEmptyDirOverrideMultiplePatches(t *testing.T) { + th := kusttest_test.MakeHarness(t) + th.WriteK("base", ` +namePrefix: b- +commonLabels: + team: foo +resources: +- deployment.yaml +configMapGenerator: +- name: configmap-in-base + literals: + - foo=bar +`) + th.WriteF("base/deployment.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx +spec: + template: + spec: + containers: + - name: nginx + image: nginx + volumeMounts: + - name: nginx-persistent-storage + mountPath: /tmp/ps + - name: sidecar + image: sidecar:latest + volumes: + - name: nginx-persistent-storage + emptyDir: {} + - configMap: + name: configmap-in-base + name: configmap-in-base +`) + th.WriteK("overlay", ` +namePrefix: a- +commonLabels: + env: staging +patchesStrategicMerge: +- deployment-patch1.yaml +- deployment-patch2.yaml +resources: +- ../base +`) + th.WriteF("overlay/deployment-patch1.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx +spec: + template: + spec: + containers: + - name: nginx + image: nginx:latest + env: + - name: ENVKEY + value: ENVVALUE + volumes: + - name: nginx-persistent-storage + emptyDir: null + gcePersistentDisk: + pdName: nginx-persistent-storage +`) + th.WriteF("overlay/deployment-patch2.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx +spec: + template: + spec: + containers: + - name: nginx + env: + - name: ANOTHERENV + value: FOO + volumes: + - name: nginx-persistent-storage +`) + m := th.Run("overlay", th.MakeDefaultOptions()) + th.AssertActualEqualsExpected(m, ` +apiVersion: apps/v1 +kind: Deployment +metadata: + labels: + env: staging + team: foo + name: a-b-nginx +spec: + selector: + matchLabels: + env: staging + team: foo + template: + metadata: + labels: + env: staging + team: foo + spec: + containers: + - env: + - name: ANOTHERENV + value: FOO + - name: ENVKEY + value: ENVVALUE + image: nginx:latest + name: nginx + volumeMounts: + - mountPath: /tmp/ps + name: nginx-persistent-storage + - image: sidecar:latest + name: sidecar + volumes: + - gcePersistentDisk: + pdName: nginx-persistent-storage + name: nginx-persistent-storage + - configMap: + name: a-b-configmap-in-base-798k5k7g9f + name: configmap-in-base +--- +apiVersion: v1 +data: + foo: bar +kind: ConfigMap +metadata: + labels: + env: staging + team: foo + name: a-b-configmap-in-base-798k5k7g9f +`) +} + func TestSimpleMultiplePatches(t *testing.T) { th := kusttest_test.MakeHarness(t) th.WriteK("base", ` @@ -405,6 +840,10 @@ metadata: func TestMultiplePatchesWithConflict(t *testing.T) { th := kusttest_test.MakeHarness(t) + opts := th.MakeDefaultOptions() + if opts.UseKyaml { + t.Skip("kyaml merging doesn't look for conflicts") + } makeCommonFileForMultiplePatchTest(th) th.WriteF("/app/overlay/staging/deployment-patch1.yaml", ` apiVersion: apps/v1 @@ -442,7 +881,7 @@ spec: - name: ENABLE_FEATURE_FOO value: FALSE `) - err := th.RunWithErr("/app/overlay/staging", th.MakeDefaultOptions()) + err := th.RunWithErr("/app/overlay/staging", opts) if err == nil { t.Fatalf("expected conflict") } diff --git a/api/resmap/factory.go b/api/resmap/factory.go index 25fdd7fd3..a93b0ad6c 100644 --- a/api/resmap/factory.go +++ b/api/resmap/factory.go @@ -126,10 +126,11 @@ func (rmF *Factory) FromSecretArgs( return rmF.FromResource(res), nil } -// Merge creates a new ResMap by merging incoming resources. +// ConflatePatches creates a new ResMap containing a merger of the +// incoming patches. // Error if conflict found. -func (rmF *Factory) Merge(incoming []*resource.Resource) (ResMap, error) { - return (&merginator{cdf: rmF.cdf}).Merge(incoming) +func (rmF *Factory) ConflatePatches(patches []*resource.Resource) (ResMap, error) { + return (&merginator{cdf: rmF.cdf}).ConflatePatches(patches) } func newResMapFromResourceSlice( diff --git a/api/resmap/factory_test.go b/api/resmap/factory_test.go index b03177705..4f1eb2827 100644 --- a/api/resmap/factory_test.go +++ b/api/resmap/factory_test.go @@ -351,13 +351,13 @@ metadata: } } -func TestMerge_Empty(t *testing.T) { - rm, err := rmF.Merge([]*resource.Resource{}) +func TestConflatePatches_Empty(t *testing.T) { + rm, err := rmF.ConflatePatches([]*resource.Resource{}) assert.NoError(t, err) assert.Equal(t, 0, rm.Size()) } -func TestMerge(t *testing.T) { +func TestConflatePatches(t *testing.T) { var ( err error yml []byte @@ -387,7 +387,7 @@ spec: `)) assert.NoError(t, err) - rm, err := rmF.Merge([]*resource.Resource{r1, r2}) + rm, err := rmF.ConflatePatches([]*resource.Resource{r1, r2}) assert.NoError(t, err) yml, err = rm.AsYaml() diff --git a/api/resmap/merginator.go b/api/resmap/merginator.go index 9af6d11c3..0ca5b347c 100644 --- a/api/resmap/merginator.go +++ b/api/resmap/merginator.go @@ -16,7 +16,7 @@ type merginator struct { result ResMap } -func (m *merginator) Merge(in []*resource.Resource) (ResMap, error) { +func (m *merginator) ConflatePatches(in []*resource.Resource) (ResMap, error) { m.result = New() m.incoming = in for index := range m.incoming { diff --git a/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer.go b/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer.go index afb5dc0fb..59975d6b0 100644 --- a/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer.go +++ b/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer.go @@ -14,7 +14,6 @@ import ( ) type plugin struct { - h *resmap.PluginHelpers loadedPatches []*resource.Resource Paths []types.PatchStrategicMerge `json:"paths,omitempty" yaml:"paths,omitempty"` Patches string `json:"patches,omitempty" yaml:"patches,omitempty"` @@ -25,7 +24,6 @@ var KustomizePlugin plugin func (p *plugin) Config( h *resmap.PluginHelpers, c []byte) (err error) { - p.h = h err = yaml.Unmarshal(c, p) if err != nil { return err @@ -40,13 +38,13 @@ func (p *plugin) Config( // 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 := p.h.ResmapFactory().RF().SliceFromBytes([]byte(onePath)) + res, err := h.ResmapFactory().RF().SliceFromBytes([]byte(onePath)) if err == nil { p.loadedPatches = append(p.loadedPatches, res...) continue } - res, err = p.h.ResmapFactory().RF().SliceFromPatches( - p.h.Loader(), []types.PatchStrategicMerge{onePath}) + res, err = h.ResmapFactory().RF().SliceFromPatches( + h.Loader(), []types.PatchStrategicMerge{onePath}) if err != nil { return err } @@ -54,7 +52,7 @@ func (p *plugin) Config( } } if p.Patches != "" { - res, err := p.h.ResmapFactory().RF().SliceFromBytes([]byte(p.Patches)) + res, err := h.ResmapFactory().RF().SliceFromBytes([]byte(p.Patches)) if err != nil { return err } @@ -65,15 +63,17 @@ func (p *plugin) Config( return fmt.Errorf( "patch appears to be empty; files=%v, Patch=%s", p.Paths, p.Patches) } - return err -} - -func (p *plugin) Transform(m resmap.ResMap) error { - patches, err := p.h.ResmapFactory().Merge(p.loadedPatches) + // Merge the patches, looking for conflicts. + m, err := h.ResmapFactory().ConflatePatches(p.loadedPatches) if err != nil { return err } - for _, patch := range patches.Resources() { + p.loadedPatches = m.Resources() + return nil +} + +func (p *plugin) Transform(m resmap.ResMap) error { + for _, patch := range p.loadedPatches { target, err := m.GetById(patch.OrgId()) if err != nil { return err