From e43565ef80e44e7ac08363a17a83d17a7ff18f18 Mon Sep 17 00:00:00 2001 From: Morten Torkildsen Date: Tue, 25 Aug 2020 16:45:50 -0700 Subject: [PATCH] Handle the --namespace flag --- cmd/apply/cmdapply.go | 15 ++- cmd/initcmd/cmdinit.go | 6 +- cmd/main.go | 2 +- cmd/preview/cmdpreview.go | 16 ++- pkg/config/initoptions.go | 89 +++++++++++------ pkg/config/initoptions_test.go | 177 ++++++++++++++++++++++++++++----- pkg/manifestreader/common.go | 8 ++ 7 files changed, 248 insertions(+), 65 deletions(-) diff --git a/cmd/apply/cmdapply.go b/cmd/apply/cmdapply.go index 9a782b0..bc44b1d 100644 --- a/cmd/apply/cmdapply.go +++ b/cmd/apply/cmdapply.go @@ -117,10 +117,21 @@ func (r *ApplyRunner) RunE(cmd *cobra.Command, args []string) error { return err } + // Fetch the namespace from the configloader. The source of this + // either the namespace flag or the context. If the namespace is provided + // with the flag, enforceNamespace will be true. In this case, it is + // an error if any of the resources in the package has a different + // namespace set. + namespace, enforceNamespace, err := r.provider.Factory().ToRawKubeConfigLoader().Namespace() + if err != nil { + return err + } + var reader manifestreader.ManifestReader readerOptions := manifestreader.ReaderOptions{ - Factory: r.provider.Factory(), - Namespace: metav1.NamespaceDefault, + Factory: r.provider.Factory(), + Namespace: namespace, + EnforceNamespace: enforceNamespace, } if len(args) == 0 { reader = &manifestreader.StreamManifestReader{ diff --git a/cmd/initcmd/cmdinit.go b/cmd/initcmd/cmdinit.go index c4ca543..164842c 100644 --- a/cmd/initcmd/cmdinit.go +++ b/cmd/initcmd/cmdinit.go @@ -6,14 +6,15 @@ package initcmd import ( "github.com/spf13/cobra" "k8s.io/cli-runtime/pkg/genericclioptions" + cmdutil "k8s.io/kubectl/pkg/cmd/util" "k8s.io/kubectl/pkg/util/i18n" "sigs.k8s.io/cli-utils/pkg/config" ) // NewCmdInit creates the `init` command, which generates the // inventory object template ConfigMap for a package. -func NewCmdInit(ioStreams genericclioptions.IOStreams) *cobra.Command { - io := config.NewInitOptions(ioStreams) +func NewCmdInit(f cmdutil.Factory, ioStreams genericclioptions.IOStreams) *cobra.Command { + io := config.NewInitOptions(f, ioStreams) cmd := &cobra.Command{ Use: "init DIRECTORY", DisableFlagsInUseLine: true, @@ -27,6 +28,5 @@ func NewCmdInit(ioStreams genericclioptions.IOStreams) *cobra.Command { }, } cmd.Flags().StringVarP(&io.InventoryID, "inventory-id", "i", "", "Identifier for group of applied resources. Must be composed of valid label characters.") - cmd.Flags().StringVarP(&io.Namespace, "inventory-namespace", "", "", "namespace for the resources to be initialized") return cmd } diff --git a/cmd/main.go b/cmd/main.go index d63e432..292f14c 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -55,7 +55,7 @@ func main() { } names := []string{"init", "apply", "preview", "diff", "destroy", "status"} - initCmd := initcmd.NewCmdInit(ioStreams) + initCmd := initcmd.NewCmdInit(f, ioStreams) updateHelp(names, initCmd) applyCmd := apply.ApplyCommand(f, ioStreams) updateHelp(names, applyCmd) diff --git a/cmd/preview/cmdpreview.go b/cmd/preview/cmdpreview.go index 46531d9..74a0cf9 100644 --- a/cmd/preview/cmdpreview.go +++ b/cmd/preview/cmdpreview.go @@ -7,7 +7,6 @@ import ( "context" "github.com/spf13/cobra" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/cli-runtime/pkg/genericclioptions" cmdutil "k8s.io/kubectl/pkg/cmd/util" "k8s.io/kubectl/pkg/util/i18n" @@ -117,10 +116,21 @@ func (r *PreviewRunner) RunE(cmd *cobra.Command, args []string) error { return err } + // Fetch the namespace from the configloader. The source of this + // either the namespace flag or the context. If the namespace is provided + // with the flag, enforceNamespace will be true. In this case, it is + // an error if any of the resources in the package has a different + // namespace set. + namespace, enforceNamespace, err := r.provider.Factory().ToRawKubeConfigLoader().Namespace() + if err != nil { + return err + } + var reader manifestreader.ManifestReader readerOptions := manifestreader.ReaderOptions{ - Factory: r.provider.Factory(), - Namespace: metav1.NamespaceDefault, + Factory: r.provider.Factory(), + Namespace: namespace, + EnforceNamespace: enforceNamespace, } if len(args) == 0 { reader = &manifestreader.StreamManifestReader{ diff --git a/pkg/config/initoptions.go b/pkg/config/initoptions.go index b791071..40ee19c 100644 --- a/pkg/config/initoptions.go +++ b/pkg/config/initoptions.go @@ -11,10 +11,9 @@ import ( "strings" "time" - "github.com/go-errors/errors" "github.com/google/uuid" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/cli-runtime/pkg/genericclioptions" + cmdutil "k8s.io/kubectl/pkg/cmd/util" "sigs.k8s.io/cli-utils/pkg/common" "sigs.k8s.io/kustomize/kyaml/kio" ) @@ -62,6 +61,8 @@ metadata: // InitOptions contains the fields necessary to generate a // inventory object template ConfigMap. type InitOptions struct { + factory cmdutil.Factory + ioStreams genericclioptions.IOStreams // Package directory argument; must be valid directory. Dir string @@ -71,8 +72,9 @@ type InitOptions struct { InventoryID string } -func NewInitOptions(ioStreams genericclioptions.IOStreams) *InitOptions { +func NewInitOptions(f cmdutil.Factory, ioStreams genericclioptions.IOStreams) *InitOptions { return &InitOptions{ + factory: f, ioStreams: ioStreams, } } @@ -89,14 +91,13 @@ func (i *InitOptions) Complete(args []string) error { return err } i.Dir = dir - if len(i.Namespace) == 0 { - // Returns default namespace if no namespace found. - namespace, err := calcPackageNamespace(i.Dir) - if err != nil { - return err - } - i.Namespace = namespace + + ns, err := findNamespace(i.factory.ToRawKubeConfigLoader(), i.Dir) + if err != nil { + return err } + i.Namespace = ns + // Set the default inventory label if one does not exist. if len(i.InventoryID) == 0 { inventoryID, err := i.defaultInventoryID() @@ -113,6 +114,35 @@ func (i *InitOptions) Complete(args []string) error { return nil } +type namespaceLoader interface { + Namespace() (string, bool, error) +} + +// findNamespace looks up the namespace that should be used for the +// inventory template of the package. If the namespace is specified with +// the --namespace flag, it will be used no matter what. If not, this +// will look at all the resource, and if all belong in the same namespace, +// it will return that namespace. Otherwise, it will return the namespace +// set in the context. +func findNamespace(loader namespaceLoader, dir string) (string, error) { + namespace, enforceNamespace, err := loader.Namespace() + if err != nil { + return "", err + } + if enforceNamespace { + return namespace, nil + } + + ns, allInSameNs, err := allInSameNamespace(dir) + if err != nil { + return "", err + } + if allInSameNs { + return ns, nil + } + return namespace, nil +} + // normalizeDir returns full absolute directory path of the // passed directory or an error. This function cleans up paths // such as current directory (.), relative directories (..), or @@ -136,35 +166,36 @@ func isDirectory(path string) bool { return false } -// calcPackageNamespace returns the namespace of the package -// config files. Assumes all namespaced resources are in the -// same namespace. Returns the default namespace if none of the -// config files has a namespace. -func calcPackageNamespace(packageDir string) (string, error) { +// allInSameNamespace goes through all resources in the package and +// checks the namespace for all of them. If they all have the namespace +// set and they all have the same value, this will return that namespace +// and the second return value will be true. Otherwise, it will not return +// a namespace and the second return value will be false. +func allInSameNamespace(packageDir string) (string, bool, error) { r := kio.LocalPackageReader{PackagePath: packageDir} nodes, err := r.Read() if err != nil { - return "", err + return "", false, err } - // Return the non-empty unique namespace if found. Cluster-scoped - // resources do not have namespace set. - currentNamespace := metav1.NamespaceDefault + var ns string for _, node := range nodes { rm, err := node.GetMeta() - if err != nil || len(rm.ObjectMeta.Namespace) == 0 { - continue + if err != nil { + return "", false, err } - if currentNamespace == metav1.NamespaceDefault { - currentNamespace = rm.ObjectMeta.Namespace + if rm.Namespace == "" { + return "", false, nil } - if currentNamespace != rm.ObjectMeta.Namespace { - return "", errors.Errorf( - "resources belong to different namespaces, a namespace is required to create the resource " + - "used for keeping track of past apply operations. Please specify ---inventory-namespace.") + if ns == "" { + ns = rm.Namespace + } else if rm.Namespace != ns { + return "", false, nil } } - // Return the default namespace if none found. - return currentNamespace, nil + if ns != "" { + return ns, true, nil + } + return "", false, nil } // defaultInventoryID returns a UUID string as a default unique diff --git a/pkg/config/initoptions_test.go b/pkg/config/initoptions_test.go index a4554ef..ca35789 100644 --- a/pkg/config/initoptions_test.go +++ b/pkg/config/initoptions_test.go @@ -14,10 +14,9 @@ import ( "github.com/stretchr/testify/assert" "k8s.io/cli-runtime/pkg/genericclioptions" + cmdtesting "k8s.io/kubectl/pkg/cmd/testing" ) -var ioStreams = genericclioptions.IOStreams{} - // writeFile writes a file under the test directory func writeFile(t *testing.T, path string, value []byte) { err := ioutil.WriteFile(path, value, 0600) @@ -42,50 +41,171 @@ metadata: namespace: namespaceB `) +var readFileC = []byte(` +apiVersion: v1 +kind: Pod +metadata: + name: objC +`) + func TestComplete(t *testing.T) { - d1, err := ioutil.TempDir("", "test-dir") - if !assert.NoError(t, err) { - assert.FailNow(t, err.Error()) - } - defer os.RemoveAll(d1) - - writeFile(t, filepath.Join(d1, "a_test.yaml"), readFileA) - writeFile(t, filepath.Join(d1, "b_test.yaml"), readFileB) - tests := map[string]struct { - args []string - isError bool + args []string + files map[string][]byte + isError bool + expectedErrMessage string + expectedNamespace string }{ "Empty args returns error": { - args: []string{}, - isError: true, + args: []string{}, + isError: true, + expectedErrMessage: "need one 'directory' arg; have 0", }, "More than one argument should fail": { - args: []string{"foo", "bar"}, - isError: true, + args: []string{"foo", "bar"}, + isError: true, + expectedErrMessage: "need one 'directory' arg; have 2", }, "Non-directory arg should fail": { - args: []string{"foo"}, - isError: true, + args: []string{"foo"}, + isError: true, + expectedErrMessage: "invalid directory argument: foo", }, "More than one namespace should fail": { - args: []string{d1}, - isError: true, + args: []string{}, + files: map[string][]byte{ + "a_test.yaml": readFileA, + "b_test.yaml": readFileB, + }, + isError: true, + expectedErrMessage: "resources belong to different namespaces", + }, + "If at least one resource doesn't have namespace, it should use the default": { + args: []string{}, + files: map[string][]byte{ + "b_test.yaml": readFileB, + "c_test.yaml": readFileC, + }, + isError: false, + expectedNamespace: "foo", + }, + "No resources without namespace should use the default namespace": { + args: []string{}, + files: map[string][]byte{ + "c_test.yaml": readFileC, + }, + isError: false, + expectedNamespace: "foo", }, } for name, tc := range tests { t.Run(name, func(t *testing.T) { - io := NewInitOptions(ioStreams) - err := io.Complete(tc.args) - if tc.isError && err == nil { - t.Errorf("Expected error, but did not receive one") + var err error + dir, err := ioutil.TempDir("", "test-dir") + if !assert.NoError(t, err) { + assert.FailNow(t, err.Error()) } + defer os.RemoveAll(dir) + + for fileName, fileContent := range tc.files { + writeFile(t, filepath.Join(dir, fileName), fileContent) + } + if len(tc.files) > 0 { + tc.args = append(tc.args, dir) + } + + tf := cmdtesting.NewTestFactory().WithNamespace("foo") + defer tf.Cleanup() + ioStreams, _, out, _ := genericclioptions.NewTestIOStreams() + io := NewInitOptions(tf, ioStreams) + err = io.Complete(tc.args) + + if err != nil { + if !tc.isError { + t.Errorf("Expected error, but did not receive one") + return + } + assert.Contains(t, err.Error(), tc.expectedErrMessage) + return + } + assert.Contains(t, out.String(), tc.expectedNamespace) }) } } +func TestFindNamespace(t *testing.T) { + testCases := map[string]struct { + namespace string + enforceNamespace bool + files map[string][]byte + expectedNamespace string + }{ + "fallback to default": { + namespace: "foo", + enforceNamespace: false, + files: map[string][]byte{ + "a_test.yaml": readFileA, + "b_test.yaml": readFileB, + }, + expectedNamespace: "foo", + }, + "enforce namespace": { + namespace: "bar", + enforceNamespace: true, + files: map[string][]byte{ + "a_test.yaml": readFileA, + }, + expectedNamespace: "bar", + }, + "use namespace from resource if all the same": { + namespace: "bar", + enforceNamespace: false, + files: map[string][]byte{ + "a_test.yaml": readFileA, + }, + expectedNamespace: "namespaceA", + }, + } + + for tn, tc := range testCases { + t.Run(tn, func(t *testing.T) { + var err error + dir, err := ioutil.TempDir("", "test-dir") + if !assert.NoError(t, err) { + assert.FailNow(t, err.Error()) + } + defer os.RemoveAll(dir) + + for fileName, fileContent := range tc.files { + writeFile(t, filepath.Join(dir, fileName), fileContent) + } + + fakeLoader := &fakeNamespaceLoader{ + namespace: tc.namespace, + enforceNamespace: tc.enforceNamespace, + } + + namespace, err := findNamespace(fakeLoader, dir) + assert.NoError(t, err) + assert.Equal(t, tc.expectedNamespace, namespace) + }) + } +} + +type fakeNamespaceLoader struct { + namespace string + enforceNamespace bool +} + +func (f *fakeNamespaceLoader) Namespace() (string, bool, error) { + return f.namespace, f.enforceNamespace, nil +} + func TestDefaultInventoryID(t *testing.T) { - io := NewInitOptions(ioStreams) + tf := cmdtesting.NewTestFactory().WithNamespace("foo") + defer tf.Cleanup() + ioStreams, _, _, _ := genericclioptions.NewTestIOStreams() //nolint:dogsled + io := NewInitOptions(tf, ioStreams) actual, err := io.defaultInventoryID() if err != nil { t.Errorf("Unxpected error during UUID generation: %v", err) @@ -172,7 +292,10 @@ func TestFillInValues(t *testing.T) { for name, tc := range tests { t.Run(name, func(t *testing.T) { - io := NewInitOptions(ioStreams) + tf := cmdtesting.NewTestFactory().WithNamespace("foo") + defer tf.Cleanup() + ioStreams, _, _, _ := genericclioptions.NewTestIOStreams() //nolint:dogsled + io := NewInitOptions(tf, ioStreams) io.Namespace = tc.namespace io.InventoryID = tc.inventoryID actual := io.fillInValues() diff --git a/pkg/manifestreader/common.go b/pkg/manifestreader/common.go index e03d968..d38c992 100644 --- a/pkg/manifestreader/common.go +++ b/pkg/manifestreader/common.go @@ -11,6 +11,7 @@ import ( "k8s.io/cli-runtime/pkg/resource" "k8s.io/kubectl/pkg/cmd/util" "sigs.k8s.io/cli-utils/pkg/apply/solver" + "sigs.k8s.io/cli-utils/pkg/inventory" ) // ManifestReader defines the interface for reading a set @@ -53,6 +54,13 @@ func setNamespaces(factory util.Factory, infos []*resource.Info, for _, inf := range infos { accessor, _ := meta.Accessor(inf.Object) + + // Exclude any inventory objects here since we don't want to change + // their namespace. + if inventory.IsInventoryObject(inf) { + continue + } + // if the resource already has the namespace set, we don't // need to do anything if ns := accessor.GetNamespace(); ns != "" {