From f500283163dad4fe80e9da8518e3842ad204390a Mon Sep 17 00:00:00 2001 From: Kumari Tanushree Date: Fri, 8 Jul 2022 15:13:39 +0530 Subject: [PATCH 01/12] 476: added default rule to order deletion of packageInstall/app before deleting svc account and RBAC --- pkg/kapp/config/default.go | 16 ++++ test/e2e/appcr_svcaccnt_delete_order_test.go | 99 ++++++++++++++++++++ 2 files changed, 115 insertions(+) create mode 100644 test/e2e/appcr_svcaccnt_delete_order_test.go diff --git a/pkg/kapp/config/default.go b/pkg/kapp/config/default.go index ddbd4f01d..d9b063656 100644 --- a/pkg/kapp/config/default.go +++ b/pkg/kapp/config/default.go @@ -515,6 +515,10 @@ changeGroupBindings: # delay other resources with load balancer provisioning # - apiVersionKindMatcher: {kind: Service, apiVersion: v1} +- name: change-groups.kapp.k14s.io/serviceaccount + resourceMatchers: &svcAcctRelatedMatchers + - apiVersionKindMatcher : {kind: ServiceAccount, apiVersion: v1} + changeRuleBindings: # Insert CRDs before all CRs - rules: @@ -582,6 +586,18 @@ changeRuleBindings: - notMatcher: matcher: *disableDefaultChangeGroupAnnMatcher +# delete packageInstall/app (kctrl app CR) before service account +- rules: + - "delete before deleting change-groups.kapp.k14s.io/serviceaccount" + resourceMatchers: + - apiVersionKindMatcher: {kind: App, apiVersion: kappctrl.k14s.io/v1alpha1} + +# delete packageInstall/app (kctrl app CR) before RBAC +- rules: + - "delete before deleting change-groups.kapp.k14s.io/rbac" + resourceMatchers: + - apiVersionKindMatcher: {kind: App, apiVersion: kappctrl.k14s.io/v1alpha1} + - rules: - "upsert after upserting change-groups.kapp.k14s.io/storage-class" ignoreIfCyclical: true diff --git a/test/e2e/appcr_svcaccnt_delete_order_test.go b/test/e2e/appcr_svcaccnt_delete_order_test.go new file mode 100644 index 000000000..779a05028 --- /dev/null +++ b/test/e2e/appcr_svcaccnt_delete_order_test.go @@ -0,0 +1,99 @@ +// Copyright 2020 VMware, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package e2e + +import ( + "strings" + "testing" +) + +func TestAppCRSvcAccntDeleteOrder(t *testing.T) { + env := BuildEnv(t) + logger := Logger{} + kapp := Kapp{t, env.Namespace, env.KappBinaryPath, logger} + kubectl := Kubectl{t, env.Namespace, logger} + + yaml := ` +apiVersion: v1 +kind: ServiceAccount +metadata: + name: default-ns-sa + namespace: kapp-test +--- +kind: Role +apiVersion: rbac.authorization.k8s.io/v1 +metadata: + name: default-ns-role + namespace: kapp-test +rules: +- apiGroups: ["*"] + resources: ["*"] + verbs: ["*"] +--- +kind: RoleBinding +apiVersion: rbac.authorization.k8s.io/v1 +metadata: + name: default-ns-role-binding + namespace: kapp-test +subjects: +- kind: ServiceAccount + name: default-ns-sa + namespace: kapp-test +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: default-ns-role +--- +apiVersion: kappctrl.k14s.io/v1alpha1 +kind: App +metadata: + name: simple-app-cr + namespace: kapp-test +spec: + serviceAccountName: default-ns-sa + fetch: + - git: + url: https://github.com/k14s/k8s-simple-app-example + ref: origin/develop + subPath: config-step-2-template + template: + - ytt: {} + deploy: + - kapp: {} +` + name := "test-appcr-svcaccnt-delete-order" + kcappname := "kc" + cleanUp := func() { + kapp.Run([]string{"delete", "-a", name}) + kapp.Run([]string{"delete", "-a", kcappname}) + } + + cleanUp() + defer cleanUp() + + logger.Section("install kapp-controller on cluster", func() { + kapp.RunWithOpts([]string{"deploy", "-f", "https://github.com/vmware-tanzu/carvel-kapp-controller/releases/latest/download/release.yml", "-a", kcappname}, + RunOpts{}) + // TODO: how to test presence of kc + }) + + logger.Section("deploy initial", func() { + kapp.RunWithOpts([]string{"deploy", "-f", "-", "-a", name}, + RunOpts{StdinReader: strings.NewReader(yaml)}) + + NewPresentClusterResource("app", "simple-app-cr", env.Namespace, kubectl) + NewPresentClusterResource("role", "default-ns-role", env.Namespace, kubectl) + NewPresentClusterResource("rolebinding", "default-ns-role-binding", env.Namespace, kubectl) + NewPresentClusterResource("serviceaccount", "default-ns-sa", env.Namespace, kubectl) + }) + + logger.Section("delete app", func() { + kapp.RunWithOpts([]string{"delete", "-a", name}, RunOpts{}) + + NewMissingClusterResource(t, "app", "simple-app-cr", env.Namespace, kubectl) + NewMissingClusterResource(t, "role", "default-ns-role", env.Namespace, kubectl) + NewMissingClusterResource(t, "rolebinding", "default-ns-role-binding", env.Namespace, kubectl) + NewMissingClusterResource(t, "serviceaccount", "default-ns-sa", env.Namespace, kubectl) + }) +} From 7a541edfbf450df7e5c870bb309cc2318178d2b1 Mon Sep 17 00:00:00 2001 From: Kumari Tanushree Date: Tue, 12 Jul 2022 13:52:43 +0530 Subject: [PATCH 02/12] 476: added unit test for delete order of app cr, svc account and RBAC --- .../diffgraph/assets/appcr-rbac-svcaccnt.yaml | 46 +++++++++ pkg/kapp/diffgraph/change_graph_test.go | 38 +++++++ test/e2e/appcr_svcaccnt_delete_order_test.go | 99 ------------------- 3 files changed, 84 insertions(+), 99 deletions(-) create mode 100644 pkg/kapp/diffgraph/assets/appcr-rbac-svcaccnt.yaml delete mode 100644 test/e2e/appcr_svcaccnt_delete_order_test.go diff --git a/pkg/kapp/diffgraph/assets/appcr-rbac-svcaccnt.yaml b/pkg/kapp/diffgraph/assets/appcr-rbac-svcaccnt.yaml new file mode 100644 index 000000000..e7f86284c --- /dev/null +++ b/pkg/kapp/diffgraph/assets/appcr-rbac-svcaccnt.yaml @@ -0,0 +1,46 @@ + + + +apiVersion: v1 +kind: ServiceAccount +metadata: + name: default-ns-sa + namespace: default +--- +kind: Role +apiVersion: rbac.authorization.k8s.io/v1 +metadata: + name: default-ns-role + namespace: default +rules: +- apiGroups: ["*"] + resources: ["*"] + verbs: ["*"] +--- +kind: RoleBinding +apiVersion: rbac.authorization.k8s.io/v1 +metadata: + name: default-ns-role-binding + namespace: default +subjects: +- kind: ServiceAccount + name: default-ns-sa + namespace: default +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: default-ns-role +--- +apiVersion: kappctrl.k14s.io/v1alpha1 +kind: App +metadata: + name: simple-app-cr + namespace: default +spec: + serviceAccountName: default-ns-sa + fetch: + - git: {} + template: + - ytt: {} + deploy: + - kapp: {} \ No newline at end of file diff --git a/pkg/kapp/diffgraph/change_graph_test.go b/pkg/kapp/diffgraph/change_graph_test.go index 75db4dbce..683f24974 100644 --- a/pkg/kapp/diffgraph/change_graph_test.go +++ b/pkg/kapp/diffgraph/change_graph_test.go @@ -4,8 +4,10 @@ package diffgraph_test import ( + "io/ioutil" "strings" "testing" + "time" ctlconf "github.com/k14s/kapp/pkg/kapp/config" ctldgraph "github.com/k14s/kapp/pkg/kapp/diffgraph" @@ -590,6 +592,42 @@ roleRef: require.Equal(t, expectedOutput, output) } +func TestAppCRSvcAccntRBACDelete(t *testing.T) { + configYAML, err := ioutil.ReadFile("assets/appcr-rbac-svcaccnt.yaml") + require.NoErrorf(t, err, "Reading appcr-rbac-svcaccnt asset") + + configRs, err := ctlres.NewFileResource(ctlres.NewBytesSource([]byte(configYAML))).Resources() + require.NoErrorf(t, err, "Parsing resources") + + rs, conf, err := ctlconf.NewConfFromResourcesWithDefaults(configRs) + require.NoErrorf(t, err, "Parsing conf defaults") + + opts := buildGraphOpts{ + resources: rs, + op: ctldgraph.ActualChangeOpDelete, + changeGroupBindings: conf.ChangeGroupBindings(), + changeRuleBindings: conf.ChangeRuleBindings(), + } + + t1 := time.Now() + + graph, err := buildChangeGraphWithOpts(opts, t) + require.NoErrorf(t, err, "Expected graph to build") + + require.Less(t, time.Now().Sub(t1), time.Duration(1*time.Second), "Graph build took too long") + + output := strings.TrimSpace(graph.PrintLinearizedStr()) + expectedOutput := strings.TrimSpace(` +(delete) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) namespace: default +--- +(delete) serviceaccount/default-ns-sa (v1) namespace: default +(delete) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default +(delete) rolebinding/default-ns-role-binding (rbac.authorization.k8s.io/v1) namespace: default +`) + + require.Equal(t, expectedOutput, output) +} + func buildChangeGraph(resourcesBs string, op ctldgraph.ActualChangeOp, t *testing.T) (*ctldgraph.ChangeGraph, error) { return buildChangeGraphWithOpts(buildGraphOpts{resourcesBs: resourcesBs, op: op}, t) } diff --git a/test/e2e/appcr_svcaccnt_delete_order_test.go b/test/e2e/appcr_svcaccnt_delete_order_test.go deleted file mode 100644 index 779a05028..000000000 --- a/test/e2e/appcr_svcaccnt_delete_order_test.go +++ /dev/null @@ -1,99 +0,0 @@ -// Copyright 2020 VMware, Inc. -// SPDX-License-Identifier: Apache-2.0 - -package e2e - -import ( - "strings" - "testing" -) - -func TestAppCRSvcAccntDeleteOrder(t *testing.T) { - env := BuildEnv(t) - logger := Logger{} - kapp := Kapp{t, env.Namespace, env.KappBinaryPath, logger} - kubectl := Kubectl{t, env.Namespace, logger} - - yaml := ` -apiVersion: v1 -kind: ServiceAccount -metadata: - name: default-ns-sa - namespace: kapp-test ---- -kind: Role -apiVersion: rbac.authorization.k8s.io/v1 -metadata: - name: default-ns-role - namespace: kapp-test -rules: -- apiGroups: ["*"] - resources: ["*"] - verbs: ["*"] ---- -kind: RoleBinding -apiVersion: rbac.authorization.k8s.io/v1 -metadata: - name: default-ns-role-binding - namespace: kapp-test -subjects: -- kind: ServiceAccount - name: default-ns-sa - namespace: kapp-test -roleRef: - apiGroup: rbac.authorization.k8s.io - kind: Role - name: default-ns-role ---- -apiVersion: kappctrl.k14s.io/v1alpha1 -kind: App -metadata: - name: simple-app-cr - namespace: kapp-test -spec: - serviceAccountName: default-ns-sa - fetch: - - git: - url: https://github.com/k14s/k8s-simple-app-example - ref: origin/develop - subPath: config-step-2-template - template: - - ytt: {} - deploy: - - kapp: {} -` - name := "test-appcr-svcaccnt-delete-order" - kcappname := "kc" - cleanUp := func() { - kapp.Run([]string{"delete", "-a", name}) - kapp.Run([]string{"delete", "-a", kcappname}) - } - - cleanUp() - defer cleanUp() - - logger.Section("install kapp-controller on cluster", func() { - kapp.RunWithOpts([]string{"deploy", "-f", "https://github.com/vmware-tanzu/carvel-kapp-controller/releases/latest/download/release.yml", "-a", kcappname}, - RunOpts{}) - // TODO: how to test presence of kc - }) - - logger.Section("deploy initial", func() { - kapp.RunWithOpts([]string{"deploy", "-f", "-", "-a", name}, - RunOpts{StdinReader: strings.NewReader(yaml)}) - - NewPresentClusterResource("app", "simple-app-cr", env.Namespace, kubectl) - NewPresentClusterResource("role", "default-ns-role", env.Namespace, kubectl) - NewPresentClusterResource("rolebinding", "default-ns-role-binding", env.Namespace, kubectl) - NewPresentClusterResource("serviceaccount", "default-ns-sa", env.Namespace, kubectl) - }) - - logger.Section("delete app", func() { - kapp.RunWithOpts([]string{"delete", "-a", name}, RunOpts{}) - - NewMissingClusterResource(t, "app", "simple-app-cr", env.Namespace, kubectl) - NewMissingClusterResource(t, "role", "default-ns-role", env.Namespace, kubectl) - NewMissingClusterResource(t, "rolebinding", "default-ns-role-binding", env.Namespace, kubectl) - NewMissingClusterResource(t, "serviceaccount", "default-ns-sa", env.Namespace, kubectl) - }) -} From 864fa468e676f268de713cab3f160762a315e55b Mon Sep 17 00:00:00 2001 From: Kumari Tanushree Date: Tue, 12 Jul 2022 14:41:58 +0530 Subject: [PATCH 03/12] 476: removed unwanted empty lines --- pkg/kapp/diffgraph/assets/appcr-rbac-svcaccnt.yaml | 6 ++---- pkg/kapp/diffgraph/change_graph_test.go | 1 - 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/pkg/kapp/diffgraph/assets/appcr-rbac-svcaccnt.yaml b/pkg/kapp/diffgraph/assets/appcr-rbac-svcaccnt.yaml index e7f86284c..ab0016499 100644 --- a/pkg/kapp/diffgraph/assets/appcr-rbac-svcaccnt.yaml +++ b/pkg/kapp/diffgraph/assets/appcr-rbac-svcaccnt.yaml @@ -1,6 +1,3 @@ - - - apiVersion: v1 kind: ServiceAccount metadata: @@ -43,4 +40,5 @@ spec: template: - ytt: {} deploy: - - kapp: {} \ No newline at end of file + - kapp: {} + \ No newline at end of file diff --git a/pkg/kapp/diffgraph/change_graph_test.go b/pkg/kapp/diffgraph/change_graph_test.go index 683f24974..64a0c73ad 100644 --- a/pkg/kapp/diffgraph/change_graph_test.go +++ b/pkg/kapp/diffgraph/change_graph_test.go @@ -624,7 +624,6 @@ func TestAppCRSvcAccntRBACDelete(t *testing.T) { (delete) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default (delete) rolebinding/default-ns-role-binding (rbac.authorization.k8s.io/v1) namespace: default `) - require.Equal(t, expectedOutput, output) } From c125f875d80b0172565ef745886054d2ffeb9a3e Mon Sep 17 00:00:00 2001 From: Kumari Tanushree Date: Tue, 12 Jul 2022 15:26:34 +0530 Subject: [PATCH 04/12] 476: added test for upsert and const for storing yaml --- .../diffgraph/assets/appcr-rbac-svcaccnt.yaml | 44 ---------- pkg/kapp/diffgraph/change_graph_test.go | 84 +++++++++++++++++-- 2 files changed, 76 insertions(+), 52 deletions(-) delete mode 100644 pkg/kapp/diffgraph/assets/appcr-rbac-svcaccnt.yaml diff --git a/pkg/kapp/diffgraph/assets/appcr-rbac-svcaccnt.yaml b/pkg/kapp/diffgraph/assets/appcr-rbac-svcaccnt.yaml deleted file mode 100644 index ab0016499..000000000 --- a/pkg/kapp/diffgraph/assets/appcr-rbac-svcaccnt.yaml +++ /dev/null @@ -1,44 +0,0 @@ -apiVersion: v1 -kind: ServiceAccount -metadata: - name: default-ns-sa - namespace: default ---- -kind: Role -apiVersion: rbac.authorization.k8s.io/v1 -metadata: - name: default-ns-role - namespace: default -rules: -- apiGroups: ["*"] - resources: ["*"] - verbs: ["*"] ---- -kind: RoleBinding -apiVersion: rbac.authorization.k8s.io/v1 -metadata: - name: default-ns-role-binding - namespace: default -subjects: -- kind: ServiceAccount - name: default-ns-sa - namespace: default -roleRef: - apiGroup: rbac.authorization.k8s.io - kind: Role - name: default-ns-role ---- -apiVersion: kappctrl.k14s.io/v1alpha1 -kind: App -metadata: - name: simple-app-cr - namespace: default -spec: - serviceAccountName: default-ns-sa - fetch: - - git: {} - template: - - ytt: {} - deploy: - - kapp: {} - \ No newline at end of file diff --git a/pkg/kapp/diffgraph/change_graph_test.go b/pkg/kapp/diffgraph/change_graph_test.go index 64a0c73ad..8b867de52 100644 --- a/pkg/kapp/diffgraph/change_graph_test.go +++ b/pkg/kapp/diffgraph/change_graph_test.go @@ -4,7 +4,6 @@ package diffgraph_test import ( - "io/ioutil" "strings" "testing" "time" @@ -592,18 +591,87 @@ roleRef: require.Equal(t, expectedOutput, output) } -func TestAppCRSvcAccntRBACDelete(t *testing.T) { - configYAML, err := ioutil.ReadFile("assets/appcr-rbac-svcaccnt.yaml") - require.NoErrorf(t, err, "Reading appcr-rbac-svcaccnt asset") +const appCRSvcAccntRBACYaml = ` +apiVersion: v1 +kind: ServiceAccount +metadata: + name: default-ns-sa + namespace: default +--- +kind: Role +apiVersion: rbac.authorization.k8s.io/v1 +metadata: + name: default-ns-role + namespace: default +rules: +- apiGroups: ["*"] + resources: ["*"] + verbs: ["*"] +--- +kind: RoleBinding +apiVersion: rbac.authorization.k8s.io/v1 +metadata: + name: default-ns-role-binding + namespace: default +subjects: +- kind: ServiceAccount + name: default-ns-sa + namespace: default +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: default-ns-role +--- +apiVersion: kappctrl.k14s.io/v1alpha1 +kind: App +metadata: + name: simple-app-cr + namespace: default +spec: + serviceAccountName: default-ns-sa + fetch: + - git: {} + template: + - ytt: {} + deploy: + - kapp: {} + ` + +func Test_AppCRSvcAccntRBACUpsert(t *testing.T) { + _, conf, err := ctlconf.NewConfFromResourcesWithDefaults(nil) + require.NoErrorf(t, err, "Parsing conf defaults") - configRs, err := ctlres.NewFileResource(ctlres.NewBytesSource([]byte(configYAML))).Resources() - require.NoErrorf(t, err, "Parsing resources") + opts := buildGraphOpts{ + resourcesBs: appCRSvcAccntRBACYaml, + op: ctldgraph.ActualChangeOpUpsert, + changeGroupBindings: conf.ChangeGroupBindings(), + changeRuleBindings: conf.ChangeRuleBindings(), + } + + t1 := time.Now() + + graph, err := buildChangeGraphWithOpts(opts, t) + require.NoErrorf(t, err, "Expected graph to build") - rs, conf, err := ctlconf.NewConfFromResourcesWithDefaults(configRs) + require.Less(t, time.Now().Sub(t1), time.Duration(1*time.Second), "Graph build took too long") + + output := strings.TrimSpace(graph.PrintLinearizedStr()) + expectedOutput := strings.TrimSpace(` +(upsert) serviceaccount/default-ns-sa (v1) namespace: default +(upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default +--- +(upsert) rolebinding/default-ns-role-binding (rbac.authorization.k8s.io/v1) namespace: default +--- +(upsert) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) namespace: default +`) + require.Equal(t, expectedOutput, output) +} +func Test_AppCRSvcAccntRBACDelete(t *testing.T) { + _, conf, err := ctlconf.NewConfFromResourcesWithDefaults(nil) require.NoErrorf(t, err, "Parsing conf defaults") opts := buildGraphOpts{ - resources: rs, + resourcesBs: appCRSvcAccntRBACYaml, op: ctldgraph.ActualChangeOpDelete, changeGroupBindings: conf.ChangeGroupBindings(), changeRuleBindings: conf.ChangeRuleBindings(), From 6b3946546166080cbc612434cd8f50a7ac8ed491 Mon Sep 17 00:00:00 2001 From: Kumari Tanushree Date: Thu, 14 Jul 2022 13:33:15 +0530 Subject: [PATCH 05/12] 476: adopted comments --- pkg/kapp/config/default.go | 4 +-- pkg/kapp/diffgraph/change_graph_test.go | 47 +++++++++---------------- 2 files changed, 18 insertions(+), 33 deletions(-) diff --git a/pkg/kapp/config/default.go b/pkg/kapp/config/default.go index d9b063656..89f1f0824 100644 --- a/pkg/kapp/config/default.go +++ b/pkg/kapp/config/default.go @@ -586,13 +586,13 @@ changeRuleBindings: - notMatcher: matcher: *disableDefaultChangeGroupAnnMatcher -# delete packageInstall/app (kctrl app CR) before service account +# delete packageInstall/app (kapp-controller App CR) before service account - rules: - "delete before deleting change-groups.kapp.k14s.io/serviceaccount" resourceMatchers: - apiVersionKindMatcher: {kind: App, apiVersion: kappctrl.k14s.io/v1alpha1} -# delete packageInstall/app (kctrl app CR) before RBAC +# delete packageInstall/app (kapp-controller App CR) before RBAC - rules: - "delete before deleting change-groups.kapp.k14s.io/rbac" resourceMatchers: diff --git a/pkg/kapp/diffgraph/change_graph_test.go b/pkg/kapp/diffgraph/change_graph_test.go index 8b867de52..c286404dd 100644 --- a/pkg/kapp/diffgraph/change_graph_test.go +++ b/pkg/kapp/diffgraph/change_graph_test.go @@ -6,7 +6,6 @@ package diffgraph_test import ( "strings" "testing" - "time" ctlconf "github.com/k14s/kapp/pkg/kapp/config" ctldgraph "github.com/k14s/kapp/pkg/kapp/diffgraph" @@ -591,7 +590,8 @@ roleRef: require.Equal(t, expectedOutput, output) } -const appCRSvcAccntRBACYaml = ` +func TestChangeGraphWithAppCR_SA_RoleAndRoleBinding(t *testing.T) { + appCRSvcAccntRBACYaml := ` apiVersion: v1 kind: ServiceAccount metadata: @@ -636,8 +636,6 @@ spec: deploy: - kapp: {} ` - -func Test_AppCRSvcAccntRBACUpsert(t *testing.T) { _, conf, err := ctlconf.NewConfFromResourcesWithDefaults(nil) require.NoErrorf(t, err, "Parsing conf defaults") @@ -648,49 +646,36 @@ func Test_AppCRSvcAccntRBACUpsert(t *testing.T) { changeRuleBindings: conf.ChangeRuleBindings(), } - t1 := time.Now() - graph, err := buildChangeGraphWithOpts(opts, t) require.NoErrorf(t, err, "Expected graph to build") - require.Less(t, time.Now().Sub(t1), time.Duration(1*time.Second), "Graph build took too long") - - output := strings.TrimSpace(graph.PrintLinearizedStr()) + output := strings.TrimSpace(graph.PrintStr()) expectedOutput := strings.TrimSpace(` (upsert) serviceaccount/default-ns-sa (v1) namespace: default (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default ---- (upsert) rolebinding/default-ns-role-binding (rbac.authorization.k8s.io/v1) namespace: default ---- + (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default (upsert) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) namespace: default + (upsert) serviceaccount/default-ns-sa (v1) namespace: default + (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default + (upsert) rolebinding/default-ns-role-binding (rbac.authorization.k8s.io/v1) namespace: default + (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default `) require.Equal(t, expectedOutput, output) -} -func Test_AppCRSvcAccntRBACDelete(t *testing.T) { - _, conf, err := ctlconf.NewConfFromResourcesWithDefaults(nil) - require.NoErrorf(t, err, "Parsing conf defaults") - - opts := buildGraphOpts{ - resourcesBs: appCRSvcAccntRBACYaml, - op: ctldgraph.ActualChangeOpDelete, - changeGroupBindings: conf.ChangeGroupBindings(), - changeRuleBindings: conf.ChangeRuleBindings(), - } - - t1 := time.Now() - graph, err := buildChangeGraphWithOpts(opts, t) + opts.op = ctldgraph.ActualChangeOpDelete + graph, err = buildChangeGraphWithOpts(opts, t) require.NoErrorf(t, err, "Expected graph to build") - require.Less(t, time.Now().Sub(t1), time.Duration(1*time.Second), "Graph build took too long") - - output := strings.TrimSpace(graph.PrintLinearizedStr()) - expectedOutput := strings.TrimSpace(` -(delete) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) namespace: default ---- + output = strings.TrimSpace(graph.PrintStr()) + expectedOutput = strings.TrimSpace(` (delete) serviceaccount/default-ns-sa (v1) namespace: default + (delete) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) namespace: default (delete) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default + (delete) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) namespace: default (delete) rolebinding/default-ns-role-binding (rbac.authorization.k8s.io/v1) namespace: default + (delete) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) namespace: default +(delete) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) namespace: default `) require.Equal(t, expectedOutput, output) } From 5d4fc8ebc6c8b36e66dbeafdf2e7f49a73ff066f Mon Sep 17 00:00:00 2001 From: Kumari Tanushree Date: Fri, 15 Jul 2022 15:37:18 +0530 Subject: [PATCH 06/12] 476: updated func name and removed unwanted commets --- pkg/kapp/config/default.go | 4 +--- pkg/kapp/diffgraph/change_graph_test.go | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/pkg/kapp/config/default.go b/pkg/kapp/config/default.go index 89f1f0824..688720d3f 100644 --- a/pkg/kapp/config/default.go +++ b/pkg/kapp/config/default.go @@ -516,7 +516,7 @@ changeGroupBindings: # - apiVersionKindMatcher: {kind: Service, apiVersion: v1} - name: change-groups.kapp.k14s.io/serviceaccount - resourceMatchers: &svcAcctRelatedMatchers + resourceMatchers: &serviceAccountRelatedMatchers - apiVersionKindMatcher : {kind: ServiceAccount, apiVersion: v1} changeRuleBindings: @@ -586,13 +586,11 @@ changeRuleBindings: - notMatcher: matcher: *disableDefaultChangeGroupAnnMatcher -# delete packageInstall/app (kapp-controller App CR) before service account - rules: - "delete before deleting change-groups.kapp.k14s.io/serviceaccount" resourceMatchers: - apiVersionKindMatcher: {kind: App, apiVersion: kappctrl.k14s.io/v1alpha1} -# delete packageInstall/app (kapp-controller App CR) before RBAC - rules: - "delete before deleting change-groups.kapp.k14s.io/rbac" resourceMatchers: diff --git a/pkg/kapp/diffgraph/change_graph_test.go b/pkg/kapp/diffgraph/change_graph_test.go index c286404dd..a4ef8281a 100644 --- a/pkg/kapp/diffgraph/change_graph_test.go +++ b/pkg/kapp/diffgraph/change_graph_test.go @@ -590,7 +590,7 @@ roleRef: require.Equal(t, expectedOutput, output) } -func TestChangeGraphWithAppCR_SA_RoleAndRoleBinding(t *testing.T) { +func TestChangeGraphWithAppCR_RoleRoleBindingAndSA(t *testing.T) { appCRSvcAccntRBACYaml := ` apiVersion: v1 kind: ServiceAccount From de288ce209a76bc36eee066f577995e1ae96a47e Mon Sep 17 00:00:00 2001 From: Kumari Tanushree Date: Fri, 15 Jul 2022 18:40:44 +0530 Subject: [PATCH 07/12] 476: updated change-group name --- pkg/kapp/config/default.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/kapp/config/default.go b/pkg/kapp/config/default.go index 688720d3f..b1bf2cf10 100644 --- a/pkg/kapp/config/default.go +++ b/pkg/kapp/config/default.go @@ -516,7 +516,7 @@ changeGroupBindings: # - apiVersionKindMatcher: {kind: Service, apiVersion: v1} - name: change-groups.kapp.k14s.io/serviceaccount - resourceMatchers: &serviceAccountRelatedMatchers + resourceMatchers: &serviceAccountMatchers - apiVersionKindMatcher : {kind: ServiceAccount, apiVersion: v1} changeRuleBindings: From 4a1538e9edd1e7d62baa1c9e01e5776649d5a4d4 Mon Sep 17 00:00:00 2001 From: Kumari Tanushree Date: Fri, 15 Jul 2022 21:33:51 +0530 Subject: [PATCH 08/12] 476: added test for packageInstall --- pkg/kapp/diffgraph/change_graph_test.go | 34 ++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/pkg/kapp/diffgraph/change_graph_test.go b/pkg/kapp/diffgraph/change_graph_test.go index a4ef8281a..0ab2d3dc6 100644 --- a/pkg/kapp/diffgraph/change_graph_test.go +++ b/pkg/kapp/diffgraph/change_graph_test.go @@ -635,7 +635,30 @@ spec: - ytt: {} deploy: - kapp: {} - ` +--- +apiVersion: packaging.carvel.dev/v1alpha1 +kind: PackageInstall +metadata: + name: pkg-demo +spec: + serviceAccountName: default-ns-sa + packageRef: + refName: simple-app.corp.com + versionSelection: + constraints: 1.0.0 + values: + - secretRef: + name: pkg-demo-values +--- +apiVersion: v1 +kind: Secret +metadata: + name: pkg-demo-values +stringData: + values.yml: | + --- + hello_msg: "to all my internet friends" +` _, conf, err := ctlconf.NewConfFromResourcesWithDefaults(nil) require.NoErrorf(t, err, "Parsing conf defaults") @@ -651,15 +674,18 @@ spec: output := strings.TrimSpace(graph.PrintStr()) expectedOutput := strings.TrimSpace(` -(upsert) serviceaccount/default-ns-sa (v1) namespace: default + (upsert) serviceaccount/default-ns-sa (v1) namespace: default (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default (upsert) rolebinding/default-ns-role-binding (rbac.authorization.k8s.io/v1) namespace: default (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default (upsert) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) namespace: default (upsert) serviceaccount/default-ns-sa (v1) namespace: default + (upsert) secret/pkg-demo-values (v1) cluster (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default (upsert) rolebinding/default-ns-role-binding (rbac.authorization.k8s.io/v1) namespace: default (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default +(upsert) packageinstall/pkg-demo (packaging.carvel.dev/v1alpha1) cluster +(upsert) secret/pkg-demo-values (v1) cluster `) require.Equal(t, expectedOutput, output) @@ -669,13 +695,15 @@ spec: output = strings.TrimSpace(graph.PrintStr()) expectedOutput = strings.TrimSpace(` -(delete) serviceaccount/default-ns-sa (v1) namespace: default + (delete) serviceaccount/default-ns-sa (v1) namespace: default (delete) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) namespace: default (delete) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default (delete) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) namespace: default (delete) rolebinding/default-ns-role-binding (rbac.authorization.k8s.io/v1) namespace: default (delete) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) namespace: default (delete) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) namespace: default +(delete) packageinstall/pkg-demo (packaging.carvel.dev/v1alpha1) cluster +(delete) secret/pkg-demo-values (v1) cluster `) require.Equal(t, expectedOutput, output) } From 113d6c54e8c5285ba48056dc23f51e49ded039b3 Mon Sep 17 00:00:00 2001 From: Kumari Tanushree Date: Tue, 19 Jul 2022 17:50:24 +0530 Subject: [PATCH 09/12] 476: added change rule for packageinstall as well and merge the multple rules in one --- pkg/kapp/config/default.go | 25 +++++++++++++++++-------- pkg/kapp/diffgraph/change_graph_test.go | 11 +++++++++-- 2 files changed, 26 insertions(+), 10 deletions(-) diff --git a/pkg/kapp/config/default.go b/pkg/kapp/config/default.go index b1bf2cf10..639a8eadc 100644 --- a/pkg/kapp/config/default.go +++ b/pkg/kapp/config/default.go @@ -517,7 +517,16 @@ changeGroupBindings: - name: change-groups.kapp.k14s.io/serviceaccount resourceMatchers: &serviceAccountMatchers - - apiVersionKindMatcher : {kind: ServiceAccount, apiVersion: v1} + - apiVersionKindMatcher: {kind: ServiceAccount, apiVersion: v1} + +- name: change-groups.kapp.k14s.io/appcr + resourceMatchers: + - apiVersionKindMatcher: {kind: App, apiVersion: kappctrl.k14s.io/v1alpha1} + +- name: change-groups.kapp.k14s.io/packageinstall + resourceMatchers: + - apiVersionKindMatcher: {kind: PackageInstall, apiVersion: packaging.carvel.dev/v1alpha1} + changeRuleBindings: # Insert CRDs before all CRs @@ -587,14 +596,14 @@ changeRuleBindings: matcher: *disableDefaultChangeGroupAnnMatcher - rules: - - "delete before deleting change-groups.kapp.k14s.io/serviceaccount" - resourceMatchers: - - apiVersionKindMatcher: {kind: App, apiVersion: kappctrl.k14s.io/v1alpha1} - -- rules: - - "delete before deleting change-groups.kapp.k14s.io/rbac" + - "upsert before upserting change-groups.kapp.k14s.io/packageinstall" + - "upsert before upserting change-groups.kapp.k14s.io/appcr" + - "delete after deleting change-groups.kapp.k14s.io/packageinstall" + - "delete after deleting change-groups.kapp.k14s.io/appcr" + ignoreIfCyclical: true resourceMatchers: - - apiVersionKindMatcher: {kind: App, apiVersion: kappctrl.k14s.io/v1alpha1} + - anyMatcher: {matchers: *serviceAccountMatchers} + - anyMatcher: {matchers: *rbacMatchers} - rules: - "upsert after upserting change-groups.kapp.k14s.io/storage-class" diff --git a/pkg/kapp/diffgraph/change_graph_test.go b/pkg/kapp/diffgraph/change_graph_test.go index 0ab2d3dc6..1697d1154 100644 --- a/pkg/kapp/diffgraph/change_graph_test.go +++ b/pkg/kapp/diffgraph/change_graph_test.go @@ -674,7 +674,7 @@ stringData: output := strings.TrimSpace(graph.PrintStr()) expectedOutput := strings.TrimSpace(` - (upsert) serviceaccount/default-ns-sa (v1) namespace: default +(upsert) serviceaccount/default-ns-sa (v1) namespace: default (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default (upsert) rolebinding/default-ns-role-binding (rbac.authorization.k8s.io/v1) namespace: default (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default @@ -685,6 +685,10 @@ stringData: (upsert) rolebinding/default-ns-role-binding (rbac.authorization.k8s.io/v1) namespace: default (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default (upsert) packageinstall/pkg-demo (packaging.carvel.dev/v1alpha1) cluster + (upsert) serviceaccount/default-ns-sa (v1) namespace: default + (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default + (upsert) rolebinding/default-ns-role-binding (rbac.authorization.k8s.io/v1) namespace: default + (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default (upsert) secret/pkg-demo-values (v1) cluster `) require.Equal(t, expectedOutput, output) @@ -695,11 +699,14 @@ stringData: output = strings.TrimSpace(graph.PrintStr()) expectedOutput = strings.TrimSpace(` - (delete) serviceaccount/default-ns-sa (v1) namespace: default +(delete) serviceaccount/default-ns-sa (v1) namespace: default + (delete) packageinstall/pkg-demo (packaging.carvel.dev/v1alpha1) cluster (delete) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) namespace: default (delete) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default + (delete) packageinstall/pkg-demo (packaging.carvel.dev/v1alpha1) cluster (delete) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) namespace: default (delete) rolebinding/default-ns-role-binding (rbac.authorization.k8s.io/v1) namespace: default + (delete) packageinstall/pkg-demo (packaging.carvel.dev/v1alpha1) cluster (delete) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) namespace: default (delete) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) namespace: default (delete) packageinstall/pkg-demo (packaging.carvel.dev/v1alpha1) cluster From 27123c836ef699eea5cb6f7ed0ce90943bf812e2 Mon Sep 17 00:00:00 2001 From: Kumari Tanushree Date: Tue, 19 Jul 2022 18:00:58 +0530 Subject: [PATCH 10/12] 476: removed extra empty line --- pkg/kapp/config/default.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/kapp/config/default.go b/pkg/kapp/config/default.go index 639a8eadc..9ce166c83 100644 --- a/pkg/kapp/config/default.go +++ b/pkg/kapp/config/default.go @@ -527,7 +527,6 @@ changeGroupBindings: resourceMatchers: - apiVersionKindMatcher: {kind: PackageInstall, apiVersion: packaging.carvel.dev/v1alpha1} - changeRuleBindings: # Insert CRDs before all CRs - rules: From 1af5a778e3ee0b164d3107ec4fc18407b79a32e1 Mon Sep 17 00:00:00 2001 From: Kumari Tanushree Date: Tue, 19 Jul 2022 21:02:28 +0530 Subject: [PATCH 11/12] 476: removed namespace name from yaml and updated variable name --- pkg/kapp/diffgraph/change_graph_test.go | 50 +++++++++++-------------- 1 file changed, 22 insertions(+), 28 deletions(-) diff --git a/pkg/kapp/diffgraph/change_graph_test.go b/pkg/kapp/diffgraph/change_graph_test.go index 1697d1154..8535a96af 100644 --- a/pkg/kapp/diffgraph/change_graph_test.go +++ b/pkg/kapp/diffgraph/change_graph_test.go @@ -591,18 +591,16 @@ roleRef: } func TestChangeGraphWithAppCR_RoleRoleBindingAndSA(t *testing.T) { - appCRSvcAccntRBACYaml := ` + yaml := ` apiVersion: v1 kind: ServiceAccount metadata: name: default-ns-sa - namespace: default --- kind: Role apiVersion: rbac.authorization.k8s.io/v1 metadata: name: default-ns-role - namespace: default rules: - apiGroups: ["*"] resources: ["*"] @@ -612,11 +610,9 @@ kind: RoleBinding apiVersion: rbac.authorization.k8s.io/v1 metadata: name: default-ns-role-binding - namespace: default subjects: - kind: ServiceAccount name: default-ns-sa - namespace: default roleRef: apiGroup: rbac.authorization.k8s.io kind: Role @@ -626,7 +622,6 @@ apiVersion: kappctrl.k14s.io/v1alpha1 kind: App metadata: name: simple-app-cr - namespace: default spec: serviceAccountName: default-ns-sa fetch: @@ -663,7 +658,7 @@ stringData: require.NoErrorf(t, err, "Parsing conf defaults") opts := buildGraphOpts{ - resourcesBs: appCRSvcAccntRBACYaml, + resourcesBs: yaml, op: ctldgraph.ActualChangeOpUpsert, changeGroupBindings: conf.ChangeGroupBindings(), changeRuleBindings: conf.ChangeRuleBindings(), @@ -674,21 +669,20 @@ stringData: output := strings.TrimSpace(graph.PrintStr()) expectedOutput := strings.TrimSpace(` -(upsert) serviceaccount/default-ns-sa (v1) namespace: default -(upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default -(upsert) rolebinding/default-ns-role-binding (rbac.authorization.k8s.io/v1) namespace: default - (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default -(upsert) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) namespace: default - (upsert) serviceaccount/default-ns-sa (v1) namespace: default - (upsert) secret/pkg-demo-values (v1) cluster - (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default - (upsert) rolebinding/default-ns-role-binding (rbac.authorization.k8s.io/v1) namespace: default - (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default +(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) namespace: default - (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default - (upsert) rolebinding/default-ns-role-binding (rbac.authorization.k8s.io/v1) namespace: default - (upsert) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default + (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 `) require.Equal(t, expectedOutput, output) @@ -699,16 +693,16 @@ stringData: output = strings.TrimSpace(graph.PrintStr()) expectedOutput = strings.TrimSpace(` -(delete) serviceaccount/default-ns-sa (v1) namespace: default +(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) namespace: default -(delete) role/default-ns-role (rbac.authorization.k8s.io/v1) namespace: default + (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) namespace: default -(delete) rolebinding/default-ns-role-binding (rbac.authorization.k8s.io/v1) namespace: default + (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) namespace: default -(delete) app/simple-app-cr (kappctrl.k14s.io/v1alpha1) namespace: default + (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 `) From 7492d3ff40eff657fc8f466b2f76240abfacba18 Mon Sep 17 00:00:00 2001 From: Kumari Tanushree Date: Wed, 20 Jul 2022 17:04:50 +0530 Subject: [PATCH 12/12] 476: added disableDefaultChangeGroupAnnMatcher to change-rule --- pkg/kapp/config/default.go | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/pkg/kapp/config/default.go b/pkg/kapp/config/default.go index 9ce166c83..5fdab8b05 100644 --- a/pkg/kapp/config/default.go +++ b/pkg/kapp/config/default.go @@ -519,11 +519,11 @@ changeGroupBindings: resourceMatchers: &serviceAccountMatchers - apiVersionKindMatcher: {kind: ServiceAccount, apiVersion: v1} -- name: change-groups.kapp.k14s.io/appcr +- name: change-groups.kapp.k14s.io/kapp-controller-app resourceMatchers: - apiVersionKindMatcher: {kind: App, apiVersion: kappctrl.k14s.io/v1alpha1} -- name: change-groups.kapp.k14s.io/packageinstall +- name: change-groups.kapp.k14s.io/kapp-controller-packageinstall resourceMatchers: - apiVersionKindMatcher: {kind: PackageInstall, apiVersion: packaging.carvel.dev/v1alpha1} @@ -595,14 +595,19 @@ changeRuleBindings: matcher: *disableDefaultChangeGroupAnnMatcher - rules: - - "upsert before upserting change-groups.kapp.k14s.io/packageinstall" - - "upsert before upserting change-groups.kapp.k14s.io/appcr" - - "delete after deleting change-groups.kapp.k14s.io/packageinstall" - - "delete after deleting change-groups.kapp.k14s.io/appcr" + - "upsert before upserting change-groups.kapp.k14s.io/kapp-controller-packageinstall" + - "upsert before upserting change-groups.kapp.k14s.io/kapp-controller-app" + - "delete after deleting change-groups.kapp.k14s.io/kapp-controller-packageinstall" + - "delete after deleting change-groups.kapp.k14s.io/kapp-controller-app" ignoreIfCyclical: true resourceMatchers: - - anyMatcher: {matchers: *serviceAccountMatchers} - - anyMatcher: {matchers: *rbacMatchers} + - andMatcher: + matchers: + - notMatcher: {matcher: *disableDefaultChangeGroupAnnMatcher} + - anyMatcher: + matchers: + - anyMatcher: {matchers: *serviceAccountMatchers} + - anyMatcher: {matchers: *rbacMatchers} - rules: - "upsert after upserting change-groups.kapp.k14s.io/storage-class"