Allow for custom property comparators

Fixes issue #1463
Co-authored-by: Alastair Houghton <alastair@alastairs-place.net>

controller/controller.go

@@ -135,10 +135,11 @@ func (c *Controller) RunOnce(ctx context.Context) error {
 	sourceEndpointsTotal.Set(float64(len(endpoints)))
 
 	plan := &plan.Plan{
-		Policies:     []plan.Policy{c.Policy},
-		Current:      records,
-		Desired:      endpoints,
-		DomainFilter: c.DomainFilter,
+		Policies:           []plan.Policy{c.Policy},
+		Current:            records,
+		Desired:            endpoints,
+		DomainFilter:       c.DomainFilter,
+		PropertyComparator: c.Registry.PropertyValuesEqual,
 	}
 
 	plan = plan.Calculate()

controller/controller_test.go

@@ -35,6 +35,7 @@ import (
 
 // mockProvider returns mock endpoints and validates changes.
 type mockProvider struct {
+	provider.BaseProvider
 	RecordsStore  []*endpoint.Endpoint
 	ExpectChanges *plan.Changes
 }

plan/plan.go

@@ -18,11 +18,15 @@ package plan
 
 import (
 	"fmt"
+	"strconv"
 	"strings"
 
+	log "github.com/sirupsen/logrus"
 	"sigs.k8s.io/external-dns/endpoint"
 )
 
+type PropertyComparator func(name string, previous string, current string) bool
+
 // Plan can convert a list of desired and current records to a series of create,
 // update and delete actions.
 type Plan struct {
@@ -37,6 +41,8 @@ type Plan struct {
 	Changes *Changes
 	// DomainFilter matches DNS names
 	DomainFilter endpoint.DomainFilter
+	// Property comparator compares custom properties of providers
+	PropertyComparator PropertyComparator
 }
 
 // Changes holds lists of actions to be executed by dns providers
@@ -135,7 +141,7 @@ func (p *Plan) Calculate() *Plan {
 			if row.current != nil && len(row.candidates) > 0 { //dns name is taken
 				update := t.resolver.ResolveUpdate(row.current, row.candidates)
 				// compare "update" to "current" to figure out if actual update is required
-				if shouldUpdateTTL(update, row.current) || targetChanged(update, row.current) || shouldUpdateProviderSpecific(update, row.current) {
+				if shouldUpdateTTL(update, row.current) || targetChanged(update, row.current) || p.shouldUpdateProviderSpecific(update, row.current) {
 					inheritOwner(row.current, update)
 					changes.UpdateNew = append(changes.UpdateNew, update)
 					changes.UpdateOld = append(changes.UpdateOld, row.current)
@@ -178,44 +184,39 @@ func shouldUpdateTTL(desired, current *endpoint.Endpoint) bool {
 	return desired.RecordTTL != current.RecordTTL
 }
 
-func shouldUpdateProviderSpecific(desired, current *endpoint.Endpoint) bool {
-	if current.ProviderSpecific == nil && len(desired.ProviderSpecific) == 0 {
-		return false
-	}
-	for _, c := range current.ProviderSpecific {
-		// don't consider target health when detecting changes
-		// see: https://github.com/kubernetes-sigs/external-dns/issues/869#issuecomment-458576954
-		if c.Name == "aws/evaluate-target-health" {
-			continue
-		}
+func (p *Plan) shouldUpdateProviderSpecific(desired, current *endpoint.Endpoint) bool {
+	desiredProperties := map[string]endpoint.ProviderSpecificProperty{}
 
-		found := false
+	if desired.ProviderSpecific != nil {
 		for _, d := range desired.ProviderSpecific {
-			if d.Name == c.Name {
-				if d.Value != c.Value {
-					// provider-specific attribute updated
-					return true
-				}
-				found = true
-				break
-			}
-		}
-		if !found {
-			// provider-specific attribute deleted
-			return true
+			desiredProperties[d.Name] = d
 		}
 	}
-	for _, d := range desired.ProviderSpecific {
-		found := false
+	if current.ProviderSpecific != nil {
 		for _, c := range current.ProviderSpecific {
-			if d.Name == c.Name {
-				found = true
-				break
+			// don't consider target health when detecting changes
+			// see: https://github.com/kubernetes-sigs/external-dns/issues/869#issuecomment-458576954
+			if c.Name == "aws/evaluate-target-health" {
+				continue
+			}
+
+			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
+				}
+			} else {
+				if p.PropertyComparator != nil {
+					if !p.PropertyComparator(c.Name, c.Value, "") {
+						return true
+					}
+				} else if c.Value != "" {
+					return true
+				}
 			}
-		}
-		if !found {
-			// provider-specific attribute added
-			return true
 		}
 	}
 
@@ -260,3 +261,27 @@ func normalizeDNSName(dnsName string) string {
 	}
 	return s
 }
+
+func CompareBoolean(defaultValue bool, name, current, previous string) bool {
+	var err error
+
+	v1, v2 := defaultValue, defaultValue
+
+	if previous != "" {
+		v1, err = strconv.ParseBool(previous)
+		if err != nil {
+			log.Errorf("Failed to parse previous property [%s]: %v", name, previous)
+			v1 = defaultValue
+		}
+	}
+
+	if current != "" {
+		v2, err = strconv.ParseBool(current)
+		if err != nil {
+			log.Errorf("Failed to parse current property [%s]: %v", name, current)
+			v2 = defaultValue
+		}
+	}
+
+	return v1 == v2
+}

plan/plan_test.go

@@ -38,6 +38,7 @@ type PlanTestSuite struct {
 	bar127AWithTTL                   *endpoint.Endpoint
 	bar127AWithProviderSpecificTrue  *endpoint.Endpoint
 	bar127AWithProviderSpecificFalse *endpoint.Endpoint
+	bar127AWithProviderSpecificUnset *endpoint.Endpoint
 	bar192A                          *endpoint.Endpoint
 	multiple1                        *endpoint.Endpoint
 	multiple2                        *endpoint.Endpoint
@@ -138,6 +139,15 @@ func (suite *PlanTestSuite) SetupTest() {
 			},
 		},
 	}
+	suite.bar127AWithProviderSpecificUnset = &endpoint.Endpoint{
+		DNSName:    "bar",
+		Targets:    endpoint.Targets{"127.0.0.1"},
+		RecordType: "A",
+		Labels: map[string]string{
+			endpoint.ResourceLabelKey: "ingress/default/bar-127",
+		},
+		ProviderSpecific: endpoint.ProviderSpecific{},
+	}
 	suite.bar192A = &endpoint.Endpoint{
 		DNSName:    "bar",
 		Targets:    endpoint.Targets{"192.168.0.1"},
@@ -291,6 +301,54 @@ func (suite *PlanTestSuite) TestSyncSecondRoundWithProviderSpecificChange() {
 	validateEntries(suite.T(), changes.Delete, expectedDelete)
 }
 
+func (suite *PlanTestSuite) TestSyncSecondRoundWithProviderSpecificDefaultFalse() {
+	current := []*endpoint.Endpoint{suite.bar127AWithProviderSpecificFalse}
+	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(false, 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)
+}
+
+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)
+}
+
 func (suite *PlanTestSuite) TestSyncSecondRoundWithOwnerInherited() {
 	current := []*endpoint.Endpoint{suite.fooV1Cname}
 	desired := []*endpoint.Endpoint{suite.fooV2Cname}

provider/akamai/akamai.go

@@ -61,6 +61,7 @@ type AkamaiConfig struct {
 
 // AkamaiProvider implements the DNS provider for Akamai.
 type AkamaiProvider struct {
+	provider.BaseProvider
 	domainFilter endpoint.DomainFilter
 	zoneIDFilter provider.ZoneIDFilter
 	config       edgegrid.Config

provider/alibabacloud/alibaba_cloud.go

@@ -67,6 +67,7 @@ type AlibabaCloudPrivateZoneAPI interface {
 
 // AlibabaCloudProvider implements the DNS provider for Alibaba Cloud.
 type AlibabaCloudProvider struct {
+	provider.BaseProvider
 	domainFilter         endpoint.DomainFilter
 	zoneIDFilter         provider.ZoneIDFilter // Private Zone only
 	MaxChangeCount       int

provider/aws/aws.go

@@ -114,6 +114,7 @@ type Route53API interface {
 
 // AWSProvider is an implementation of Provider for AWS Route53.
 type AWSProvider struct {
+	provider.BaseProvider
 	client               Route53API
 	dryRun               bool
 	batchChangeSize      int

provider/awssd/aws_sd.go

@@ -37,6 +37,7 @@ import (
 	"sigs.k8s.io/external-dns/endpoint"
 	"sigs.k8s.io/external-dns/pkg/apis/externaldns"
 	"sigs.k8s.io/external-dns/plan"
+	"sigs.k8s.io/external-dns/provider"
 )
 
 const (
@@ -73,6 +74,7 @@ type AWSSDClient interface {
 
 // AWSSDProvider is an implementation of Provider for AWS Cloud Map.
 type AWSSDProvider struct {
+	provider.BaseProvider
 	client AWSSDClient
 	dryRun bool
 	// only consider namespaces ending in this suffix

provider/azure/azure.go

@@ -67,6 +67,7 @@ type RecordSetsClient interface {
 
 // AzureProvider implements the DNS provider for Microsoft's Azure cloud platform.
 type AzureProvider struct {
+	provider.BaseProvider
 	domainFilter                 endpoint.DomainFilter
 	zoneIDFilter                 provider.ZoneIDFilter
 	dryRun                       bool

provider/azure/azure_private_dns.go

@@ -46,6 +46,7 @@ type PrivateRecordSetsClient interface {
 
 // AzurePrivateDNSProvider implements the DNS provider for Microsoft's Azure Private DNS service
 type AzurePrivateDNSProvider struct {
+	provider.BaseProvider
 	domainFilter     endpoint.DomainFilter
 	zoneIDFilter     provider.ZoneIDFilter
 	dryRun           bool

provider/cloudflare/cloudflare.go

@@ -101,6 +101,7 @@ func (z zoneService) ListZonesContext(ctx context.Context, opts ...cloudflare.Re
 
 // CloudFlareProvider is an implementation of Provider for CloudFlare DNS.
 type CloudFlareProvider struct {
+	provider.BaseProvider
 	Client cloudFlareDNS
 	// only consider hosted zones managing domains ending in this suffix
 	domainFilter      endpoint.DomainFilter
@@ -211,6 +212,14 @@ func (p *CloudFlareProvider) ApplyChanges(ctx context.Context, changes *plan.Cha
 	return p.submitChanges(ctx, combinedChanges)
 }
 
+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

provider/cloudflare/cloudflare_test.go

@@ -903,6 +903,98 @@ func TestCloudflareGroupByNameAndType(t *testing.T) {
 	}
 }
 
+func TestProviderPropertiesIdempotency(t *testing.T) {
+	testCases := []struct {
+		ProviderProxiedByDefault bool
+		RecordsAreProxied        bool
+		ShouldBeUpdated          bool
+	}{
+		{
+			ProviderProxiedByDefault: false,
+			RecordsAreProxied:        false,
+			ShouldBeUpdated:          false,
+		},
+		{
+			ProviderProxiedByDefault: true,
+			RecordsAreProxied:        true,
+			ShouldBeUpdated:          false,
+		},
+		{
+			ProviderProxiedByDefault: true,
+			RecordsAreProxied:        false,
+			ShouldBeUpdated:          true,
+		},
+		{
+			ProviderProxiedByDefault: false,
+			RecordsAreProxied:        true,
+			ShouldBeUpdated:          true,
+		},
+	}
+
+	for _, test := range testCases {
+		client := NewMockCloudFlareClientWithRecords(map[string][]cloudflare.DNSRecord{
+			"001": {
+				{
+					ID:      "1234567890",
+					ZoneID:  "001",
+					Name:    "foobar.bar.com",
+					Type:    endpoint.RecordTypeA,
+					TTL:     120,
+					Content: "1.2.3.4",
+					Proxied: test.RecordsAreProxied,
+				},
+			},
+		})
+
+		provider := &CloudFlareProvider{
+			Client:           client,
+			proxiedByDefault: test.ProviderProxiedByDefault,
+		}
+		ctx := context.Background()
+
+		current, err := provider.Records(ctx)
+		if err != nil {
+			t.Errorf("should not fail, %s", err)
+		}
+		assert.Equal(t, 1, len(current))
+
+		desired := []*endpoint.Endpoint{}
+		for _, c := range current {
+			// Copy all except ProviderSpecific fields
+			desired = append(desired, &endpoint.Endpoint{
+				DNSName:       c.DNSName,
+				Targets:       c.Targets,
+				RecordType:    c.RecordType,
+				SetIdentifier: c.SetIdentifier,
+				RecordTTL:     c.RecordTTL,
+				Labels:        c.Labels,
+			})
+		}
+
+		plan := plan.Plan{
+			Current:            current,
+			Desired:            desired,
+			PropertyComparator: provider.PropertyValuesEqual,
+		}
+
+		plan = *plan.Calculate()
+		assert.NotNil(t, plan.Changes, "should have plan")
+		if plan.Changes == nil {
+			return
+		}
+		assert.Equal(t, 0, len(plan.Changes.Create), "should not have creates")
+		assert.Equal(t, 0, len(plan.Changes.Delete), "should not have deletes")
+
+		if test.ShouldBeUpdated {
+			assert.Equal(t, 1, len(plan.Changes.UpdateNew), "should not have new updates")
+			assert.Equal(t, 1, len(plan.Changes.UpdateOld), "should not have old updates")
+		} else {
+			assert.Equal(t, 0, len(plan.Changes.UpdateNew), "should not have new updates")
+			assert.Equal(t, 0, len(plan.Changes.UpdateOld), "should not have old updates")
+		}
+	}
+}
+
 func TestCloudflareComplexUpdate(t *testing.T) {
 	client := NewMockCloudFlareClientWithRecords(map[string][]cloudflare.DNSRecord{
 		"001": ExampleDomain,

provider/coredns/coredns.go

@@ -57,6 +57,7 @@ type coreDNSClient interface {
 }
 
 type coreDNSProvider struct {
+	provider.BaseProvider
 	dryRun        bool
 	coreDNSPrefix string
 	domainFilter  endpoint.DomainFilter

provider/designate/designate.go

@@ -227,6 +227,7 @@ func (c designateClient) DeleteRecordSet(zoneID, recordSetID string) error {
 
 // designate provider type
 type designateProvider struct {
+	provider.BaseProvider
 	client designateClientInterface
 
 	// only consider hosted zones managing domains ending in this suffix

provider/digitalocean/digital_ocean.go

@@ -45,6 +45,7 @@ const (
 
 // DigitalOceanProvider is an implementation of Provider for Digital Ocean's DNS.
 type DigitalOceanProvider struct {
+	provider.BaseProvider
 	Client godo.DomainsService
 	// only consider hosted zones managing domains ending in this suffix
 	domainFilter endpoint.DomainFilter

provider/dnsimple/dnsimple.go

@@ -77,6 +77,7 @@ func (z dnsimpleZoneService) UpdateRecord(ctx context.Context, accountID string,
 }
 
 type dnsimpleProvider struct {
+	provider.BaseProvider
 	client       dnsimpleZoneServiceInterface
 	identity     dnsimpleIdentityService
 	accountID    string

provider/dyn/dyn.go

@@ -104,6 +104,7 @@ func (snap *ZoneSnapshot) StoreRecordsForSerial(zone string, serial int, records
 
 // DynProvider is the actual interface impl.
 type dynProviderState struct {
+	provider.BaseProvider
 	DynConfig
 	LastLoginErrorTime int64
 

provider/exoscale/exoscale.go

@@ -25,6 +25,7 @@ import (
 
 	"sigs.k8s.io/external-dns/endpoint"
 	"sigs.k8s.io/external-dns/plan"
+	"sigs.k8s.io/external-dns/provider"
 )
 
 // EgoscaleClientI for replaceable implementation
@@ -38,6 +39,7 @@ type EgoscaleClientI interface {
 
 // ExoscaleProvider initialized as dns provider with no records
 type ExoscaleProvider struct {
+	provider.BaseProvider
 	domain         endpoint.DomainFilter
 	client         EgoscaleClientI
 	filter         *zoneFilter

provider/google/google.go

@@ -99,6 +99,7 @@ func (c changesService) Create(project string, managedZone string, change *dns.C
 
 // GoogleProvider is an implementation of Provider for Google CloudDNS.
 type GoogleProvider struct {
+	provider.BaseProvider
 	// The Google project to work in
 	project string
 	// Enabled dry-run will print any modifying actions rather than execute them.

provider/infoblox/infoblox.go

@@ -49,6 +49,7 @@ type InfobloxConfig struct {
 
 // InfobloxProvider implements the DNS provider for Infoblox.
 type InfobloxProvider struct {
+	provider.BaseProvider
 	client       ibclient.IBConnector
 	domainFilter endpoint.DomainFilter
 	zoneIDFilter provider.ZoneIDFilter