Skip to content

Commit

Permalink
Extract record types conflict resolver
Browse files Browse the repository at this point in the history
  • Loading branch information
@cronik
cronik committed Aug 30, 2023
1 parent 20bff0a commit cf756a7
Show file tree
Hide file tree
Showing 4 changed files with 227 additions and 50 deletions.
53 changes: 53 additions & 0 deletions plan/conflict.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package plan
import (
"sort"

log "github.com/sirupsen/logrus"
"sigs.k8s.io/external-dns/endpoint"
)

Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down
161 changes: 160 additions & 1 deletion plan/conflict_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ limitations under the License.
package plan

import (
"reflect"
"testing"

"github.com/stretchr/testify/suite"

"sigs.k8s.io/external-dns/endpoint"
)

Expand All @@ -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
Expand Down Expand Up @@ -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"},
Expand Down Expand Up @@ -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))
}
49 changes: 7 additions & 42 deletions plan/plan.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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))
}
}
}

Expand All @@ -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)
Expand Down
14 changes: 7 additions & 7 deletions plan/plan_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{}},
Expand All @@ -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{}
Expand Down

0 comments on commit cf756a7

Please sign in to comment.