Merge pull request #3702 from johngmyers/provider-specific-2

Remove PropertyValuesEqual method from Provider interface

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()

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 != ""
 			}
 		}
 	}

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,8 +774,7 @@ func TestShouldUpdateProviderSpecific(tt *testing.T) {
 					{Name: "custom/property", Value: "true"},
 				},
 			},
-			propertyComparator: comparator,
-			shouldUpdate:       false,
+			shouldUpdate: false,
 		},
 		{
 			name: "custom property value changed",
@@ -798,54 +788,10 @@ func TestShouldUpdateProviderSpecific(tt *testing.T) {
 					{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",
-			current: &endpoint.Endpoint{
-				ProviderSpecific: []endpoint.ProviderSpecificProperty{
-					{Name: "custom/property", Value: "true"},
-				},
-			},
-			desired: &endpoint.Endpoint{
-				ProviderSpecific: []endpoint.ProviderSpecificProperty{
-					{Name: "custom/property", Value: "false"},
-				},
-			},
 			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)

provider/cloudflare/cloudflare.go

@@ -297,14 +297,6 @@ func (p *CloudFlareProvider) ApplyChanges(ctx context.Context, changes *plan.Cha
 	return p.submitChanges(ctx, cloudflareChanges)
 }
 
-func (p *CloudFlareProvider) PropertyValuesEqual(name string, previous string, current string) bool {
-	if name == source.CloudflareProxiedKey {
-		return plan.CompareBoolean(p.proxiedByDefault, name, previous, current)
-	}
-
-	return p.BaseProvider.PropertyValuesEqual(name, previous, current)
-}
-
 // submitChanges takes a zone and a collection of Changes and sends them as a single transaction.
 func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloudFlareChange) error {
 	// return early if there is nothing to change
@@ -379,9 +371,12 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud
 func (p *CloudFlareProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint {
 	adjustedEndpoints := []*endpoint.Endpoint{}
 	for _, e := range endpoints {
-		if shouldBeProxied(e, p.proxiedByDefault) {
+		proxied := shouldBeProxied(e, p.proxiedByDefault)
+		if proxied {
 			e.RecordTTL = 0
 		}
+		e.SetProviderSpecificProperty(source.CloudflareProxiedKey, strconv.FormatBool(proxied))
+
 		adjustedEndpoints = append(adjustedEndpoints, e)
 	}
 	return adjustedEndpoints

provider/cloudflare/cloudflare_test.go

@@ -302,6 +302,8 @@ func AssertActions(t *testing.T, provider *CloudFlareProvider, endpoints []*endp
 		t.Fatalf("cannot fetch records, %s", err)
 	}
 
+	endpoints = provider.AdjustEndpoints(endpoints)
+
 	plan := &plan.Plan{
 		Current:        records,
 		Desired:        endpoints,
@@ -1145,11 +1147,12 @@ 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()
@@ -1188,7 +1191,7 @@ func TestCloudflareComplexUpdate(t *testing.T) {
 
 	plan := &plan.Plan{
 		Current: records,
-		Desired: []*endpoint.Endpoint{
+		Desired: provider.AdjustEndpoints([]*endpoint.Endpoint{
 			{
 				DNSName:    "foobar.bar.com",
 				Targets:    endpoint.Targets{"1.2.3.4", "2.3.4.5"},
@@ -1202,7 +1205,7 @@ func TestCloudflareComplexUpdate(t *testing.T) {
 					},
 				},
 			},
-		},
+		}),
 		DomainFilter:   endpoint.NewDomainFilter([]string{"bar.com"}),
 		ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME},
 	}

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

provider/ibmcloud/ibmcloud.go

@@ -385,22 +385,16 @@ func (p *IBMCloudProvider) ApplyChanges(ctx context.Context, changes *plan.Chang
 	return p.submitChanges(ctx, ibmcloudChanges)
 }
 
-func (p *IBMCloudProvider) PropertyValuesEqual(name string, previous string, current string) bool {
-	if name == proxyFilter {
-		return plan.CompareBoolean(p.proxiedByDefault, name, previous, current)
-	}
-
-	return p.BaseProvider.PropertyValuesEqual(name, previous, current)
-}
-
 // AdjustEndpoints modifies the endpoints as needed by the specific provider
 func (p *IBMCloudProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint {
 	adjustedEndpoints := []*endpoint.Endpoint{}
 	for _, e := range endpoints {
 		log.Debugf("adjusting endpont: %v", *e)
-		if shouldBeProxied(e, p.proxiedByDefault) {
+		proxied := shouldBeProxied(e, p.proxiedByDefault)
+		if proxied {
 			e.RecordTTL = 0
 		}
+		e.SetProviderSpecificProperty(proxyFilter, strconv.FormatBool(proxied))
 
 		adjustedEndpoints = append(adjustedEndpoints, e)
 	}

provider/ibmcloud/ibmcloud_test.go

@@ -277,7 +277,7 @@ func TestPrivate_ApplyChanges(t *testing.T) {
 	p := newTestIBMCloudProvider(true)
 
 	changes := plan.Changes{
-		Create: []*endpoint.Endpoint{
+		Create: p.AdjustEndpoints([]*endpoint.Endpoint{
 			{
 				DNSName:    "newA.example.com",
 				RecordType: "A",
@@ -302,7 +302,7 @@ func TestPrivate_ApplyChanges(t *testing.T) {
 				RecordTTL:  240,
 				Targets:    endpoint.NewTargets("\"heritage=external-dns,external-dns/owner=tower-pdns\""),
 			},
-		},
+		}),
 		UpdateOld: []*endpoint.Endpoint{
 			{
 				DNSName:    "test.example.com",
@@ -311,14 +311,14 @@ func TestPrivate_ApplyChanges(t *testing.T) {
 				Targets:    endpoint.NewTargets("1.2.3.4"),
 			},
 		},
-		UpdateNew: []*endpoint.Endpoint{
+		UpdateNew: p.AdjustEndpoints([]*endpoint.Endpoint{
 			{
 				DNSName:    "test.example.com",
 				RecordType: "A",
 				RecordTTL:  180,
 				Targets:    endpoint.NewTargets("1.2.3.4", "5.6.7.8"),
 			},
-		},
+		}),
 		Delete: []*endpoint.Endpoint{
 			{
 				DNSName:    "test.example.com",
@@ -347,7 +347,7 @@ func TestAdjustEndpoints(t *testing.T) {
 			ProviderSpecific: endpoint.ProviderSpecific{
 				{
 					Name:  "ibmcloud-proxied",
-					Value: "true",
+					Value: "1",
 				},
 			},
 		},
@@ -357,6 +357,8 @@ func TestAdjustEndpoints(t *testing.T) {
 
 	assert.Equal(t, endpoint.TTL(0), ep[0].RecordTTL)
 	assert.Equal(t, "test.example.com", ep[0].DNSName)
+	proxied, _ := ep[0].GetProviderSpecificProperty("ibmcloud-proxied")
+	assert.Equal(t, "true", proxied)
 }
 
 func TestPrivateZone_withFilterID(t *testing.T) {

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
 }

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{}
 }

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")
-}

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)

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)

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)

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
 }

registry/txt.go

@@ -297,11 +297,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)