From 4c8b5dc45a42e9ae86992f85f5c8b10b45a651d6 Mon Sep 17 00:00:00 2001 From: Kyle Cronin Date: Sat, 2 Sep 2023 16:35:16 -0400 Subject: [PATCH] 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. --- plan/conflict.go | 13 +++++++------ plan/conflict_test.go | 42 +++++++++++++++++++++--------------------- plan/plan.go | 6 +++--- plan/plan_test.go | 17 ++++++++--------- 4 files changed, 39 insertions(+), 39 deletions(-) diff --git a/plan/conflict.go b/plan/conflict.go index 283f36d404..81b8c91880 100644 --- a/plan/conflict.go +++ b/plan/conflict.go @@ -67,11 +67,12 @@ 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 { + // no conflicts if only a single desired record type for the domain if len(row.candidates) <= 1 { return row.records } @@ -92,19 +93,19 @@ func (s PerResource) ResolveRecordTypes(key planKey, row *planTableRow) map[stri // conflict was found, remove candiates of non-preferred record types if cname && other { - log.Warnf("Domain %s contains conflicting record type candidates, keeping CNAME record", key.dnsName) + log.Infof("Domain %s contains conflicting record type candidates, discarding CNAME record", key.dnsName) records := map[string]*domainEndpoints{} for recordType, recs := range row.records { + // policy is to prefer the non-CNAME record types when a conflict is found if recordType == endpoint.RecordTypeCNAME { - // policy is to prefer the CNAME record type when a conflic is found - records[recordType] = recs - } else { // discard candidates of conflicting records // keep currect so they can be deleted records[recordType] = &domainEndpoints{ current: recs.current, candidates: []*endpoint.Endpoint{}, } + } else { + records[recordType] = recs } } diff --git a/plan/conflict_test.go b/plan/conflict_test.go index 3fcba8220c..d09d028374 100644 --- 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,14 +223,14 @@ 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}, }, }, @@ -238,47 +238,47 @@ func (suite *ResolverSuite) TestPerResource_ResolveRecordTypes() { }, 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.go b/plan/plan.go index 5300ebf46e..cf0aa0098f 100644 --- a/plan/plan.go +++ b/plan/plan.go @@ -97,9 +97,9 @@ func newPlanTable() planTable { // TODO: make resolver configurable // planTableRow represents a set of current and desired domain resource records. type planTableRow struct { - // current corresponds to the records currently occupying dns name on the dns provider. More than 1 record may - // be represented here, for example A and AAAA. If current domain record is CNAME, no other record types are allowed - // per [RFC 1034 3.6.2] + // current corresponds to the records currently occupying dns name on the dns provider. More than one record may + // be represented here: for example A and AAAA. If the current domain record is a CNAME, no other record types + // are allowed per [RFC 1034 3.6.2] // // [RFC 1034 3.6.2]: https://datatracker.ietf.org/doc/html/rfc1034#autoid-15 current []*endpoint.Endpoint diff --git a/plan/plan_test.go b/plan/plan_test.go index 577e34773e..acd5116c83 100644 --- 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{}