From 407b20780a25307a13dd08abfbf2ab18312dcfb6 Mon Sep 17 00:00:00 2001 From: Anshul Sirur Date: Mon, 31 Jul 2023 16:06:07 +0200 Subject: [PATCH] Skip API operations for diff run Diff run should be usable when the authorised RBAC role doesn't have write permissions for resources in the cluster. Diff run on app creation used the dry run feature of the Kubernetes API, to avoid persisting kapp ConfigMap changes. However this still checks RBAC permissions, and errored if the role did not have write permissions for ConfigMaps. The kapp ConfigMap was also being updated when doing a diff run for an existing app. This skips the API operations for updating the ConfigMap when doing a diff run. Fixes #791. Signed-off-by: Anshul Sirur --- pkg/kapp/app/recorded_app.go | 18 +++++-- test/e2e/diff_test.go | 97 ++++++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 3 deletions(-) diff --git a/pkg/kapp/app/recorded_app.go b/pkg/kapp/app/recorded_app.go index d5a54aa86..6f16f6f2f 100644 --- a/pkg/kapp/app/recorded_app.go +++ b/pkg/kapp/app/recorded_app.go @@ -138,6 +138,9 @@ func (a *RecordedApp) CreateOrUpdate(prevAppName string, labels map[string]strin } if foundMigratedApp { a.isMigrated = true + if isDiffRun { + return false, nil + } return false, a.updateApp(app, labels) } @@ -146,6 +149,9 @@ func (a *RecordedApp) CreateOrUpdate(prevAppName string, labels map[string]strin return false, err } if foundNonMigratedApp { + if isDiffRun { + return false, nil + } if a.isMigrationEnabled() { return false, a.migrate(app, labels, a.fqName()) } @@ -162,6 +168,9 @@ func (a *RecordedApp) CreateOrUpdate(prevAppName string, labels map[string]strin } if foundMigratedPrevApp { a.isMigrated = true + if isDiffRun { + return false, nil + } return false, a.renameConfigMap(app, a.fqName(), a.nsName) } @@ -170,6 +179,9 @@ func (a *RecordedApp) CreateOrUpdate(prevAppName string, labels map[string]strin return false, err } if foundNonMigratedPrevApp { + if isDiffRun { + return false, nil + } if a.isMigrationEnabled() { return false, a.migrate(app, labels, a.fqName()) } @@ -225,12 +237,12 @@ func (a *RecordedApp) create(labels map[string]string, isDiffRun bool) error { return err } - createOpts := metav1.CreateOptions{} if isDiffRun { - createOpts.DryRun = []string{metav1.DryRunAll} + a.setMeta(*configMap) + return nil } - app, err := a.coreClient.CoreV1().ConfigMaps(a.nsName).Create(context.TODO(), configMap, createOpts) + app, err := a.coreClient.CoreV1().ConfigMaps(a.nsName).Create(context.TODO(), configMap, metav1.CreateOptions{}) a.setMeta(*app) return err diff --git a/test/e2e/diff_test.go b/test/e2e/diff_test.go index b4b8f751a..e0c089bb8 100644 --- a/test/e2e/diff_test.go +++ b/test/e2e/diff_test.go @@ -4,6 +4,7 @@ package e2e import ( + "fmt" "strings" "testing" @@ -402,3 +403,99 @@ metadata: NewMissingClusterResource(t, "configmap", name, env.Namespace, kubectl) } + +func TestDiffRunReadOnlyContext(t *testing.T) { + env := BuildEnv(t) + logger := Logger{} + kapp := Kapp{t, env.Namespace, env.KappBinaryPath, Logger{}} + kubectl := Kubectl{t, env.Namespace, Logger{}} + + rbac := ` +--- +apiVersion: v1 +kind: ServiceAccount +metadata: + name: scoped-sa +--- +apiVersion: v1 +kind: Secret +metadata: + name: scoped-sa + annotations: + kubernetes.io/service-account.name: scoped-sa +type: kubernetes.io/service-account-token +--- +kind: Role +apiVersion: rbac.authorization.k8s.io/v1 +metadata: + name: scoped-role +rules: +- apiGroups: [""] + resources: ["configmaps"] + verbs: ["get", "list", "watch"] +--- +kind: RoleBinding +apiVersion: rbac.authorization.k8s.io/v1 +metadata: + name: scoped-role-binding +subjects: +- kind: ServiceAccount + name: scoped-sa +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: scoped-role +` + + rbacName := "test-e2e-rbac-app" + scopedContext := "scoped-context" + scopedUser := "scoped-user" + appName := "diff-run-read-only" + + cleanUp := func() { + kapp.Run([]string{"delete", "-a", rbacName}) + kapp.Run([]string{"delete", "-a", appName}) + } + cleanUp() + defer cleanUp() + + kapp.RunWithOpts([]string{"deploy", "-a", rbacName, "-f", "-"}, RunOpts{IntoNs: true, StdinReader: strings.NewReader(rbac)}) + cleanUpContext := ScopedContext(t, kubectl, "scoped-sa", scopedContext, scopedUser) + defer cleanUpContext() + + yaml1 := ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: config +` + + logger.Section("diff-run app create using read-only role", func() { + _, _ = kapp.RunWithOpts([]string{"deploy", "-f", "-", "-a", appName, "--diff-run", fmt.Sprintf("--kubeconfig-context=%s", scopedContext)}, + RunOpts{IntoNs: true, AllowError: false, StdinReader: strings.NewReader(yaml1)}) + + NewMissingClusterResource(t, "configmap", appName, env.Namespace, kubectl) + }) + + yaml2 := ` +apiVersion: v1 +kind: ConfigMap +metadata: + name: config + annotations: + foo: bar +` + + logger.Section("diff-run app update using read-only role", func() { + _, _ = kapp.RunWithOpts([]string{"deploy", "-f", "-", "-a", appName}, + RunOpts{IntoNs: true, AllowError: false, StdinReader: strings.NewReader(yaml1)}) + + NewPresentClusterResource("configmap", appName, env.Namespace, kubectl) + + _, _ = kapp.RunWithOpts([]string{"deploy", "-f", "-", "-a", appName, "--diff-run", fmt.Sprintf("--kubeconfig-context=%s", scopedContext)}, + RunOpts{IntoNs: true, AllowError: false, StdinReader: strings.NewReader(yaml2)}) + + c := NewPresentClusterResource("configmap", appName, env.Namespace, kubectl) + require.NotContains(t, c.res.Annotations(), "foo", "Expected configmap to not have foo annotation") + }) +}