diff --git a/Makefile b/Makefile index b1186822..b4097c50 100644 --- a/Makefile +++ b/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. diff --git a/internal/external-dns/plan/plan.go b/internal/external-dns/plan/plan.go index 5207039f..64018469 100644 --- a/internal/external-dns/plan/plan.go +++ b/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() diff --git a/internal/external-dns/plan/plan_test.go b/internal/external-dns/plan/plan_test.go index 5141c2d9..7da5c45c 100644 --- a/internal/external-dns/plan/plan_test.go +++ b/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. diff --git a/internal/external-dns/provider/inmemory/inmemory.go b/internal/external-dns/provider/inmemory/inmemory.go index 732b1d48..1f2a8672 100644 --- a/internal/external-dns/provider/inmemory/inmemory.go +++ b/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 {