From 3c02c65750bb0f8efa70c387322345b9045076a1 Mon Sep 17 00:00:00 2001 From: Praveen Rewar <8457124+praveenrewar@users.noreply.github.com> Date: Tue, 23 Jan 2024 15:38:32 +0530 Subject: [PATCH] Do not overwrite kapp deploy status during delete Signed-off-by: Praveen Rewar <8457124+praveenrewar@users.noreply.github.com> --- pkg/app/app_reconcile.go | 20 ++-- .../kappcontroller/namespace_deletion_test.go | 96 +++++++++++++++++++ 2 files changed, 109 insertions(+), 7 deletions(-) diff --git a/pkg/app/app_reconcile.go b/pkg/app/app_reconcile.go index fc36ab365..e96535235 100644 --- a/pkg/app/app_reconcile.go +++ b/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{ diff --git a/test/e2e/kappcontroller/namespace_deletion_test.go b/test/e2e/kappcontroller/namespace_deletion_test.go index b7e9e82a3..c75810326 100644 --- a/test/e2e/kappcontroller/namespace_deletion_test.go +++ b/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") + }) +}