Merge pull request #3911 from johngmyers/adjust-err

chore(providers): allow AdjustEndpoints to return error

controller/controller.go

@@ -18,6 +18,7 @@ package controller
 
 import (
 	"context"
+	"fmt"
 	"sync"
 	"time"
 
@@ -214,7 +215,10 @@ func (c *Controller) RunOnce(ctx context.Context) error {
 	vARecords, vAAAARecords := countMatchingAddressRecords(endpoints, records)
 	verifiedARecords.Set(float64(vARecords))
 	verifiedAAAARecords.Set(float64(vAAAARecords))
-	endpoints = c.Registry.AdjustEndpoints(endpoints)
+	endpoints, err = c.Registry.AdjustEndpoints(endpoints)
+	if err != nil {
+		return fmt.Errorf("adjusting endpoints: %w", err)
+	}
 	registryFilter := c.Registry.GetDomainFilter()
 
 	plan := &plan.Plan{

provider/aws/aws.go

@@ -660,7 +660,7 @@ func (p *AWSProvider) newChanges(action string, endpoints []*endpoint.Endpoint)
 // unneeded (potentially failing) changes.
 // Example: CNAME endpoints pointing to ELBs will have a `alias` provider-specific property
 // added to match the endpoints generated from existing alias records in Route53.
-func (p *AWSProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint {
+func (p *AWSProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) ([]*endpoint.Endpoint, error) {
 	for _, ep := range endpoints {
 		alias := false
 		if ep.RecordType != endpoint.RecordTypeCNAME {
@@ -692,7 +692,7 @@ func (p *AWSProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoin
 			ep.DeleteProviderSpecificProperty(providerSpecificEvaluateTargetHealth)
 		}
 	}
-	return endpoints
+	return endpoints, nil
 }
 
 // newChange returns a route53 Change and a boolean indicating if there should also be a change to a AAAA record

provider/aws/aws_test.go

@@ -529,7 +529,8 @@ func TestAWSAdjustEndpoints(t *testing.T) {
 		endpoint.NewEndpoint("cname-test-elb-no-eth.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeCNAME, "foo.eu-central-1.elb.amazonaws.com").WithProviderSpecific(providerSpecificEvaluateTargetHealth, "false"), // eth = evaluate target health
 	}
 
-	records = provider.AdjustEndpoints(records)
+	records, err := provider.AdjustEndpoints(records)
+	assert.NoError(t, err)
 
 	validateEndpoints(t, provider, records, []*endpoint.Endpoint{
 		endpoint.NewEndpoint("a-test.zone-1.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeA, "8.8.8.8"),
@@ -1610,7 +1611,8 @@ func TestAWSBatchChangeSetExceedingNameChange(t *testing.T) {
 func validateEndpoints(t *testing.T, provider *AWSProvider, endpoints []*endpoint.Endpoint, expected []*endpoint.Endpoint) {
 	assert.True(t, testutils.SameEndpoints(endpoints, expected), "actual and expected endpoints don't match. %+v:%+v", endpoints, expected)
 
-	normalized := provider.AdjustEndpoints(endpoints)
+	normalized, err := provider.AdjustEndpoints(endpoints)
+	assert.NoError(t, err)
 	assert.True(t, testutils.SameEndpoints(normalized, expected), "actual and normalized endpoints don't match. %+v:%+v", endpoints, normalized)
 }
 

provider/cloudflare/cloudflare.go

@@ -368,7 +368,7 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud
 }
 
 // AdjustEndpoints modifies the endpoints as needed by the specific provider
-func (p *CloudFlareProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint {
+func (p *CloudFlareProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) ([]*endpoint.Endpoint, error) {
 	adjustedEndpoints := []*endpoint.Endpoint{}
 	for _, e := range endpoints {
 		proxied := shouldBeProxied(e, p.proxiedByDefault)
@@ -379,7 +379,7 @@ func (p *CloudFlareProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*
 
 		adjustedEndpoints = append(adjustedEndpoints, e)
 	}
-	return adjustedEndpoints
+	return adjustedEndpoints, nil
 }
 
 // changesByZone separates a multi-zone change into a single change per zone.

provider/cloudflare/cloudflare_test.go

@@ -302,7 +302,8 @@ func AssertActions(t *testing.T, provider *CloudFlareProvider, endpoints []*endp
 		t.Fatalf("cannot fetch records, %s", err)
 	}
 
-	endpoints = provider.AdjustEndpoints(endpoints)
+	endpoints, err = provider.AdjustEndpoints(endpoints)
+	assert.NoError(t, err)
 	domainFilter := endpoint.NewDomainFilter([]string{"bar.com"})
 	plan := &plan.Plan{
 		Current:        records,
@@ -1147,7 +1148,8 @@ func TestProviderPropertiesIdempotency(t *testing.T) {
 				})
 			}
 
-			desired = provider.AdjustEndpoints(desired)
+			desired, err = provider.AdjustEndpoints(desired)
+			assert.NoError(t, err)
 
 			plan := plan.Plan{
 				Current:        current,
@@ -1190,23 +1192,25 @@ func TestCloudflareComplexUpdate(t *testing.T) {
 	}
 
 	domainFilter := endpoint.NewDomainFilter([]string{"bar.com"})
-	plan := &plan.Plan{
-		Current: records,
-		Desired: provider.AdjustEndpoints([]*endpoint.Endpoint{
-			{
-				DNSName:    "foobar.bar.com",
-				Targets:    endpoint.Targets{"1.2.3.4", "2.3.4.5"},
-				RecordType: endpoint.RecordTypeA,
-				RecordTTL:  endpoint.TTL(defaultCloudFlareRecordTTL),
-				Labels:     endpoint.Labels{},
-				ProviderSpecific: endpoint.ProviderSpecific{
-					{
-						Name:  "external-dns.alpha.kubernetes.io/cloudflare-proxied",
-						Value: "true",
-					},
+	endpoints, err := provider.AdjustEndpoints([]*endpoint.Endpoint{
+		{
+			DNSName:    "foobar.bar.com",
+			Targets:    endpoint.Targets{"1.2.3.4", "2.3.4.5"},
+			RecordType: endpoint.RecordTypeA,
+			RecordTTL:  endpoint.TTL(defaultCloudFlareRecordTTL),
+			Labels:     endpoint.Labels{},
+			ProviderSpecific: endpoint.ProviderSpecific{
+				{
+					Name:  "external-dns.alpha.kubernetes.io/cloudflare-proxied",
+					Value: "true",
 				},
 			},
-		}),
+		},
+	})
+	assert.NoError(t, err)
+	plan := &plan.Plan{
+		Current:        records,
+		Desired:        endpoints,
 		DomainFilter:   endpoint.MatchAllDomainFilters{&domainFilter},
 		ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME},
 	}

provider/ibmcloud/ibmcloud.go

@@ -386,7 +386,7 @@ func (p *IBMCloudProvider) ApplyChanges(ctx context.Context, changes *plan.Chang
 }
 
 // AdjustEndpoints modifies the endpoints as needed by the specific provider
-func (p *IBMCloudProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint {
+func (p *IBMCloudProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) ([]*endpoint.Endpoint, error) {
 	adjustedEndpoints := []*endpoint.Endpoint{}
 	for _, e := range endpoints {
 		log.Debugf("adjusting endpont: %v", *e)
@@ -398,7 +398,7 @@ func (p *IBMCloudProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*en
 
 		adjustedEndpoints = append(adjustedEndpoints, e)
 	}
-	return adjustedEndpoints
+	return adjustedEndpoints, nil
 }
 
 // submitChanges takes a zone and a collection of Changes and sends them as a single transaction.

provider/ibmcloud/ibmcloud_test.go

@@ -276,33 +276,46 @@ func TestPublic_ApplyChanges(t *testing.T) {
 func TestPrivate_ApplyChanges(t *testing.T) {
 	p := newTestIBMCloudProvider(true)
 
-	changes := plan.Changes{
-		Create: p.AdjustEndpoints([]*endpoint.Endpoint{
-			{
-				DNSName:    "newA.example.com",
-				RecordType: "A",
-				RecordTTL:  120,
-				Targets:    endpoint.NewTargets("4.3.2.1"),
-				ProviderSpecific: endpoint.ProviderSpecific{
-					{
-						Name:  "ibmcloud-vpc",
-						Value: "crn:v1:staging:public:is:us-south:a/0821fa9f9ebcc7b7c9a0d6e9bf9442a4::vpc:be33cdad-9a03-4bfa-82ca-eadb9f1de688",
-					},
+	endpointsCreate, err := p.AdjustEndpoints([]*endpoint.Endpoint{
+		{
+			DNSName:    "newA.example.com",
+			RecordType: "A",
+			RecordTTL:  120,
+			Targets:    endpoint.NewTargets("4.3.2.1"),
+			ProviderSpecific: endpoint.ProviderSpecific{
+				{
+					Name:  "ibmcloud-vpc",
+					Value: "crn:v1:staging:public:is:us-south:a/0821fa9f9ebcc7b7c9a0d6e9bf9442a4::vpc:be33cdad-9a03-4bfa-82ca-eadb9f1de688",
 				},
 			},
-			{
-				DNSName:    "newCNAME.example.com",
-				RecordType: "CNAME",
-				RecordTTL:  180,
-				Targets:    endpoint.NewTargets("newA.example.com"),
-			},
-			{
-				DNSName:    "newTXT.example.com",
-				RecordType: "TXT",
-				RecordTTL:  240,
-				Targets:    endpoint.NewTargets("\"heritage=external-dns,external-dns/owner=tower-pdns\""),
-			},
-		}),
+		},
+		{
+			DNSName:    "newCNAME.example.com",
+			RecordType: "CNAME",
+			RecordTTL:  180,
+			Targets:    endpoint.NewTargets("newA.example.com"),
+		},
+		{
+			DNSName:    "newTXT.example.com",
+			RecordType: "TXT",
+			RecordTTL:  240,
+			Targets:    endpoint.NewTargets("\"heritage=external-dns,external-dns/owner=tower-pdns\""),
+		},
+	})
+	assert.NoError(t, err)
+
+	endpointsUpdate, err := p.AdjustEndpoints([]*endpoint.Endpoint{
+		{
+			DNSName:    "test.example.com",
+			RecordType: "A",
+			RecordTTL:  180,
+			Targets:    endpoint.NewTargets("1.2.3.4", "5.6.7.8"),
+		},
+	})
+	assert.NoError(t, err)
+
+	changes := plan.Changes{
+		Create: endpointsCreate,
 		UpdateOld: []*endpoint.Endpoint{
 			{
 				DNSName:    "test.example.com",
@@ -311,14 +324,7 @@ func TestPrivate_ApplyChanges(t *testing.T) {
 				Targets:    endpoint.NewTargets("1.2.3.4"),
 			},
 		},
-		UpdateNew: p.AdjustEndpoints([]*endpoint.Endpoint{
-			{
-				DNSName:    "test.example.com",
-				RecordType: "A",
-				RecordTTL:  180,
-				Targets:    endpoint.NewTargets("1.2.3.4", "5.6.7.8"),
-			},
-		}),
+		UpdateNew: endpointsUpdate,
 		Delete: []*endpoint.Endpoint{
 			{
 				DNSName:    "test.example.com",
@@ -329,7 +335,7 @@ func TestPrivate_ApplyChanges(t *testing.T) {
 		},
 	}
 	ctx := context.Background()
-	err := p.ApplyChanges(ctx, &changes)
+	err = p.ApplyChanges(ctx, &changes)
 	if err != nil {
 		t.Errorf("should not fail, %s", err)
 	}
@@ -353,7 +359,8 @@ func TestAdjustEndpoints(t *testing.T) {
 		},
 	}
 
-	ep := p.AdjustEndpoints(endpoints)
+	ep, err := p.AdjustEndpoints(endpoints)
+	assert.NoError(t, err)
 
 	assert.Equal(t, endpoint.TTL(0), ep[0].RecordTTL)
 	assert.Equal(t, "test.example.com", ep[0].DNSName)

provider/infoblox/infoblox.go

@@ -376,14 +376,14 @@ func (p *ProviderConfig) Records(ctx context.Context) (endpoints []*endpoint.End
 	return endpoints, nil
 }
 
-func (p *ProviderConfig) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint {
+func (p *ProviderConfig) AdjustEndpoints(endpoints []*endpoint.Endpoint) ([]*endpoint.Endpoint, error) {
 	// Update user specified TTL (0 == disabled)
 	for i := range endpoints {
 		endpoints[i].RecordTTL = endpoint.TTL(p.cacheDuration)
 	}
 
 	if !p.createPTR {
-		return endpoints
+		return endpoints, nil
 	}
 
 	// for all A records, we want to create PTR records
@@ -403,7 +403,7 @@ func (p *ProviderConfig) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endp
 		}
 	}
 
-	return endpoints
+	return endpoints, nil
 }
 
 // ApplyChanges applies the given changes.

provider/plural/plural.go

@@ -83,8 +83,8 @@ func (p *PluralProvider) Records(_ context.Context) (endpoints []*endpoint.Endpo
 	return
 }
 
-func (p *PluralProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint {
-	return endpoints
+func (p *PluralProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) ([]*endpoint.Endpoint, error) {
+	return endpoints, nil
 }
 
 func (p *PluralProvider) ApplyChanges(_ context.Context, diffs *plan.Changes) error {

provider/provider.go

@@ -36,14 +36,14 @@ type Provider interface {
 	// the endpoints that the provider returns in `Records` so that the change plan will not have
 	// unnecessary (potentially failing) changes. It may also modify other fields, add, or remove
 	// Endpoints. It is permitted to modify the supplied endpoints.
-	AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint
+	AdjustEndpoints(endpoints []*endpoint.Endpoint) ([]*endpoint.Endpoint, error)
 	GetDomainFilter() endpoint.DomainFilter
 }
 
 type BaseProvider struct{}
 
-func (b BaseProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint {
-	return endpoints
+func (b BaseProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) ([]*endpoint.Endpoint, error) {
+	return endpoints, nil
 }
 
 func (b BaseProvider) GetDomainFilter() endpoint.DomainFilter {

provider/scaleway/scaleway.go

@@ -92,7 +92,7 @@ func NewScalewayProvider(ctx context.Context, domainFilter endpoint.DomainFilter
 }
 
 // AdjustEndpoints is used to normalize the endoints
-func (p *ScalewayProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint {
+func (p *ScalewayProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) ([]*endpoint.Endpoint, error) {
 	eps := make([]*endpoint.Endpoint, len(endpoints))
 	for i := range endpoints {
 		eps[i] = endpoints[i]
@@ -103,7 +103,7 @@ func (p *ScalewayProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*en
 			eps[i] = eps[i].WithProviderSpecific(scalewayPriorityKey, fmt.Sprintf("%d", scalewayDefaultPriority))
 		}
 	}
-	return eps
+	return eps, nil
 }
 
 // Zones returns the list of hosted zones.

provider/scaleway/scaleway_test.go

@@ -220,7 +220,8 @@ func TestScalewayProvider_AdjustEndpoints(t *testing.T) {
 		},
 	}
 
-	after := provider.AdjustEndpoints(before)
+	after, err := provider.AdjustEndpoints(before)
+	assert.NoError(t, err)
 	for i := range after {
 		if !checkRecordEquality(after[i], expected[i]) {
 			t.Errorf("got record %s instead of %s", after[i], expected[i])

registry/aws_sd_registry.go

@@ -100,6 +100,6 @@ func (sdr *AWSSDRegistry) updateLabels(endpoints []*endpoint.Endpoint) {
 }
 
 // AdjustEndpoints modifies the endpoints as needed by the specific provider
-func (sdr *AWSSDRegistry) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint {
+func (sdr *AWSSDRegistry) AdjustEndpoints(endpoints []*endpoint.Endpoint) ([]*endpoint.Endpoint, error) {
 	return sdr.provider.AdjustEndpoints(endpoints)
 }

registry/dynamodb.go

@@ -337,7 +337,7 @@ func (im *DynamoDBRegistry) ApplyChanges(ctx context.Context, changes *plan.Chan
 }
 
 // AdjustEndpoints modifies the endpoints as needed by the specific provider.
-func (im *DynamoDBRegistry) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint {
+func (im *DynamoDBRegistry) AdjustEndpoints(endpoints []*endpoint.Endpoint) ([]*endpoint.Endpoint, error) {
 	return im.provider.AdjustEndpoints(endpoints)
 }
 

registry/noop.go

@@ -55,6 +55,6 @@ func (im *NoopRegistry) ApplyChanges(ctx context.Context, changes *plan.Changes)
 }
 
 // AdjustEndpoints modifies the endpoints as needed by the specific provider
-func (im *NoopRegistry) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint {
+func (im *NoopRegistry) AdjustEndpoints(endpoints []*endpoint.Endpoint) ([]*endpoint.Endpoint, error) {
 	return im.provider.AdjustEndpoints(endpoints)
 }

registry/registry.go

@@ -30,7 +30,7 @@ import (
 type Registry interface {
 	Records(ctx context.Context) ([]*endpoint.Endpoint, error)
 	ApplyChanges(ctx context.Context, changes *plan.Changes) error
-	AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint
+	AdjustEndpoints(endpoints []*endpoint.Endpoint) ([]*endpoint.Endpoint, error)
 	GetDomainFilter() endpoint.DomainFilter
 	OwnerID() string
 }

registry/txt.go

@@ -296,7 +296,7 @@ func (im *TXTRegistry) ApplyChanges(ctx context.Context, changes *plan.Changes)
 }
 
 // AdjustEndpoints modifies the endpoints as needed by the specific provider
-func (im *TXTRegistry) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint {
+func (im *TXTRegistry) AdjustEndpoints(endpoints []*endpoint.Endpoint) ([]*endpoint.Endpoint, error) {
 	return im.provider.AdjustEndpoints(endpoints)
 }