From bc7a59b94ad7e2e8b9da21fdfe09b14991e500da Mon Sep 17 00:00:00 2001 From: kumari tanushree Date: Tue, 13 Dec 2022 16:42:13 +0530 Subject: [PATCH 1/3] changes done to avoid listing labled resources for newly created app --- pkg/kapp/app/interfaces.go | 2 +- pkg/kapp/app/labeled_app.go | 6 ++++-- pkg/kapp/app/recorded_app.go | 14 +++++++------- pkg/kapp/cmd/app/deploy.go | 8 +++++--- pkg/kapp/resources/identified_resources_list.go | 6 ++++++ 5 files changed, 23 insertions(+), 13 deletions(-) diff --git a/pkg/kapp/app/interfaces.go b/pkg/kapp/app/interfaces.go index ac47eea40..6e528f29e 100644 --- a/pkg/kapp/app/interfaces.go +++ b/pkg/kapp/app/interfaces.go @@ -22,7 +22,7 @@ type App interface { UsedGKs() (*[]schema.GroupKind, error) UpdateUsedGVsAndGKs([]schema.GroupVersion, []schema.GroupKind) error - CreateOrUpdate(map[string]string, bool) error + CreateOrUpdate(map[string]string, bool) (bool, error) Exists() (bool, string, error) Delete() error Rename(string, string) error diff --git a/pkg/kapp/app/labeled_app.go b/pkg/kapp/app/labeled_app.go index ff5128f45..2cff4b5e2 100644 --- a/pkg/kapp/app/labeled_app.go +++ b/pkg/kapp/app/labeled_app.go @@ -44,8 +44,10 @@ func (a *LabeledApp) UsedGVs() ([]schema.GroupVersion, error) func (a *LabeledApp) UsedGKs() (*[]schema.GroupKind, error) { return nil, nil } func (a *LabeledApp) UpdateUsedGVsAndGKs([]schema.GroupVersion, []schema.GroupKind) error { return nil } -func (a *LabeledApp) CreateOrUpdate(labels map[string]string, isDiffRun bool) error { return nil } -func (a *LabeledApp) Exists() (bool, string, error) { return true, "", nil } +func (a *LabeledApp) CreateOrUpdate(labels map[string]string, isDiffRun bool) (bool, error) { + return false, nil +} +func (a *LabeledApp) Exists() (bool, string, error) { return true, "", nil } func (a *LabeledApp) Delete() error { labelSelector, err := a.LabelSelector() diff --git a/pkg/kapp/app/recorded_app.go b/pkg/kapp/app/recorded_app.go index ab022f6ab..07a1b7801 100644 --- a/pkg/kapp/app/recorded_app.go +++ b/pkg/kapp/app/recorded_app.go @@ -127,32 +127,32 @@ func (a *RecordedApp) UpdateUsedGVsAndGKs(gvs []schema.GroupVersion, gks []schem }) } -func (a *RecordedApp) CreateOrUpdate(labels map[string]string, isDiffRun bool) error { +func (a *RecordedApp) CreateOrUpdate(labels map[string]string, isDiffRun bool) (bool, error) { defer a.logger.DebugFunc("CreateOrUpdate").Finish() app, foundMigratedApp, err := a.find(a.fqName()) if err != nil { - return err + return false, err } if foundMigratedApp { a.isMigrated = true - return a.updateApp(app, labels) + return false, a.updateApp(app, labels) } app, foundNonMigratedApp, err := a.find(a.name) if err != nil { - return err + return false, err } if foundNonMigratedApp { if a.isMigrationEnabled() { - return a.migrate(app, labels, a.fqName()) + return false, a.migrate(app, labels, a.fqName()) } - return a.updateApp(app, labels) + return false, a.updateApp(app, labels) } - return a.create(labels, isDiffRun) + return true, a.create(labels, isDiffRun) } func (a *RecordedApp) find(name string) (*corev1.ConfigMap, bool, error) { diff --git a/pkg/kapp/cmd/app/deploy.go b/pkg/kapp/cmd/app/deploy.go index 53376e855..37f409340 100644 --- a/pkg/kapp/cmd/app/deploy.go +++ b/pkg/kapp/cmd/app/deploy.go @@ -92,6 +92,7 @@ func NewDeployCmd(o *DeployOptions, flagsFactory cmdcore.FlagsFactory) *cobra.Co } func (o *DeployOptions) Run() error { + var newApp bool failingAPIServicesPolicy := o.ResourceTypesFlags.FailingAPIServicePolicy() app, supportObjs, err := Factory(o.depsFactory, o.AppFlags, o.ResourceTypesFlags, o.logger) @@ -107,7 +108,7 @@ func (o *DeployOptions) Run() error { if o.PrevAppFlags.PrevAppName != "" { err = app.RenamePrevApp(o.PrevAppFlags.PrevAppName, appLabels, o.DiffFlags.Run) } else { - err = app.CreateOrUpdate(appLabels, o.DiffFlags.Run) + newApp, err = app.CreateOrUpdate(appLabels, o.DiffFlags.Run) } if err != nil { @@ -158,7 +159,7 @@ func (o *DeployOptions) Run() error { } existingResources, existingPodRs, err := o.existingResources( - newResources, labeledResources, resourceFilter, supportObjs.Apps, usedGKs, append(meta.LastChange.Namespaces, nsNames...)) + newResources, labeledResources, resourceFilter, supportObjs.Apps, usedGKs, append(meta.LastChange.Namespaces, nsNames...), newApp) if err != nil { return err } @@ -352,7 +353,7 @@ func (o *DeployOptions) newResourcesFromFiles() ([]ctlres.Resource, error) { func (o *DeployOptions) existingResources(newResources []ctlres.Resource, labeledResources *ctlres.LabeledResources, resourceFilter ctlres.ResourceFilter, - apps ctlapp.Apps, usedGKs []schema.GroupKind, resourceNamespaces []string) ([]ctlres.Resource, []ctlres.Resource, error) { + apps ctlapp.Apps, usedGKs []schema.GroupKind, resourceNamespaces []string, newApp bool) ([]ctlres.Resource, []ctlres.Resource, error) { labelErrorResolutionFunc := func(key string, val string) string { items, _ := apps.List(nil) @@ -378,6 +379,7 @@ func (o *DeployOptions) existingResources(newResources []ctlres.Resource, IdentifiedResourcesListOpts: ctlres.IdentifiedResourcesListOpts{ GKsScope: usedGKs, ResourceNamespaces: resourceNamespaces, + NewApp: newApp, }, } diff --git a/pkg/kapp/resources/identified_resources_list.go b/pkg/kapp/resources/identified_resources_list.go index 21bb98056..82568beab 100644 --- a/pkg/kapp/resources/identified_resources_list.go +++ b/pkg/kapp/resources/identified_resources_list.go @@ -15,6 +15,7 @@ type IdentifiedResourcesListOpts struct { IgnoreCachedResTypes bool GKsScope []schema.GroupKind ResourceNamespaces []string + NewApp bool } func (r IdentifiedResources) List(labelSelector labels.Selector, resRefs []ResourceRef, opts IdentifiedResourcesListOpts) ([]Resource, error) { @@ -25,6 +26,11 @@ func (r IdentifiedResources) List(labelSelector labels.Selector, resRefs []Resou return nil, err } + // avoid listing labeled resources for newly created app + if opts.NewApp { + return nil, nil + } + // TODO non-listable types resTypes = Listable(resTypes) From c8e9118652dc2ac9db242434591dfd2b30bb8722 Mon Sep 17 00:00:00 2001 From: kumari tanushree Date: Tue, 27 Dec 2022 16:28:33 +0530 Subject: [PATCH 2/3] updated variable name and added return on newApp check before listing all resources from server --- pkg/kapp/cmd/app/deploy.go | 10 +++++----- pkg/kapp/resources/identified_resources_list.go | 6 ------ pkg/kapp/resources/labeled_resources.go | 16 ++++++++++++---- 3 files changed, 17 insertions(+), 15 deletions(-) diff --git a/pkg/kapp/cmd/app/deploy.go b/pkg/kapp/cmd/app/deploy.go index 37f409340..0afb3d82a 100644 --- a/pkg/kapp/cmd/app/deploy.go +++ b/pkg/kapp/cmd/app/deploy.go @@ -92,7 +92,7 @@ func NewDeployCmd(o *DeployOptions, flagsFactory cmdcore.FlagsFactory) *cobra.Co } func (o *DeployOptions) Run() error { - var newApp bool + var isNewApp bool failingAPIServicesPolicy := o.ResourceTypesFlags.FailingAPIServicePolicy() app, supportObjs, err := Factory(o.depsFactory, o.AppFlags, o.ResourceTypesFlags, o.logger) @@ -108,7 +108,7 @@ func (o *DeployOptions) Run() error { if o.PrevAppFlags.PrevAppName != "" { err = app.RenamePrevApp(o.PrevAppFlags.PrevAppName, appLabels, o.DiffFlags.Run) } else { - newApp, err = app.CreateOrUpdate(appLabels, o.DiffFlags.Run) + isNewApp, err = app.CreateOrUpdate(appLabels, o.DiffFlags.Run) } if err != nil { @@ -159,7 +159,7 @@ func (o *DeployOptions) Run() error { } existingResources, existingPodRs, err := o.existingResources( - newResources, labeledResources, resourceFilter, supportObjs.Apps, usedGKs, append(meta.LastChange.Namespaces, nsNames...), newApp) + newResources, labeledResources, resourceFilter, supportObjs.Apps, usedGKs, append(meta.LastChange.Namespaces, nsNames...), isNewApp) if err != nil { return err } @@ -353,7 +353,7 @@ func (o *DeployOptions) newResourcesFromFiles() ([]ctlres.Resource, error) { func (o *DeployOptions) existingResources(newResources []ctlres.Resource, labeledResources *ctlres.LabeledResources, resourceFilter ctlres.ResourceFilter, - apps ctlapp.Apps, usedGKs []schema.GroupKind, resourceNamespaces []string, newApp bool) ([]ctlres.Resource, []ctlres.Resource, error) { + apps ctlapp.Apps, usedGKs []schema.GroupKind, resourceNamespaces []string, isNewApp bool) ([]ctlres.Resource, []ctlres.Resource, error) { labelErrorResolutionFunc := func(key string, val string) string { items, _ := apps.List(nil) @@ -370,6 +370,7 @@ func (o *DeployOptions) existingResources(newResources []ctlres.Resource, ExistingNonLabeledResourcesCheck: o.DeployFlags.ExistingNonLabeledResourcesCheck, ExistingNonLabeledResourcesCheckConcurrency: o.DeployFlags.ExistingNonLabeledResourcesCheckConcurrency, SkipResourceOwnershipCheck: o.DeployFlags.OverrideOwnershipOfExistingResources, + IsNewApp: isNewApp, // Prevent accidently overriding kapp state records DisallowedResourcesByLabelKeys: []string{ctlapp.KappIsAppLabelKey}, @@ -379,7 +380,6 @@ func (o *DeployOptions) existingResources(newResources []ctlres.Resource, IdentifiedResourcesListOpts: ctlres.IdentifiedResourcesListOpts{ GKsScope: usedGKs, ResourceNamespaces: resourceNamespaces, - NewApp: newApp, }, } diff --git a/pkg/kapp/resources/identified_resources_list.go b/pkg/kapp/resources/identified_resources_list.go index 82568beab..21bb98056 100644 --- a/pkg/kapp/resources/identified_resources_list.go +++ b/pkg/kapp/resources/identified_resources_list.go @@ -15,7 +15,6 @@ type IdentifiedResourcesListOpts struct { IgnoreCachedResTypes bool GKsScope []schema.GroupKind ResourceNamespaces []string - NewApp bool } func (r IdentifiedResources) List(labelSelector labels.Selector, resRefs []ResourceRef, opts IdentifiedResourcesListOpts) ([]Resource, error) { @@ -26,11 +25,6 @@ func (r IdentifiedResources) List(labelSelector labels.Selector, resRefs []Resou return nil, err } - // avoid listing labeled resources for newly created app - if opts.NewApp { - return nil, nil - } - // TODO non-listable types resTypes = Listable(resTypes) diff --git a/pkg/kapp/resources/labeled_resources.go b/pkg/kapp/resources/labeled_resources.go index bf3f26620..8c556f638 100644 --- a/pkg/kapp/resources/labeled_resources.go +++ b/pkg/kapp/resources/labeled_resources.go @@ -93,6 +93,7 @@ type AllAndMatchingOpts struct { ExistingNonLabeledResourcesCheck bool ExistingNonLabeledResourcesCheckConcurrency int SkipResourceOwnershipCheck bool + IsNewApp bool DisallowedResourcesByLabelKeys []string LabelErrorResolutionFunc func(string, string) string @@ -107,15 +108,22 @@ type AllAndMatchingOpts struct { func (a *LabeledResources) AllAndMatching(newResources []Resource, opts AllAndMatchingOpts) ([]Resource, error) { defer a.logger.DebugFunc("AllAndMatching").Finish() - resources, err := a.All(opts.IdentifiedResourcesListOpts) - if err != nil { - return nil, err + var ( + resources []Resource + err error + ) + + // avoid listing labeled resources for newly created app + if !opts.IsNewApp { + resources, err = a.All(opts.IdentifiedResourcesListOpts) + if err != nil { + return nil, err + } } var nonLabeledResources []Resource if opts.ExistingNonLabeledResourcesCheck { - var err error nonLabeledResources, err = a.findNonLabeledResources( resources, newResources, opts.ExistingNonLabeledResourcesCheckConcurrency) if err != nil { From 167c82712f83c39f1678987c397abf7782b12125 Mon Sep 17 00:00:00 2001 From: kumari tanushree Date: Fri, 6 Jan 2023 22:12:35 +0530 Subject: [PATCH 3/3] added e2e test --- test/e2e/create_update_delete_test.go | 92 +++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/test/e2e/create_update_delete_test.go b/test/e2e/create_update_delete_test.go index 91644bf7b..533e049b6 100644 --- a/test/e2e/create_update_delete_test.go +++ b/test/e2e/create_update_delete_test.go @@ -443,3 +443,95 @@ data: cleanUp() }) } + +func TestAppDeploy_With_Existing_OR_New_Res(t *testing.T) { + env := BuildEnv(t) + logger := Logger{} + kapp := Kapp{t, env.Namespace, env.KappBinaryPath, logger} + kubectl := Kubectl{t, env.Namespace, logger} + + yaml1 := ` +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm1 +data: + key: value +` + yaml2 := ` +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm2 +data: + key: value +` + yaml3 := ` +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: cm2 +data: + key: value2 +` + + name := "test-app-deploy-with-existing-res" + name2 := "test-app-deploy-with-new-res" + cleanUp := func() { + kapp.Run([]string{"delete", "-a", name}) + kapp.Run([]string{"delete", "-a", name2}) + } + + cleanUp() + defer cleanUp() + + kubectl.RunWithOpts([]string{"apply", "-f", "-"}, + RunOpts{StdinReader: strings.NewReader(yaml1)}) + NewPresentClusterResource("configmap", "cm1", env.Namespace, kubectl) + + logger.Section("deploy app with existing resource", func() { + out, _ := kapp.RunWithOpts([]string{"deploy", "-f", "-", "-a", name, "--diff-changes"}, + RunOpts{StdinReader: strings.NewReader(yaml1)}) + + expectedOutput := ` +@@ update configmap/cm1 (v1) namespace: kapp-test @@ + ... + 4, 4 metadata: + 5 - annotations: + 6 - kubectl.kubernetes.io/last-applied-configuration: | + 7 - {"apiVersion":"v1","data":{"key":"value"},"kind":"ConfigMap","metadata":{"annotations":{},"name":"cm1","namespace":"kapp-test"}} + 8, 5 creationTimestamp: "2006-01-02T15:04:05Z07:00" + 6 + labels: + 7 + kapp.k14s.io/app: "-replaced-" + 8 + kapp.k14s.io/association: -replaced- +` + out = replaceTimestampWithDfaultValue(out) + out = replaceLabelValues(out) + require.Contains(t, out, expectedOutput, "output does not match") + }) + + logger.Section("deploy app with all new resources", func() { + kapp.RunWithOpts([]string{"deploy", "-f", "-", "-a", name2}, + RunOpts{StdinReader: strings.NewReader(yaml2)}) + NewPresentClusterResource("configmap", "cm2", env.Namespace, kubectl) + }) + + logger.Section("deploy again by updating resource", func() { + out, _ := kapp.RunWithOpts([]string{"deploy", "-f", "-", "-a", name2, "--diff-changes"}, + RunOpts{StdinReader: strings.NewReader(yaml3)}) + + expectedOutput := ` +@@ update configmap/cm2 (v1) namespace: kapp-test @@ + ... + 1, 1 data: + 2 - key: value + 2 + key: value2 + 3, 3 kind: ConfigMap + 4, 4 metadata: +` + require.Contains(t, out, expectedOutput, "output does not match") + }) +}