Plan record type conflict should prefer A and AAAA records

endpoint.Targets.Less gives priority to A and AAAA over CNAME records so the plan record type conflict resolver should also prefer A/AAAA.
kubernetes-sigs · Sep 2, 2023 · 9cc5da4 · 9cc5da4
1 parent cf756a7
commit 9cc5da4
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 34 deletions.
diff --git a/plan/conflict.go b/plan/conflict.go
@@ -67,8 +67,8 @@ func (s PerResource) ResolveUpdate(current *endpoint.Endpoint, candidates []*end
 // ResolveRecordTypes attempts to detect and resolve record type conflicts in desired
 // endpoints for a domain. For eample if the there is more than 1 candidate and at lease one
 // of them is a CNAME. Per [RFC 1034 3.6.2] domains that contain a CNAME can not contain any
-// other record types. The default policy will prefer CNAME record types when a conflict is
-// detected.
+// other record types. The default policy will prefer A and AAAA record types when a conflict is
+// detected (consistent with [endpoint.Targets.Less]).
 //
 // [RFC 1034 3.6.2]: https://datatracker.ietf.org/doc/html/rfc1034#autoid-15
 func (s PerResource) ResolveRecordTypes(key planKey, row *planTableRow) map[string]*domainEndpoints {
@@ -95,8 +95,8 @@ func (s PerResource) ResolveRecordTypes(key planKey, row *planTableRow) map[stri
 		log.Warnf("Domain %s contains conflicting record type candidates, keeping CNAME record", key.dnsName)
 		records := map[string]*domainEndpoints{}
 		for recordType, recs := range row.records {
-			if recordType == endpoint.RecordTypeCNAME {
-				// policy is to prefer the CNAME record type when a conflic is found
+			if recordType != endpoint.RecordTypeCNAME {
+				// policy is to prefer the A and AAAA record type when a conflict is found
 				records[recordType] = recs
 			} else {
 				// discard candidates of conflicting records

diff --git a/plan/conflict_test.go b/plan/conflict_test.go
@@ -34,7 +34,7 @@ type ResolverSuite struct {
 	fooV2Cname          *endpoint.Endpoint
 	fooV2CnameDuplicate *endpoint.Endpoint
 	fooA5               *endpoint.Endpoint
-	fooAAA5             *endpoint.Endpoint
+	fooAAAA5            *endpoint.Endpoint
 	bar127A             *endpoint.Endpoint
 	bar192A             *endpoint.Endpoint
 	bar127AAnother      *endpoint.Endpoint
@@ -77,7 +77,7 @@ func (suite *ResolverSuite) SetupTest() {
 			endpoint.ResourceLabelKey: "ingress/default/foo-5",
 		},
 	}
-	suite.fooAAA5 = &endpoint.Endpoint{
+	suite.fooAAAA5 = &endpoint.Endpoint{
 		DNSName:    "foo",
 		Targets:    endpoint.Targets{"2001:DB8::1"},
 		RecordType: "AAAA",
@@ -194,17 +194,17 @@ func (suite *ResolverSuite) TestPerResource_ResolveRecordTypes() {
 			},
 		},
 		{
-			name: "no conflict: a and aaa records",
+			name: "no conflict: a and aaaa records",
 			args: args{
 				key: planKey{dnsName: "foo"},
 				row: &planTableRow{
-					candidates: []*endpoint.Endpoint{suite.fooA5, suite.fooAAA5},
+					candidates: []*endpoint.Endpoint{suite.fooA5, suite.fooAAAA5},
 					records: map[string]*domainEndpoints{
 						endpoint.RecordTypeA: {
 							candidates: []*endpoint.Endpoint{suite.fooA5},
 						},
 						endpoint.RecordTypeAAAA: {
-							candidates: []*endpoint.Endpoint{suite.fooAAA5},
+							candidates: []*endpoint.Endpoint{suite.fooAAAA5},
 						},
 					},
 				},
@@ -214,7 +214,7 @@ func (suite *ResolverSuite) TestPerResource_ResolveRecordTypes() {
 					candidates: []*endpoint.Endpoint{suite.fooA5},
 				},
 				endpoint.RecordTypeAAAA: {
-					candidates: []*endpoint.Endpoint{suite.fooAAA5},
+					candidates: []*endpoint.Endpoint{suite.fooAAAA5},
 				},
 			},
 		},
@@ -223,62 +223,62 @@ func (suite *ResolverSuite) TestPerResource_ResolveRecordTypes() {
 			args: args{
 				key: planKey{dnsName: "foo"},
 				row: &planTableRow{
-					current:    []*endpoint.Endpoint{suite.fooA5},
+					current:    []*endpoint.Endpoint{suite.fooV1Cname},
 					candidates: []*endpoint.Endpoint{suite.fooV1Cname, suite.fooA5},
 					records: map[string]*domainEndpoints{
 						endpoint.RecordTypeCNAME: {
+							current:    suite.fooV1Cname,
 							candidates: []*endpoint.Endpoint{suite.fooV1Cname},
 						},
 						endpoint.RecordTypeA: {
-							current:    suite.fooA5,
 							candidates: []*endpoint.Endpoint{suite.fooA5},
 						},
 					},
 				},
 			},
 			want: map[string]*domainEndpoints{
 				endpoint.RecordTypeCNAME: {
-					candidates: []*endpoint.Endpoint{suite.fooV1Cname},
+					current:    suite.fooV1Cname,
+					candidates: []*endpoint.Endpoint{},
 				},
 				endpoint.RecordTypeA: {
-					current:    suite.fooA5,
-					candidates: []*endpoint.Endpoint{},
+					candidates: []*endpoint.Endpoint{suite.fooA5},
 				},
 			},
 		},
 		{
-			name: "conflict: cname, a, and aaa records",
+			name: "conflict: cname, a, and aaaa records",
 			args: args{
 				key: planKey{dnsName: "foo"},
 				row: &planTableRow{
-					current:    []*endpoint.Endpoint{suite.fooA5, suite.fooAAA5},
-					candidates: []*endpoint.Endpoint{suite.fooV1Cname, suite.fooA5, suite.fooAAA5},
+					current:    []*endpoint.Endpoint{suite.fooA5, suite.fooAAAA5},
+					candidates: []*endpoint.Endpoint{suite.fooV1Cname, suite.fooA5, suite.fooAAAA5},
 					records: map[string]*domainEndpoints{
 						endpoint.RecordTypeCNAME: {
 							candidates: []*endpoint.Endpoint{suite.fooV1Cname},
 						},
 						endpoint.RecordTypeA: {
 							current:    suite.fooA5,
-							candidates: []*endpoint.Endpoint{},
+							candidates: []*endpoint.Endpoint{suite.fooA5},
 						},
 						endpoint.RecordTypeAAAA: {
-							current:    suite.fooAAA5,
-							candidates: []*endpoint.Endpoint{},
+							current:    suite.fooAAAA5,
+							candidates: []*endpoint.Endpoint{suite.fooAAAA5},
 						},
 					},
 				},
 			},
 			want: map[string]*domainEndpoints{
 				endpoint.RecordTypeCNAME: {
-					candidates: []*endpoint.Endpoint{suite.fooV1Cname},
+					candidates: []*endpoint.Endpoint{},
 				},
 				endpoint.RecordTypeA: {
 					current:    suite.fooA5,
-					candidates: []*endpoint.Endpoint{},
+					candidates: []*endpoint.Endpoint{suite.fooA5},
 				},
 				endpoint.RecordTypeAAAA: {
-					current:    suite.fooAAA5,
-					candidates: []*endpoint.Endpoint{},
+					current:    suite.fooAAAA5,
+					candidates: []*endpoint.Endpoint{suite.fooAAAA5},
 				},
 			},
 		},

diff --git a/plan/plan_test.go b/plan/plan_test.go
@@ -609,23 +609,22 @@ func (suite *PlanTestSuite) TestConflictingCurrentNoDesired() {
 
 // TestCurrentWithConflictingDesired simulates where the desired records result in conflicting records types.
 // This could be the result of multiple sources generating conflicting records types. In this case the conflict
-// resolver should prefer the CNAME record candidate and delete the other records.
+// resolver should prefer the A and AAAA record candidate and delete the other records.
 func (suite *PlanTestSuite) TestCurrentWithConflictingDesired() {
-	suite.fooA5.Labels[endpoint.OwnerLabelKey] = "nerf"
-	suite.fooAAAA.Labels[endpoint.OwnerLabelKey] = "nerf"
-	current := []*endpoint.Endpoint{suite.fooA5, suite.fooAAAA}
+	suite.fooV1Cname.Labels[endpoint.OwnerLabelKey] = "nerf"
+	current := []*endpoint.Endpoint{suite.fooV1Cname}
 	desired := []*endpoint.Endpoint{suite.fooV1Cname, suite.fooA5, suite.fooAAAA}
-	expectedCreate := []*endpoint.Endpoint{suite.fooV1Cname}
+	expectedCreate := []*endpoint.Endpoint{suite.fooA5, suite.fooAAAA}
 	expectedUpdateOld := []*endpoint.Endpoint{}
 	expectedUpdateNew := []*endpoint.Endpoint{}
-	expectedDelete := []*endpoint.Endpoint{suite.fooA5, suite.fooAAAA}
+	expectedDelete := []*endpoint.Endpoint{suite.fooV1Cname}
 
 	p := &Plan{
 		Policies:       []Policy{&SyncPolicy{}},
 		Current:        current,
 		Desired:        desired,
 		ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeAAAA, endpoint.RecordTypeCNAME},
-		OwnerID:        suite.fooA5.Labels[endpoint.OwnerLabelKey],
+		OwnerID:        suite.fooV1Cname.Labels[endpoint.OwnerLabelKey],
 	}
 
 	changes := p.Calculate().Changes
@@ -637,11 +636,11 @@ func (suite *PlanTestSuite) TestCurrentWithConflictingDesired() {
 
 // TestNoCurrentWithConflictingDesired simulates where the desired records result in conflicting records types.
 // This could be the result of multiple sources generating conflicting records types. In this case there the
-// conflict resolver should prefer the CNAME record and drop the other candidate record types.
+// conflict resolver should prefer the A and AAAA record and drop the other candidate record types.
 func (suite *PlanTestSuite) TestNoCurrentWithConflictingDesired() {
 	current := []*endpoint.Endpoint{}
 	desired := []*endpoint.Endpoint{suite.fooV1Cname, suite.fooA5, suite.fooAAAA}
-	expectedCreate := []*endpoint.Endpoint{suite.fooV1Cname}
+	expectedCreate := []*endpoint.Endpoint{suite.fooA5, suite.fooAAAA}
 	expectedUpdateOld := []*endpoint.Endpoint{}
 	expectedUpdateNew := []*endpoint.Endpoint{}
 	expectedDelete := []*endpoint.Endpoint{}