Do not overwrite kapp deploy status during delete (#1460)

Signed-off-by: Praveen Rewar <[email protected]>

Author: praveenrewar

pkg/app/app_reconcile.go

@@ -166,13 +166,14 @@ func (a *App) updateLastDeploy(result exec.CmdRunResult) exec.CmdRunResult {
 	result = result.WithFriendlyYAMLStrings()
 
 	a.app.Status.Deploy = &v1alpha1.AppStatusDeploy{
-		Stdout:    result.Stdout,
-		Stderr:    result.Stderr,
-		Finished:  result.Finished,
-		ExitCode:  result.ExitCode,
-		Error:     result.ErrorStr(),
-		StartedAt: a.app.Status.Deploy.StartedAt,
-		UpdatedAt: metav1.NewTime(time.Now().UTC()),
+		Stdout:           result.Stdout,
+		Stderr:           result.Stderr,
+		Finished:         result.Finished,
+		ExitCode:         result.ExitCode,
+		Error:            result.ErrorStr(),
+		StartedAt:        a.app.Status.Deploy.StartedAt,
+		UpdatedAt:        metav1.NewTime(time.Now().UTC()),
+		KappDeployStatus: a.app.Status.Deploy.KappDeployStatus,
 	}
 
 	defer a.updateStatus("marking last deploy")
@@ -181,6 +182,11 @@ func (a *App) updateLastDeploy(result exec.CmdRunResult) exec.CmdRunResult {
 		return result
 	}
 
+	// Do not overwrite kapp deploy status during delete
+	if len(a.metadata.LastChange.Namespaces) == 0 {
+		return result
+	}
+
 	usedGKs := []metav1.GroupKind{}
 	for _, gk := range a.metadata.UsedGKs {
 		usedGKs = append(usedGKs, metav1.GroupKind{

test/e2e/kappcontroller/namespace_deletion_test.go

@@ -9,6 +9,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"github.com/vmware-tanzu/carvel-kapp-controller/test/e2e"
 )
 
@@ -292,3 +293,98 @@ spec:
 		assert.Error(t, err, "Expected to get time out error, but did not")
 	})
 }
+
+func Test_NamespaceDelete_AppWithResourcesInOutOfOrderTerminatingNamespaces(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:
+           finalizers:
+           - kubernetes
+           name: configmap
+           namespace: %s
+          data:
+           key: value
+  template:
+  - ytt: {}
+  deploy:
+  - kapp:
+      delete:
+        rawOptions: ["--wait-timeout=2s"]`, 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() {
+		// Deleting the namespace directly could delete the service account, so let's delete the App first
+		_, err := kubectl.RunWithOpts([]string{"delete", "app", name, "-n", nsName1, "--timeout=10s"}, e2e.RunOpts{AllowError: true, NoNamespace: true})
+		require.Error(t, err, "Expected to timeout")
+
+		// Deletion should timeout as test-ns-2 is not being terminated and the cm present in it has a finalizer
+		_, err = kubectl.RunWithOpts([]string{"delete", "ns", nsName1, "--timeout=10s"}, e2e.RunOpts{AllowError: true, NoNamespace: true})
+		require.Error(t, err, "Expected to timeout")
+
+		// At this point the service account should be deleted,
+		// so removing the finalizer on cm in test-ns-2 wouldn't help
+		kubectl.RunWithOpts([]string{"patch", "cm", "configmap", "--type=json", "--patch", `[{ "op": "remove", "path": "/metadata/finalizers"}]`,
+			"-n", nsName2}, e2e.RunOpts{NoNamespace: true})
+
+		kubectl.RunWithOpts([]string{"get", "App", name, "-n", nsName1}, e2e.RunOpts{NoNamespace: true})
+
+		// Deleting the second namespace now should do a noop delete for the App
+		kubectl.RunWithOpts([]string{"delete", "ns", nsName2, "--timeout=1m"}, e2e.RunOpts{NoNamespace: true})
+
+		_, err = kubectl.RunWithOpts([]string{"get", "App", name, "-n", nsName1}, e2e.RunOpts{NoNamespace: true, AllowError: true})
+		require.Error(t, err, "Expected not found error")
+	})
+}