diff --git a/endpoint/endpoint.go b/endpoint/endpoint.go index 99b6d3f36e..9c2c2138f3 100644 --- a/endpoint/endpoint.go +++ b/endpoint/endpoint.go @@ -222,11 +222,7 @@ func (e *Endpoint) WithSetIdentifier(setIdentifier string) *Endpoint { // warrant its own field on the Endpoint object itself. It differs from Labels in the fact that it's // not persisted in the Registry but only kept in memory during a single record synchronization. func (e *Endpoint) WithProviderSpecific(key, value string) *Endpoint { - if e.ProviderSpecific == nil { - e.ProviderSpecific = ProviderSpecific{} - } - - e.ProviderSpecific = append(e.ProviderSpecific, ProviderSpecificProperty{Name: key, Value: value}) + e.SetProviderSpecificProperty(key, value) return e } @@ -240,6 +236,30 @@ func (e *Endpoint) GetProviderSpecificProperty(key string) (string, bool) { return "", false } +// SetProviderSpecificProperty sets the value of a ProviderSpecificProperty. +func (e *Endpoint) SetProviderSpecificProperty(key string, value string) { + for i, providerSpecific := range e.ProviderSpecific { + if providerSpecific.Name == key { + e.ProviderSpecific[i] = ProviderSpecificProperty{ + Name: key, + Value: value, + } + return + } + } + + e.ProviderSpecific = append(e.ProviderSpecific, ProviderSpecificProperty{Name: key, Value: value}) +} + +func (e *Endpoint) DeleteProviderSpecificProperty(key string) { + for i, providerSpecific := range e.ProviderSpecific { + if providerSpecific.Name == key { + e.ProviderSpecific = append(e.ProviderSpecific[:i], e.ProviderSpecific[i+1:]...) + return + } + } +} + 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) } diff --git a/provider/aws/aws.go b/provider/aws/aws.go index ac23eb7b8f..be1997c7ed 100644 --- a/provider/aws/aws.go +++ b/provider/aws/aws.go @@ -47,10 +47,12 @@ const ( // As we are using the standard AWS client, this should already be compliant. // Hence, ifever AWS decides to raise this limit, we will automatically reduce the pressure on rate limits route53PageSize = "300" - // provider specific key that designates whether an AWS ALIAS record has the EvaluateTargetHealth - // field set to true. - providerSpecificAlias = "alias" - providerSpecificTargetHostedZone = "aws/target-hosted-zone" + // providerSpecificAlias specifies whether a CNAME endpoint maps to an AWS ALIAS record. + providerSpecificAlias = "alias" + providerSpecificTargetHostedZone = "aws/target-hosted-zone" + // providerSpecificEvaluateTargetHealth specifies whether an AWS ALIAS record + // has the EvaluateTargetHealth field set to true. Present iff the endpoint + // has a `providerSpecificAlias` value of `true`. providerSpecificEvaluateTargetHealth = "aws/evaluate-target-health" providerSpecificWeight = "aws/weight" providerSpecificRegion = "aws/region" @@ -283,13 +285,6 @@ 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 { @@ -390,7 +385,11 @@ func (p *AWSProvider) records(ctx context.Context, zones map[string]*route53.Hos targets[idx] = aws.StringValue(rr.Value) } - newEndpoints = append(newEndpoints, endpoint.NewEndpointWithTTL(wildcardUnescape(aws.StringValue(r.Name)), aws.StringValue(r.Type), ttl, targets...)) + ep := endpoint.NewEndpointWithTTL(wildcardUnescape(aws.StringValue(r.Name)), aws.StringValue(r.Type), ttl, targets...) + if aws.StringValue(r.Type) == endpoint.RecordTypeCNAME { + ep = ep.WithProviderSpecific(providerSpecificAlias, "false") + } + newEndpoints = append(newEndpoints, ep) } if r.AliasTarget != nil { @@ -466,8 +465,12 @@ func (p *AWSProvider) requiresDeleteCreate(old *endpoint.Endpoint, new *endpoint } // an ALIAS record change to/from a CNAME - if old.RecordType == endpoint.RecordTypeCNAME && useAlias(old, p.preferCNAME) != useAlias(new, p.preferCNAME) { - return true + if old.RecordType == endpoint.RecordTypeCNAME { + oldAlias, _ := old.GetProviderSpecificProperty(providerSpecificAlias) + newAlias, _ := new.GetProviderSpecificProperty(providerSpecificAlias) + if oldAlias != newAlias { + return true + } } // a set identifier change @@ -667,23 +670,29 @@ func (p *AWSProvider) newChanges(action string, endpoints []*endpoint.Endpoint) func (p *AWSProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint { for _, ep := range endpoints { alias := false - if aliasString, ok := ep.GetProviderSpecificProperty(providerSpecificAlias); ok { + if ep.RecordType != endpoint.RecordTypeCNAME { + ep.DeleteProviderSpecificProperty(providerSpecificAlias) + } else if aliasString, ok := ep.GetProviderSpecificProperty(providerSpecificAlias); ok { alias = aliasString == "true" - } else if useAlias(ep, p.preferCNAME) { - alias = true - log.Debugf("Modifying endpoint: %v, setting %s=true", ep, providerSpecificAlias) - ep.ProviderSpecific = append(ep.ProviderSpecific, endpoint.ProviderSpecificProperty{ - Name: providerSpecificAlias, - Value: "true", - }) + if !alias && aliasString != "false" { + ep.SetProviderSpecificProperty(providerSpecificAlias, "false") + } + } else { + alias = useAlias(ep, p.preferCNAME) + log.Debugf("Modifying endpoint: %v, setting %s=%v", ep, providerSpecificAlias, alias) + ep.SetProviderSpecificProperty(providerSpecificAlias, strconv.FormatBool(alias)) } - if _, ok := ep.GetProviderSpecificProperty(providerSpecificEvaluateTargetHealth); alias && !ok { - log.Debugf("Modifying endpoint: %v, setting %s=%t", ep, providerSpecificEvaluateTargetHealth, p.evaluateTargetHealth) - ep.ProviderSpecific = append(ep.ProviderSpecific, endpoint.ProviderSpecificProperty{ - Name: providerSpecificEvaluateTargetHealth, - Value: fmt.Sprintf("%t", p.evaluateTargetHealth), - }) + if alias { + if prop, ok := ep.GetProviderSpecificProperty(providerSpecificEvaluateTargetHealth); ok { + if prop != "true" && prop != "false" { + ep.SetProviderSpecificProperty(providerSpecificEvaluateTargetHealth, "false") + } + } else { + ep.SetProviderSpecificProperty(providerSpecificEvaluateTargetHealth, strconv.FormatBool(p.evaluateTargetHealth)) + } + } else { + ep.DeleteProviderSpecificProperty(providerSpecificEvaluateTargetHealth) } } return endpoints diff --git a/provider/aws/aws_test.go b/provider/aws/aws_test.go index 3a3fbf2581..4ed172d98b 100644 --- a/provider/aws/aws_test.go +++ b/provider/aws/aws_test.go @@ -494,7 +494,7 @@ func TestAWSRecords(t *testing.T) { records, err := provider.Records(context.Background()) require.NoError(t, err) - validateEndpoints(t, records, []*endpoint.Endpoint{ + validateEndpoints(t, provider, records, []*endpoint.Endpoint{ endpoint.NewEndpointWithTTL("list-test.zone-1.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeA, endpoint.TTL(recordTTL), "1.2.3.4"), endpoint.NewEndpointWithTTL("list-test.zone-2.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeA, endpoint.TTL(recordTTL), "8.8.8.8"), endpoint.NewEndpointWithTTL("*.wildcard-test.zone-2.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeA, endpoint.TTL(recordTTL), "8.8.8.8"), @@ -511,7 +511,7 @@ func TestAWSRecords(t *testing.T) { endpoint.NewEndpointWithTTL("geolocation-test.zone-1.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeA, endpoint.TTL(recordTTL), "1.2.3.4").WithSetIdentifier("test-set-1").WithProviderSpecific(providerSpecificGeolocationContinentCode, "EU"), endpoint.NewEndpointWithTTL("geolocation-test.zone-1.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeA, endpoint.TTL(recordTTL), "4.3.2.1").WithSetIdentifier("test-set-2").WithProviderSpecific(providerSpecificGeolocationCountryCode, "DE"), endpoint.NewEndpointWithTTL("geolocation-subdivision-test.zone-1.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeA, endpoint.TTL(recordTTL), "1.2.3.4").WithSetIdentifier("test-set-1").WithProviderSpecific(providerSpecificGeolocationSubdivisionCode, "NY"), - endpoint.NewEndpointWithTTL("healthcheck-test.zone-1.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeCNAME, endpoint.TTL(recordTTL), "foo.example.com").WithSetIdentifier("test-set-1").WithProviderSpecific(providerSpecificWeight, "10").WithProviderSpecific(providerSpecificHealthCheckID, "foo-bar-healthcheck-id"), + endpoint.NewEndpointWithTTL("healthcheck-test.zone-1.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeCNAME, endpoint.TTL(recordTTL), "foo.example.com").WithSetIdentifier("test-set-1").WithProviderSpecific(providerSpecificWeight, "10").WithProviderSpecific(providerSpecificHealthCheckID, "foo-bar-healthcheck-id").WithProviderSpecific(providerSpecificAlias, "false"), endpoint.NewEndpointWithTTL("healthcheck-test.zone-1.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeA, endpoint.TTL(recordTTL), "4.3.2.1").WithSetIdentifier("test-set-2").WithProviderSpecific(providerSpecificWeight, "20").WithProviderSpecific(providerSpecificHealthCheckID, "abc-def-healthcheck-id"), endpoint.NewEndpointWithTTL("mail.zone-1.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeMX, endpoint.TTL(recordTTL), "10 mailhost1.example.com", "20 mailhost2.example.com"), }) @@ -529,11 +529,11 @@ 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 } - provider.AdjustEndpoints(records) + records = provider.AdjustEndpoints(records) - validateEndpoints(t, records, []*endpoint.Endpoint{ + 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"), - endpoint.NewEndpoint("cname-test.zone-1.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeCNAME, "foo.example.com"), + endpoint.NewEndpoint("cname-test.zone-1.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeCNAME, "foo.example.com").WithProviderSpecific(providerSpecificAlias, "false"), endpoint.NewEndpoint("cname-test-alias.zone-1.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeCNAME, "alias-target.zone-2.ext-dns-test-2.teapot.zalan.do").WithProviderSpecific(providerSpecificAlias, "true").WithProviderSpecific(providerSpecificEvaluateTargetHealth, "true"), endpoint.NewEndpoint("cname-test-elb.zone-2.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeCNAME, "foo.eu-central-1.elb.amazonaws.com").WithProviderSpecific(providerSpecificAlias, "true").WithProviderSpecific(providerSpecificEvaluateTargetHealth, "true"), endpoint.NewEndpoint("cname-test-elb-no-alias.zone-2.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeCNAME, "foo.eu-central-1.elb.amazonaws.com").WithProviderSpecific(providerSpecificAlias, "false"), @@ -1426,7 +1426,7 @@ func TestAWSsubmitChanges(t *testing.T) { records, err := provider.Records(ctx) require.NoError(t, err) - validateEndpoints(t, records, endpoints) + validateEndpoints(t, provider, records, endpoints) } func TestAWSsubmitChangesError(t *testing.T) { @@ -1607,8 +1607,11 @@ func TestAWSBatchChangeSetExceedingNameChange(t *testing.T) { require.Equal(t, 0, len(batchCs)) } -func validateEndpoints(t *testing.T, endpoints []*endpoint.Endpoint, expected []*endpoint.Endpoint) { +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) + assert.True(t, testutils.SameEndpoints(normalized, expected), "actual and normalized endpoints don't match. %+v:%+v", endpoints, normalized) } func validateAWSZones(t *testing.T, zones map[string]*route53.HostedZone, expected map[string]*route53.HostedZone) { @@ -1840,51 +1843,6 @@ 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, - ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME}, - } - 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"), @@ -1908,7 +1866,7 @@ func setAWSRecords(t *testing.T, provider *AWSProvider, records []*route53.Resou endpoints, err := provider.Records(ctx) require.NoError(t, err) - validateEndpoints(t, endpoints, []*endpoint.Endpoint{}) + validateEndpoints(t, provider, endpoints, []*endpoint.Endpoint{}) var changes Route53Changes for _, record := range records { @@ -2071,13 +2029,13 @@ func TestRequiresDeleteCreate(t *testing.T) { provider, _ := newAWSProvider(t, endpoint.NewDomainFilter([]string{"foo.bar."}), provider.NewZoneIDFilter([]string{}), provider.NewZoneTypeFilter(""), defaultEvaluateTargetHealth, false, nil) oldRecordType := endpoint.NewEndpointWithTTL("recordType", endpoint.RecordTypeA, endpoint.TTL(recordTTL), "8.8.8.8") - newRecordType := endpoint.NewEndpointWithTTL("recordType", endpoint.RecordTypeCNAME, endpoint.TTL(recordTTL), "bar") + newRecordType := endpoint.NewEndpointWithTTL("recordType", endpoint.RecordTypeCNAME, endpoint.TTL(recordTTL), "bar").WithProviderSpecific(providerSpecificAlias, "false") assert.False(t, provider.requiresDeleteCreate(oldRecordType, oldRecordType), "actual and expected endpoints don't match. %+v:%+v", oldRecordType, oldRecordType) assert.True(t, provider.requiresDeleteCreate(oldRecordType, newRecordType), "actual and expected endpoints don't match. %+v:%+v", oldRecordType, newRecordType) - oldCNAMEAlias := endpoint.NewEndpointWithTTL("CNAMEAlias", endpoint.RecordTypeCNAME, endpoint.TTL(recordTTL), "bar") - newCNAMEAlias := endpoint.NewEndpointWithTTL("CNAMEAlias", endpoint.RecordTypeCNAME, endpoint.TTL(recordTTL), "bar.us-east-1.elb.amazonaws.com") + oldCNAMEAlias := endpoint.NewEndpointWithTTL("CNAMEAlias", endpoint.RecordTypeCNAME, endpoint.TTL(recordTTL), "bar").WithProviderSpecific(providerSpecificAlias, "false") + newCNAMEAlias := endpoint.NewEndpointWithTTL("CNAMEAlias", endpoint.RecordTypeCNAME, endpoint.TTL(recordTTL), "bar.us-east-1.elb.amazonaws.com").WithProviderSpecific(providerSpecificAlias, "true") assert.False(t, provider.requiresDeleteCreate(oldCNAMEAlias, oldCNAMEAlias), "actual and expected endpoints don't match. %+v:%+v", oldCNAMEAlias, oldCNAMEAlias.DNSName) assert.True(t, provider.requiresDeleteCreate(oldCNAMEAlias, newCNAMEAlias), "actual and expected endpoints don't match. %+v:%+v", oldCNAMEAlias, newCNAMEAlias) diff --git a/provider/provider.go b/provider/provider.go index 06791204dc..3c8d358f17 100644 --- a/provider/provider.go +++ b/provider/provider.go @@ -30,6 +30,13 @@ 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 canonicalizes a set of candidate endpoints. + // It is called with a set of candidate endpoints obtained from the various sources. + // It returns a set modified as required by the provider. The provider is responsible for + // adding, removing, and modifying the ProviderSpecific properties to match + // 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 GetDomainFilter() endpoint.DomainFilterInterface }