From c586e47569c6ba144e8cf65391edc7a2e65c8da1 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 | 34 +++++++++++---- test/e2e/diff_test.go | 83 ++++++++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+), 8 deletions(-) diff --git a/pkg/kapp/app/recorded_app.go b/pkg/kapp/app/recorded_app.go index d5a54aa86..9836189e6 100644 --- a/pkg/kapp/app/recorded_app.go +++ b/pkg/kapp/app/recorded_app.go @@ -33,6 +33,7 @@ type RecordedApp struct { name string nsName string isMigrated bool + isDiffRun bool creationTimestamp time.Time appChangesUseAppLabel bool @@ -48,7 +49,7 @@ func NewRecordedApp(name, nsName string, creationTimestamp time.Time, coreClient identifiedResources ctlres.IdentifiedResources, appInDiffNsHintMsgFunc func(string) string, logger logger.Logger) *RecordedApp { // Always trim suffix, even if user added it manually (to avoid double migration) - return &RecordedApp{strings.TrimSuffix(name, AppSuffix), nsName, false, creationTimestamp, false, coreClient, identifiedResources, appInDiffNsHintMsgFunc, + return &RecordedApp{strings.TrimSuffix(name, AppSuffix), nsName, false, false, creationTimestamp, false, coreClient, identifiedResources, appInDiffNsHintMsgFunc, nil, logger.NewPrefixed("RecordedApp")} } @@ -132,6 +133,10 @@ func (a *RecordedApp) UpdateUsedGVsAndGKs(gvs []schema.GroupVersion, gks []schem func (a *RecordedApp) CreateOrUpdate(prevAppName string, labels map[string]string, isDiffRun bool) (bool, error) { defer a.logger.DebugFunc("CreateOrUpdate").Finish() + if isDiffRun { + a.isDiffRun = true + } + app, foundMigratedApp, err := a.find(a.fqName()) if err != nil { return false, err @@ -153,7 +158,7 @@ func (a *RecordedApp) CreateOrUpdate(prevAppName string, labels map[string]strin } if prevAppName == "" { - return true, a.create(labels, isDiffRun) + return true, a.create(labels) } app, foundMigratedPrevApp, err := a.find(prevAppName + AppSuffix) @@ -176,7 +181,7 @@ func (a *RecordedApp) CreateOrUpdate(prevAppName string, labels map[string]strin return false, a.renameConfigMap(app, a.name, a.nsName) } - return true, a.create(labels, isDiffRun) + return true, a.create(labels) } func (a *RecordedApp) find(name string) (*corev1.ConfigMap, bool, error) { @@ -190,7 +195,7 @@ func (a *RecordedApp) find(name string) (*corev1.ConfigMap, bool, error) { return cm, true, nil } -func (a *RecordedApp) create(labels map[string]string, isDiffRun bool) error { +func (a *RecordedApp) create(labels map[string]string) error { configMap := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: a.name, @@ -225,12 +230,13 @@ 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) + + if a.isDiffRun { + 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 @@ -242,6 +248,10 @@ func (a *RecordedApp) updateApp(existingConfigMap *corev1.ConfigMap, labels map[ return err } + if a.isDiffRun { + return nil + } + _, err = a.coreClient.CoreV1().ConfigMaps(a.nsName).Update(context.TODO(), existingConfigMap, metav1.UpdateOptions{}) if err != nil { return fmt.Errorf("Updating app: %w", err) @@ -266,6 +276,10 @@ func (a *RecordedApp) migrate(c *corev1.ConfigMap, labels map[string]string, new return err } + if a.isDiffRun { + return nil + } + _, err = a.coreClient.CoreV1().ConfigMaps(a.nsName).Update(context.TODO(), c, metav1.UpdateOptions{}) if err != nil { return fmt.Errorf("Updating app: %w", err) @@ -559,6 +573,10 @@ func (a *RecordedApp) update(doFunc func(*Meta)) error { return err } + if a.isDiffRun { + return nil + } + _, err = a.coreClient.CoreV1().ConfigMaps(a.nsName).Update(context.TODO(), change, metav1.UpdateOptions{}) if err != nil { return fmt.Errorf("Updating app: %w", err) diff --git a/test/e2e/diff_test.go b/test/e2e/diff_test.go index b4b8f751a..81675257f 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,85 @@ 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 + namespace: __ns__ +--- +apiVersion: v1 +kind: Secret +metadata: + name: scoped-sa + namespace: __ns__ + 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 + namespace: __ns__ +rules: +- apiGroups: [""] + resources: ["configmaps"] + verbs: ["get", "list", "watch"] +--- +kind: RoleBinding +apiVersion: rbac.authorization.k8s.io/v1 +metadata: + name: scoped-role-binding + namespace: __ns__ +subjects: +- kind: ServiceAccount + name: scoped-sa + namespace: __ns__ +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: scoped-role + namespace: __ns__ +` + + rbac = strings.ReplaceAll(rbac, "__ns__", env.Namespace) + + 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{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 using scoped context", 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) + }) +}