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