From deb2b21cbe7a1b19480478e229991ac11c5478fb Mon Sep 17 00:00:00 2001 From: Natasha Sarkar Date: Wed, 2 Dec 2020 18:02:09 -0800 Subject: [PATCH 1/2] added allowresourceidchanges bool to options struct --- api/krusty/gvknpatch_test.go | 622 +++++++++++++++++++++++++++++++++++ api/krusty/options.go | 17 +- 2 files changed, 633 insertions(+), 6 deletions(-) create mode 100644 api/krusty/gvknpatch_test.go diff --git a/api/krusty/gvknpatch_test.go b/api/krusty/gvknpatch_test.go new file mode 100644 index 000000000..e18a6383a --- /dev/null +++ b/api/krusty/gvknpatch_test.go @@ -0,0 +1,622 @@ +// Copyright 2019 The Kubernetes Authors. +// SPDX-License-Identifier: Apache-2.0 + +package krusty_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + kusttest_test "sigs.k8s.io/kustomize/api/testutils/kusttest" +) + +// Regression test for https://github.com/kubernetes-sigs/kustomize/issues/3280 +// GVKN shouldn't change with default options +func TestKeepOriginalGVKN(t *testing.T) { + th := kusttest_test.MakeHarness(t) + + th.WriteF("apps/deployment.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: old-name +spec: + template: + spec: + containers: + - name: nginx + image: nginx +`) + + th.WriteF("apps/patch.yaml", ` +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: new-name +`) + + th.WriteK("apps", ` +resources: +- deployment.yaml + +patches: +- path: patch.yaml + target: + kind: Deployment +`) + + m := th.Run("apps", th.MakeDefaultOptions()) + th.AssertActualEqualsExpected(m, ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: old-name +spec: + template: + spec: + containers: + - image: nginx + name: nginx +`) +} + +// https://github.com/kubernetes-sigs/kustomize/issues/3280 +// These tests document behavior that will change +func TestChangeName(t *testing.T) { + th := kusttest_test.MakeHarness(t) + + th.WriteF("apps/deployment.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: old-name +spec: + template: + spec: + containers: + - name: nginx + image: nginx +`) + + th.WriteF("apps/patch.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: new-name +`) + + th.WriteK("apps", ` +resources: +- deployment.yaml + +patches: +- path: patch.yaml + target: + kind: Deployment +`) + + options := th.MakeDefaultOptions() + options.AllowResourceIdChanges = true + + // name should become `new-name` + m := th.Run("apps", options) + th.AssertActualEqualsExpected(m, ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: old-name +spec: + template: + spec: + containers: + - image: nginx + name: nginx +`) +} + +func TestChangeKind(t *testing.T) { + th := kusttest_test.MakeHarness(t) + + th.WriteF("apps/deployment.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: old-name +spec: + template: + spec: + containers: + - name: nginx + image: nginx +`) + + th.WriteF("apps/patch.yaml", ` +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: old-name +`) + + th.WriteK("apps", ` +resources: +- deployment.yaml + +patches: +- path: patch.yaml + target: + kind: Deployment +`) + + options := th.MakeDefaultOptions() + options.AllowResourceIdChanges = true + + // kind should become `StatefulSet` + m := th.Run("apps", options) + th.AssertActualEqualsExpected(m, ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: old-name +spec: + template: + spec: + containers: + - image: nginx + name: nginx +`) +} + +func TestChangeNameAndKind(t *testing.T) { + th := kusttest_test.MakeHarness(t) + + th.WriteF("apps/deployment.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: old-name +spec: + template: + spec: + containers: + - name: nginx + image: nginx +`) + + th.WriteF("apps/patch.yaml", ` +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: new-name +`) + + th.WriteK("apps", ` +resources: +- deployment.yaml + +patches: +- path: patch.yaml + target: + kind: Deployment +`) + + options := th.MakeDefaultOptions() + options.AllowResourceIdChanges = true + + // kind should become `StatefulSet` + // name should become `new-name` + m := th.Run("apps", options) + th.AssertActualEqualsExpected(m, ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: old-name +spec: + template: + spec: + containers: + - image: nginx + name: nginx +`) +} + +// https://github.com/kubernetes-sigs/kustomize/issues/3280 +// Should be able to refer to a resource with either its +// original GVKN or its current one +func TestPatchOriginalName(t *testing.T) { + th := kusttest_test.MakeHarness(t) + + th.WriteF("base/deployment.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: old-name +spec: + template: + spec: + containers: + - name: nginx + image: nginx +`) + th.WriteF("base/patch.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: new-name +`) + th.WriteK("base", ` +resources: +- deployment.yaml + +patches: +- path: patch.yaml + target: + kind: Deployment +`) + + th.WriteK("overlay", ` +resources: +- ../base +patchesStrategicMerge: +- depPatch.yaml +`) + th.WriteF("overlay/depPatch.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: old-name +spec: + replicas: 999 +`) + + options := th.MakeDefaultOptions() + options.AllowResourceIdChanges = true + + // name should become `new-name` + m := th.Run("overlay", options) + th.AssertActualEqualsExpected(m, ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: old-name +spec: + replicas: 999 + template: + spec: + containers: + - image: nginx + name: nginx +`) +} + +func TestPatchNewName(t *testing.T) { + th := kusttest_test.MakeHarness(t) + + th.WriteF("base/deployment.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: old-name +spec: + template: + spec: + containers: + - name: nginx + image: nginx +`) + th.WriteF("base/patch.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: new-name +`) + th.WriteK("base", ` +resources: +- deployment.yaml + +patches: +- path: patch.yaml + target: + kind: Deployment +`) + + th.WriteK("overlay", ` +resources: +- ../base +patchesStrategicMerge: +- depPatch.yaml +`) + th.WriteF("overlay/depPatch.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: new-name +spec: + replicas: 999 +`) + + options := th.MakeDefaultOptions() + options.AllowResourceIdChanges = true + + // depPatch cannot find target with the name `new-name` + // because base/patch.yaml can't yet edit the name + assert.Error(t, th.RunWithErr("overlay", options)) +} + +func TestPatchOriginalNameAndKind(t *testing.T) { + th := kusttest_test.MakeHarness(t) + + th.WriteF("base/deployment.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: old-name +spec: + template: + spec: + containers: + - name: nginx + image: nginx +`) + th.WriteF("base/patch.yaml", ` +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: new-name +`) + th.WriteK("base", ` +resources: +- deployment.yaml + +patches: +- path: patch.yaml + target: + kind: Deployment +`) + + th.WriteK("overlay", ` +resources: +- ../base +patchesStrategicMerge: +- depPatch.yaml +`) + th.WriteF("overlay/depPatch.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: old-name +spec: + replicas: 999 +`) + + options := th.MakeDefaultOptions() + options.AllowResourceIdChanges = true + + // kind should become `StatefulSet` + // name should become `new-name` + m := th.Run("overlay", options) + th.AssertActualEqualsExpected(m, ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: old-name +spec: + replicas: 999 + template: + spec: + containers: + - image: nginx + name: nginx +`) +} + +func TestPatchNewNameAndKind(t *testing.T) { + th := kusttest_test.MakeHarness(t) + + th.WriteF("base/deployment.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: old-name +spec: + template: + spec: + containers: + - name: nginx + image: nginx +`) + th.WriteF("base/patch.yaml", ` +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: new-name +`) + th.WriteK("base", ` +resources: +- deployment.yaml + +patches: +- path: patch.yaml + target: + kind: Deployment +`) + + th.WriteK("overlay", ` +resources: +- ../base +patchesStrategicMerge: +- depPatch.yaml +`) + th.WriteF("overlay/depPatch.yaml", ` +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: new-name +spec: + replicas: 999 +`) + + options := th.MakeDefaultOptions() + options.AllowResourceIdChanges = true + + // depPatch cannot find target with kind `StatefulSet` and name `new-name` + // because base/patch.yaml can't yet edit the kind or name + assert.Error(t, th.RunWithErr("overlay", options)) +} + +// Use original name, but new kind +// Should fail, even after #3280 is done, because this ID is invalid +func TestPatchOriginalNameAndNewKind(t *testing.T) { + th := kusttest_test.MakeHarness(t) + + th.WriteF("base/deployment.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: old-name +spec: + template: + spec: + containers: + - name: nginx + image: nginx +`) + th.WriteF("base/patch.yaml", ` +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: new-name +`) + th.WriteK("base", ` +resources: +- deployment.yaml + +patches: +- path: patch.yaml + target: + kind: Deployment +`) + + th.WriteK("overlay", ` +resources: +- ../base +patchesStrategicMerge: +- depPatch.yaml +`) + th.WriteF("overlay/depPatch.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: new-name +spec: + replicas: 999 +`) + + options := th.MakeDefaultOptions() + options.AllowResourceIdChanges = true + + // depPatch cannot find target with kind `Deployment` and name `new-name` + // because the resource never had this GVKN + assert.Error(t, th.RunWithErr("overlay", options)) +} + +// Here is a structure of a kustomization of two components, component1 +// and component2, that both use a shared deployment definition, which +// component2 adjusts by changing the kind via a patch. This test case +// checks that it does not have a conflicting definition. +// Currently documents broken behavior. +// +// root +// / \ +// component1/overlay component2/overlay +// | | +// component1/base component2/base +// \ / +// base +// +// This is the directory layout: +// +// ├── component1 +// │ ├── base +// │ │ └── kustomization.yaml +// │ └── overlay +// │ └── kustomization.yaml +// ├── component2 +// │ ├── base +// │ │ └── kustomization.yaml +// │ └── overlay +// │ └── kustomization.yaml +// ├── shared +// │ ├── kustomization.yaml +// │ └── deployment.yaml +// ├── kustomization.yaml + +func TestBaseReuseNameAndKindConflict(t *testing.T) { + th := kusttest_test.MakeHarness(t) + + th.WriteK("/app/shared", ` +resources: + - deployment.yaml +`) + th.WriteF("/app/shared/deployment.yaml", ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: my-deploy +spec: + template: + spec: + containers: + - name: nginx + image: nginx +`) + + th.WriteK("/app/component1/base", ` +resources: + - ../../shared +`) + th.WriteK("/app/component1/overlay", ` +resources: + - ../base + +namePrefix: overlay- +`) + + th.WriteK("/app/component2/base", ` +resources: + - ../../shared + +patches: +- path: patch.yaml + target: + kind: Deployment + name: my-deploy +`) + th.WriteF("/app/component2/base/patch.yaml", ` +apiVersion: apps/v1 +kind: StatefulSet +metadata: + name: my-stateful-set +`) + th.WriteK("/app/component2/overlay", ` +resources: + - ../base + +namePrefix: overlay- +`) + + th.WriteK("/app", ` +resources: + - component1/overlay + - component2/overlay +`) + + options := th.MakeDefaultOptions() + options.AllowResourceIdChanges = true + + // Error occurs when app/component2/base tries to load the shared resources + // because the kind is not (yet) allowed to change yet + // so it loads a second Deployment with the name my-deploy + // instead of a StatefulSet as specified by the patch. + // Will be fixed by #3280. + assert.Error(t, th.RunWithErr("overlay", options)) +} diff --git a/api/krusty/options.go b/api/krusty/options.go index 041ca729d..1811179ea 100644 --- a/api/krusty/options.go +++ b/api/krusty/options.go @@ -36,16 +36,21 @@ type Options struct { // When true, use kyaml/ packages to manipulate KRM yaml. // When false, use k8sdeps/ instead (uses k8s.io/api* packages). UseKyaml bool + + // When true, allow name and kind changing via a patch + // When false, patch name/kind don't overwrite target name/kind + AllowResourceIdChanges bool } // MakeDefaultOptions returns a default instance of Options. func MakeDefaultOptions() *Options { return &Options{ - DoLegacyResourceSort: false, - AddManagedbyLabel: false, - LoadRestrictions: types.LoadRestrictionsRootOnly, - DoPrune: false, - PluginConfig: konfig.DisabledPluginConfig(), - UseKyaml: false, + DoLegacyResourceSort: false, + AddManagedbyLabel: false, + LoadRestrictions: types.LoadRestrictionsRootOnly, + DoPrune: false, + PluginConfig: konfig.DisabledPluginConfig(), + UseKyaml: false, + AllowResourceIdChanges: false, } } From 724bbe94528cdd182c659c407f05002113444c28 Mon Sep 17 00:00:00 2001 From: Natasha Sarkar Date: Wed, 2 Dec 2020 18:03:09 -0800 Subject: [PATCH 2/2] connected allowresourcesidchanges bool to allow_id_changes flag --- kustomize/internal/commands/build/build.go | 2 ++ .../build/flagallowresourceidchanges.go | 23 +++++++++++++++++++ 2 files changed, 25 insertions(+) create mode 100644 kustomize/internal/commands/build/flagallowresourceidchanges.go diff --git a/kustomize/internal/commands/build/build.go b/kustomize/internal/commands/build/build.go index 5b0f4fb04..f2b50cfcf 100644 --- a/kustomize/internal/commands/build/build.go +++ b/kustomize/internal/commands/build/build.go @@ -99,6 +99,7 @@ func NewCmdBuild(out io.Writer) *cobra.Command { addFlagReorderOutput(cmd.Flags()) addFlagEnableManagedbyLabel(cmd.Flags()) addFlagEnableKyaml(cmd.Flags()) + addFlagAllowResourceIdChanges(cmd.Flags()) return cmd } @@ -137,6 +138,7 @@ func (o *Options) makeOptions() *krusty.Options { } opts.AddManagedbyLabel = isManagedbyLabelEnabled() opts.UseKyaml = flagEnableKyamlValue + opts.AllowResourceIdChanges = flagAllowResourceIdChangesValue return opts } diff --git a/kustomize/internal/commands/build/flagallowresourceidchanges.go b/kustomize/internal/commands/build/flagallowresourceidchanges.go new file mode 100644 index 000000000..5a9240e90 --- /dev/null +++ b/kustomize/internal/commands/build/flagallowresourceidchanges.go @@ -0,0 +1,23 @@ +// Copyright 2019 The Kubernetes Authors. +// SPDX-License-Identifier: Apache-2.0 + +package build + +import ( + "github.com/spf13/pflag" +) + +const ( + flagAllowResourceIdChangesName = "allow_id_changes" + flagAllowResourceIdChangesHelp = `enable changes to a resourceId` +) + +var ( + flagAllowResourceIdChangesValue = false +) + +func addFlagAllowResourceIdChanges(set *pflag.FlagSet) { + set.BoolVar( + &flagAllowResourceIdChangesValue, flagAllowResourceIdChangesName, + false, flagAllowResourceIdChangesHelp) +}