diff --git a/pkg/app/app.go b/pkg/app/app.go index 8ac6eec15..69b0bf1ae 100644 --- 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 index 6bfbd9dea..9bc6afcc7 100644 --- 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 index 11a74eb73..b7e9e82a3 100644 --- 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}