Add change rule to delete SA before namespace (carvel-dev#593)

* 579: added deletion change rule to delete SA before namespace

* 579: updated manifest for unit test

Co-authored-by: Kumari Tanushree <ktanushree@ktanushree-a01.vmware.com>

pkg/kapp/config/default.go

@@ -584,6 +584,17 @@ changeRuleBindings:
       - notMatcher:
           matcher: *disableDefaultChangeGroupAnnMatcher
 
+# Delete namespaces after deleting namespaced SAs so that resources like 
+# kapp-controller PackageInstalls can be deleted gracefully.
+- rules:  
+  - "delete before deleting change-groups.kapp.k14s.io/namespaces-{namespace}"
+  ignoreIfCyclical: true
+  resourceMatchers:
+  - andMatcher: 
+      matchers:
+      - notMatcher: {matcher: *disableDefaultChangeGroupAnnMatcher}
+      - anyMatcher: {matchers: *serviceAccountMatchers}
+
 # Insert roles/ClusterRoles before inserting any roleBinding/ClusterRoleBinding
 # Sometimes Binding Creation fail as corresponding Role is not created.
 # https://github.com/vmware-tanzu/carvel-kapp/issues/145

pkg/kapp/diffgraph/change_graph_cf_for_k8s_test.go

@@ -413,7 +413,6 @@ const (
 (delete) role/kpack-watcher-pod-logs-reader (rbac.authorization.k8s.io/v1) namespace: cf-workloads
 (delete) rolebinding/kpack-watcher-pod-logs-binding (rbac.authorization.k8s.io/v1) namespace: cf-workloads
 (delete) configmap/cloud-controller-ng-yaml (v1) namespace: cf-system
-(delete) namespace/cf-workloads-staging (v1) cluster
 (delete) deployment/capi-clock (apps/v1) namespace: cf-system
 (delete) deployment/capi-deployment-updater (apps/v1) namespace: cf-system
 (delete) configmap/nginx (v1) namespace: cf-system
@@ -446,7 +445,6 @@ const (
 (delete) networkpolicy/allow-app-ingress-from-ingressgateway (networking.k8s.io/v1) namespace: cf-workloads
 (delete) secret/app-registry-credentials (v1) namespace: cf-workloads
 (delete) secret/istio-ingressgateway-certs (v1) namespace: istio-system
-(delete) namespace/kpack (v1) cluster
 (delete) deployment/kpack-controller (apps/v1) namespace: kpack
 (delete) serviceaccount/controller (v1) namespace: kpack
 (delete) clusterrole/kpack-controller-admin (rbac.authorization.k8s.io/v1) cluster
@@ -479,7 +477,6 @@ const (
 (delete) secret/metric-proxy-cert (v1) namespace: cf-system
 (delete) secret/metric-proxy-ca (v1) namespace: cf-system
 (delete) deployment/metric-proxy (apps/v1) namespace: cf-system
-(delete) namespace/cf-blobstore (v1) cluster
 (delete) secret/cf-blobstore-minio (v1) namespace: cf-blobstore
 (delete) configmap/cf-blobstore-minio (v1) namespace: cf-blobstore
 (delete) persistentvolumeclaim/cf-blobstore-minio (v1) namespace: cf-blobstore
@@ -492,7 +489,6 @@ const (
 (delete) service/cfroutesync (v1) namespace: cf-system
 (delete) clusterrole/istio-reader-istio-system (rbac.authorization.k8s.io/v1) cluster
 (delete) clusterrolebinding/istio-reader-istio-system (rbac.authorization.k8s.io/v1) cluster
-(delete) namespace/istio-system (v1) cluster
 (delete) serviceaccount/istio-reader-service-account (v1) namespace: istio-system
 (delete) clusterrole/istio-citadel-istio-system (rbac.authorization.k8s.io/v1) cluster
 (delete) clusterrolebinding/istio-citadel-istio-system (rbac.authorization.k8s.io/v1) cluster
@@ -554,7 +550,6 @@ const (
 (delete) networkpolicy/mixer-network-policy (networking.k8s.io/v1) namespace: istio-system
 (delete) networkpolicy/sidecar-injector-network-policy (networking.k8s.io/v1) namespace: istio-system
 (delete) networkpolicy/ingressgateway-network-policy-pilot (networking.k8s.io/v1) namespace: istio-system
-(delete) namespace/metacontroller (v1) cluster
 (delete) serviceaccount/metacontroller (v1) namespace: metacontroller
 (delete) clusterrole/metacontroller (rbac.authorization.k8s.io/v1) cluster
 (delete) clusterrolebinding/metacontroller (rbac.authorization.k8s.io/v1) cluster
@@ -568,12 +563,18 @@ const (
 (delete) service/cf-db-postgresql-headless (v1) namespace: cf-db
 (delete) service/cf-db-postgresql (v1) namespace: cf-db
 (delete) statefulset/cf-db-postgresql (apps/v1) namespace: cf-db
-(delete) namespace/cf-system (v1) cluster
 (delete) configmap/uaa-config (v1) namespace: cf-system
 (delete) deployment/uaa (apps/v1) namespace: cf-system
 (delete) service/uaa (v1) namespace: cf-system
 (delete) serviceaccount/uaa (v1) namespace: cf-system
 (delete) secret/uaa-certs (v1) namespace: cf-system
+---
+(delete) namespace/cf-workloads-staging (v1) cluster
+(delete) namespace/kpack (v1) cluster
+(delete) namespace/cf-blobstore (v1) cluster
+(delete) namespace/istio-system (v1) cluster
+(delete) namespace/metacontroller (v1) cluster
+(delete) namespace/cf-system (v1) cluster
 (delete) namespace/cf-workloads (v1) cluster
 `
 )

pkg/kapp/diffgraph/change_graph_test.go

@@ -593,14 +593,21 @@ roleRef:
 func TestChangeGraphWithAppCR_RoleRoleBindingAndSA(t *testing.T) {
 	yaml := `
 apiVersion: v1
+kind: Namespace
+metadata:
+  name: test
+---
+apiVersion: v1
 kind: ServiceAccount
 metadata:
   name: default-ns-sa
+  namespace: test
 ---
 kind: Role
 apiVersion: rbac.authorization.k8s.io/v1
 metadata:
   name: default-ns-role
+  namespace: test
 rules:
 - apiGroups: ["*"]
   resources: ["*"]
@@ -610,9 +617,11 @@ kind: RoleBinding
 apiVersion: rbac.authorization.k8s.io/v1
 metadata:
   name: default-ns-role-binding
+  namespace: test
 subjects:
 - kind: ServiceAccount
   name: default-ns-sa
+  namespace: test
 roleRef:
   apiGroup: rbac.authorization.k8s.io
   kind: Role
@@ -622,6 +631,7 @@ apiVersion: kappctrl.k14s.io/v1alpha1
 kind: App
 metadata:
   name: simple-app-cr
+  namespace: test
 spec:
   serviceAccountName: default-ns-sa
   fetch:
@@ -635,6 +645,7 @@ apiVersion: packaging.carvel.dev/v1alpha1
 kind: PackageInstall
 metadata:
   name: pkg-demo
+  namespace: test
 spec:
   serviceAccountName: default-ns-sa
   packageRef:
@@ -649,6 +660,7 @@ apiVersion: v1
 kind: Secret
 metadata:
   name: pkg-demo-values
+  namespace: test
 stringData:
   values.yml: |
     ---
@@ -669,21 +681,41 @@ stringData:
 
 	output := strings.TrimSpace(graph.PrintStr())
 	expectedOutput := strings.TrimSpace(`
-(upsert) serviceaccount/default-ns-sa (v1) cluster
-(upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) cluster
-(upsert) rolebinding/default-ns-role-binding (rbac.authorization.k8s.io/v1) cluster
-  (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) cluster
-(upsert) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) cluster
-  (upsert) serviceaccount/default-ns-sa (v1) cluster
-  (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) cluster
-  (upsert) rolebinding/default-ns-role-binding (rbac.authorization.k8s.io/v1) cluster
-    (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) cluster
-(upsert) packageinstall/pkg-demo (packaging.carvel.dev/v1alpha1) cluster
-  (upsert) serviceaccount/default-ns-sa (v1) cluster
-  (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) cluster
-  (upsert) rolebinding/default-ns-role-binding (rbac.authorization.k8s.io/v1) cluster
-    (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) cluster
-(upsert) secret/pkg-demo-values (v1) cluster
+(upsert) namespace/test (v1) cluster
+(upsert) serviceaccount/default-ns-sa (v1) namespace: test
+  (upsert) namespace/test (v1) cluster
+(upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: test
+  (upsert) namespace/test (v1) cluster
+(upsert) rolebinding/default-ns-role-binding (rbac.authorization.k8s.io/v1) namespace: test
+  (upsert) namespace/test (v1) cluster
+  (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: test
+    (upsert) namespace/test (v1) cluster
+(upsert) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) namespace: test
+  (upsert) namespace/test (v1) cluster
+  (upsert) serviceaccount/default-ns-sa (v1) namespace: test
+    (upsert) namespace/test (v1) cluster
+  (upsert) secret/pkg-demo-values (v1) namespace: test
+    (upsert) namespace/test (v1) cluster
+  (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: test
+    (upsert) namespace/test (v1) cluster
+  (upsert) rolebinding/default-ns-role-binding (rbac.authorization.k8s.io/v1) namespace: test
+    (upsert) namespace/test (v1) cluster
+    (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: test
+      (upsert) namespace/test (v1) cluster
+(upsert) packageinstall/pkg-demo (packaging.carvel.dev/v1alpha1) namespace: test
+  (upsert) namespace/test (v1) cluster
+  (upsert) serviceaccount/default-ns-sa (v1) namespace: test
+    (upsert) namespace/test (v1) cluster
+  (upsert) secret/pkg-demo-values (v1) namespace: test
+    (upsert) namespace/test (v1) cluster
+  (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: test
+    (upsert) namespace/test (v1) cluster
+  (upsert) rolebinding/default-ns-role-binding (rbac.authorization.k8s.io/v1) namespace: test
+    (upsert) namespace/test (v1) cluster
+    (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: test
+      (upsert) namespace/test (v1) cluster
+(upsert) secret/pkg-demo-values (v1) namespace: test
+  (upsert) namespace/test (v1) cluster
 `)
 	require.Equal(t, expectedOutput, output)
 
@@ -693,18 +725,86 @@ stringData:
 
 	output = strings.TrimSpace(graph.PrintStr())
 	expectedOutput = strings.TrimSpace(`
-(delete) serviceaccount/default-ns-sa (v1) cluster
-  (delete) packageinstall/pkg-demo (packaging.carvel.dev/v1alpha1) cluster
-  (delete) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) cluster
-(delete) role/default-ns-role (rbac.authorization.k8s.io/v1) cluster
-  (delete) packageinstall/pkg-demo (packaging.carvel.dev/v1alpha1) cluster
-  (delete) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) cluster
-(delete) rolebinding/default-ns-role-binding (rbac.authorization.k8s.io/v1) cluster
-  (delete) packageinstall/pkg-demo (packaging.carvel.dev/v1alpha1) cluster
-  (delete) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) cluster
-(delete) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) cluster
-(delete) packageinstall/pkg-demo (packaging.carvel.dev/v1alpha1) cluster
-(delete) secret/pkg-demo-values (v1) cluster
+(delete) namespace/test (v1) cluster
+  (delete) serviceaccount/default-ns-sa (v1) namespace: test
+    (delete) packageinstall/pkg-demo (packaging.carvel.dev/v1alpha1) namespace: test
+    (delete) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) namespace: test
+(delete) serviceaccount/default-ns-sa (v1) namespace: test
+  (delete) packageinstall/pkg-demo (packaging.carvel.dev/v1alpha1) namespace: test
+  (delete) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) namespace: test
+(delete) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: test
+  (delete) packageinstall/pkg-demo (packaging.carvel.dev/v1alpha1) namespace: test
+  (delete) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) namespace: test
+(delete) rolebinding/default-ns-role-binding (rbac.authorization.k8s.io/v1) namespace: test
+  (delete) packageinstall/pkg-demo (packaging.carvel.dev/v1alpha1) namespace: test
+  (delete) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) namespace: test
+(delete) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) namespace: test
+(delete) packageinstall/pkg-demo (packaging.carvel.dev/v1alpha1) namespace: test
+(delete) secret/pkg-demo-values (v1) namespace: test
+`)
+	require.Equal(t, expectedOutput, output)
+}
+
+func TestChangeGraphWithNamespaceAndSA(t *testing.T) {
+	yaml := `
+apiVersion: v1
+kind: Namespace
+metadata:
+  name: test1
+---
+apiVersion: v1
+kind: ServiceAccount
+metadata:
+  name: default-sa1
+  namespace: test1
+---
+apiVersion: v1
+kind: Namespace
+metadata:
+  name: test2
+---
+apiVersion: v1
+kind: ServiceAccount
+metadata:
+  name: default-sa2
+  namespace: test2
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: simple-cm
+  namespace: test2
+data:
+  hello_msg: carvel
+---
+apiVersion: v1
+kind: ServiceAccount
+metadata:
+  name: default-sa3
+  namespace: default
+`
+	_, conf, err := ctlconf.NewConfFromResourcesWithDefaults(nil)
+	require.NoErrorf(t, err, "Parsing conf defaults")
+
+	opts := buildGraphOpts{
+		resourcesBs:         yaml,
+		op:                  ctldgraph.ActualChangeOpDelete,
+		changeGroupBindings: conf.ChangeGroupBindings(),
+		changeRuleBindings:  conf.ChangeRuleBindings(),
+	}
+	graph, err := buildChangeGraphWithOpts(opts, t)
+	require.NoErrorf(t, err, "Expected graph to build")
+
+	output := strings.TrimSpace(graph.PrintStr())
+	expectedOutput := strings.TrimSpace(`
+(delete) namespace/test1 (v1) cluster
+  (delete) serviceaccount/default-sa1 (v1) namespace: test1
+(delete) serviceaccount/default-sa1 (v1) namespace: test1
+(delete) namespace/test2 (v1) cluster
+  (delete) serviceaccount/default-sa2 (v1) namespace: test2
+(delete) serviceaccount/default-sa2 (v1) namespace: test2
+(delete) configmap/simple-cm (v1) namespace: test2
+(delete) serviceaccount/default-sa3 (v1) namespace: default
 `)
 	require.Equal(t, expectedOutput, output)
 }