Fix app-namespace usage for cluster options (#1333)

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 <[email protected]>

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())

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
 }

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,14 +77,17 @@ 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)
 	if err != nil {
 		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

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

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

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{}

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)
+}