From a56d676266a2f9825967c3bf5c0a74136622f383 Mon Sep 17 00:00:00 2001 From: Praveen Rewar <8457124+praveenrewar@users.noreply.github.com> Date: Thu, 14 Sep 2023 20:34:47 +0530 Subject: [PATCH] Do not create change when app-changes-max-to-keep=0 (#807) Existing behaviour for --app-changes-max-to-keep=0: - Create an app change before deployment begins, and garbage collect it at the end New behaviour: Do not create an app change at all, but still keep the last change details in the meta configmap. Signed-off-by: Praveen Rewar <8457124+praveenrewar@users.noreply.github.com> --- pkg/kapp/app/change.go | 7 +++ pkg/kapp/app/interfaces.go | 2 +- pkg/kapp/app/labeled_app.go | 6 +- pkg/kapp/app/recorded_app.go | 6 +- pkg/kapp/app/recorded_app_changes.go | 22 ++++--- pkg/kapp/app/touch.go | 4 +- pkg/kapp/cmd/app/deploy.go | 9 +-- test/e2e/app_change_test.go | 90 ++++++++++++++++++++++++++++ 8 files changed, 126 insertions(+), 20 deletions(-) diff --git a/pkg/kapp/app/change.go b/pkg/kapp/app/change.go index e95076428..c55acb477 100644 --- a/pkg/kapp/app/change.go +++ b/pkg/kapp/app/change.go @@ -20,6 +20,8 @@ type ChangeImpl struct { meta ChangeMeta createdAt time.Time + + appChangesMaxToKeep int } var _ Change = &ChangeImpl{} @@ -55,6 +57,11 @@ func (c *ChangeImpl) Delete() error { } func (c *ChangeImpl) update(doFunc func(*ChangeMeta)) error { + if c.appChangesMaxToKeep == 0 { + doFunc(&c.meta) + return nil + } + change, err := c.coreClient.CoreV1().ConfigMaps(c.nsName).Get(context.TODO(), c.name, metav1.GetOptions{}) if err != nil { return fmt.Errorf("Getting app change: %w", err) diff --git a/pkg/kapp/app/interfaces.go b/pkg/kapp/app/interfaces.go index 3099241b8..594b94317 100644 --- a/pkg/kapp/app/interfaces.go +++ b/pkg/kapp/app/interfaces.go @@ -30,7 +30,7 @@ type App interface { // Sorted as first is oldest Changes() ([]Change, error) LastChange() (Change, error) - BeginChange(ChangeMeta) (Change, error) + BeginChange(ChangeMeta, int) (Change, error) GCChanges(max int, reviewFunc func(changesToDelete []Change) error) (int, int, error) } diff --git a/pkg/kapp/app/labeled_app.go b/pkg/kapp/app/labeled_app.go index b470c1160..ec985d43e 100644 --- a/pkg/kapp/app/labeled_app.go +++ b/pkg/kapp/app/labeled_app.go @@ -75,9 +75,9 @@ func (a *LabeledApp) Rename(_ string, _ string) error { return fmt.Errorf("Not s func (a *LabeledApp) Meta() (Meta, error) { return Meta{}, nil } -func (a *LabeledApp) Changes() ([]Change, error) { return nil, nil } -func (a *LabeledApp) LastChange() (Change, error) { return nil, nil } -func (a *LabeledApp) BeginChange(ChangeMeta) (Change, error) { return NoopChange{}, nil } +func (a *LabeledApp) Changes() ([]Change, error) { return nil, nil } +func (a *LabeledApp) LastChange() (Change, error) { return nil, nil } +func (a *LabeledApp) BeginChange(ChangeMeta, int) (Change, error) { return NoopChange{}, nil } func (a *LabeledApp) GCChanges(_ int, _ func(changesToDelete []Change) error) (int, int, error) { return 0, 0, nil } diff --git a/pkg/kapp/app/recorded_app.go b/pkg/kapp/app/recorded_app.go index 6f16f6f2f..b9a3fd828 100644 --- a/pkg/kapp/app/recorded_app.go +++ b/pkg/kapp/app/recorded_app.go @@ -510,7 +510,7 @@ func (a *RecordedApp) LastChange() (Change, error) { return nil, err } - if len(meta.LastChangeName) == 0 { + if meta.LastChange.Successful == nil { return nil, nil } @@ -524,13 +524,13 @@ func (a *RecordedApp) LastChange() (Change, error) { return change, nil } -func (a *RecordedApp) BeginChange(meta ChangeMeta) (Change, error) { +func (a *RecordedApp) BeginChange(meta ChangeMeta, appChangesMaxToKeep int) (Change, error) { appMeta, err := a.meta() if err != nil { return nil, err } - change, err := NewRecordedAppChanges(a.nsName, a.name, appMeta.LabelValue, a.appChangesUseAppLabel, a.coreClient).Begin(meta) + change, err := NewRecordedAppChanges(a.nsName, a.name, appMeta.LabelValue, a.appChangesUseAppLabel, a.coreClient).Begin(meta, appChangesMaxToKeep) if err != nil { return nil, err } diff --git a/pkg/kapp/app/recorded_app_changes.go b/pkg/kapp/app/recorded_app_changes.go index 3da4848e4..5cb0889de 100644 --- a/pkg/kapp/app/recorded_app_changes.go +++ b/pkg/kapp/app/recorded_app_changes.go @@ -112,7 +112,7 @@ func (a RecordedAppChanges) DeleteAll() error { return nil } -func (a RecordedAppChanges) Begin(meta ChangeMeta) (*ChangeImpl, error) { +func (a RecordedAppChanges) Begin(meta ChangeMeta, appChangesMaxToKeep int) (*ChangeImpl, error) { newMeta := ChangeMeta{ StartedAt: time.Now().UTC(), Description: meta.Description, @@ -137,16 +137,22 @@ func (a RecordedAppChanges) Begin(meta ChangeMeta) (*ChangeImpl, error) { configMap.ObjectMeta.Labels[legacyChangeLabelKey] = a.appName } - createdChange, err := a.coreClient.CoreV1().ConfigMaps(a.nsName).Create(context.TODO(), configMap, metav1.CreateOptions{}) - if err != nil { - return nil, fmt.Errorf("Creating app change: %w", err) + changeName := "" + + if appChangesMaxToKeep > 0 { + createdChange, err := a.coreClient.CoreV1().ConfigMaps(a.nsName).Create(context.TODO(), configMap, metav1.CreateOptions{}) + if err != nil { + return nil, fmt.Errorf("Creating app change: %w", err) + } + changeName = createdChange.Name } change := &ChangeImpl{ - name: createdChange.Name, - nsName: createdChange.Namespace, - coreClient: a.coreClient, - meta: newMeta, + name: changeName, + nsName: a.nsName, + coreClient: a.coreClient, + meta: newMeta, + appChangesMaxToKeep: appChangesMaxToKeep, } return change, nil diff --git a/pkg/kapp/app/touch.go b/pkg/kapp/app/touch.go index fde004452..d655fbf54 100644 --- a/pkg/kapp/app/touch.go +++ b/pkg/kapp/app/touch.go @@ -8,6 +8,8 @@ type Touch struct { Description string Namespaces []string IgnoreSuccessErr bool + + AppChangesMaxToKeep int } func (t Touch) Do(doFunc func() error) error { @@ -16,7 +18,7 @@ func (t Touch) Do(doFunc func() error) error { Namespaces: t.Namespaces, } - change, err := t.App.BeginChange(meta) + change, err := t.App.BeginChange(meta, t.AppChangesMaxToKeep) if err != nil { return err } diff --git a/pkg/kapp/cmd/app/deploy.go b/pkg/kapp/cmd/app/deploy.go index 53bfdfd91..e0141fd79 100644 --- a/pkg/kapp/cmd/app/deploy.go +++ b/pkg/kapp/cmd/app/deploy.go @@ -220,10 +220,11 @@ func (o *DeployOptions) Run() error { }() touch := ctlapp.Touch{ - App: app, - Description: "update: " + changeSummary, - Namespaces: nsNames, - IgnoreSuccessErr: true, + App: app, + Description: "update: " + changeSummary, + Namespaces: nsNames, + IgnoreSuccessErr: true, + AppChangesMaxToKeep: o.DeployFlags.AppChangesMaxToKeep, } err = touch.Do(func() error { diff --git a/test/e2e/app_change_test.go b/test/e2e/app_change_test.go index 558adc460..4abfb3e3e 100644 --- a/test/e2e/app_change_test.go +++ b/test/e2e/app_change_test.go @@ -13,6 +13,7 @@ import ( uitest "github.com/cppforlife/go-cli-ui/ui/test" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/vmware-tanzu/carvel-kapp/pkg/kapp/app" "gopkg.in/yaml.v2" ) @@ -222,6 +223,95 @@ metadata: require.Equal(t, yamlSubset{LastChange: lastChange{Namespaces: []string{env.Namespace}}, UsedGKs: []usedGK{{Group: "", Kind: "Secret"}}}, thirdConfigMap) } +func TestAppChangesMaxToKeep(t *testing.T) { + env := BuildEnv(t) + logger := Logger{} + kapp := Kapp{t, env.Namespace, env.KappBinaryPath, logger} + kubectl := Kubectl{t, env.Namespace, Logger{}} + + rbacName := "test-e2e-rbac-app" + scopedContext := "scoped-context" + scopedUser := "scoped-user" + name := "test-app-changes-max-to-keep" + + cleanUp := func() { + kapp.Run([]string{"delete", "-a", rbacName}) + kapp.Run([]string{"delete", "-a", name}) + } + + cleanUp() + defer cleanUp() + + rbac := fmt.Sprintf(` +--- +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", "patch", "update", "create"] # no delete permission +- apiGroups: [""] + resources: ["configmaps"] + resourceNames: ["%s", "%s"] + verbs: ["delete"] # delete permission for meta configmap only +- apiGroups: [""] + resources: ["secrets"] + verbs: ["*"] +--- +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 +`, name, name+app.AppSuffix) + + 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: Secret +metadata: + name: redis-config +` + logger.Section("Setting app-changes-max-to-keep to 0 doesn't create new app-changes", func() { + out, _ := kapp.RunWithOpts([]string{"deploy", "-f", "-", "-a", name, "--app-changes-max-to-keep=0", fmt.Sprintf("--kubeconfig-context=%s", scopedContext)}, + RunOpts{IntoNs: true, StdinReader: strings.NewReader(yaml1)}) + + out, _ = kapp.RunWithOpts([]string{"app-change", "ls", "-a", name, "--json"}, RunOpts{}) + + resp := uitest.JSONUIFromBytes(t, []byte(out)) + + require.Equal(t, 0, len(resp.Tables[0].Rows), "Expected to have 0 app changes") + + out = kapp.Run([]string{"delete", "-a", name, fmt.Sprintf("--kubeconfig-context=%s", scopedContext)}) + }) + +} + type usedGK struct { Group string `yaml:"Group"` Kind string `yaml:"Kind"`