From 2ea8ff3470161956f3ed87ae1f5553ff22eba3f6 Mon Sep 17 00:00:00 2001 From: Praveen Rewar <8457124+praveenrewar@users.noreply.github.com> Date: Sun, 24 Sep 2023 20:57:10 +0530 Subject: [PATCH] Fix app-namespace usage for cluster options During the introduction of defaultNamespace feature, we started using --app-namespace flag from kapp which should be used carefully when using cluster options instead of service account Signed-off-by: Praveen Rewar <8457124+praveenrewar@users.noreply.github.com> --- pkg/deploy/kapp.go | 9 +- pkg/kubeconfig/kubeconfig.go | 14 +- pkg/kubeconfig/kubeconfig_restricted.go | 8 +- pkg/kubeconfig/kubeconfig_secrets.go | 6 +- pkg/kubeconfig/service_accounts.go | 7 +- .../kappcontroller/default_namespace_test.go | 133 ++++++++++++++++++ test/e2e/secrets.go | 41 ++++++ 7 files changed, 206 insertions(+), 12 deletions(-) create mode 100644 test/e2e/secrets.go diff --git a/pkg/deploy/kapp.go b/pkg/deploy/kapp.go index 5618bab79b..49d599521c 100644 --- a/pkg/deploy/kapp.go +++ b/pkg/deploy/kapp.go @@ -63,7 +63,7 @@ func (a *Kapp) Deploy(tplOutput string, startedApplyingFunc func(), metadataFile := filepath.Join(tmpMetadataDir.Path(), "app-metadata.yml") - args, err := a.addDeployArgs([]string{"deploy", "--app-metadata-file-output", metadataFile, "--prev-app", a.oldManagedName(), "-f", "-", "--app-namespace", a.appNamespace}) + args, err := a.addDeployArgs([]string{"deploy", "--app-metadata-file-output", metadataFile, "--prev-app", a.oldManagedName(), "-f", "-"}) if err != nil { return exec.NewCmdRunResultWithErr(err) } @@ -90,7 +90,7 @@ func (a *Kapp) Deploy(tplOutput string, startedApplyingFunc func(), // Delete takes the app name, it shells out, running kapp delete ... func (a *Kapp) Delete(startedApplyingFunc func(), changedFunc func(exec.CmdRunResult)) exec.CmdRunResult { - args, err := a.addDeleteArgs([]string{"delete", "--prev-app", a.oldManagedName(), "--app-namespace", a.appNamespace}) + args, err := a.addDeleteArgs([]string{"delete", "--prev-app", a.oldManagedName()}) if err != nil { return exec.NewCmdRunResultWithErr(err) } @@ -120,7 +120,6 @@ func (a *Kapp) Inspect() exec.CmdRunResult { // TODO is there a better way to deal with this? "--filter", `{"not":{"resource":{"kinds":["PodMetrics"]}}}`, "--tty", - "--app-namespace", a.appNamespace, }) if err != nil { return exec.NewCmdRunResultWithErr(err) @@ -260,6 +259,10 @@ func (a *Kapp) addGenericArgs(args []string, appName string) ([]string, []string args = append(args, []string{"--namespace", a.clusterAccess.Namespace}...) } + if len(a.clusterAccess.DeployNamespace) > 0 { + args = append(args, []string{"--app-namespace", a.clusterAccess.DeployNamespace}...) + } + switch { case a.clusterAccess.Kubeconfig != nil: env = append(env, "KAPP_KUBECONFIG_YAML="+a.clusterAccess.Kubeconfig.AsYAML()) diff --git a/pkg/kubeconfig/kubeconfig.go b/pkg/kubeconfig/kubeconfig.go index 7fbf5e6ea1..93ed80ce85 100644 --- a/pkg/kubeconfig/kubeconfig.go +++ b/pkg/kubeconfig/kubeconfig.go @@ -32,6 +32,8 @@ type AccessInfo struct { Name string Namespace string + DeployNamespace string + Kubeconfig *Restricted DangerousUsePodServiceAccount bool } @@ -47,7 +49,7 @@ func NewKubeconfig(coreClient kubernetes.Interface, log logr.Logger) *Kubeconfig // ClusterAccess takes cluster info and a ServiceAccount Name, and returns a populated kubeconfig that can connect to a cluster. // if the saName is empty then you'll connect to a cluster via the clusterOpts inside the genericOpts, otherwise you'll use the specified SA. -func (k Kubeconfig) ClusterAccess(saName string, clusterOpts *v1alpha1.AppCluster, accessLocation AccessLocation, preferredNamespace string) (AccessInfo, error) { +func (k Kubeconfig) ClusterAccess(saName string, clusterOpts *v1alpha1.AppCluster, accessLocation AccessLocation, defaultNamespace string) (AccessInfo, error) { var err error var clusterAccessInfo AccessInfo @@ -63,13 +65,19 @@ func (k Kubeconfig) ClusterAccess(saName string, clusterOpts *v1alpha1.AppCluste if err != nil { return AccessInfo{}, err } + // Use kubeconfig preferred namespace as deploy namespace if + // defaultNamespace is provided and cluster.namespace is not provided, + if defaultNamespace != "" && clusterAccessInfo.DeployNamespace == "" { + clusterAccessInfo.DeployNamespace = clusterAccessInfo.Kubeconfig.defaultNamespace + } default: return AccessInfo{}, fmt.Errorf("Expected service account or cluster specified") } - // If preferredNamespace is "", then kubeconfig preferred namespace will be used - clusterAccessInfo.Namespace = preferredNamespace + if defaultNamespace != "" { + clusterAccessInfo.Namespace = defaultNamespace + } return clusterAccessInfo, nil } diff --git a/pkg/kubeconfig/kubeconfig_restricted.go b/pkg/kubeconfig/kubeconfig_restricted.go index 8f11e0d690..346b54369b 100644 --- a/pkg/kubeconfig/kubeconfig_restricted.go +++ b/pkg/kubeconfig/kubeconfig_restricted.go @@ -13,6 +13,8 @@ import ( // Restricted contains a kubernetes kubeconfig as a string type Restricted struct { result string + + defaultNamespace string } // NewKubeconfigRestricted takes kubeconfig yaml as input and returns kubeconfig yaml with certain fields restricted (removed). @@ -65,6 +67,7 @@ func NewKubeconfigRestricted(input string) (*Restricted, error) { }) } + defaultNamespace := "default" // TODO: Use the value from client-go directly for _, inputCtx := range inputConfig.Contexts { resultConfig.Contexts = append(resultConfig.Contexts, clientcmd.NamedContext{ Name: inputCtx.Name, @@ -74,6 +77,9 @@ func NewKubeconfigRestricted(input string) (*Restricted, error) { Namespace: inputCtx.Context.Namespace, }, }) + if inputCtx.Name == inputConfig.CurrentContext && inputCtx.Context.Namespace != "" { + defaultNamespace = inputCtx.Context.Namespace + } } bs, err := yaml.Marshal(resultConfig) @@ -81,7 +87,7 @@ func NewKubeconfigRestricted(input string) (*Restricted, error) { return nil, fmt.Errorf("Marshaling kubeconfig: %s", err) } - return &Restricted{string(bs)}, nil + return &Restricted{string(bs), defaultNamespace}, nil } // AsYAML returns a string formatted kubernetes kubeconfig diff --git a/pkg/kubeconfig/kubeconfig_secrets.go b/pkg/kubeconfig/kubeconfig_secrets.go index d0fb2c1bf0..36ea370523 100644 --- a/pkg/kubeconfig/kubeconfig_secrets.go +++ b/pkg/kubeconfig/kubeconfig_secrets.go @@ -49,8 +49,10 @@ func (s *Secrets) Find(accessLocation AccessLocation, Name: accessLocation.Name, // Override destination namespace; if it's empty // assume kubeconfig contains preferred namespace - Namespace: clusterOpts.Namespace, - Kubeconfig: kubeconfigRestricted, + Namespace: clusterOpts.Namespace, + // Use provided namespace as app namespace + DeployNamespace: clusterOpts.Namespace, + Kubeconfig: kubeconfigRestricted, } return pgoForCluster, nil diff --git a/pkg/kubeconfig/service_accounts.go b/pkg/kubeconfig/service_accounts.go index 7b089e877a..fde14ca9ad 100644 --- a/pkg/kubeconfig/service_accounts.go +++ b/pkg/kubeconfig/service_accounts.go @@ -51,9 +51,10 @@ func (s *ServiceAccounts) Find(accessLocation AccessLocation, saName string) (Ac } pgoForSA := AccessInfo{ - Name: accessLocation.Name, - Namespace: "", // Assume kubeconfig contains preferred namespace from SA - Kubeconfig: kubeconfigRestricted, + Name: accessLocation.Name, + Namespace: "", // Assume kubeconfig contains preferred namespace from SA + DeployNamespace: accessLocation.Namespace, // App namespace is same as SA namespace + Kubeconfig: kubeconfigRestricted, } return pgoForSA, nil diff --git a/test/e2e/kappcontroller/default_namespace_test.go b/test/e2e/kappcontroller/default_namespace_test.go index ebac6fe6fd..31fd02a6f2 100644 --- a/test/e2e/kappcontroller/default_namespace_test.go +++ b/test/e2e/kappcontroller/default_namespace_test.go @@ -5,6 +5,7 @@ package kappcontroller import ( "fmt" + "os" "strings" "testing" @@ -71,6 +72,138 @@ spec: kubectl.RunWithOpts([]string{"get", "configmap", name + ".app", "-n", env.Namespace}, e2e.RunOpts{NoNamespace: true}) } +func Test_AppDefaultNamespace_WithTargetCluster(t *testing.T) { + targetClusterKubeconfig := os.Getenv("TEST_E2E_TARGET_CLUSTER_KUBECONFIG") + if targetClusterKubeconfig == "" { + t.Skip("Skipping test as target cluster kubeconfig is not set") + } + + kubeconfigFile, err := os.CreateTemp("", "e2e-kubeconfig-*") + assert.NoError(t, err) + defer os.Remove(kubeconfigFile.Name()) + + _, err = kubeconfigFile.Write([]byte(targetClusterKubeconfig)) + assert.NoError(t, err) + + env := e2e.BuildEnv(t) + logger := e2e.Logger{} + kapp := e2e.Kapp{t, env.Namespace, logger} + kubectl := e2e.Kubectl{t, env.Namespace, logger} + + name := "app-default-namespace-target-cluster" + defaultNamespace := "e2e-default-namespace" + clusterNamespace := "e2e-cluster-namespace" + namespaceApp := "namespace-app" + secretName := "e2e-kubeconfig-secret" + + cleanUp := func() { + kapp.Run([]string{"delete", "-a", name}) + + } + cleanUpTargetCluster := func() { + kapp.RunWithOpts([]string{"delete", "-a", namespaceApp, "--kubeconfig", kubeconfigFile.Name()}, e2e.RunOpts{NoNamespace: true}) + } + + cleanUp() + cleanUpTargetCluster() + defer cleanUp() + defer cleanUpTargetCluster() + + namespaceYAML := fmt.Sprintf(`--- +apiVersion: v1 +kind: Namespace +metadata: + name: %s +--- +apiVersion: v1 +kind: Namespace +metadata: + name: %s`, defaultNamespace, clusterNamespace) + + secret := e2e.Secrets{secretName, env.Namespace, targetClusterKubeconfig} + appYAML := `--- +apiVersion: kappctrl.k14s.io/v1alpha1 +kind: App +metadata: + name: %s + namespace: %s + annotations: + kapp.k14s.io/change-group: "kappctrl-e2e.k14s.io/apps" +spec: + cluster: + namespace: %s + kubeconfigSecretRef: + name: %s + defaultNamespace: %s + fetch: + - inline: + paths: + file.yml: | + apiVersion: v1 + kind: ConfigMap + metadata: + name: my-cm + data: + key: value + template: + - ytt: {} + deploy: + - kapp: {}` + + // create test namespaces on target cluster + kapp.RunWithOpts([]string{"deploy", "-a", namespaceApp, "-f", "-", "--kubeconfig", kubeconfigFile.Name()}, e2e.RunOpts{NoNamespace: true, StdinReader: strings.NewReader(namespaceYAML)}) + + // Provide both _defaultNamespace_ and _cluster.namespace_ + _, err = kapp.RunWithOpts([]string{"deploy", "-a", name, "-f", "-"}, e2e.RunOpts{AllowError: true, + StdinReader: strings.NewReader(fmt.Sprintf(appYAML, name, env.Namespace, clusterNamespace, secretName, defaultNamespace) + secret.ForTargetCluster())}) + assert.NoError(t, err, "Expected app deploy to succeed, it did not") + + // Assert that app resources are in defaultNamespace + kubectl.RunWithOpts([]string{"get", "configmap", "my-cm", "-n", defaultNamespace, "--kubeconfig", kubeconfigFile.Name()}, e2e.RunOpts{NoNamespace: true}) + + // Assert that kapp metaconfigmap is in cluster.namespace + kubectl.RunWithOpts([]string{"get", "configmap", name + ".app", "-n", clusterNamespace, "--kubeconfig", kubeconfigFile.Name()}, e2e.RunOpts{NoNamespace: true}) + + cleanUp() + + // Provide only _cluster.namespace_ + _, err = kapp.RunWithOpts([]string{"deploy", "-a", name, "-f", "-"}, e2e.RunOpts{AllowError: true, + StdinReader: strings.NewReader(fmt.Sprintf(appYAML, name, env.Namespace, clusterNamespace, secretName, "") + secret.ForTargetCluster())}) + assert.NoError(t, err, "Expected app deploy to succeed, it did not") + + // Assert that app resources are in cluster.namespace + kubectl.RunWithOpts([]string{"get", "configmap", "my-cm", "-n", clusterNamespace, "--kubeconfig", kubeconfigFile.Name()}, e2e.RunOpts{NoNamespace: true}) + + // Assert that kapp metaconfigmap is in cluster.namespace + kubectl.RunWithOpts([]string{"get", "configmap", name + ".app", "-n", clusterNamespace, "--kubeconfig", kubeconfigFile.Name()}, e2e.RunOpts{NoNamespace: true}) + + cleanUp() + + // Provide only _defaultNamespace_ + _, err = kapp.RunWithOpts([]string{"deploy", "-a", name, "-f", "-"}, e2e.RunOpts{AllowError: true, + StdinReader: strings.NewReader(fmt.Sprintf(appYAML, name, env.Namespace, "", secretName, defaultNamespace) + secret.ForTargetCluster())}) + assert.NoError(t, err, "Expected app deploy to succeed, it did not") + + // Assert that app resources are in defaultNamespace + kubectl.RunWithOpts([]string{"get", "configmap", "my-cm", "-n", defaultNamespace, "--kubeconfig", kubeconfigFile.Name()}, e2e.RunOpts{NoNamespace: true}) + + // Assert that kapp metaconfigmap is in kubeconfig preferred namespace (default) + kubectl.RunWithOpts([]string{"get", "configmap", name + ".app", "-n", "default", "--kubeconfig", kubeconfigFile.Name()}, e2e.RunOpts{NoNamespace: true}) + + cleanUp() + + // Do not provide _defaultNamespace_ and _cluster.namespace_ + _, err = kapp.RunWithOpts([]string{"deploy", "-a", name, "-f", "-"}, e2e.RunOpts{AllowError: true, + StdinReader: strings.NewReader(fmt.Sprintf(appYAML, name, env.Namespace, "", secretName, "") + secret.ForTargetCluster())}) + assert.NoError(t, err, "Expected app deploy to succeed, it did not") + + // Assert that app resources are in kubeconfig preferred namespace (default) + kubectl.RunWithOpts([]string{"get", "configmap", "my-cm", "-n", "default", "--kubeconfig", kubeconfigFile.Name()}, e2e.RunOpts{NoNamespace: true}) + + // Assert that kapp metaconfigmap is in kubeconfig preferred namespace (default) + kubectl.RunWithOpts([]string{"get", "configmap", name + ".app", "-n", "default", "--kubeconfig", kubeconfigFile.Name()}, e2e.RunOpts{NoNamespace: true}) +} + func Test_PackageInstall_DefaultNamespace(t *testing.T) { env := e2e.BuildEnv(t) logger := e2e.Logger{} diff --git a/test/e2e/secrets.go b/test/e2e/secrets.go new file mode 100644 index 0000000000..d62520beef --- /dev/null +++ b/test/e2e/secrets.go @@ -0,0 +1,41 @@ +// Copyright 2023 VMware, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package e2e + +import ( + "fmt" + "strings" +) + +// Secrets represents a secret with target cluster kubeconfig +type Secrets struct { + Name string + Namespace string + Kubeconfig string +} + +// ForTargetCluster can be used to get secret with target cluster kubeconfig +func (s Secrets) ForTargetCluster() string { + indentedKubeconfig := "" + for _, line := range strings.Split(s.Kubeconfig, "\n") { + if line != "" { + indentedKubeconfig += " " + line + "\n" + } + } + return fmt.Sprintf(` +--- +apiVersion: v1 +kind: Secret +metadata: + name: %s + namespace: %s + annotations: + kapp.k14s.io/change-rule.apps: "delete after deleting kappctrl-e2e.k14s.io/apps" + kapp.k14s.io/change-rule.instpkgs: "delete after deleting kappctrl-e2e.k14s.io/packageinstalls" +type: Opaque +stringData: + value: | +%s +`, s.Name, s.Namespace, indentedKubeconfig) +}