diff --git a/plan/conflict.go b/plan/conflict.go index 9b1f29cfe..283f36d40 100644 --- a/plan/conflict.go +++ b/plan/conflict.go @@ -19,6 +19,7 @@ package plan import ( "sort" + log "github.com/sirupsen/logrus" "sigs.k8s.io/external-dns/endpoint" ) @@ -27,6 +28,7 @@ import ( type ConflictResolver interface { ResolveCreate(candidates []*endpoint.Endpoint) *endpoint.Endpoint ResolveUpdate(current *endpoint.Endpoint, candidates []*endpoint.Endpoint) *endpoint.Endpoint + ResolveRecordTypes(key planKey, row *planTableRow) map[string]*domainEndpoints } // PerResource allows only one resource to own a given dns name @@ -62,6 +64,57 @@ func (s PerResource) ResolveUpdate(current *endpoint.Endpoint, candidates []*end return s.ResolveCreate(candidates) } +// 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. +// +// [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 { + if len(row.candidates) <= 1 { + return row.records + } + + cname := false + other := false + for _, c := range row.candidates { + if c.RecordType == endpoint.RecordTypeCNAME { + cname = true + } else { + other = true + } + + if cname && other { + break + } + } + + // 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) + 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 + 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{}, + } + } + } + + return records + } + + // no conflict, return all records types + return row.records +} + // less returns true if endpoint x is less than y func (s PerResource) less(x, y *endpoint.Endpoint) bool { return x.Targets.IsLess(y.Targets) diff --git a/plan/conflict_test.go b/plan/conflict_test.go index 491b42bff..3fcba8220 100644 --- a/plan/conflict_test.go +++ b/plan/conflict_test.go @@ -17,10 +17,10 @@ limitations under the License. package plan import ( + "reflect" "testing" "github.com/stretchr/testify/suite" - "sigs.k8s.io/external-dns/endpoint" ) @@ -34,6 +34,7 @@ type ResolverSuite struct { fooV2Cname *endpoint.Endpoint fooV2CnameDuplicate *endpoint.Endpoint fooA5 *endpoint.Endpoint + fooAAA5 *endpoint.Endpoint bar127A *endpoint.Endpoint bar192A *endpoint.Endpoint bar127AAnother *endpoint.Endpoint @@ -76,6 +77,14 @@ func (suite *ResolverSuite) SetupTest() { endpoint.ResourceLabelKey: "ingress/default/foo-5", }, } + suite.fooAAA5 = &endpoint.Endpoint{ + DNSName: "foo", + Targets: endpoint.Targets{"2001:DB8::1"}, + RecordType: "AAAA", + Labels: map[string]string{ + endpoint.ResourceLabelKey: "ingress/default/foo-5", + }, + } suite.bar127A = &endpoint.Endpoint{ DNSName: "bar", Targets: endpoint.Targets{"127.0.0.1"}, @@ -133,6 +142,156 @@ func (suite *ResolverSuite) TestStrictResolver() { suite.Equal(suite.bar127A, suite.perResource.ResolveUpdate(suite.legacyBar192A, []*endpoint.Endpoint{suite.bar127A, suite.bar192A}), " legacy record's resource value will not match, should pick minimum") } +func (suite *ResolverSuite) TestPerResource_ResolveRecordTypes() { + type args struct { + key planKey + row *planTableRow + } + tests := []struct { + name string + args args + want map[string]*domainEndpoints + }{ + { + name: "no conflict: cname record", + args: args{ + key: planKey{dnsName: "foo"}, + row: &planTableRow{ + candidates: []*endpoint.Endpoint{suite.fooV1Cname}, + records: map[string]*domainEndpoints{ + endpoint.RecordTypeCNAME: { + candidates: []*endpoint.Endpoint{suite.fooV1Cname}, + }, + }, + }, + }, + want: map[string]*domainEndpoints{ + endpoint.RecordTypeCNAME: { + candidates: []*endpoint.Endpoint{suite.fooV1Cname}, + }, + }, + }, + { + name: "no conflict: a record", + args: args{ + key: planKey{dnsName: "foo"}, + row: &planTableRow{ + current: []*endpoint.Endpoint{suite.fooA5}, + candidates: []*endpoint.Endpoint{suite.fooA5}, + records: map[string]*domainEndpoints{ + endpoint.RecordTypeA: { + current: suite.fooA5, + candidates: []*endpoint.Endpoint{suite.fooA5}, + }, + }, + }, + }, + want: map[string]*domainEndpoints{ + endpoint.RecordTypeA: { + current: suite.fooA5, + candidates: []*endpoint.Endpoint{suite.fooA5}, + }, + }, + }, + { + name: "no conflict: a and aaa records", + args: args{ + key: planKey{dnsName: "foo"}, + row: &planTableRow{ + candidates: []*endpoint.Endpoint{suite.fooA5, suite.fooAAA5}, + records: map[string]*domainEndpoints{ + endpoint.RecordTypeA: { + candidates: []*endpoint.Endpoint{suite.fooA5}, + }, + endpoint.RecordTypeAAAA: { + candidates: []*endpoint.Endpoint{suite.fooAAA5}, + }, + }, + }, + }, + want: map[string]*domainEndpoints{ + endpoint.RecordTypeA: { + candidates: []*endpoint.Endpoint{suite.fooA5}, + }, + endpoint.RecordTypeAAAA: { + candidates: []*endpoint.Endpoint{suite.fooAAA5}, + }, + }, + }, + { + name: "conflict: cname and a records", + args: args{ + key: planKey{dnsName: "foo"}, + row: &planTableRow{ + current: []*endpoint.Endpoint{suite.fooA5}, + candidates: []*endpoint.Endpoint{suite.fooV1Cname, suite.fooA5}, + records: map[string]*domainEndpoints{ + endpoint.RecordTypeCNAME: { + 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}, + }, + endpoint.RecordTypeA: { + current: suite.fooA5, + candidates: []*endpoint.Endpoint{}, + }, + }, + }, + { + name: "conflict: cname, a, and aaa 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}, + records: map[string]*domainEndpoints{ + endpoint.RecordTypeCNAME: { + candidates: []*endpoint.Endpoint{suite.fooV1Cname}, + }, + endpoint.RecordTypeA: { + current: suite.fooA5, + candidates: []*endpoint.Endpoint{}, + }, + endpoint.RecordTypeAAAA: { + current: suite.fooAAA5, + candidates: []*endpoint.Endpoint{}, + }, + }, + }, + }, + want: map[string]*domainEndpoints{ + endpoint.RecordTypeCNAME: { + candidates: []*endpoint.Endpoint{suite.fooV1Cname}, + }, + endpoint.RecordTypeA: { + current: suite.fooA5, + candidates: []*endpoint.Endpoint{}, + }, + endpoint.RecordTypeAAAA: { + current: suite.fooAAA5, + candidates: []*endpoint.Endpoint{}, + }, + }, + }, + } + for _, tt := range tests { + suite.Run(tt.name, func() { + if got := suite.perResource.ResolveRecordTypes(tt.args.key, tt.args.row); !reflect.DeepEqual(got, tt.want) { + suite.T().Errorf("PerResource.ResolveRecordTypes() = %v, want %v", got, tt.want) + } + }) + } +} + func TestConflictResolver(t *testing.T) { suite.Run(t, new(ResolverSuite)) } diff --git a/plan/plan.go b/plan/plan.go index 1b8d75e53..5300ebf46 100644 --- a/plan/plan.go +++ b/plan/plan.go @@ -118,33 +118,6 @@ type domainEndpoints struct { candidates []*endpoint.Endpoint } -// hasCandidateRecordTypeConflict returns true if the candidates set contains conflicting or invalid record types. -// 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. -// -// [RFC 1034 3.6.2]: https://datatracker.ietf.org/doc/html/rfc1034#autoid-15 -func (t planTableRow) hasCandidateRecordTypeConflict() bool { - if len(t.candidates) <= 1 { - return false - } - - cname := false - other := false - for _, c := range t.candidates { - if c.RecordType == endpoint.RecordTypeCNAME { - cname = true - } else { - other = true - } - - if cname && other { - return true - } - } - - return false -} - func (t planTableRow) String() string { return fmt.Sprintf("planTableRow{current=%v, candidates=%v}", t.current, t.candidates) } @@ -209,14 +182,11 @@ func (p *Plan) Calculate() *Plan { for key, row := range t.rows { // dns name not taken if len(row.current) == 0 { - // TODO how to resolve conflicting source candidate record types - if row.hasCandidateRecordTypeConflict() { - log.Warnf("Domain %s contains conflicting record type candidates, no updates planned", key.dnsName) - continue - } - - for _, records := range row.records { - changes.Create = append(changes.Create, t.resolver.ResolveCreate(records.candidates)) + recordsByType := t.resolver.ResolveRecordTypes(key, row) + for _, records := range recordsByType { + if len(records.candidates) > 0 { + changes.Create = append(changes.Create, t.resolver.ResolveCreate(records.candidates)) + } } } @@ -227,16 +197,11 @@ func (p *Plan) Calculate() *Plan { // dns name is taken if len(row.current) > 0 && len(row.candidates) > 0 { - // TODO how to resolve conflicting source candidate record types - if row.hasCandidateRecordTypeConflict() { - log.Warnf("Domain %s contains conflicting record type candidates, no updates planned", key.dnsName) - continue - } - creates := []*endpoint.Endpoint{} // apply changes for each record type - for _, records := range row.records { + recordsByType := t.resolver.ResolveRecordTypes(key, row) + for _, records := range recordsByType { // record type not desired if records.current != nil && len(records.candidates) == 0 { changes.Delete = append(changes.Delete, records.current) diff --git a/plan/plan_test.go b/plan/plan_test.go index d05e567ca..577e34773 100644 --- a/plan/plan_test.go +++ b/plan/plan_test.go @@ -608,17 +608,17 @@ 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 there are -// no changes planned since there is no conflict resolver for this situation. +// 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. func (suite *PlanTestSuite) TestCurrentWithConflictingDesired() { suite.fooA5.Labels[endpoint.OwnerLabelKey] = "nerf" suite.fooAAAA.Labels[endpoint.OwnerLabelKey] = "nerf" current := []*endpoint.Endpoint{suite.fooA5, suite.fooAAAA} desired := []*endpoint.Endpoint{suite.fooV1Cname, suite.fooA5, suite.fooAAAA} - expectedCreate := []*endpoint.Endpoint{} + expectedCreate := []*endpoint.Endpoint{suite.fooV1Cname} expectedUpdateOld := []*endpoint.Endpoint{} expectedUpdateNew := []*endpoint.Endpoint{} - expectedDelete := []*endpoint.Endpoint{} + expectedDelete := []*endpoint.Endpoint{suite.fooA5, suite.fooAAAA} p := &Plan{ Policies: []Policy{&SyncPolicy{}}, @@ -636,12 +636,12 @@ 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 are -// no changes planned since there is no conflict resolver for this situation. +// 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. func (suite *PlanTestSuite) TestNoCurrentWithConflictingDesired() { current := []*endpoint.Endpoint{} desired := []*endpoint.Endpoint{suite.fooV1Cname, suite.fooA5, suite.fooAAAA} - expectedCreate := []*endpoint.Endpoint{} + expectedCreate := []*endpoint.Endpoint{suite.fooV1Cname} expectedUpdateOld := []*endpoint.Endpoint{} expectedUpdateNew := []*endpoint.Endpoint{} expectedDelete := []*endpoint.Endpoint{}