Add new method to provider interface to implement provider specific c…

…hanges (#1868) * adds tests for shouldUpdateProviderSpecific Signed-off-by: Raffaele Di Fazio <difazio.raffaele@gmail.com> * move AWS health to where it belongs Signed-off-by: Raffaele Di Fazio <difazio.raffaele@gmail.com> * add test that breaks things Signed-off-by: Raffaele Di Fazio <difazio.raffaele@gmail.com> * adds adjustendpoints method Signed-off-by: Raffaele Di Fazio <raffo@github.com> * fix controller Signed-off-by: Raffaele Di Fazio <raffo@github.com> * actually pass the provider where needed Signed-off-by: Raffaele Di Fazio <raffo@github.com> * OMG goland do your go fmt thing Signed-off-by: Raffaele Di Fazio <raffo@github.com> * use registry as proxy Signed-off-by: Raffaele Di Fazio <raffo@github.com> * make linter happy Signed-off-by: Raffaele Di Fazio <raffo@github.com> * change AdjustEndpoints signature Signed-off-by: Raffaele Di Fazio <raffo@github.com> * fix typo Signed-off-by: Raffaele Di Fazio <raffo@github.com> * actually use adjusted endpoints Signed-off-by: Raffaele Di Fazio <raffo@github.com> * revert cloudflare change Signed-off-by: Raffaele Di Fazio <raffo@github.com> * Update provider/cloudflare/cloudflare.go Co-authored-by: Nick Jüttner <nick@juni.io> Co-authored-by: Nick Jüttner <nick@juni.io>
kubernetes-sigs · Dec 10, 2020 · f5aa1c4 · f5aa1c4
1 parent 9d2aaf0
commit f5aa1c4
Show file tree

Hide file tree

Showing 14 changed files with 285 additions and 19 deletions.
diff --git a/controller/controller.go b/controller/controller.go
@@ -139,6 +139,8 @@ func (c *Controller) RunOnce(ctx context.Context) error {
 	}
 	sourceEndpointsTotal.Set(float64(len(endpoints)))
 
+	endpoints = c.Registry.AdjustEndpoints(endpoints)
+
 	plan := &plan.Plan{
 		Policies:           []plan.Policy{c.Policy},
 		Current:            records,

diff --git a/plan/plan.go b/plan/plan.go
@@ -194,12 +194,6 @@ func (p *Plan) shouldUpdateProviderSpecific(desired, current *endpoint.Endpoint)
 	}
 	if current.ProviderSpecific != nil {
 		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
-			}
-
 			if d, ok := desiredProperties[c.Name]; ok {
 				if p.PropertyComparator != nil {
 					if !p.PropertyComparator(c.Name, c.Value, d.Value) {

diff --git a/plan/plan_test.go b/plan/plan_test.go
@@ -686,3 +686,132 @@ func TestNormalizeDNSName(t *testing.T) {
 		assert.Equal(t, r.expect, gotName)
 	}
 }
+
+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: "skip AWS target health",
+			current: &endpoint.Endpoint{
+				DNSName: "foo.com",
+				ProviderSpecific: []endpoint.ProviderSpecificProperty{
+					{Name: "aws/evaluate-target-health", Value: "true"},
+				},
+			},
+			desired: &endpoint.Endpoint{
+				DNSName: "bar.com",
+				ProviderSpecific: []endpoint.ProviderSpecificProperty{
+					{Name: "aws/evaluate-target-health", Value: "true"},
+				},
+			},
+			propertyComparator: comparator,
+			shouldUpdate:       false,
+		},
+		{
+			name: "custom property unchanged",
+			current: &endpoint.Endpoint{
+				ProviderSpecific: []endpoint.ProviderSpecificProperty{
+					{Name: "custom/property", Value: "true"},
+				},
+			},
+			desired: &endpoint.Endpoint{
+				ProviderSpecific: []endpoint.ProviderSpecificProperty{
+					{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",
+			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",
+			current: &endpoint.Endpoint{
+				ProviderSpecific: []endpoint.ProviderSpecificProperty{
+					{Name: "custom/property", Value: "true"},
+				},
+			},
+			desired: &endpoint.Endpoint{
+				ProviderSpecific: []endpoint.ProviderSpecificProperty{
+					{Name: "new/property", Value: "true"},
+				},
+			},
+			shouldUpdate: true,
+		},
+	} {
+		tt.Run(test.name, func(t *testing.T) {
+			plan := &Plan{
+				Current:            []*endpoint.Endpoint{test.current},
+				Desired:            []*endpoint.Endpoint{test.desired},
+				PropertyComparator: test.propertyComparator,
+			}
+			b := plan.shouldUpdateProviderSpecific(test.desired, test.current)
+			assert.Equal(t, test.shouldUpdate, b)
+
+		})
+	}
+}
diff --git a/provider/aws/aws.go b/provider/aws/aws.go
@@ -205,6 +205,13 @@ func NewAWSProvider(awsConfig AWSConfig) (*AWSProvider, error) {
 	return provider, nil
 }
 
+func (p *AWSProvider) PropertyValuesEqual(name string, previous string, current string) bool {
+	if name == "aws/evaluate-target-health" {
+		return true
+	}
+	return p.BaseProvider.PropertyValuesEqual(name, previous, current)
+}
+
 // Zones returns the list of hosted zones.
 func (p *AWSProvider) Zones(ctx context.Context) (map[string]*route53.HostedZone, error) {
 	if p.zonesCache.zones != nil && time.Since(p.zonesCache.age) < p.zonesCache.duration {

diff --git a/provider/aws/aws_test.go b/provider/aws/aws_test.go
@@ -1115,6 +1115,50 @@ func TestAWSSuitableZones(t *testing.T) {
 	}
 }
 
+func TestAWSHealthTargetAnnotation(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: "skip AWS target health",
+			current: &endpoint.Endpoint{
+				RecordType: "A",
+				DNSName:    "foo.com",
+				ProviderSpecific: []endpoint.ProviderSpecificProperty{
+					{Name: "aws/evaluate-target-health", Value: "true"},
+				},
+			},
+			desired: &endpoint.Endpoint{
+				DNSName:    "foo.com",
+				RecordType: "A",
+				ProviderSpecific: []endpoint.ProviderSpecificProperty{
+					{Name: "aws/evaluate-target-health", Value: "false"},
+				},
+			},
+			propertyComparator: comparator,
+			shouldUpdate:       false,
+		},
+	} {
+		tt.Run(test.name, func(t *testing.T) {
+			provider := &AWSProvider{}
+			plan := &plan.Plan{
+				Current:            []*endpoint.Endpoint{test.current},
+				Desired:            []*endpoint.Endpoint{test.desired},
+				PropertyComparator: provider.PropertyValuesEqual,
+			}
+			plan = plan.Calculate()
+			assert.Equal(t, test.shouldUpdate, len(plan.Changes.UpdateNew) == 1)
+		})
+	}
+}
+
 func createAWSZone(t *testing.T, provider *AWSProvider, zone *route53.HostedZone) {
 	params := &route53.CreateHostedZoneInput{
 		CallerReference:  aws.String("external-dns.alpha.kubernetes.io/test-zone"),

diff --git a/provider/cloudflare/cloudflare.go b/provider/cloudflare/cloudflare.go
@@ -331,6 +331,18 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud
 	return nil
 }
 
+// AdjustEndpoints modifies the endpoints as needed by the specific provider
+func (p *CloudFlareProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint {
+	adjustedEndpoints := []*endpoint.Endpoint{}
+	for _, e := range endpoints {
+		if shouldBeProxied(e, p.proxiedByDefault) {
+			e.RecordTTL = 0
+		}
+		adjustedEndpoints = append(adjustedEndpoints, e)
+	}
+	return adjustedEndpoints
+}
+
 // changesByZone separates a multi-zone change into a single change per zone.
 func (p *CloudFlareProvider) changesByZone(zones []cloudflare.Zone, changeSet []*cloudFlareChange) map[string][]*cloudFlareChange {
 	changes := make(map[string][]*cloudFlareChange)

diff --git a/provider/cloudflare/cloudflare_test.go b/provider/cloudflare/cloudflare_test.go
@@ -1133,3 +1133,60 @@ func TestCloudflareComplexUpdate(t *testing.T) {
 		},
 	})
 }
+
+func TestCustomTTLWithEnabledProxyNotChanged(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)
+	}
+
+	endpoints := []*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",
+				},
+			},
+		},
+	}
+
+	provider.AdjustEndpoints(endpoints)
+
+	plan := &plan.Plan{
+		Current:      records,
+		Desired:      endpoints,
+		DomainFilter: endpoint.NewDomainFilter([]string{"bar.com"}),
+	}
+
+	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")
+}
diff --git a/provider/provider.go b/provider/provider.go
@@ -30,11 +30,16 @@ 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(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint
 }
 
 type BaseProvider struct {
 }
 
+func (b BaseProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint {
+	return endpoints
+}
+
 func (b BaseProvider) PropertyValuesEqual(name, previous, current string) bool {
 	return previous == current
 }

diff --git a/registry/aws_sd_registry.go b/registry/aws_sd_registry.go
@@ -91,3 +91,8 @@ 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/noop.go b/registry/noop.go
@@ -50,3 +50,8 @@ func (im *NoopRegistry) ApplyChanges(ctx context.Context, changes *plan.Changes)
 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
@@ -33,6 +33,7 @@ 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
 }
 
 //TODO(ideahitme): consider moving this to Plan

diff --git a/registry/txt.go b/registry/txt.go
@@ -208,6 +208,11 @@ func (im *TXTRegistry) PropertyValuesEqual(name string, previous string, current
 	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)
+}
+
 /**
   TXT registry specific private methods
 */

diff --git a/source/ingress_test.go b/source/ingress_test.go
@@ -997,31 +997,31 @@ func testIngressEndpoints(t *testing.T) {
 			},
 		},
 		{
-			title:                    "ignore tls section",
-			targetNamespace:          "",
-			ignoreIngressTLSSpec:     true,
+			title:                "ignore tls section",
+			targetNamespace:      "",
+			ignoreIngressTLSSpec: true,
 			ingressItems: []fakeIngress{
 				{
-					name:      "fake1",
-					namespace: namespace,
+					name:        "fake1",
+					namespace:   namespace,
 					tlsdnsnames: [][]string{{"example.org"}},
 					ips:         []string{"1.2.3.4"},
 				},
-		  },
+			},
 			expected: []*endpoint.Endpoint{},
 		},
 		{
-			title:                    "reading tls section",
-			targetNamespace:          "",
-			ignoreIngressTLSSpec:     false,
+			title:                "reading tls section",
+			targetNamespace:      "",
+			ignoreIngressTLSSpec: false,
 			ingressItems: []fakeIngress{
 				{
-					name:      "fake1",
-					namespace: namespace,
+					name:        "fake1",
+					namespace:   namespace,
 					tlsdnsnames: [][]string{{"example.org"}},
 					ips:         []string{"1.2.3.4"},
 				},
-		  },
+			},
 			expected: []*endpoint.Endpoint{
 				{
 					DNSName: "example.org",