Enhance fallback to noop logic for apps to account for multiple names…

…paces (#1394) * Enhance fallback to noop logic for apps to account for multiple namespaces Signed-off-by: Soumik Majumder <[email protected]> * Add tests for fallback to noop cases spanning multiple clusters Signed-off-by: Soumik Majumder <[email protected]> --------- Signed-off-by: Soumik Majumder <[email protected]>
carvel-dev · Nov 15, 2023 · 934d244 · 934d244
1 parent 6d83c43
commit 934d244
Show file tree

Hide file tree

Showing 3 changed files with 180 additions and 18 deletions.
diff --git a/pkg/app/app.go b/pkg/app/app.go
@@ -276,6 +276,35 @@ func (a *App) ConfigMapRefs() map[reftracker.RefKey]struct{} {
 	return configMaps
 }
 
+func (a *App) noopDeleteDueToTerminatingNamespaces() bool {
+	if a.app.Status.Deploy == nil || a.app.Status.Deploy.KappDeployStatus == nil || a.app.Spec.ServiceAccountName == "" {
+		return false
+	}
+	if !a.isNamespaceTerminating(a.app.Namespace) {
+		return false
+	}
+	// Ensure that no cluster scoped resources are created by the app
+	// and all affected namespaces are terminating
+	for _, ns := range a.app.Status.Deploy.KappDeployStatus.AssociatedResources.Namespaces {
+		if ns == "(cluster)" {
+			return false
+		}
+		if !a.isNamespaceTerminating(ns) {
+			return false
+		}
+	}
+	a.log.Info("Safely performing noop delete to avoid blocking namespace deletion")
+	return true
+}
+
+func (a *App) isNamespaceTerminating(namespace string) bool {
+	status, err := a.compInfo.NamespaceStatus(namespace)
+	if err != nil {
+		a.log.Error(err, "Error getting app namespace status", "app", a.app.Name, "namespace", a.app.Namespace)
+	}
+	return status.Phase == v1.NamespaceTerminating
+}
+
 // HasImageOrImgpkgBundle is used to determine if the
 // App's spec contains a fetch stage for an image or
 // imgpkgbundle. It is mainly used to determine whether
@@ -292,11 +321,3 @@ func (a App) HasImageOrImgpkgBundle() bool {
 	}
 	return false
 }
-
-func (a App) isNamespaceTerminating() bool {
-	status, err := a.compInfo.NamespaceStatus(a.app.Namespace)
-	if err != nil {
-		a.log.Error(err, "Error getting app namespace status", "app", a.app.Name, "namespace", a.app.Namespace)
-	}
-	return status.Phase == v1.NamespaceTerminating
-}
diff --git a/pkg/app/app_deploy.go b/pkg/app/app_deploy.go
@@ -57,15 +57,7 @@ func (a *App) delete(changedFunc func(exec.CmdRunResult)) exec.CmdRunResult {
 
 	var result exec.CmdRunResult
 
-	appResourcesInSameNs := a.app.Status.Deploy != nil && a.app.Status.Deploy.KappDeployStatus != nil &&
-		len(a.app.Status.Deploy.KappDeployStatus.AssociatedResources.Namespaces) == 1 &&
-		a.app.Status.Deploy.KappDeployStatus.AssociatedResources.Namespaces[0] == a.app.Namespace
-
-	// Use noopDelete if the namespace is terminating and app resources are in same namespace because
-	// the app resources will be automatically deleted including the kapp ServiceAccount
-	noopDelete := a.isNamespaceTerminating() && appResourcesInSameNs && a.app.Spec.Cluster == nil
-
-	if !a.app.Spec.NoopDelete && !noopDelete {
+	if !a.app.Spec.NoopDelete && !a.noopDeleteDueToTerminatingNamespaces() {
 		for _, dep := range a.app.Spec.Deploy {
 			switch {
 			case dep.Kapp != nil:

diff --git a/test/e2e/kappcontroller/namespace_deletion_test.go b/test/e2e/kappcontroller/namespace_deletion_test.go
@@ -68,7 +68,156 @@ spec:
 	})
 }
 
-func Test_NamespaceDelete_AppWithResourcesInDifferentNamespaces(t *testing.T) {
+func Test_NamespaceDelete_AppWithResourcesInDifferentTerminatingNamespaces(t *testing.T) {
+	env := e2e.BuildEnv(t)
+	logger := e2e.Logger{}
+	kapp := e2e.Kapp{t, env.Namespace, logger}
+	kubectl := e2e.Kubectl{t, env.Namespace, logger}
+
+	nsName1 := "ns-delete-1"
+	nsName2 := "ns-delete-2"
+	nsApp := "testnamespaces"
+	name := "resources-in-different-namespaces"
+
+	namespaceTemplate := `---
+apiVersion: v1
+kind: Namespace
+metadata:
+  name: %v`
+	namespaceYAML := fmt.Sprintf(namespaceTemplate, nsName1) + "\n" + fmt.Sprintf(namespaceTemplate, nsName2)
+
+	appYaml := fmt.Sprintf(`---
+apiVersion: kappctrl.k14s.io/v1alpha1
+kind: App
+metadata:
+  name: %s
+spec:
+  serviceAccountName: kappctrl-e2e-ns-sa
+  fetch:
+  - inline:
+      paths:
+        file.yml: |
+          apiVersion: v1
+          kind: ConfigMap
+          metadata:
+           name: configmap
+           namespace: %s
+          data:
+           key: value
+          ---
+          apiVersion: v1
+          kind: ConfigMap
+          metadata:
+           name: configmap
+           namespace: %s
+          data:
+           key: value
+  template:
+  - ytt: {}
+  deploy:
+  - kapp: {}`, name, nsName1, nsName2) + e2e.ServiceAccounts{nsName1}.ForClusterYAML()
+
+	cleanUp := func() {
+		kapp.Run([]string{"delete", "-a", name})
+	}
+
+	cleanUpTestNamespace := func() {
+		kapp.Run([]string{"delete", "-a", name})
+		kapp.Run([]string{"delete", "-a", nsApp})
+	}
+
+	cleanUp()
+	defer cleanUp()
+	defer cleanUpTestNamespace()
+
+	logger.Section("create namespace and deploy App", func() {
+		kapp.RunWithOpts([]string{"deploy", "-a", nsApp, "-f", "-"}, e2e.RunOpts{StdinReader: strings.NewReader(namespaceYAML)})
+		kapp.RunWithOpts([]string{"deploy", "-f", "-", "-a", name, "--into-ns", nsName1}, e2e.RunOpts{StdinReader: strings.NewReader(appYaml)})
+	})
+
+	logger.Section("delete namespace", func() {
+		// delete SA first to reduce flakiness, sometimes SA deletion happens after app is deleted
+		kubectl.RunWithOpts([]string{"delete", "serviceaccount", "kappctrl-e2e-ns-sa", "-n", nsName1},
+			e2e.RunOpts{NoNamespace: true})
+		kubectl.Run([]string{"delete", "ns", nsName1, nsName2, "--timeout=1m"})
+	})
+}
+
+func Test_NamespaceDelete_AppWithClusterScopedResources(t *testing.T) {
+	env := e2e.BuildEnv(t)
+	logger := e2e.Logger{}
+	kapp := e2e.Kapp{t, env.Namespace, logger}
+	kubectl := e2e.Kubectl{t, env.Namespace, logger}
+
+	nsName := "ns-delete-1"
+	name := "app-with-cluster-scoped-resource"
+	inAppNsName := "delete-test-ns"
+
+	namespaceYAML := fmt.Sprintf(`---
+apiVersion: v1
+kind: Namespace
+metadata:
+  name: %v`, nsName)
+
+	appYaml := fmt.Sprintf(`---
+apiVersion: kappctrl.k14s.io/v1alpha1
+kind: App
+metadata:
+  name: %s
+spec:
+  serviceAccountName: kappctrl-e2e-ns-sa
+  fetch:
+  - inline:
+      paths:
+        file.yml: |
+          apiVersion: v1
+          kind: Namespace
+          metadata:
+            name: %s
+          ---
+          apiVersion: v1
+          kind: ConfigMap
+          metadata:
+           name: configmap
+           namespace: %s
+          data:
+           key: value
+  template:
+  - ytt: {}
+  deploy:
+  - kapp: {}`, name, inAppNsName, nsName) + e2e.ServiceAccounts{nsName}.ForClusterYAML()
+
+	cleanUp := func() {
+		kapp.Run([]string{"delete", "-a", name})
+	}
+
+	cleanUpTestNamespace := func() {
+		kubectl.Run([]string{"delete", "ns", inAppNsName})
+		kubectl.RunWithOpts([]string{"patch", "App", name, "--type=json", "--patch", `[{ "op": "replace", "path": "/spec/noopDelete", "value": true}]`,
+			"-n", nsName}, e2e.RunOpts{NoNamespace: true})
+		kapp.Run([]string{"delete", "-a", nsName})
+	}
+
+	cleanUp()
+	defer cleanUp()
+	defer cleanUpTestNamespace()
+
+	logger.Section("create namespace and deploy App", func() {
+		kapp.RunWithOpts([]string{"deploy", "-a", nsName, "-f", "-"}, e2e.RunOpts{StdinReader: strings.NewReader(namespaceYAML)})
+		kapp.RunWithOpts([]string{"deploy", "-f", "-", "-a", name, "--into-ns", nsName}, e2e.RunOpts{StdinReader: strings.NewReader(appYaml)})
+	})
+
+	logger.Section("delete namespace", func() {
+		// delete SA first to reduce flakiness, sometimes SA deletion happens after app is deleted
+		kubectl.RunWithOpts([]string{"delete", "serviceaccount", "kappctrl-e2e-ns-sa", "-n", nsName},
+			e2e.RunOpts{NoNamespace: true})
+		_, err := kubectl.RunWithOpts([]string{"delete", "ns", nsName, "--timeout=30s"},
+			e2e.RunOpts{AllowError: true})
+		assert.Error(t, err, "Expected to get time out error, but did not")
+	})
+}
+
+func Test_NamespaceDelete_AppWithWithOneNonTerminatingAffectedNamespace(t *testing.T) {
 	env := e2e.BuildEnv(t)
 	logger := e2e.Logger{}
 	kapp := e2e.Kapp{t, env.Namespace, logger}