Allow for normalizing any fields of desired endpoint

controller/controller.go

@@ -140,11 +140,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,
-		PropertyComparator: c.Registry.PropertyValuesEqual,
+		Policies:                 []plan.Policy{c.Policy},
+		Current:                  records,
+		Desired:                  endpoints,
+		DomainFilter:             c.DomainFilter,
+		NormalizeDesiredEndpoint: c.Registry.NormalizeDesiredEndpoint,
 	}
 
 	plan = plan.Calculate()

endpoint/endpoint.go

@@ -19,6 +19,7 @@ package endpoint
 import (
 	"fmt"
 	"sort"
+	"strconv"
 	"strings"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -191,10 +192,52 @@ func (e *Endpoint) GetProviderSpecificProperty(key string) (ProviderSpecificProp
 	return ProviderSpecificProperty{}, false
 }
 
+type NormalizeProviderSpecificConfig map[string]func(string) string
+
+// Selects and normalizes fields specified by config
+func (e *Endpoint) NormalizeProviderSpecific(config NormalizeProviderSpecificConfig) {
+	normalized := ProviderSpecific{}
+
+	for name, normalize := range config {
+		if property, ok := e.GetProviderSpecificProperty(name); ok {
+			normalized = append(normalized, ProviderSpecificProperty{
+				Name:  name,
+				Value: normalize(property.Value),
+			})
+		} else {
+			normalized = append(normalized, ProviderSpecificProperty{
+				Name:  name,
+				Value: normalize(""),
+			})
+		}
+	}
+
+	e.ProviderSpecific = normalized
+}
+
 func (e *Endpoint) String() string {
 	return fmt.Sprintf("%s %d IN %s %s %s %s", e.DNSName, e.RecordTTL, e.RecordType, e.SetIdentifier, e.Targets, e.ProviderSpecific)
 }
 
+// Normalizes boolean value of custom property
+func NormalizeBoolean(value string, defaultValue bool) string {
+	val, err := strconv.ParseBool(value)
+
+	if err != nil {
+		if defaultValue {
+			return "true"
+		} else {
+			return "false"
+		}
+	}
+
+	if val {
+		return "true"
+	}
+
+	return "false"
+}
+
 // DNSEndpointSpec defines the desired state of DNSEndpoint
 type DNSEndpointSpec struct {
 	Endpoints []*Endpoint `json:"endpoints,omitempty"`

plan/plan.go

@@ -18,15 +18,11 @@ package plan
 
 import (
 	"fmt"
-	"strconv"
 	"strings"
 
 	"sigs.k8s.io/external-dns/endpoint"
 )
 
-// PropertyComparator is used in Plan for comparing the previous and current custom annotations.
-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 {
@@ -41,8 +37,8 @@ type Plan struct {
 	Changes *Changes
 	// DomainFilter matches DNS names
 	DomainFilter endpoint.DomainFilter
-	// Property comparator compares custom properties of providers
-	PropertyComparator PropertyComparator
+	// Function normalizing desired endpoint (e.g. applying defaults)
+	NormalizeDesiredEndpoint func(endpoint *endpoint.Endpoint)
 }
 
 // Changes holds lists of actions to be executed by dns providers
@@ -71,12 +67,13 @@ bar.com |                | [->191.1.1.1, ->190.1.1.1]  |  = create (bar.com -> 1
 "=", i.e. result of calculation relies on supplied ConflictResolver
 */
 type planTable struct {
+	plan     *Plan
 	rows     map[string]map[string]*planTableRow
 	resolver ConflictResolver
 }
 
-func newPlanTable() planTable { //TODO: make resolver configurable
-	return planTable{map[string]map[string]*planTableRow{}, PerResource{}}
+func newPlanTable(p *Plan) planTable { //TODO: make resolver configurable
+	return planTable{p, map[string]map[string]*planTableRow{}, PerResource{}}
 }
 
 // planTableRow
@@ -102,8 +99,11 @@ func (t planTable) addCurrent(e *endpoint.Endpoint) {
 	t.rows[dnsName][e.SetIdentifier].current = e
 }
 
-func (t planTable) addCandidate(e *endpoint.Endpoint) {
+func (t planTable) addDesired(e *endpoint.Endpoint) {
 	dnsName := normalizeDNSName(e.DNSName)
+	if t.plan.NormalizeDesiredEndpoint != nil {
+		t.plan.NormalizeDesiredEndpoint(e)
+	}
 	if _, ok := t.rows[dnsName]; !ok {
 		t.rows[dnsName] = make(map[string]*planTableRow)
 	}
@@ -117,13 +117,13 @@ func (t planTable) addCandidate(e *endpoint.Endpoint) {
 // state. It then passes those changes to the current policy for further
 // processing. It returns a copy of Plan with the changes populated.
 func (p *Plan) Calculate() *Plan {
-	t := newPlanTable()
+	t := newPlanTable(p)
 
 	for _, current := range filterRecordsForPlan(p.Current, p.DomainFilter) {
 		t.addCurrent(current)
 	}
 	for _, desired := range filterRecordsForPlan(p.Desired, p.DomainFilter) {
-		t.addCandidate(desired)
+		t.addDesired(desired)
 	}
 
 	changes := &Changes{}
@@ -201,19 +201,11 @@ func (p *Plan) shouldUpdateProviderSpecific(desired, current *endpoint.Endpoint)
 			}
 
 			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 {
+				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 != "" {
+				if c.Value != "" {
 					return true
 				}
 			}
@@ -261,28 +253,3 @@ func normalizeDNSName(dnsName string) string {
 	}
 	return s
 }
-
-// CompareBoolean is an implementation of PropertyComparator for comparing boolean-line values
-// For example external-dns.alpha.kubernetes.io/cloudflare-proxied: "true"
-// If value doesn't parse as boolean, the defaultValue is used
-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 {
-			v1 = defaultValue
-		}
-	}
-
-	if current != "" {
-		v2, err = strconv.ParseBool(current)
-		if err != nil {
-			v2 = defaultValue
-		}
-	}
-
-	return v1 == v2
-}

plan/plan_test.go

@@ -313,8 +313,12 @@ func (suite *PlanTestSuite) TestSyncSecondRoundWithProviderSpecificDefaultFalse(
 		Policies: []Policy{&SyncPolicy{}},
 		Current:  current,
 		Desired:  desired,
-		PropertyComparator: func(name, previous, current string) bool {
-			return CompareBoolean(false, name, previous, current)
+		NormalizeDesiredEndpoint: func(e *endpoint.Endpoint) {
+			e.NormalizeProviderSpecific(endpoint.NormalizeProviderSpecificConfig{
+				"external-dns.alpha.kubernetes.io/cloudflare-proxied": func(value string) string {
+					return endpoint.NormalizeBoolean(value, false)
+				},
+			})
 		},
 	}
 
@@ -337,8 +341,12 @@ func (suite *PlanTestSuite) TestSyncSecondRoundWithProviderSpecificDefualtTrue()
 		Policies: []Policy{&SyncPolicy{}},
 		Current:  current,
 		Desired:  desired,
-		PropertyComparator: func(name, previous, current string) bool {
-			return CompareBoolean(true, name, previous, current)
+		NormalizeDesiredEndpoint: func(e *endpoint.Endpoint) {
+			e.NormalizeProviderSpecific(endpoint.NormalizeProviderSpecificConfig{
+				"external-dns.alpha.kubernetes.io/cloudflare-proxied": func(value string) string {
+					return endpoint.NormalizeBoolean(value, true)
+				},
+			})
 		},
 	}
 

provider/cloudflare/cloudflare.go

@@ -258,14 +258,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
@@ -386,6 +378,18 @@ func (p *CloudFlareProvider) newCloudFlareChange(action string, endpoint *endpoi
 	}
 }
 
+func (p *CloudFlareProvider) NormalizeDesiredEndpoint(e *endpoint.Endpoint) {
+	if shouldBeProxied(e, p.proxiedByDefault) {
+		e.RecordTTL = 0
+	}
+
+	e.NormalizeProviderSpecific(endpoint.NormalizeProviderSpecificConfig{
+		source.CloudflareProxiedKey: func(value string) string {
+			return endpoint.NormalizeBoolean(value, p.proxiedByDefault)
+		},
+	})
+}
+
 func shouldBeProxied(endpoint *endpoint.Endpoint, proxiedByDefault bool) bool {
 	proxied := proxiedByDefault
 

provider/cloudflare/cloudflare_test.go

@@ -250,9 +250,10 @@ func AssertActions(t *testing.T, provider *CloudFlareProvider, endpoints []*endp
 	}
 
 	plan := &plan.Plan{
-		Current:      records,
-		Desired:      endpoints,
-		DomainFilter: endpoint.NewDomainFilter([]string{"bar.com"}),
+		Current:                  records,
+		Desired:                  endpoints,
+		DomainFilter:             endpoint.NewDomainFilter([]string{"bar.com"}),
+		NormalizeDesiredEndpoint: provider.NormalizeDesiredEndpoint,
 	}
 
 	changes := plan.Calculate().Changes
@@ -1035,9 +1036,9 @@ func TestProviderPropertiesIdempotency(t *testing.T) {
 		}
 
 		plan := plan.Plan{
-			Current:            current,
-			Desired:            desired,
-			PropertyComparator: provider.PropertyValuesEqual,
+			Current:                  current,
+			Desired:                  desired,
+			NormalizeDesiredEndpoint: provider.NormalizeDesiredEndpoint,
 		}
 
 		plan = *plan.Calculate()
@@ -1091,7 +1092,8 @@ func TestCloudflareComplexUpdate(t *testing.T) {
 				},
 			},
 		},
-		DomainFilter: endpoint.NewDomainFilter([]string{"bar.com"}),
+		DomainFilter:             endpoint.NewDomainFilter([]string{"bar.com"}),
+		NormalizeDesiredEndpoint: provider.NormalizeDesiredEndpoint,
 	}
 
 	planned := plan.Calculate()
@@ -1133,3 +1135,57 @@ func TestCloudflareComplexUpdate(t *testing.T) {
 		},
 	})
 }
+
+func TestCustomTTLWithEnabeledProxyNotChanged(t *testing.T) {
+	client := NewMockCloudFlareClientWithRecords(map[string][]cloudflare.DNSRecord{
+		"001": []cloudflare.DNSRecord{
+			{
+				ID:      "1234567890",
+				ZoneID:  "001",
+				Name:    "foobar.bar.com",
+				Type:    endpoint.RecordTypeA,
+				TTL:     1,
+				Content: "1.2.3.4",
+				Proxied: true,
+			},
+		},
+	})
+
+	provider := &CloudFlareProvider{
+		Client: client,
+	}
+
+	records, err := provider.Records(context.Background())
+
+	if err != nil {
+		t.Errorf("should not fail, %s", err)
+	}
+
+	plan := &plan.Plan{
+		Current: records,
+		Desired: []*endpoint.Endpoint{
+			{
+				DNSName:    "foobar.bar.com",
+				Targets:    endpoint.Targets{"1.2.3.4"},
+				RecordType: endpoint.RecordTypeA,
+				RecordTTL:  300,
+				Labels:     endpoint.Labels{},
+				ProviderSpecific: endpoint.ProviderSpecific{
+					{
+						Name:  "external-dns.alpha.kubernetes.io/cloudflare-proxied",
+						Value: "true",
+					},
+				},
+			},
+		},
+		DomainFilter:             endpoint.NewDomainFilter([]string{"bar.com"}),
+		NormalizeDesiredEndpoint: provider.NormalizeDesiredEndpoint,
+	}
+
+	planned := plan.Calculate()
+
+	assert.Equal(t, 0, len(planned.Changes.Create), "no new changes should be here")
+	assert.Equal(t, 0, len(planned.Changes.UpdateNew), "no new changes should be here")
+	assert.Equal(t, 0, len(planned.Changes.UpdateOld), "no new changes should be here")
+	assert.Equal(t, 0, len(planned.Changes.Delete), "no new changes should be here")
+}

provider/provider.go

@@ -29,14 +29,13 @@ 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
+	NormalizeDesiredEndpoint(endpoint *endpoint.Endpoint)
 }
 
 type BaseProvider struct {
 }
 
-func (b BaseProvider) PropertyValuesEqual(name, previous, current string) bool {
-	return previous == current
+func (b BaseProvider) NormalizeDesiredEndpoint(endpoint *endpoint.Endpoint) {
 }
 
 type contextKey struct {

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

@@ -88,6 +88,7 @@ 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)
+// NormalizeDesiredEndpoint normalizes desired endpoint (e.g. applies defaults)
+func (sdr *AWSSDRegistry) NormalizeDesiredEndpoint(endpoint *endpoint.Endpoint) {
+	sdr.provider.NormalizeDesiredEndpoint(endpoint)
 }