fix: Multiple same host/target records

Fixes an issue where if you had multiple records with the same host and
target values all contributing to the same record set, removing one of
them would cause invalid records (0 target values) to be returned from
the plan.

Fixes an issue in the inmemory provider where it was allowing records
with 0 target values to be added, and improves checks to avoid a panic.

Signed-off-by: Michael Nairn <[email protected]>

internal/external-dns/plan/plan.go

@@ -279,15 +279,6 @@ func (p *Plan) Calculate() *Plan {
 					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.previous != nil && len(candidate.Targets) > 1 {
-							removeEndpointTargets(records.previous.Targets, candidate)
-						}
-						//ToDo Need a test that tests the deletion of a record with two owners who are adding the same value
-						// If you delete one owner record, it currently removes the endpoint form desired causing an invalid record
-
 						managedChanges.updates = append(managedChanges.updates, &endpointUpdate{desired: candidate, current: records.current, previous: records.previous, isDelete: true})
 						managedChanges.dnsNameOwners[key.dnsName] = append(managedChanges.dnsNameOwners[key.dnsName], owners...)
 					}
@@ -505,6 +496,18 @@ func (e *managedRecordSetChanges) calculateDesired(update *endpointUpdate) {
 		return
 	}
 
+	// If we are deleting we need to remove the previous target values, but not if we are going to be left with none.
+	// This can happen if you are creating multiple records with the same hostname on the same cluster since they will
+	// all have the same addresses. If we get to this point we also know there is still another owner so removing all the
+	// target values is never going to be correct.
+	if update.isDelete && update.previous != nil {
+		desiredCopy := update.desired.DeepCopy()
+		removeEndpointTargets(update.previous.Targets, desiredCopy)
+		if len(desiredCopy.Targets) > 0 {
+			update.desired.Targets = desiredCopy.Targets
+		}
+	}
+
 	currentCopy := update.current.DeepCopy()
 	desiredCopy := update.desired.DeepCopy()
 

internal/external-dns/plan/plan_test.go

@@ -62,6 +62,8 @@ type PlanTestSuite struct {
 	barA3OwnerNone *endpoint.Endpoint
 	barA4OwnerNone *endpoint.Endpoint
 	fooA1Owner1    *endpoint.Endpoint
+	fooA1Owner2    *endpoint.Endpoint
+	fooA1Owner12   *endpoint.Endpoint
 	fooA2Owner1    *endpoint.Endpoint
 	fooA2Owner2    *endpoint.Endpoint
 	fooA12Owner12  *endpoint.Endpoint
@@ -324,6 +326,22 @@ func (suite *PlanTestSuite) SetupTest() {
 			endpoint.OwnerLabelKey: "owner1",
 		},
 	}
+	suite.fooA1Owner2 = &endpoint.Endpoint{
+		DNSName:    "foo",
+		RecordType: "A",
+		Targets:    endpoint.Targets{"1.1.1.1"},
+		Labels: map[string]string{
+			endpoint.OwnerLabelKey: "owner2",
+		},
+	}
+	suite.fooA1Owner12 = &endpoint.Endpoint{
+		DNSName:    "foo",
+		RecordType: "A",
+		Targets:    endpoint.Targets{"1.1.1.1"},
+		Labels: map[string]string{
+			endpoint.OwnerLabelKey: "owner1&&owner2",
+		},
+	}
 	suite.fooA2Owner1 = &endpoint.Endpoint{
 		DNSName:    "foo",
 		RecordType: "A",
@@ -1679,6 +1697,32 @@ func (suite *PlanTestSuite) TestMultiOwnerARecordDelete() {
 	assert.Empty(suite.T(), cp.Errors)
 }
 
+// Should delete record with the same host and target from two owners.
+func (suite *PlanTestSuite) TestMultiOwnerARecordDeleteSameAddress() {
+	current := []*endpoint.Endpoint{suite.fooA1Owner12}
+	previous := []*endpoint.Endpoint{suite.fooA1Owner2}
+	desired := []*endpoint.Endpoint{}
+	expectedChanges := &plan.Changes{
+		Create:    []*endpoint.Endpoint{},
+		UpdateOld: []*endpoint.Endpoint{suite.fooA1Owner12.DeepCopy()},
+		UpdateNew: []*endpoint.Endpoint{suite.fooA1Owner1.DeepCopy()},
+		Delete:    []*endpoint.Endpoint{},
+	}
+
+	p := &Plan{
+		OwnerID:        "owner2",
+		Policies:       []Policy{&SyncPolicy{}},
+		Current:        current,
+		Desired:        desired,
+		Previous:       previous,
+		ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeAAAA, endpoint.RecordTypeCNAME},
+	}
+
+	cp := p.Calculate()
+	validateChanges(suite.T(), cp.Changes, expectedChanges)
+	assert.Empty(suite.T(), cp.Errors)
+}
+
 //CNAME Records
 
 // Should create record with plan owner.

internal/external-dns/provider/inmemory/inmemory.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"reflect"
 	"strings"
 	"sync"
 
@@ -42,6 +43,8 @@ var (
 	ErrRecordNotFound = errors.New("record not found")
 	// ErrDuplicateRecordFound when record is repeated in create/update/delete
 	ErrDuplicateRecordFound = errors.New("invalid batch request")
+	// ErrNoTargetValue when record has no target values in create/update
+	ErrNoTargetValue = errors.New("record has no target values")
 )
 
 // InMemoryProvider - dns provider only used for testing purposes
@@ -378,6 +381,9 @@ func (c *InMemoryClient) validateChangeBatch(zone string, changes *plan.Changes)
 	}
 	mesh := sets.New[endpoint.EndpointKey]()
 	for _, newEndpoint := range changes.Create {
+		if len(newEndpoint.Targets) == 0 {
+			return ErrNoTargetValue
+		}
 		if _, exists := curZone[newEndpoint.Key()]; exists {
 			return ErrRecordAlreadyExists
 		}
@@ -386,6 +392,9 @@ func (c *InMemoryClient) validateChangeBatch(zone string, changes *plan.Changes)
 		}
 	}
 	for _, updateEndpoint := range changes.UpdateNew {
+		if len(updateEndpoint.Targets) == 0 {
+			return ErrNoTargetValue
+		}
 		if _, exists := curZone[updateEndpoint.Key()]; !exists {
 			return ErrRecordNotFound
 		}
@@ -394,12 +403,12 @@ func (c *InMemoryClient) validateChangeBatch(zone string, changes *plan.Changes)
 		}
 	}
 	for _, updateOldEndpoint := range changes.UpdateOld {
-		if rec, exists := curZone[updateOldEndpoint.Key()]; !exists || rec.Targets[0] != updateOldEndpoint.Targets[0] {
+		if rec, exists := curZone[updateOldEndpoint.Key()]; !exists || !reflect.DeepEqual(rec.Targets, updateOldEndpoint.Targets) {
 			return ErrRecordNotFound
 		}
 	}
 	for _, deleteEndpoint := range changes.Delete {
-		if rec, exists := curZone[deleteEndpoint.Key()]; !exists || rec.Targets[0] != deleteEndpoint.Targets[0] {
+		if rec, exists := curZone[deleteEndpoint.Key()]; !exists || !reflect.DeepEqual(rec.Targets, deleteEndpoint.Targets) {
 			return ErrRecordNotFound
 		}
 		if err := c.updateMesh(mesh, deleteEndpoint); err != nil {