Address review comments

Author: alfredkrohmer

internal/testutils/endpoint.go

@@ -25,6 +25,12 @@ import (
 
 /** test utility functions for endpoints verifications */
 
+type byNames endpoint.ProviderSpecific
+
+func (p byNames) Len() int           { return len(p) }
+func (p byNames) Swap(i, j int)      { p[i], p[j] = p[j], p[i] }
+func (p byNames) Less(i, j int) bool { return p[i].Name < p[j].Name }
+
 type byAllFields []*endpoint.Endpoint
 
 func (b byAllFields) Len() int      { return len(b) }
@@ -102,5 +108,9 @@ func SamePlanChanges(a, b map[string][]*endpoint.Endpoint) bool {
 
 // SameProviderSpecific verifies that two maps contain the same string/string key/value pairs
 func SameProviderSpecific(a, b endpoint.ProviderSpecific) bool {
-	return reflect.DeepEqual(a, b)
+	sa := a
+	sb := b
+	sort.Sort(byNames(sa))
+	sort.Sort(byNames(sb))
+	return reflect.DeepEqual(sa, sb)
 }

provider/aws/aws.go

@@ -402,8 +402,6 @@ func (p *AWSProvider) DeleteRecords(ctx context.Context, endpoints []*endpoint.E
 
 func (p *AWSProvider) doRecords(ctx context.Context, action string, endpoints []*endpoint.Endpoint) error {
 	zones, err := p.Zones(ctx)
-	p.AdjustEndpoints(endpoints)
-
 	if err != nil {
 		return errors.Wrapf(err, "failed to list zones, aborting %s doRecords action", action)
 	}
@@ -412,6 +410,9 @@ func (p *AWSProvider) doRecords(ctx context.Context, action string, endpoints []
 	if err != nil {
 		log.Errorf("failed to list records while preparing %s doRecords action: %s", action, err)
 	}
+
+	p.AdjustEndpoints(endpoints)
+
 	return p.submitChanges(ctx, p.newChanges(action, endpoints, records, zones), zones)
 }
 
@@ -561,11 +562,16 @@ func (p *AWSProvider) newChanges(action string, endpoints []*endpoint.Endpoint,
 	return changes
 }
 
+// AdjustEndpoints modifies the provided endpoints (coming from various sources) to match
+// the endpoints that the provider returns in `Records` so that the change plan will not have
+// 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 {
 	for _, ep := range endpoints {
 		alias := false
-		if _, ok := ep.GetProviderSpecificProperty(providerSpecificAlias); ok {
-			alias = true
+		if aliasString, ok := ep.GetProviderSpecificProperty(providerSpecificAlias); ok {
+			alias = aliasString.Value == "true"
 		} else if useAlias(ep, p.preferCNAME) {
 			alias = true
 			log.Debugf("Modifying endpoint: %v, setting %s=true", ep, providerSpecificAlias)
@@ -843,7 +849,8 @@ func useAlias(ep *endpoint.Endpoint, preferCNAME bool) bool {
 	return false
 }
 
-// isAWSAlias determines if a given hostname is an AWS Alias record
+// isAWSAlias determines if a given endpoint is supposed to create an AWS Alias record
+// and (if so) returns the target hosted zone ID
 func isAWSAlias(ep *endpoint.Endpoint) string {
 	prop, exists := ep.GetProviderSpecificProperty(providerSpecificAlias)
 	if exists && prop.Value == "true" && ep.RecordType == endpoint.RecordTypeCNAME && len(ep.Targets) > 0 {

provider/aws/aws_test.go

@@ -354,6 +354,30 @@ func TestAWSRecords(t *testing.T) {
 	})
 }
 
+func TestAWSAdjustEndpoints(t *testing.T) {
+	provider, _ := newAWSProvider(t, endpoint.NewDomainFilter([]string{"ext-dns-test-2.teapot.zalan.do."}), provider.NewZoneIDFilter([]string{}), provider.NewZoneTypeFilter(""), defaultEvaluateTargetHealth, false, []*endpoint.Endpoint{})
+
+	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-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"),
+		endpoint.NewEndpoint("cname-test-elb.zone-2.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeCNAME, "foo.eu-central-1.elb.amazonaws.com"),
+		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"),
+		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)
+
+	validateEndpoints(t, 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-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"),
+		endpoint.NewEndpoint("cname-test-elb-no-eth.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeCNAME, "foo.eu-central-1.elb.amazonaws.com").WithProviderSpecific(providerSpecificAlias, "true").WithProviderSpecific(providerSpecificEvaluateTargetHealth, "false"), // eth = evaluate target health
+	})
+}
+
 func TestAWSCreateRecords(t *testing.T) {
 	customTTL := endpoint.TTL(60)
 	provider, _ := newAWSProvider(t, endpoint.NewDomainFilter([]string{"ext-dns-test-2.teapot.zalan.do."}), provider.NewZoneIDFilter([]string{}), provider.NewZoneTypeFilter(""), defaultEvaluateTargetHealth, false, []*endpoint.Endpoint{})