Skip to content

Commit

Permalink
Unblock app deletion when namespace is terminating
Browse files Browse the repository at this point in the history
and app resources are in the same namespace only

Signed-off-by: Praveen Rewar <8457124+praveenrewar@users.noreply.github.com>
  • Loading branch information
praveenrewar committed May 25, 2023
1 parent 8e7185d commit cf316da
Show file tree
Hide file tree
Showing 7 changed files with 227 additions and 10 deletions.
10 changes: 10 additions & 0 deletions pkg/app/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/vmware-tanzu/carvel-kapp-controller/pkg/metrics"
"github.com/vmware-tanzu/carvel-kapp-controller/pkg/reftracker"
"github.com/vmware-tanzu/carvel-kapp-controller/pkg/template"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/yaml"
)
Expand All @@ -24,6 +25,7 @@ type ComponentInfo interface {
KappControllerVersion() (semver.Version, error)
KubernetesVersion(serviceAccountName string, specCluster *v1alpha1.AppCluster, objMeta *metav1.ObjectMeta) (semver.Version, error)
KubernetesAPIs() ([]string, error)
NamespaceStatus(name string) (v1.NamespaceStatus, error)
}

type Hooks struct {
Expand Down Expand Up @@ -290,3 +292,11 @@ 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
}
8 changes: 7 additions & 1 deletion pkg/app/app_deploy.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,13 @@ func (a *App) delete(changedFunc func(exec.CmdRunResult)) exec.CmdRunResult {
}

var result exec.CmdRunResult
if !a.app.Spec.NoopDelete {

// 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() && a.app.Status.Deploy != nil && a.app.Status.Deploy.KappDeployStatus != nil &&
len(a.app.Status.Deploy.KappDeployStatus.AssociatedResources.Namespaces) == 1

if !a.app.Spec.NoopDelete && !noopDelete {
for _, dep := range a.app.Spec.Deploy {
switch {
case dep.Kapp != nil:
Expand Down
18 changes: 12 additions & 6 deletions pkg/app/app_reconcile_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"github.com/vmware-tanzu/carvel-kapp-controller/pkg/metrics"
"github.com/vmware-tanzu/carvel-kapp-controller/pkg/template"
corev1 "k8s.io/api/core/v1"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/uuid"
k8sfake "k8s.io/client-go/kubernetes/fake"
Expand Down Expand Up @@ -232,12 +233,13 @@ func Test_TemplateError_DisplayedInStatus_UsefulErrorMessageProperty(t *testing.
}

type FakeComponentInfo struct {
KCVersion semver.Version
KCVersionCount *int
K8sVersion semver.Version
K8sVersionCount *int
K8sAPIs []string
K8sAPIsCount *int
KCVersion semver.Version
KCVersionCount *int
K8sVersion semver.Version
K8sVersionCount *int
K8sAPIs []string
K8sAPIsCount *int
AppNamespaceStatus v1.NamespaceStatus
}

func (f FakeComponentInfo) KubernetesAPIs() ([]string, error) {
Expand All @@ -254,3 +256,7 @@ func (f FakeComponentInfo) KubernetesVersion(_ string, _ *v1alpha1.AppCluster, _
*f.K8sVersionCount++
return f.K8sVersion, nil
}

func (f FakeComponentInfo) NamespaceStatus(_ string) (v1.NamespaceStatus, error) {
return f.AppNamespaceStatus, nil
}
12 changes: 9 additions & 3 deletions pkg/app/app_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/vmware-tanzu/carvel-kapp-controller/pkg/kubeconfig"
"github.com/vmware-tanzu/carvel-kapp-controller/pkg/reftracker"
"github.com/vmware-tanzu/carvel-kapp-controller/pkg/template"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
k8sfake "k8s.io/client-go/kubernetes/fake"
logf "sigs.k8s.io/controller-runtime/pkg/log"
Expand Down Expand Up @@ -156,9 +157,10 @@ func Test_ConfigMapRefs_RetrievesNoConfigMapRefs_WhenNonePresent(t *testing.T) {
}

type FakeComponentInfo struct {
KCVersion semver.Version
K8sVersion semver.Version
K8sAPIs []string
KCVersion semver.Version
K8sVersion semver.Version
K8sAPIs []string
AppNamespaceStatus v1.NamespaceStatus
}

func (f FakeComponentInfo) KubernetesAPIs() ([]string, error) {
Expand All @@ -172,3 +174,7 @@ func (f FakeComponentInfo) KappControllerVersion() (semver.Version, error) {
func (f FakeComponentInfo) KubernetesVersion(_ string, _ *v1alpha1.AppCluster, _ *metav1.ObjectMeta) (semver.Version, error) {
return f.K8sVersion, nil
}

func (f FakeComponentInfo) NamespaceStatus(_ string) (v1.NamespaceStatus, error) {
return f.AppNamespaceStatus, nil
}
11 changes: 11 additions & 0 deletions pkg/componentinfo/component_info.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@
package componentinfo

import (
"context"
"fmt"

"github.com/k14s/semver/v4"
"github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/kappctrl/v1alpha1"
"github.com/vmware-tanzu/carvel-kapp-controller/pkg/kubeconfig"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/tools/clientcmd"
Expand Down Expand Up @@ -83,6 +85,15 @@ func (ci *ComponentInfo) KubernetesAPIs() ([]string, error) {
return metav1.ExtractGroupVersions(groups), nil
}

// NamespaceStatus returns the status of the App namespace
func (ci *ComponentInfo) NamespaceStatus(name string) (v1.NamespaceStatus, error) {
namespace, err := ci.coreClient.CoreV1().Namespaces().Get(context.TODO(), name, metav1.GetOptions{})
if err != nil {
return v1.NamespaceStatus{}, err
}
return namespace.Status, nil
}

// parseAndScrubVersion parses version string and removes Pre and Build from the version
func (*ComponentInfo) parseAndScrubVersion(version string) (semver.Version, error) {
retv, err := semver.ParseTolerant(version)
Expand Down
145 changes: 145 additions & 0 deletions test/e2e/kappcontroller/namespace_deletion_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
// Copyright 2023 VMware, Inc.
// SPDX-License-Identifier: Apache-2.0

package kappcontroller

import (
"fmt"
"strings"
"testing"

"github.com/stretchr/testify/assert"
"github.com/vmware-tanzu/carvel-kapp-controller/test/e2e"
)

func Test_NamespaceDelete_AppWithResourcesInSameNamespace(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"
name := "resources-in-same-namespace"

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: ConfigMap
metadata:
name: configmap
namespace: %s
data:
key: value
template:
- ytt: {}
deploy:
- kapp: {}`, name, nsName) + e2e.ServiceAccounts{nsName}.ForNamespaceYAML()

cleanUp := func() {
kapp.Run([]string{"delete", "-a", name})
kapp.Run([]string{"delete", "-a", nsName})
}

cleanUp()
defer cleanUp()

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() {
kubectl.Run([]string{"delete", "ns", nsName, "--timeout=1m"})
})
}

func Test_NamespaceDelete_AppWithResourcesInDifferentNamespaces(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"
name := "resources-in-different-namespaces"

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: 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, nsName, env.Namespace) + e2e.ServiceAccounts{nsName}.ForClusterYAML()

cleanUp := func() {
kapp.Run([]string{"delete", "-a", name})
}

cleanUpTestNamespace := func() {
kubectl.Run([]string{"delete", "configmap", "configmap"})
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")
})
}
33 changes: 33 additions & 0 deletions test/e2e/service_accounts.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,3 +51,36 @@ roleRef:
name: kappctrl-e2e-ns-role
`, sa.Namespace)
}

// ForClusterYAML can be used to get service account with cluster wide permissions
func (sa ServiceAccounts) ForClusterYAML() string {
return fmt.Sprintf(`
---
apiVersion: v1
kind: ServiceAccount
metadata:
name: kappctrl-e2e-ns-sa
---
kind: ClusterRole
apiVersion: rbac.authorization.k8s.io/v1
metadata:
name: kappctrl-e2e-ns-role
rules:
- apiGroups: ["*"]
resources: ["*"]
verbs: ["*"]
---
kind: ClusterRoleBinding
apiVersion: rbac.authorization.k8s.io/v1
metadata:
name: kappctrl-e2e-ns-role-binding
subjects:
- kind: ServiceAccount
name: kappctrl-e2e-ns-sa
namespace: %s
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: ClusterRole
name: kappctrl-e2e-ns-role
`, sa.Namespace)
}

0 comments on commit cf316da

Please sign in to comment.