Merge pull request #284 from mikenairn/fix_plan_and_inmemory_provider

fix: Multiple same host/target records

Makefile

@@ -239,13 +239,13 @@ build: manifests generate fmt vet ## Build manager binary.
 run: GIT_SHA=$(shell git rev-parse HEAD || echo "unknown")
 run: DIRTY=$(shell hack/check-git-dirty.sh || echo "unknown")
 run: manifests generate fmt vet ## Run a controller from your host.
-	go run -ldflags "-X main.gitSHA=${GIT_SHA} -X main.dirty=${DIRTY}" ./cmd/main.go --zap-devel --provider inmemory,aws,google,azure
+	go run -ldflags "-X main.gitSHA=${GIT_SHA} -X main.dirty=${DIRTY}" --race ./cmd/main.go --zap-devel --provider inmemory,aws,google,azure
 
 .PHONY: run-with-probes
 run-with-probes: GIT_SHA=$(shell git rev-parse HEAD || echo "unknown")
 run-with-probes: DIRTY=$(shell hack/check-git-dirty.sh || echo "unknown")
 run-with-probes: manifests generate fmt vet ## Run a controller from your host.
-	go run -ldflags "-X main.gitSHA=${GIT_SHA} -X main.dirty=${DIRTY}" ./cmd/main.go --zap-devel --provider inmemory,aws,google,azure
+	go run -ldflags "-X main.gitSHA=${GIT_SHA} -X main.dirty=${DIRTY}" --race  ./cmd/main.go --zap-devel --provider inmemory,aws,google,azure
 
 # If you wish built the manager image targeting other platforms you can use the --platform flag.
 # (i.e. docker build --platform linux/arm64 ). However, you must enable docker buildKit for it.

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 {