From eb2e9d8e9f0decc28ff539e50fba97f507295a3a Mon Sep 17 00:00:00 2001 From: Tim Heurich Date: Tue, 30 May 2023 16:44:51 +0200 Subject: [PATCH] feat: introduce new config for excluding fields on history-less resources Signed-off-by: Tim Heurich --- pkg/kapp/cmd/app/delete.go | 2 +- pkg/kapp/cmd/app/deploy.go | 2 +- pkg/kapp/cmd/app/inspect.go | 2 +- pkg/kapp/cmd/tools/diff.go | 2 +- pkg/kapp/config/conf.go | 10 +++ pkg/kapp/config/config.go | 14 +++++ pkg/kapp/config/default.go | 1 + pkg/kapp/config/default_test.go | 2 +- pkg/kapp/diff/change_factory.go | 17 +++-- pkg/kapp/diff/change_set_test.go | 10 +-- pkg/kapp/diff/resource_with_history.go | 25 +++++--- test/e2e/config_test.go | 86 ++------------------------ test/e2e/order_test.go | 2 +- 13 files changed, 67 insertions(+), 108 deletions(-) diff --git a/pkg/kapp/cmd/app/delete.go b/pkg/kapp/cmd/app/delete.go index d1b86ff12..344b5a7fa 100644 --- a/pkg/kapp/cmd/app/delete.go +++ b/pkg/kapp/cmd/app/delete.go @@ -233,7 +233,7 @@ func (o *DeleteOptions) calculateAndPresentChanges(existingResources []ctlres.Re ) { // Figure out changes for X existing resources -> 0 new resources - changeFactory := ctldiff.NewChangeFactory(nil, nil) + changeFactory := ctldiff.NewChangeFactory(nil, nil, nil) changeSetFactory := ctldiff.NewChangeSetFactory(o.DiffFlags.ChangeSetOpts, changeFactory) changes, err := changeSetFactory.New(existingResources, nil).Calculate() diff --git a/pkg/kapp/cmd/app/deploy.go b/pkg/kapp/cmd/app/deploy.go index 314544845..13c00a213 100644 --- a/pkg/kapp/cmd/app/deploy.go +++ b/pkg/kapp/cmd/app/deploy.go @@ -405,7 +405,7 @@ func (o *DeployOptions) calculateAndPresentChanges(existingResources, var clusterChangeSet ctlcap.ClusterChangeSet { // Figure out changes for X existing resources -> X new resources - changeFactory := ctldiff.NewChangeFactory(conf.RebaseMods(), conf.DiffAgainstLastAppliedFieldExclusionMods()) + changeFactory := ctldiff.NewChangeFactory(conf.RebaseMods(), conf.DiffAgainstLastAppliedFieldExclusionMods(), conf.DiffAgainstExistingFieldExclusionMods()) changeSetFactory := ctldiff.NewChangeSetFactory(o.DiffFlags.ChangeSetOpts, changeFactory) err := ctldiff.NewRenewableResources(existingResources, newResources).Prepare() diff --git a/pkg/kapp/cmd/app/inspect.go b/pkg/kapp/cmd/app/inspect.go index 2c7cb3bcd..64d866fa9 100644 --- a/pkg/kapp/cmd/app/inspect.go +++ b/pkg/kapp/cmd/app/inspect.go @@ -97,7 +97,7 @@ func (o *InspectOptions) Run() error { switch { case o.Raw: for _, res := range resources { - historylessRes, err := ctldiff.NewResourceWithHistory(res, nil, nil).HistorylessResource() + historylessRes, err := ctldiff.NewResourceWithoutHistory(res, nil).Resource() if err != nil { return err } diff --git a/pkg/kapp/cmd/tools/diff.go b/pkg/kapp/cmd/tools/diff.go index 99ed9106c..87ce94e7b 100644 --- a/pkg/kapp/cmd/tools/diff.go +++ b/pkg/kapp/cmd/tools/diff.go @@ -48,7 +48,7 @@ func (o *DiffOptions) Run() error { return err } - changeFactory := ctldiff.NewChangeFactory(nil, nil) + changeFactory := ctldiff.NewChangeFactory(nil, nil, nil) changes, err := ctldiff.NewChangeSet(existingResources, newResources, o.DiffFlags.ChangeSetOpts, changeFactory).Calculate() if err != nil { diff --git a/pkg/kapp/config/conf.go b/pkg/kapp/config/conf.go index 7b0c0fdb8..d8daf2ccc 100644 --- a/pkg/kapp/config/conf.go +++ b/pkg/kapp/config/conf.go @@ -102,6 +102,16 @@ func (c Conf) DiffAgainstLastAppliedFieldExclusionMods() []ctlres.FieldRemoveMod return mods } +func (c Conf) DiffAgainstExistingFieldExclusionMods() []ctlres.FieldRemoveMod { + var mods []ctlres.FieldRemoveMod + for _, config := range c.configs { + for _, rule := range config.DiffAgainstExistingFieldExclusionRules { + mods = append(mods, rule.AsMod()) + } + } + return mods +} + func (c Conf) OwnershipLabelMods() func(kvs map[string]string) []ctlres.StringMapAppendMod { return func(kvs map[string]string) []ctlres.StringMapAppendMod { var mods []ctlres.StringMapAppendMod diff --git a/pkg/kapp/config/config.go b/pkg/kapp/config/config.go index f299d9296..eb28929ef 100644 --- a/pkg/kapp/config/config.go +++ b/pkg/kapp/config/config.go @@ -33,6 +33,7 @@ type Config struct { AdditionalLabels map[string]string DiffAgainstLastAppliedFieldExclusionRules []DiffAgainstLastAppliedFieldExclusionRule + DiffAgainstExistingFieldExclusionRules []DiffAgainstExistingFieldExclusionRule // TODO additional? // TODO validations @@ -96,6 +97,11 @@ type DiffAgainstLastAppliedFieldExclusionRule struct { Path ctlres.Path } +type DiffAgainstExistingFieldExclusionRule struct { + ResourceMatchers []ResourceMatcher + Path ctlres.Path +} + type OwnershipLabelRule struct { ResourceMatchers []ResourceMatcher Path ctlres.Path @@ -290,6 +296,14 @@ func (r DiffAgainstLastAppliedFieldExclusionRule) AsMod() ctlres.FieldRemoveMod Path: r.Path, } } +func (r DiffAgainstExistingFieldExclusionRule) AsMod() ctlres.FieldRemoveMod { + return ctlres.FieldRemoveMod{ + ResourceMatcher: ctlres.AnyMatcher{ + Matchers: ResourceMatchers(r.ResourceMatchers).AsResourceMatchers(), + }, + Path: r.Path, + } +} func (r OwnershipLabelRule) AsMod(kvs map[string]string) ctlres.StringMapAppendMod { return ctlres.StringMapAppendMod{ diff --git a/pkg/kapp/config/default.go b/pkg/kapp/config/default.go index 3376f15d9..081fe06f5 100644 --- a/pkg/kapp/config/default.go +++ b/pkg/kapp/config/default.go @@ -231,6 +231,7 @@ diffAgainstLastAppliedFieldExclusionRules: - path: [metadata, annotations, "deployment.kubernetes.io/revision"] resourceMatchers: *appsV1DeploymentWithRevAnnKey +diffAgainstExistingFieldExclusionRules: - path: [status] resourceMatchers: - allMatcher: {} diff --git a/pkg/kapp/config/default_test.go b/pkg/kapp/config/default_test.go index 976aad555..e8ace3f56 100644 --- a/pkg/kapp/config/default_test.go +++ b/pkg/kapp/config/default_test.go @@ -15,7 +15,7 @@ import ( func TestDefaultTemplateRules(t *testing.T) { _, defaultConfig, err := config.NewConfFromResourcesWithDefaults([]ctlres.Resource{}) require.NoError(t, err) - changeFactory := ctldiff.NewChangeFactory(defaultConfig.RebaseMods(), defaultConfig.DiffAgainstLastAppliedFieldExclusionMods()) + changeFactory := ctldiff.NewChangeFactory(defaultConfig.RebaseMods(), defaultConfig.DiffAgainstLastAppliedFieldExclusionMods(), defaultConfig.DiffAgainstExistingFieldExclusionMods()) testCases := []struct { description string diff --git a/pkg/kapp/diff/change_factory.go b/pkg/kapp/diff/change_factory.go index 7fbff3891..f4e4f2f90 100644 --- a/pkg/kapp/diff/change_factory.go +++ b/pkg/kapp/diff/change_factory.go @@ -10,12 +10,13 @@ import ( type ChangeFactory struct { rebaseMods []ctlres.ResourceModWithMultiple diffAgainstLastAppliedFieldExclusionMods []ctlres.FieldRemoveMod + diffAgainstExistingFieldExclusionRules []ctlres.FieldRemoveMod } func NewChangeFactory(rebaseMods []ctlres.ResourceModWithMultiple, - diffAgainstLastAppliedFieldExclusionMods []ctlres.FieldRemoveMod) ChangeFactory { + diffAgainstLastAppliedFieldExclusionMods []ctlres.FieldRemoveMod, diffAgainstExistingFieldExclusionRules []ctlres.FieldRemoveMod) ChangeFactory { - return ChangeFactory{rebaseMods, diffAgainstLastAppliedFieldExclusionMods} + return ChangeFactory{rebaseMods, diffAgainstLastAppliedFieldExclusionMods, diffAgainstExistingFieldExclusionRules} } func (f ChangeFactory) NewChangeAgainstLastApplied(existingRes, newRes ctlres.Resource) (Change, error) { @@ -36,7 +37,7 @@ func (f ChangeFactory) NewChangeAgainstLastApplied(existingRes, newRes ctlres.Re existingRes = rebasedLastAppliedRes } - historylessExistingRes, err := f.NewResourceWithHistory(existingRes).HistorylessResource() + historylessExistingRes, err := f.newResourceWithoutHistory(existingRes).Resource() if err != nil { return nil, err } @@ -45,7 +46,7 @@ func (f ChangeFactory) NewChangeAgainstLastApplied(existingRes, newRes ctlres.Re } if newRes != nil { - historylessNewRes, err := f.NewResourceWithHistory(newRes).HistorylessResource() + historylessNewRes, err := f.newResourceWithoutHistory(newRes).Resource() if err != nil { return nil, err } @@ -63,7 +64,7 @@ func (f ChangeFactory) NewChangeAgainstLastApplied(existingRes, newRes ctlres.Re func (f ChangeFactory) NewExactChange(existingRes, newRes ctlres.Resource) (Change, error) { if existingRes != nil { - historylessExistingRes, err := f.NewResourceWithHistory(existingRes).HistorylessResource() + historylessExistingRes, err := f.newResourceWithoutHistory(existingRes).Resource() if err != nil { return nil, err } @@ -72,7 +73,7 @@ func (f ChangeFactory) NewExactChange(existingRes, newRes ctlres.Resource) (Chan } if newRes != nil { - historylessNewRes, err := f.NewResourceWithHistory(newRes).HistorylessResource() + historylessNewRes, err := f.newResourceWithoutHistory(newRes).Resource() if err != nil { return nil, err } @@ -91,3 +92,7 @@ func (f ChangeFactory) NewExactChange(existingRes, newRes ctlres.Resource) (Chan func (f ChangeFactory) NewResourceWithHistory(resource ctlres.Resource) ResourceWithHistory { return NewResourceWithHistory(resource, &f, f.diffAgainstLastAppliedFieldExclusionMods) } + +func (f ChangeFactory) newResourceWithoutHistory(resource ctlres.Resource) ResourceWithoutHistory { + return NewResourceWithoutHistory(resource, f.diffAgainstExistingFieldExclusionRules) +} diff --git a/pkg/kapp/diff/change_set_test.go b/pkg/kapp/diff/change_set_test.go index 5859a7efa..f42340222 100644 --- a/pkg/kapp/diff/change_set_test.go +++ b/pkg/kapp/diff/change_set_test.go @@ -48,7 +48,7 @@ metadata: }, } - changeFactory := ctldiff.NewChangeFactory(mods, nil) + changeFactory := ctldiff.NewChangeFactory(mods, nil, nil) changeSet := ctldiff.NewChangeSet([]ctlres.Resource{existingRes}, []ctlres.Resource{newRes}, ctldiff.ChangeSetOpts{}, changeFactory) @@ -106,7 +106,7 @@ metadata: }, } - changeFactory := ctldiff.NewChangeFactory(mods, nil) + changeFactory := ctldiff.NewChangeFactory(mods, nil, nil) changeSet := ctldiff.NewChangeSet([]ctlres.Resource{existingRes}, []ctlres.Resource{newRes}, ctldiff.ChangeSetOpts{}, changeFactory) @@ -174,7 +174,7 @@ metadata: }, } - changeFactory := ctldiff.NewChangeFactory(rebaseMods, ignoreFieldsMods) + changeFactory := ctldiff.NewChangeFactory(rebaseMods, ignoreFieldsMods, nil) changeSet := ctldiff.NewChangeSet([]ctlres.Resource{existingRes}, []ctlres.Resource{newRes}, ctldiff.ChangeSetOpts{AgainstLastApplied: true}, changeFactory) @@ -246,7 +246,7 @@ metadata: }, } - changeFactory := ctldiff.NewChangeFactory(rebaseMods, ignoreFieldsMods) + changeFactory := ctldiff.NewChangeFactory(rebaseMods, ignoreFieldsMods, nil) changeSet := ctldiff.NewChangeSet([]ctlres.Resource{existingRes}, []ctlres.Resource{newRes}, ctldiff.ChangeSetOpts{AgainstLastApplied: true}, changeFactory) @@ -304,7 +304,7 @@ metadata: }, } - changeFactory := ctldiff.NewChangeFactory(mods, nil) + changeFactory := ctldiff.NewChangeFactory(mods, nil, nil) changeSet := ctldiff.NewChangeSet([]ctlres.Resource{existingRes}, []ctlres.Resource{newRes}, ctldiff.ChangeSetOpts{}, changeFactory) diff --git a/pkg/kapp/diff/resource_with_history.go b/pkg/kapp/diff/resource_with_history.go index dca747367..34115aa7f 100644 --- a/pkg/kapp/diff/resource_with_history.go +++ b/pkg/kapp/diff/resource_with_history.go @@ -36,9 +36,8 @@ func NewResourceWithHistory(resource ctlres.Resource, return ResourceWithHistory{resource.DeepCopy(), changeFactory, diffAgainstLastAppliedFieldExclusionMods} } - -func (r ResourceWithHistory) HistorylessResource() (ctlres.Resource, error) { - return resourceWithoutHistory{r.resource}.Resource() +func NewResourceWithoutHistory(resource ctlres.Resource, fieldExclusionMods []ctlres.FieldRemoveMod) ResourceWithoutHistory { + return ResourceWithoutHistory{res: resource, fieldExclusionMods: fieldExclusionMods} } // LastAppliedResource will return "last applied" resource that was saved @@ -156,12 +155,12 @@ func (r ResourceWithHistory) recalculateLastAppliedChange() ([]Change, string, s func (r ResourceWithHistory) newExactHistorylessChange(existingRes, newRes ctlres.Resource) (Change, error) { // If annotations are not removed line numbers will be mismatched - existingRes, err := resourceWithoutHistory{existingRes}.Resource() + existingRes, err := ResourceWithoutHistory{existingRes, nil}.Resource() if err != nil { return nil, err } - newRes, err = resourceWithoutHistory{newRes}.Resource() + newRes, err = ResourceWithoutHistory{newRes, nil}.Resource() if err != nil { return nil, err } @@ -169,11 +168,12 @@ func (r ResourceWithHistory) newExactHistorylessChange(existingRes, newRes ctlre return r.changeFactory.NewExactChange(existingRes, newRes) } -type resourceWithoutHistory struct { - res ctlres.Resource +type ResourceWithoutHistory struct { + res ctlres.Resource + fieldExclusionMods []ctlres.FieldRemoveMod } -func (r resourceWithoutHistory) Resource() (ctlres.Resource, error) { +func (r ResourceWithoutHistory) Resource() (ctlres.Resource, error) { res := r.res.DeepCopy() for _, t := range r.removeAppliedResAnnKeysMods() { @@ -183,10 +183,17 @@ func (r resourceWithoutHistory) Resource() (ctlres.Resource, error) { } } + for _, t := range r.fieldExclusionMods { + err := t.Apply(res) + if err != nil { + return nil, err + } + } + return res, nil } -func (resourceWithoutHistory) removeAppliedResAnnKeysMods() []ctlres.ResourceMod { +func (ResourceWithoutHistory) removeAppliedResAnnKeysMods() []ctlres.ResourceMod { return []ctlres.ResourceMod{ ctlres.FieldRemoveMod{ ResourceMatcher: ctlres.AllMatcher{}, diff --git a/test/e2e/config_test.go b/test/e2e/config_test.go index 69bb3cd19..e56c9ea3c 100644 --- a/test/e2e/config_test.go +++ b/test/e2e/config_test.go @@ -559,7 +559,7 @@ func asYAML(t *testing.T, val interface{}) string { return string(bs) } -func TestDefaultConfig_ExcludeDiffAgainstLastAppliedStatus(t *testing.T) { +func TestDefaultConfig_ExcludeDiffAgainstExistingStatus(t *testing.T) { env := BuildEnv(t) logger := Logger{} kapp := Kapp{t, env.Namespace, env.KappBinaryPath, logger} @@ -591,38 +591,6 @@ status: conditions: [] storedVersions: [] ` - - updatedStatusYaml := ` ---- -apiVersion: apiextensions.k8s.io/v1 -kind: CustomResourceDefinition -metadata: - name: tests.kapp.example -spec: - group: kapp.example - names: - kind: Test - plural: tests - scope: Namespaced - versions: - - name: v1alpha1 - schema: - openAPIV3Schema: - type: object - served: true - storage: true -status: - acceptedNames: - kind: "" - plural: "" - conditions: - - message: some conflicts found - reason: NoConflicts - status: "True" - type: NamesAccepted - storedVersions: [] -` - updatedYaml := ` --- apiVersion: apiextensions.k8s.io/v1 @@ -668,20 +636,6 @@ status: -linesss- schema: -linesss- openAPIV3Schema: -linesss- type: object --linesss- status: --linesss- acceptedNames:` - - expectedChangesForStatusUpdate := ` -@@ update customresourcedefinition/tests.kapp.example (apiextensions.k8s.io/v1) cluster @@ - ... --linesss- plural: "" --linesss- conditions: [] --linesss- conditions: --linesss- - message: some conflicts found --linesss- reason: NoConflicts --linesss- status: "True" --linesss- type: NamesAccepted --linesss- storedVersions: [] -linesss- ` expectedChangesForExternalUpdate := ` @@ -703,31 +657,7 @@ status: -linesss- plural: tests -linesss- singular: test -linesss- scope: Namespaced --linesss- versions: - ... --linesss- acceptedNames: --linesss- kind: Test --linesss- listKind: TestList --linesss- plural: tests --linesss- singular: test --linesss- conditions: --linesss- - lastTransitionTime: "2006-01-02T15:04:05Z07:00" --linesss- message: no conflicts found --linesss- reason: NoConflicts --linesss- status: "True" --linesss- type: NamesAccepted --linesss- - lastTransitionTime: "2006-01-02T15:04:05Z07:00" --linesss- message: the initial names have been accepted --linesss- reason: InitialNamesAccepted --linesss- status: "True" --linesss- type: Established --linesss- storedVersions: --linesss- - v1alpha1 --linesss- kind: "" --linesss- plural: "" --linesss- conditions: [] --linesss- storedVersions: [] --linesss- ` +-linesss- versions:` name := "test-default-config-exclude-diff-against-last-applied-status" cleanUp := func() { @@ -763,18 +693,10 @@ status: out, _ := kapp.RunWithOpts([]string{"deploy", "-f", "-", "-a", name, "--diff-run", "-c"}, RunOpts{IntoNs: true, AllowError: true, StdinReader: strings.NewReader(updatedYaml)}) - // last applied status is used for diffing, hence no changes to status are detected + // status is ignored in diff checkChangesOutput(t, replaceTimestampWithDfaultValue(out), expectedChangesForUpdate) }) - logger.Section("deploy with some changes to status field", func() { - out, _ := kapp.RunWithOpts([]string{"deploy", "-f", "-", "-a", name, "--diff-run", "-c"}, - RunOpts{IntoNs: true, AllowError: true, StdinReader: strings.NewReader(updatedStatusYaml)}) - - // if status is updated, diffing is done against last applied status - checkChangesOutput(t, replaceTimestampWithDfaultValue(out), expectedChangesForStatusUpdate) - }) - logger.Section("deploy after some changes are made by external controller", func() { patchCRD := `[{ "op": "add", "path": "/metadata/annotations/test", "value": "test-ann-val"}]` @@ -784,7 +706,7 @@ status: out, _ := kapp.RunWithOpts([]string{"deploy", "-f", "-", "-a", name, "-c"}, RunOpts{IntoNs: true, AllowError: true, StdinReader: strings.NewReader(yaml)}) - // status is not ignored when smart diff is not used + // status is ignored when smart diff is not used checkChangesOutput(t, replaceTimestampWithDfaultValue(out), expectedChangesForExternalUpdate) }) } diff --git a/test/e2e/order_test.go b/test/e2e/order_test.go index f122a9155..328f47541 100644 --- a/test/e2e/order_test.go +++ b/test/e2e/order_test.go @@ -315,7 +315,7 @@ metadata: break } } - patch := `[{ "op": "add", "path": "/status/conditions/-", "value": {type: "Available", status: "True"}}]` + patch := `[{ "op": "add", "path": "/status", "value": {"conditions": [{"type": "Available", "status": "True"}]}}]` PatchClusterResource("CronTab", "my-new-cron-object-1", env.Namespace, patch, kubectl) }()