From 5affab056180762e2045a7b96f335e254d14d3d7 Mon Sep 17 00:00:00 2001 From: John Gardiner Myers Date: Fri, 16 Jun 2023 20:49:27 -0700 Subject: [PATCH] Remove PropertyValuesEqual method from Provider interface --- controller/controller.go | 11 +-- plan/plan.go | 18 +--- plan/plan_test.go | 131 +++++++------------------ provider/cloudflare/cloudflare_test.go | 7 +- provider/designate/designate.go | 8 -- provider/plural/plural.go | 4 - provider/provider.go | 5 - provider/provider_test.go | 9 -- registry/aws_sd_registry.go | 4 - registry/dynamodb.go | 5 - registry/noop.go | 5 - registry/registry.go | 1 - registry/txt.go | 5 - 13 files changed, 48 insertions(+), 165 deletions(-) diff --git a/controller/controller.go b/controller/controller.go index 369c054822..4502776b75 100644 --- a/controller/controller.go +++ b/controller/controller.go @@ -217,12 +217,11 @@ func (c *Controller) RunOnce(ctx context.Context) error { endpoints = c.Registry.AdjustEndpoints(endpoints) plan := &plan.Plan{ - Policies: []plan.Policy{c.Policy}, - Current: records, - Desired: endpoints, - DomainFilter: endpoint.MatchAllDomainFilters{c.DomainFilter, c.Registry.GetDomainFilter()}, - PropertyComparator: c.Registry.PropertyValuesEqual, - ManagedRecords: c.ManagedRecordTypes, + Policies: []plan.Policy{c.Policy}, + Current: records, + Desired: endpoints, + DomainFilter: endpoint.MatchAllDomainFilters{c.DomainFilter, c.Registry.GetDomainFilter()}, + ManagedRecords: c.ManagedRecordTypes, } plan = plan.Calculate() diff --git a/plan/plan.go b/plan/plan.go index 312bb261b1..5f9ef0cc51 100644 --- a/plan/plan.go +++ b/plan/plan.go @@ -44,8 +44,6 @@ type Plan struct { Changes *Changes // DomainFilter matches DNS names DomainFilter endpoint.DomainFilterInterface - // Property comparator compares custom properties of providers - PropertyComparator PropertyComparator // DNS record types that will be considered for management ManagedRecords []string } @@ -217,21 +215,9 @@ func (p *Plan) shouldUpdateProviderSpecific(desired, current *endpoint.Endpoint) if current.ProviderSpecific != nil { for _, c := range current.ProviderSpecific { if d, ok := desiredProperties[c.Name]; ok { - if p.PropertyComparator != nil { - if !p.PropertyComparator(c.Name, c.Value, d.Value) { - return true - } - } else if c.Value != d.Value { - return true - } + return c.Value != d.Value } else { - if p.PropertyComparator != nil { - if !p.PropertyComparator(c.Name, c.Value, "") { - return true - } - } else if c.Value != "" { - return true - } + return c.Value != "" } } } diff --git a/plan/plan_test.go b/plan/plan_test.go index cc9e56bd5a..e012aea52a 100644 --- a/plan/plan_test.go +++ b/plan/plan_test.go @@ -342,21 +342,18 @@ func (suite *PlanTestSuite) TestSyncSecondRoundWithProviderSpecificChange() { validateEntries(suite.T(), changes.Delete, expectedDelete) } -func (suite *PlanTestSuite) TestSyncSecondRoundWithProviderSpecificDefaultFalse() { +func (suite *PlanTestSuite) TestSyncSecondRoundWithProviderSpecificRemoval() { current := []*endpoint.Endpoint{suite.bar127AWithProviderSpecificFalse} desired := []*endpoint.Endpoint{suite.bar127AWithProviderSpecificUnset} expectedCreate := []*endpoint.Endpoint{} - expectedUpdateOld := []*endpoint.Endpoint{} - expectedUpdateNew := []*endpoint.Endpoint{} + expectedUpdateOld := []*endpoint.Endpoint{suite.bar127AWithProviderSpecificFalse} + expectedUpdateNew := []*endpoint.Endpoint{suite.bar127AWithProviderSpecificUnset} expectedDelete := []*endpoint.Endpoint{} p := &Plan{ - Policies: []Policy{&SyncPolicy{}}, - Current: current, - Desired: desired, - PropertyComparator: func(name, previous, current string) bool { - return CompareBoolean(false, name, previous, current) - }, + Policies: []Policy{&SyncPolicy{}}, + Current: current, + Desired: desired, ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME}, } @@ -367,29 +364,28 @@ func (suite *PlanTestSuite) TestSyncSecondRoundWithProviderSpecificDefaultFalse( validateEntries(suite.T(), changes.Delete, expectedDelete) } -func (suite *PlanTestSuite) TestSyncSecondRoundWithProviderSpecificDefualtTrue() { - current := []*endpoint.Endpoint{suite.bar127AWithProviderSpecificTrue} - desired := []*endpoint.Endpoint{suite.bar127AWithProviderSpecificUnset} - expectedCreate := []*endpoint.Endpoint{} - expectedUpdateOld := []*endpoint.Endpoint{} - expectedUpdateNew := []*endpoint.Endpoint{} - expectedDelete := []*endpoint.Endpoint{} - - p := &Plan{ - Policies: []Policy{&SyncPolicy{}}, - Current: current, - Desired: desired, - PropertyComparator: func(name, previous, current string) bool { - return CompareBoolean(true, name, previous, current) - }, - } - - changes := p.Calculate().Changes - validateEntries(suite.T(), changes.Create, expectedCreate) - validateEntries(suite.T(), changes.UpdateNew, expectedUpdateNew) - validateEntries(suite.T(), changes.UpdateOld, expectedUpdateOld) - validateEntries(suite.T(), changes.Delete, expectedDelete) -} +// todo: this is currently an expect-fail +//func (suite *PlanTestSuite) TestSyncSecondRoundWithProviderSpecificAddition() { +// current := []*endpoint.Endpoint{suite.bar127AWithProviderSpecificUnset} +// desired := []*endpoint.Endpoint{suite.bar127AWithProviderSpecificTrue} +// expectedCreate := []*endpoint.Endpoint{} +// expectedUpdateOld := []*endpoint.Endpoint{suite.bar127AWithProviderSpecificUnset} +// expectedUpdateNew := []*endpoint.Endpoint{suite.bar127AWithProviderSpecificTrue} +// expectedDelete := []*endpoint.Endpoint{} +// +// p := &Plan{ +// Policies: []Policy{&SyncPolicy{}}, +// Current: current, +// Desired: desired, +// ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME}, +// } +// +// changes := p.Calculate().Changes +// validateEntries(suite.T(), changes.Create, expectedCreate) +// validateEntries(suite.T(), changes.UpdateNew, expectedUpdateNew) +// validateEntries(suite.T(), changes.UpdateOld, expectedUpdateOld) +// validateEntries(suite.T(), changes.Delete, expectedDelete) +//} func (suite *PlanTestSuite) TestSyncSecondRoundWithOwnerInherited() { current := []*endpoint.Endpoint{suite.fooV1Cname} @@ -744,15 +740,11 @@ func TestNormalizeDNSName(t *testing.T) { } func TestShouldUpdateProviderSpecific(tt *testing.T) { - comparator := func(name, previous, current string) bool { - return previous == current - } for _, test := range []struct { - name string - current *endpoint.Endpoint - desired *endpoint.Endpoint - propertyComparator func(name, previous, current string) bool - shouldUpdate bool + name string + current *endpoint.Endpoint + desired *endpoint.Endpoint + shouldUpdate bool }{ { name: "skip AWS target health", @@ -768,8 +760,7 @@ func TestShouldUpdateProviderSpecific(tt *testing.T) { {Name: "aws/evaluate-target-health", Value: "true"}, }, }, - propertyComparator: comparator, - shouldUpdate: false, + shouldUpdate: false, }, { name: "custom property unchanged", @@ -783,55 +774,10 @@ func TestShouldUpdateProviderSpecific(tt *testing.T) { {Name: "custom/property", Value: "true"}, }, }, - propertyComparator: comparator, - shouldUpdate: false, - }, - { - name: "custom property value changed", - current: &endpoint.Endpoint{ - ProviderSpecific: []endpoint.ProviderSpecificProperty{ - {Name: "custom/property", Value: "true"}, - }, - }, - desired: &endpoint.Endpoint{ - ProviderSpecific: []endpoint.ProviderSpecificProperty{ - {Name: "custom/property", Value: "false"}, - }, - }, - propertyComparator: comparator, - shouldUpdate: true, - }, - { - name: "custom property key changed", - current: &endpoint.Endpoint{ - ProviderSpecific: []endpoint.ProviderSpecificProperty{ - {Name: "custom/property", Value: "true"}, - }, - }, - desired: &endpoint.Endpoint{ - ProviderSpecific: []endpoint.ProviderSpecificProperty{ - {Name: "new/property", Value: "true"}, - }, - }, - propertyComparator: comparator, - shouldUpdate: true, - }, - { - name: "desired has same key and value as current but not comparator is set", - current: &endpoint.Endpoint{ - ProviderSpecific: []endpoint.ProviderSpecificProperty{ - {Name: "custom/property", Value: "true"}, - }, - }, - desired: &endpoint.Endpoint{ - ProviderSpecific: []endpoint.ProviderSpecificProperty{ - {Name: "custom/property", Value: "true"}, - }, - }, shouldUpdate: false, }, { - name: "desired has same key and different value as current but not comparator is set", + name: "custom property value changed", current: &endpoint.Endpoint{ ProviderSpecific: []endpoint.ProviderSpecificProperty{ {Name: "custom/property", Value: "true"}, @@ -845,7 +791,7 @@ func TestShouldUpdateProviderSpecific(tt *testing.T) { shouldUpdate: true, }, { - name: "desired has different key from current but not comparator is set", + name: "custom property key changed", current: &endpoint.Endpoint{ ProviderSpecific: []endpoint.ProviderSpecificProperty{ {Name: "custom/property", Value: "true"}, @@ -861,10 +807,9 @@ func TestShouldUpdateProviderSpecific(tt *testing.T) { } { tt.Run(test.name, func(t *testing.T) { plan := &Plan{ - Current: []*endpoint.Endpoint{test.current}, - Desired: []*endpoint.Endpoint{test.desired}, - PropertyComparator: test.propertyComparator, - ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME}, + Current: []*endpoint.Endpoint{test.current}, + Desired: []*endpoint.Endpoint{test.desired}, + ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME}, } b := plan.shouldUpdateProviderSpecific(test.desired, test.current) assert.Equal(t, test.shouldUpdate, b) diff --git a/provider/cloudflare/cloudflare_test.go b/provider/cloudflare/cloudflare_test.go index c53dafb493..f29a6cd8d6 100644 --- a/provider/cloudflare/cloudflare_test.go +++ b/provider/cloudflare/cloudflare_test.go @@ -1150,10 +1150,9 @@ func TestProviderPropertiesIdempotency(t *testing.T) { desired = provider.AdjustEndpoints(desired) plan := plan.Plan{ - Current: current, - Desired: desired, - PropertyComparator: provider.PropertyValuesEqual, - ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME}, + Current: current, + Desired: desired, + ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME}, } plan = *plan.Calculate() diff --git a/provider/designate/designate.go b/provider/designate/designate.go index ac9afa7960..ee3db2af38 100644 --- a/provider/designate/designate.go +++ b/provider/designate/designate.go @@ -340,14 +340,6 @@ func (p designateProvider) Records(ctx context.Context) ([]*endpoint.Endpoint, e return result, nil } -func (p designateProvider) PropertyValuesEqual(name, previous, current string) bool { - if name == designateRecordSetID || name == designateZoneID || name == designateOriginalRecords { - return true - } - - return previous == current -} - // temporary structure to hold recordset parameters so that we could aggregate endpoints into recordsets type recordSet struct { dnsName string diff --git a/provider/plural/plural.go b/provider/plural/plural.go index c4dba70cc6..bcf87f1d5c 100644 --- a/provider/plural/plural.go +++ b/provider/plural/plural.go @@ -83,10 +83,6 @@ func (p *PluralProvider) Records(_ context.Context) (endpoints []*endpoint.Endpo return } -func (p *PluralProvider) PropertyValuesEqual(name, previous, current string) bool { - return p.BaseProvider.PropertyValuesEqual(name, previous, current) -} - func (p *PluralProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint { return endpoints } diff --git a/provider/provider.go b/provider/provider.go index 73cd76dd90..c1414bb538 100644 --- a/provider/provider.go +++ b/provider/provider.go @@ -29,7 +29,6 @@ import ( type Provider interface { Records(ctx context.Context) ([]*endpoint.Endpoint, error) ApplyChanges(ctx context.Context, changes *plan.Changes) error - PropertyValuesEqual(name string, previous string, current string) bool // AdjustEndpoints canonicalizes a set of candidate endpoints. // It is called with a set of candidate endpoints obtained from the various sources. // It returns a set modified as required by the provider. The provider is responsible for @@ -47,10 +46,6 @@ func (b BaseProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoin return endpoints } -func (b BaseProvider) PropertyValuesEqual(name, previous, current string) bool { - return previous == current -} - func (b BaseProvider) GetDomainFilter() endpoint.DomainFilter { return endpoint.DomainFilter{} } diff --git a/provider/provider_test.go b/provider/provider_test.go index ae389e9207..f6e0c571cf 100644 --- a/provider/provider_test.go +++ b/provider/provider_test.go @@ -54,12 +54,3 @@ func TestDifference(t *testing.T) { assert.Equal(t, remove, []string{"foo"}) assert.Equal(t, leave, []string{"bar"}) } - -func TestBaseProviderPropertyEquality(t *testing.T) { - p := BaseProvider{} - assert.True(t, p.PropertyValuesEqual("some.property", "", ""), "Both properties not present") - assert.False(t, p.PropertyValuesEqual("some.property", "", "Foo"), "First property missing") - assert.False(t, p.PropertyValuesEqual("some.property", "Foo", ""), "Second property missing") - assert.True(t, p.PropertyValuesEqual("some.property", "Foo", "Foo"), "Properties the same") - assert.False(t, p.PropertyValuesEqual("some.property", "Foo", "Bar"), "Attributes differ") -} diff --git a/registry/aws_sd_registry.go b/registry/aws_sd_registry.go index 49f153acfd..1918f6dbc8 100644 --- a/registry/aws_sd_registry.go +++ b/registry/aws_sd_registry.go @@ -95,10 +95,6 @@ func (sdr *AWSSDRegistry) updateLabels(endpoints []*endpoint.Endpoint) { } } -func (sdr *AWSSDRegistry) PropertyValuesEqual(name string, previous string, current string) bool { - return sdr.provider.PropertyValuesEqual(name, previous, current) -} - // AdjustEndpoints modifies the endpoints as needed by the specific provider func (sdr *AWSSDRegistry) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint { return sdr.provider.AdjustEndpoints(endpoints) diff --git a/registry/dynamodb.go b/registry/dynamodb.go index cb3987499a..9f09fdf0d6 100644 --- a/registry/dynamodb.go +++ b/registry/dynamodb.go @@ -248,11 +248,6 @@ func (im *DynamoDBRegistry) ApplyChanges(ctx context.Context, changes *plan.Chan }) } -// PropertyValuesEqual compares two attribute values for equality. -func (im *DynamoDBRegistry) PropertyValuesEqual(name string, previous string, current string) bool { - return im.provider.PropertyValuesEqual(name, previous, current) -} - // AdjustEndpoints modifies the endpoints as needed by the specific provider. func (im *DynamoDBRegistry) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint { return im.provider.AdjustEndpoints(endpoints) diff --git a/registry/noop.go b/registry/noop.go index 8a034db19d..eb10bb319c 100644 --- a/registry/noop.go +++ b/registry/noop.go @@ -50,11 +50,6 @@ func (im *NoopRegistry) ApplyChanges(ctx context.Context, changes *plan.Changes) return im.provider.ApplyChanges(ctx, changes) } -// PropertyValuesEqual compares two property values for equality -func (im *NoopRegistry) PropertyValuesEqual(attribute string, previous string, current string) bool { - return im.provider.PropertyValuesEqual(attribute, previous, current) -} - // AdjustEndpoints modifies the endpoints as needed by the specific provider func (im *NoopRegistry) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint { return im.provider.AdjustEndpoints(endpoints) diff --git a/registry/registry.go b/registry/registry.go index d99ba05f7f..dc244e0d31 100644 --- a/registry/registry.go +++ b/registry/registry.go @@ -32,7 +32,6 @@ import ( type Registry interface { Records(ctx context.Context) ([]*endpoint.Endpoint, error) ApplyChanges(ctx context.Context, changes *plan.Changes) error - PropertyValuesEqual(attribute string, previous string, current string) bool AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint GetDomainFilter() endpoint.DomainFilter } diff --git a/registry/txt.go b/registry/txt.go index 95abf385a2..a0e5677cf2 100644 --- a/registry/txt.go +++ b/registry/txt.go @@ -284,11 +284,6 @@ func (im *TXTRegistry) ApplyChanges(ctx context.Context, changes *plan.Changes) return im.provider.ApplyChanges(ctx, filteredChanges) } -// PropertyValuesEqual compares two attribute values for equality -func (im *TXTRegistry) PropertyValuesEqual(name string, previous string, current string) bool { - return im.provider.PropertyValuesEqual(name, previous, current) -} - // AdjustEndpoints modifies the endpoints as needed by the specific provider func (im *TXTRegistry) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint { return im.provider.AdjustEndpoints(endpoints)