Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(providers): allow AdjustEndpoints to return error #3911

Merged
merged 1 commit into from
Sep 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion controller/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package controller

import (
"context"
"fmt"
"sync"
"time"

Expand Down Expand Up @@ -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{
Expand Down
4 changes: 2 additions & 2 deletions provider/aws/aws.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down
6 changes: 4 additions & 2 deletions provider/aws/aws_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down Expand Up @@ -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)
}

Expand Down
4 changes: 2 additions & 2 deletions provider/cloudflare/cloudflare.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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.
Expand Down
38 changes: 21 additions & 17 deletions provider/cloudflare/cloudflare_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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},
}
Expand Down
4 changes: 2 additions & 2 deletions provider/ibmcloud/ibmcloud.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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.
Expand Down
77 changes: 42 additions & 35 deletions provider/ibmcloud/ibmcloud_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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)
}
Expand All @@ -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)
Expand Down
6 changes: 3 additions & 3 deletions provider/infoblox/infoblox.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -403,7 +403,7 @@ func (p *ProviderConfig) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endp
}
}

return endpoints
return endpoints, nil
}

// ApplyChanges applies the given changes.
Expand Down
4 changes: 2 additions & 2 deletions provider/plural/plural.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
6 changes: 3 additions & 3 deletions provider/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
4 changes: 2 additions & 2 deletions provider/scaleway/scaleway.go
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -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.
Expand Down
3 changes: 2 additions & 1 deletion provider/scaleway/scaleway_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Expand Down
2 changes: 1 addition & 1 deletion registry/aws_sd_registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,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)
}
2 changes: 1 addition & 1 deletion registry/dynamodb.go
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,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)
}

Expand Down
2 changes: 1 addition & 1 deletion registry/noop.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,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)
}
2 changes: 1 addition & 1 deletion registry/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,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
}

Expand Down
2 changes: 1 addition & 1 deletion registry/txt.go
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,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)
}

Expand Down
Loading