From 0ff55b5dd62b1c3c2fc8f38bacfac9ad00755e8b Mon Sep 17 00:00:00 2001 From: Michael Nairn Date: Mon, 8 Apr 2024 19:07:24 +0100 Subject: [PATCH] Update plan for Distibuted DNS Initial work to modify the external-dns plan to work in the proposed way for distubuted dns. Updates the plan to treat all endpoints passed to it (current/desired) as part of a single shared set of records. The calculation logic now takes into account other endpoints added to the plan when calculating the desired changes for any other endpoint also added. The plan calculation logic is broken into two steps: 1. Calculate what endpoints will be created/updated and deleted based on the current and desired records passed to the plan. For each endpoint that will still exist if these changes were to be applied(create/update), a map of dnsNames and their owners is calculated. 2. Create the external-dns Changes for the endpoints calculated in step one. For all endpoints calculated to be an update the target values are re-calculated using the information gathered in step one about ownership of dnsNames. If we detect that a CNAME has a "managed" target value (it's in our owner map) we use the information known about who owns it to manipulate the desired targets values. plan+tests A records with Owners Ignore all update calculations if plan owner is not set, this will default back to using desired only as the value during update --- internal/controller/dnsrecord_controller.go | 1 + internal/external-dns/plan/labels.go | 6 + internal/external-dns/plan/plan.go | 327 +++++++++++++++----- internal/external-dns/plan/plan_test.go | 231 +++++++++++++- internal/external-dns/registry/txt.go | 7 +- internal/external-dns/registry/txt_test.go | 18 +- 6 files changed, 498 insertions(+), 92 deletions(-) create mode 100644 internal/external-dns/plan/labels.go diff --git a/internal/controller/dnsrecord_controller.go b/internal/controller/dnsrecord_controller.go index 9a1fd35..b1dcdde 100644 --- a/internal/controller/dnsrecord_controller.go +++ b/internal/controller/dnsrecord_controller.go @@ -355,6 +355,7 @@ func (r *DNSRecordReconciler) applyChanges(ctx context.Context, dnsRecord *v1alp Policies: []externaldnsplan.Policy{policy}, Current: zoneEndpoints, Desired: specEndpoints, + Previous: statusEndpoints, //Note: We can't just filter domains by `managedZone.Spec.DomainName` it needs to be the exact root domain for this particular record DomainFilter: externaldnsendpoint.MatchAllDomainFilters{&rootDomainFilter}, ManagedRecords: managedDNSRecordTypes, diff --git a/internal/external-dns/plan/labels.go b/internal/external-dns/plan/labels.go new file mode 100644 index 0000000..c5df23a --- /dev/null +++ b/internal/external-dns/plan/labels.go @@ -0,0 +1,6 @@ +package plan + +const ( + // OwnerLabelDeliminator is a deliminator used between owners in the OwnerLabelKey value when multiple owners are assigned. + OwnerLabelDeliminator = "&&" +) diff --git a/internal/external-dns/plan/plan.go b/internal/external-dns/plan/plan.go index 086020c..5687115 100644 --- a/internal/external-dns/plan/plan.go +++ b/internal/external-dns/plan/plan.go @@ -18,7 +18,7 @@ package plan import ( "fmt" - "strconv" + "slices" "strings" log "github.com/sirupsen/logrus" @@ -35,6 +35,8 @@ type PropertyComparator func(name string, previous string, current string) bool type Plan struct { // List of current records Current []*endpoint.Endpoint + // List of the records that were successfully resolved by this instance previously. + Previous []*endpoint.Endpoint // List of desired records Desired []*endpoint.Endpoint // Policies under which the desired changes are calculated @@ -93,6 +95,8 @@ type planTableRow struct { // // [RFC 1034 3.6.2]: https://datatracker.ietf.org/doc/html/rfc1034#autoid-15 current []*endpoint.Endpoint + // previous corresponds to the list of records that were last used to create/update this dnsName. + previous []*endpoint.Endpoint // candidates corresponds to the list of records which would like to have this dnsName. candidates []*endpoint.Endpoint // records is a grouping of current and candidates by record type, for example A, AAAA, CNAME. @@ -104,6 +108,8 @@ type planTableRow struct { type domainEndpoints struct { // current corresponds to existing record from the registry. Maybe nil if no current record of the type exists. current *endpoint.Endpoint + // previous corresponds to the record which was previously used doe dnsName during the last create/update. + previous *endpoint.Endpoint // candidates corresponds to the list of records which would like to have this dnsName. candidates []*endpoint.Endpoint } @@ -118,6 +124,12 @@ func (t planTable) addCurrent(e *endpoint.Endpoint) { t.rows[key].records[e.RecordType].current = e } +func (t planTable) addPrevious(e *endpoint.Endpoint) { + key := t.newPlanKey(e) + t.rows[key].previous = append(t.rows[key].previous, e) + t.rows[key].records[e.RecordType].previous = e +} + func (t planTable) addCandidate(e *endpoint.Endpoint) { key := t.newPlanKey(e) t.rows[key].candidates = append(t.rows[key].candidates, e) @@ -156,77 +168,133 @@ func (p *Plan) Calculate() *Plan { for _, current := range filterRecordsForPlan(p.Current, p.DomainFilter, p.ManagedRecords, p.ExcludeRecords) { t.addCurrent(current) } + for _, previous := range filterRecordsForPlan(p.Previous, p.DomainFilter, p.ManagedRecords, p.ExcludeRecords) { + t.addPrevious(previous) + } for _, desired := range filterRecordsForPlan(p.Desired, p.DomainFilter, p.ManagedRecords, p.ExcludeRecords) { t.addCandidate(desired) } - changes := &externaldnsplan.Changes{} + managedChanges := managedRecordSetChanges{ + ownerID: p.OwnerID, + creates: []*endpoint.Endpoint{}, + deletes: []*endpoint.Endpoint{}, + updates: []*endpointUpdate{}, + dnsNameOwners: map[string][]string{}, + } for key, row := range t.rows { - // dns name not taken + if _, ok := managedChanges.dnsNameOwners[key.dnsName]; !ok { + managedChanges.dnsNameOwners[key.dnsName] = []string{} + } + + // dns name not taken (Create) if len(row.current) == 0 { 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)) + managedChanges.creates = append(managedChanges.creates, t.resolver.ResolveCreate(records.candidates)) + managedChanges.dnsNameOwners[key.dnsName] = []string{p.OwnerID} } } } - // dns name released or possibly owned by a different external dns + // dns name released or possibly owned by a different external dns (Delete) if len(row.current) > 0 && len(row.candidates) == 0 { - changes.Delete = append(changes.Delete, row.current...) + recordsByType := t.resolver.ResolveRecordTypes(key, row) + for _, records := range recordsByType { + if records.current != nil { + candidate := records.current.DeepCopy() + owners := []string{} + if endpointOwner, hasOwner := records.current.Labels[endpoint.OwnerLabelKey]; hasOwner && p.OwnerID != "" { + owners = strings.Split(endpointOwner, OwnerLabelDeliminator) + for i, v := range owners { + if v == p.OwnerID { + owners = append(owners[:i], owners[i+1:]...) + break + } + } + slices.Sort(owners) + owners = slices.Compact[[]string, string](owners) + candidate.Labels[endpoint.OwnerLabelKey] = strings.Join(owners, OwnerLabelDeliminator) + managedChanges.dnsNameOwners[key.dnsName] = append(managedChanges.dnsNameOwners[key.dnsName], owners...) + } + + if len(owners) == 0 { + managedChanges.deletes = append(managedChanges.deletes, records.current) + } else { + //ToDO Not ideal that this is also manipulating the desired record values here but we dont know if + // the update was caused by the deletion of a record or not later so we have to remove the previous + // values from the desired like this for now. + if records.current.RecordType == endpoint.RecordTypeA { + if records.previous != nil { + removeEndpointTargets(records.previous.Targets, candidate) + } + } + managedChanges.updates = append(managedChanges.updates, &endpointUpdate{desired: candidate, current: records.current, previous: records.previous}) + } + } + } } - // dns name is taken + // dns name is taken (Update) if len(row.current) > 0 && len(row.candidates) > 0 { - creates := []*endpoint.Endpoint{} - // apply changes for each record type 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) - } - // new record type desired - if records.current == nil && len(records.candidates) > 0 { - update := t.resolver.ResolveCreate(records.candidates) - // creates are evaluated after all domain records have been processed to - // validate that this external dns has ownership claim on the domain before - // adding the records to planned changes. - creates = append(creates, update) - } + //ToDo Deal with record type changing + //ToDo Mark this is a conflict so we can propagate it out + //// record type not desired + //if records.current != nil && len(records.candidates) == 0 { + // changes.Delete = append(changes.Delete, records.current) + //} + // + //// new record type desired + //if records.current == nil && len(records.candidates) > 0 { + // update := t.resolver.ResolveCreate(records.candidates) + // // creates are evaluated after all domain records have been processed to + // // validate that this external dns has ownership claim on the domain before + // // adding the records to planned changes. + // creates = append(creates, update) + //} // update existing record if records.current != nil && len(records.candidates) > 0 { - update := t.resolver.ResolveUpdate(records.current, records.candidates) - - if shouldUpdateTTL(update, records.current) || targetChanged(update, records.current) || p.shouldUpdateProviderSpecific(update, records.current) { - inheritOwner(records.current, update) - changes.UpdateNew = append(changes.UpdateNew, update) - changes.UpdateOld = append(changes.UpdateOld, records.current) + candidate := t.resolver.ResolveUpdate(records.current, records.candidates) + current := records.current.DeepCopy() + if endpointOwner, hasOwner := current.Labels[endpoint.OwnerLabelKey]; hasOwner { + if p.OwnerID == "" { + // Only allow owned records to be updated by other owned records + //ToDo Mark this is a conflict so we can propagate it out + continue + } + + owners := strings.Split(endpointOwner, OwnerLabelDeliminator) + owners = append(owners, p.OwnerID) + slices.Sort(owners) + owners = slices.Compact[[]string, string](owners) + current.Labels[endpoint.OwnerLabelKey] = strings.Join(owners, OwnerLabelDeliminator) + managedChanges.dnsNameOwners[key.dnsName] = append(managedChanges.dnsNameOwners[key.dnsName], owners...) + } else { + if p.OwnerID != "" { + // Only allow unowned records to be updated by other unowned records + //ToDo Mark this is a conflict so we can propagate it out + continue + } } - } - } - - if len(creates) > 0 { - // only add creates if the external dns has ownership claim on the domain - ownersMatch := true - for _, current := range row.current { - if p.OwnerID != "" && !current.IsOwnedBy(p.OwnerID) { - ownersMatch = false - } - } - - if ownersMatch { - changes.Create = append(changes.Create, creates...) + inheritOwner(current, candidate) + managedChanges.updates = append(managedChanges.updates, &endpointUpdate{desired: candidate, current: records.current, previous: records.previous}) } } } + + slices.Sort(managedChanges.dnsNameOwners[key.dnsName]) + managedChanges.dnsNameOwners[key.dnsName] = slices.Compact[[]string, string](managedChanges.dnsNameOwners[key.dnsName]) } + changes := managedChanges.Calculate() + for _, pol := range p.Policies { changes = pol.Apply(changes) } @@ -234,8 +302,9 @@ func (p *Plan) Calculate() *Plan { // filter out updates this external dns does not have ownership claim over if p.OwnerID != "" { changes.Delete = endpoint.FilterEndpointsByOwnerID(p.OwnerID, changes.Delete) - changes.UpdateOld = endpoint.FilterEndpointsByOwnerID(p.OwnerID, changes.UpdateOld) - changes.UpdateNew = endpoint.FilterEndpointsByOwnerID(p.OwnerID, changes.UpdateNew) + //ToDo Ideally we would still be able to ensure ownership on update + //changes.UpdateOld = endpoint.FilterEndpointsByOwnerID(p.OwnerID, changes.UpdateOld) + //changes.UpdateNew = endpoint.FilterEndpointsByOwnerID(p.OwnerID, changes.UpdateNew) } plan := &Plan{ @@ -258,10 +327,155 @@ func inheritOwner(from, to *endpoint.Endpoint) { to.Labels[endpoint.OwnerLabelKey] = from.Labels[endpoint.OwnerLabelKey] } +type endpointUpdate struct { + current *endpoint.Endpoint + previous *endpoint.Endpoint + desired *endpoint.Endpoint +} + +func (e *endpointUpdate) ShouldUpdate() bool { + return shouldUpdateOwner(e.desired, e.current) || shouldUpdateTTL(e.desired, e.current) || targetChanged(e.desired, e.current) || shouldUpdateProviderSpecific(e.desired, e.current) +} + +type managedRecordSetChanges struct { + ownerID string + creates []*endpoint.Endpoint + deletes []*endpoint.Endpoint + updates []*endpointUpdate + dnsNameOwners map[string][]string +} + +func (e *managedRecordSetChanges) Calculate() *externaldnsplan.Changes { + changes := &externaldnsplan.Changes{ + Create: e.creates, + Delete: e.deletes, + } + + for _, update := range e.updates { + e.calculateDesired(update) + if update.ShouldUpdate() { + changes.UpdateNew = append(changes.UpdateNew, update.desired) + changes.UpdateOld = append(changes.UpdateOld, update.current) + } + } + + return changes +} + +// calculateDesired changes the value of update.desired based on all information (desired/current/previous) available about the endpoint. +func (e *managedRecordSetChanges) calculateDesired(update *endpointUpdate) { + if e.ownerID == "" { + log.Infof("skipping update of desired for %s, no ownerID set for plan", update.desired.DNSName) + return + } + + // If the record is using a `SetIdentifier` the provider will only ever allow a single target value for any record type (A or CNAME) + // In the case just return and the desired target value will be used (i.e. AWS route53 geo or weighted records) + if update.current.SetIdentifier != "" { + log.Infof("skipping update of desired for %s, has SetIdentifier", update.desired.DNSName) + return + } + + currentCopy := update.current.DeepCopy() + + // A Records can be merged, but we remove the known previous target values first in order to ensure potentially stale values are removed + if update.current.RecordType == endpoint.RecordTypeA { + if update.previous != nil { + removeEndpointTargets(update.previous.Targets, currentCopy) + } + mergeEndpointTargets(update.desired, currentCopy) + } + + // CNAME records can be merged, it's expected that the provider implementation understands that a CNAME might have + // multiple target values and adjusts accordingly during apply. + if update.current.RecordType == endpoint.RecordTypeCNAME { + mergeEndpointTargets(update.desired, currentCopy) + + desiredCopy := update.desired.DeepCopy() + + // Calculate if any of the new desired targets are also managed dnsNames within this record set. + // If a target is not managed, do nothing and continue with the current targets. + // If a target is managed: + // - If after the update the dnsName will no longer be owned by this endpoint(update.desired), remove it from the list of targets. + // - If after the update the dnsName will have no owners (it's going to be deleted), remove it from the list of targets. + for idx := range desiredCopy.Targets { + t := desiredCopy.Targets[idx] + tDNSName := normalizeDNSName(t) + log.Infof("checking target %s owners", t) + if tOwners, tIsManaged := e.dnsNameOwners[tDNSName]; tIsManaged { + log.Infof("target dnsName %s is managed and has owners %v", tDNSName, tOwners) + + // If the target has no owners we can just remove it + if len(tOwners) == 0 { + removeEndpointTarget(t, update.desired) + break + } + + // Remove the target if there is no mutual ownership between the desired endpoint and the managed target + if eOwners, eIsManaged := e.dnsNameOwners[normalizeDNSName(desiredCopy.DNSName)]; eIsManaged { + hasMutualOwner := false + for _, ownerID := range eOwners { + if slices.Contains(tOwners, ownerID) { + hasMutualOwner = true + break + } + } + if !hasMutualOwner { + removeEndpointTarget(t, update.desired) + } + } + + } + } + } +} + +func removeEndpointTarget(target string, endpoint *endpoint.Endpoint) { + removeEndpointTargets([]string{target}, endpoint) +} + +func removeEndpointTargets(targets []string, endpoint *endpoint.Endpoint) { + undesiredMap := map[string]string{} + for idx := range targets { + undesiredMap[targets[idx]] = targets[idx] + } + desiredTargets := []string{} + for idx := range endpoint.Targets { + if _, ok := undesiredMap[endpoint.Targets[idx]]; ok { + endpoint.DeleteProviderSpecificProperty(endpoint.Targets[idx]) + } else { + desiredTargets = append(desiredTargets, endpoint.Targets[idx]) + } + } + endpoint.Targets = desiredTargets +} + +func mergeEndpointTargets(desired, current *endpoint.Endpoint) { + desired.Targets = append(desired.Targets, current.Targets...) + slices.Sort(desired.Targets) + desired.Targets = slices.Compact[[]string, string](desired.Targets) + + for idx := range desired.Targets { + if val, ok := current.GetProviderSpecificProperty(desired.Targets[idx]); ok { + desired.DeleteProviderSpecificProperty(desired.Targets[idx]) + desired.SetProviderSpecificProperty(desired.Targets[idx], val) + } + } +} + func targetChanged(desired, current *endpoint.Endpoint) bool { return !desired.Targets.Same(current.Targets) } +func shouldUpdateOwner(desired, current *endpoint.Endpoint) bool { + currentOwner, hasCurrentOwner := current.Labels[endpoint.OwnerLabelKey] + desiredOwner, hasDesiredOwner := desired.Labels[endpoint.OwnerLabelKey] + if hasCurrentOwner && hasDesiredOwner { + return currentOwner != desiredOwner + } + return false +} + func shouldUpdateTTL(desired, current *endpoint.Endpoint) bool { if !desired.RecordTTL.IsConfigured() { return false @@ -269,7 +483,7 @@ func shouldUpdateTTL(desired, current *endpoint.Endpoint) bool { return desired.RecordTTL != current.RecordTTL } -func (p *Plan) shouldUpdateProviderSpecific(desired, current *endpoint.Endpoint) bool { +func shouldUpdateProviderSpecific(desired, current *endpoint.Endpoint) bool { desiredProperties := map[string]endpoint.ProviderSpecificProperty{} for _, d := range desired.ProviderSpecific { @@ -323,31 +537,6 @@ func normalizeDNSName(dnsName string) string { return s } -// CompareBoolean is an implementation of PropertyComparator for comparing boolean-line values -// For example external-dns.alpha.kubernetes.io/cloudflare-proxied: "true" -// If value doesn't parse as boolean, the defaultValue is used -func CompareBoolean(defaultValue bool, name, current, previous string) bool { - var err error - - v1, v2 := defaultValue, defaultValue - - if previous != "" { - v1, err = strconv.ParseBool(previous) - if err != nil { - v1 = defaultValue - } - } - - if current != "" { - v2, err = strconv.ParseBool(current) - if err != nil { - v2 = defaultValue - } - } - - return v1 == v2 -} - func IsManagedRecord(record string, managedRecords, excludeRecords []string) bool { for _, r := range excludeRecords { if record == r { diff --git a/internal/external-dns/plan/plan_test.go b/internal/external-dns/plan/plan_test.go index 34c06a0..d2c69ad 100644 --- a/internal/external-dns/plan/plan_test.go +++ b/internal/external-dns/plan/plan_test.go @@ -55,6 +55,18 @@ type PlanTestSuite struct { domainFilterFiltered2 *endpoint.Endpoint domainFilterFiltered3 *endpoint.Endpoint domainFilterExcluded *endpoint.Endpoint + fooA1OwnerNone *endpoint.Endpoint + fooA2OwnerNone *endpoint.Endpoint + barA3OwnerNone *endpoint.Endpoint + barA4OwnerNone *endpoint.Endpoint + fooA1Owner1 *endpoint.Endpoint + fooA2Owner1 *endpoint.Endpoint + fooA2Owner2 *endpoint.Endpoint + fooA12Owner12 *endpoint.Endpoint + barA3Owner1 *endpoint.Endpoint + barA3Owner2 *endpoint.Endpoint + barA4Owner1 *endpoint.Endpoint + barA4Owner2 *endpoint.Endpoint } func (suite *PlanTestSuite) SetupTest() { @@ -247,6 +259,91 @@ func (suite *PlanTestSuite) SetupTest() { Targets: endpoint.Targets{"1.1.1.1"}, RecordType: "A", } + //Muti Owner + suite.fooA1OwnerNone = &endpoint.Endpoint{ + DNSName: "foo", + Targets: endpoint.Targets{"1.1.1.1"}, + RecordType: "A", + } + suite.fooA2OwnerNone = &endpoint.Endpoint{ + DNSName: "foo", + Targets: endpoint.Targets{"2.2.2.2"}, + RecordType: "A", + } + suite.barA3OwnerNone = &endpoint.Endpoint{ + DNSName: "bar", + Targets: endpoint.Targets{"3.3.3.3"}, + RecordType: "A", + } + suite.barA4OwnerNone = &endpoint.Endpoint{ + DNSName: "bar", + Targets: endpoint.Targets{"4.4.4.4"}, + RecordType: "A", + } + suite.fooA1Owner1 = &endpoint.Endpoint{ + DNSName: "foo", + Targets: endpoint.Targets{"1.1.1.1"}, + RecordType: "A", + Labels: map[string]string{ + endpoint.OwnerLabelKey: "owner1", + }, + } + suite.fooA2Owner1 = &endpoint.Endpoint{ + DNSName: "foo", + Targets: endpoint.Targets{"2.2.2.2"}, + RecordType: "A", + Labels: map[string]string{ + endpoint.OwnerLabelKey: "owner1", + }, + } + suite.fooA2Owner2 = &endpoint.Endpoint{ + DNSName: "foo", + Targets: endpoint.Targets{"2.2.2.2"}, + RecordType: "A", + Labels: map[string]string{ + endpoint.OwnerLabelKey: "owner2", + }, + } + suite.fooA12Owner12 = &endpoint.Endpoint{ + DNSName: "foo", + Targets: endpoint.Targets{"1.1.1.1", "2.2.2.2"}, + RecordType: "A", + Labels: map[string]string{ + endpoint.OwnerLabelKey: "owner1&&owner2", + }, + } + suite.barA3Owner1 = &endpoint.Endpoint{ + DNSName: "bar", + Targets: endpoint.Targets{"3.3.3.3"}, + RecordType: "A", + Labels: map[string]string{ + endpoint.OwnerLabelKey: "owner1", + }, + } + suite.barA3Owner2 = &endpoint.Endpoint{ + DNSName: "bar", + Targets: endpoint.Targets{"3.3.3.3"}, + RecordType: "A", + Labels: map[string]string{ + endpoint.OwnerLabelKey: "owner2", + }, + } + suite.barA4Owner1 = &endpoint.Endpoint{ + DNSName: "bar", + Targets: endpoint.Targets{"4.4.4.4"}, + RecordType: "A", + Labels: map[string]string{ + endpoint.OwnerLabelKey: "owner1", + }, + } + suite.barA4Owner2 = &endpoint.Endpoint{ + DNSName: "bar", + Targets: endpoint.Targets{"4.4.4.4"}, + RecordType: "A", + Labels: map[string]string{ + endpoint.OwnerLabelKey: "owner2", + }, + } } func (suite *PlanTestSuite) TestSyncFirstRound() { @@ -425,6 +522,7 @@ func (suite *PlanTestSuite) TestSyncSecondRoundWithProviderSpecificAddition() { } func (suite *PlanTestSuite) TestSyncSecondRoundWithOwnerInherited() { + suite.T().Skip("Skipping incompatible test") current := []*endpoint.Endpoint{suite.fooV1Cname} desired := []*endpoint.Endpoint{suite.fooV2Cname} @@ -484,6 +582,7 @@ func (suite *PlanTestSuite) TestIdempotency() { } func (suite *PlanTestSuite) TestRecordTypeChange() { + suite.T().Skip("Skipping incompatible test, plan does not allow record types to change") current := []*endpoint.Endpoint{suite.fooV1Cname} desired := []*endpoint.Endpoint{suite.fooA5} expectedCreate := []*endpoint.Endpoint{suite.fooA5} @@ -510,6 +609,7 @@ func (suite *PlanTestSuite) TestRecordTypeChange() { } func (suite *PlanTestSuite) TestExistingCNameWithDualStackDesired() { + suite.T().Skip("Skipping incompatible test, plan does not allow record types to change") current := []*endpoint.Endpoint{suite.fooV1Cname} desired := []*endpoint.Endpoint{suite.fooA5, suite.fooAAAA} expectedCreate := []*endpoint.Endpoint{suite.fooA5, suite.fooAAAA} @@ -536,6 +636,7 @@ func (suite *PlanTestSuite) TestExistingCNameWithDualStackDesired() { } func (suite *PlanTestSuite) TestExistingDualStackWithCNameDesired() { + suite.T().Skip("Skipping incompatible test, plan does not allow record types to change") suite.fooA5.Labels[endpoint.OwnerLabelKey] = "nerf" suite.fooAAAA.Labels[endpoint.OwnerLabelKey] = "nerf" current := []*endpoint.Endpoint{suite.fooA5, suite.fooAAAA} @@ -599,6 +700,7 @@ func (suite *PlanTestSuite) TestExistingOwnerNotMatchingDualStackDesired() { // caching issues. In this case since the desired records are not conflicting // the updates will end up with the conflict resolved. func (suite *PlanTestSuite) TestConflictingCurrentNonConflictingDesired() { + suite.T().Skip("Skipping incompatible test, plan does not allow record types to change") suite.fooA5.Labels[endpoint.OwnerLabelKey] = suite.fooV1Cname.Labels[endpoint.OwnerLabelKey] current := []*endpoint.Endpoint{suite.fooV1Cname, suite.fooA5} desired := []*endpoint.Endpoint{suite.fooA5} @@ -660,6 +762,7 @@ func (suite *PlanTestSuite) TestConflictingCurrentNoDesired() { // This could be the result of multiple sources generating conflicting records types. In this case the conflict // resolver should prefer the A and AAAA record candidate and delete the other records. func (suite *PlanTestSuite) TestCurrentWithConflictingDesired() { + suite.T().Skip("Skipping incompatible test, plan does not allow record types to change") suite.fooV1Cname.Labels[endpoint.OwnerLabelKey] = "nerf" current := []*endpoint.Endpoint{suite.fooV1Cname} desired := []*endpoint.Endpoint{suite.fooV1Cname, suite.fooA5, suite.fooAAAA} @@ -1013,6 +1116,7 @@ func (suite *PlanTestSuite) TestDualStackRecordsDelete() { } func (suite *PlanTestSuite) TestDualStackToSingleStack() { + suite.T().Skip("Skipping incompatible test, plan does not allow record types to change") current := []*endpoint.Endpoint{suite.dsA, suite.dsAAAA} desired := []*endpoint.Endpoint{suite.dsA} expectedDelete := []*endpoint.Endpoint{suite.dsAAAA} @@ -1035,6 +1139,126 @@ func (suite *PlanTestSuite) TestDualStackToSingleStack() { validateChanges(suite.T(), changes, expectedChanges) } +// Should create record with plan owner (Plan.OwnerID == "owner2") +func (suite *PlanTestSuite) TestMultiOwnerARecordCreate() { + current := []*endpoint.Endpoint{} + desired := []*endpoint.Endpoint{suite.fooA1Owner1} + expectedChanges := &plan.Changes{ + Create: []*endpoint.Endpoint{suite.fooA1Owner1}, + UpdateOld: []*endpoint.Endpoint{}, + UpdateNew: []*endpoint.Endpoint{}, + Delete: []*endpoint.Endpoint{}, + } + + p := &Plan{ + OwnerID: "owner1", + Policies: []Policy{&SyncPolicy{}}, + Current: current, + Desired: desired, + ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeAAAA, endpoint.RecordTypeCNAME}, + } + + changes := p.Calculate().Changes + validateChanges(suite.T(), changes, expectedChanges) +} + +// Should allow owned records(fooA1Owner1) to be updated from the creation of other owned (Plan.OwnerID == "owner2") records (fooA2OwnerNone) with the same dns name (foo). +func (suite *PlanTestSuite) TestMultiOwnerARecordUpdateCreate() { + current := []*endpoint.Endpoint{suite.fooA1Owner1, suite.barA3Owner2} + previous := []*endpoint.Endpoint{suite.barA3Owner2} + desired := []*endpoint.Endpoint{suite.fooA2OwnerNone, suite.barA3Owner2} + expectedChanges := &plan.Changes{ + Create: []*endpoint.Endpoint{}, + UpdateOld: []*endpoint.Endpoint{suite.fooA1Owner1}, + UpdateNew: []*endpoint.Endpoint{suite.fooA12Owner12}, + Delete: []*endpoint.Endpoint{}, + } + + p := &Plan{ + OwnerID: "owner2", + Policies: []Policy{&SyncPolicy{}}, + Current: current, + Previous: previous, + Desired: desired, + ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeAAAA, endpoint.RecordTypeCNAME}, + } + + changes := p.Calculate().Changes + validateChanges(suite.T(), changes, expectedChanges) +} + +// Should allow owned records(barA3Owner2) to be updated by the plan owner (Plan.OwnerID == "owner2"). +func (suite *PlanTestSuite) TestMultiOwnerARecordUpdate() { + current := []*endpoint.Endpoint{suite.barA3Owner2} + previous := []*endpoint.Endpoint{suite.barA3Owner2} + desired := []*endpoint.Endpoint{suite.barA4OwnerNone} + expectedChanges := &plan.Changes{ + Create: []*endpoint.Endpoint{}, + UpdateOld: []*endpoint.Endpoint{suite.barA3Owner2}, + UpdateNew: []*endpoint.Endpoint{suite.barA4Owner2}, + Delete: []*endpoint.Endpoint{}, + } + + p := &Plan{ + OwnerID: "owner2", + Policies: []Policy{&SyncPolicy{}}, + Current: current, + Previous: previous, + Desired: desired, + ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeAAAA, endpoint.RecordTypeCNAME}, + } + + changes := p.Calculate().Changes + validateChanges(suite.T(), changes, expectedChanges) +} + +// Should not allow owned records to be updated by unowned records (Plan.OwnerID == "") +func (suite *PlanTestSuite) TestNoOwnerARecordUpdate() { + current := []*endpoint.Endpoint{suite.fooA1Owner1} + desired := []*endpoint.Endpoint{suite.fooA2OwnerNone} + expectedChanges := &plan.Changes{ + Create: []*endpoint.Endpoint{}, + UpdateOld: []*endpoint.Endpoint{}, + UpdateNew: []*endpoint.Endpoint{}, + Delete: []*endpoint.Endpoint{}, + } + + p := &Plan{ + Policies: []Policy{&SyncPolicy{}}, + Current: current, + Desired: desired, + ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeAAAA, endpoint.RecordTypeCNAME}, + } + + changes := p.Calculate().Changes + validateChanges(suite.T(), changes, expectedChanges) +} + +// Should only delete records owned by plan owner (Plan.OwnerID == "owner2") and update records with shared ownership. +func (suite *PlanTestSuite) TestMultiOwnerARecordDelete() { + current := []*endpoint.Endpoint{suite.barA3Owner1, suite.barA4Owner2, suite.fooA12Owner12} + previous := []*endpoint.Endpoint{suite.barA4Owner2, suite.fooA2Owner2} + desired := []*endpoint.Endpoint{} + expectedChanges := &plan.Changes{ + Create: []*endpoint.Endpoint{}, + UpdateOld: []*endpoint.Endpoint{suite.fooA12Owner12}, + UpdateNew: []*endpoint.Endpoint{suite.fooA1Owner1}, + Delete: []*endpoint.Endpoint{suite.barA4Owner2}, + } + + p := &Plan{ + OwnerID: "owner2", + Policies: []Policy{&SyncPolicy{}}, + Current: current, + Desired: desired, + Previous: previous, + ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeAAAA, endpoint.RecordTypeCNAME}, + } + + changes := p.Calculate().Changes + validateChanges(suite.T(), changes, expectedChanges) +} + func TestPlan(t *testing.T) { suite.Run(t, new(PlanTestSuite)) } @@ -1197,12 +1421,7 @@ func TestShouldUpdateProviderSpecific(tt *testing.T) { }, } { tt.Run(test.name, func(t *testing.T) { - plan := &Plan{ - Current: []*endpoint.Endpoint{test.current}, - Desired: []*endpoint.Endpoint{test.desired}, - ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME}, - } - b := plan.shouldUpdateProviderSpecific(test.desired, test.current) + b := shouldUpdateProviderSpecific(test.desired, test.current) assert.Equal(t, test.shouldUpdate, b) }) } diff --git a/internal/external-dns/registry/txt.go b/internal/external-dns/registry/txt.go index 004be79..604e192 100644 --- a/internal/external-dns/registry/txt.go +++ b/internal/external-dns/registry/txt.go @@ -246,9 +246,10 @@ func (im *TXTRegistry) generateTXTRecord(r *endpoint.Endpoint) []*endpoint.Endpo // for each created/deleted record it will also take into account TXT records for creation/deletion func (im *TXTRegistry) ApplyChanges(ctx context.Context, changes *plan.Changes) error { filteredChanges := &plan.Changes{ - Create: changes.Create, - UpdateNew: endpoint.FilterEndpointsByOwnerID(im.ownerID, changes.UpdateNew), - UpdateOld: endpoint.FilterEndpointsByOwnerID(im.ownerID, changes.UpdateOld), + Create: changes.Create, + //ToDo Ideally we would still be able to ensure ownership on update + UpdateNew: changes.UpdateNew, + UpdateOld: changes.UpdateOld, Delete: endpoint.FilterEndpointsByOwnerID(im.ownerID, changes.Delete), } for _, r := range filteredChanges.Create { diff --git a/internal/external-dns/registry/txt_test.go b/internal/external-dns/registry/txt_test.go index 574d803..b2fb74d 100644 --- a/internal/external-dns/registry/txt_test.go +++ b/internal/external-dns/registry/txt_test.go @@ -841,9 +841,7 @@ func testTXTRegistryApplyChangesNoPrefix(t *testing.T) { newEndpointWithOwner("txt.bar.test-zone.example.org", "baz.test-zone.example.org", endpoint.RecordTypeCNAME, ""), newEndpointWithOwner("qux.test-zone.example.org", "random", endpoint.RecordTypeTXT, ""), newEndpointWithOwner("tar.test-zone.example.org", "tar.loadbalancer.com", endpoint.RecordTypeCNAME, ""), - newEndpointWithOwner("txt.tar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), newEndpointWithOwner("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME, ""), - newEndpointWithOwner("foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), newEndpointWithOwner("cname-foobar.test-zone.example.org", "\"heritage=external-dns,external-dns/owner=owner\"", endpoint.RecordTypeTXT, ""), }, }) @@ -858,12 +856,8 @@ func testTXTRegistryApplyChangesNoPrefix(t *testing.T) { Delete: []*endpoint.Endpoint{ newEndpointWithOwner("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner"), }, - UpdateNew: []*endpoint.Endpoint{ - newEndpointWithOwner("tar.test-zone.example.org", "new-tar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner-2"), - }, - UpdateOld: []*endpoint.Endpoint{ - newEndpointWithOwner("tar.test-zone.example.org", "tar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner-2"), - }, + UpdateNew: []*endpoint.Endpoint{}, + UpdateOld: []*endpoint.Endpoint{}, } expected := &plan.Changes{ Create: []*endpoint.Endpoint{ @@ -1414,12 +1408,8 @@ func TestNewTXTScheme(t *testing.T) { Delete: []*endpoint.Endpoint{ newEndpointWithOwner("foobar.test-zone.example.org", "foobar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner"), }, - UpdateNew: []*endpoint.Endpoint{ - newEndpointWithOwner("tar.test-zone.example.org", "new-tar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner-2"), - }, - UpdateOld: []*endpoint.Endpoint{ - newEndpointWithOwner("tar.test-zone.example.org", "tar.loadbalancer.com", endpoint.RecordTypeCNAME, "owner-2"), - }, + UpdateNew: []*endpoint.Endpoint{}, + UpdateOld: []*endpoint.Endpoint{}, } expected := &plan.Changes{ Create: []*endpoint.Endpoint{