From d9622ba1a855d30f8d4e92fea687f7e19d28f889 Mon Sep 17 00:00:00 2001 From: Michael Nairn Date: Fri, 25 Oct 2024 14:31:56 +0100 Subject: [PATCH 1/2] 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 --- internal/external-dns/plan/plan.go | 21 +++++---- internal/external-dns/plan/plan_test.go | 44 +++++++++++++++++++ .../provider/inmemory/inmemory.go | 13 +++++- 3 files changed, 67 insertions(+), 11 deletions(-) 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 { From c2a3c3a76a3cbd70dfcabbb352718bc1ee583d32 Mon Sep 17 00:00:00 2001 From: Michael Nairn Date: Fri, 25 Oct 2024 14:35:47 +0100 Subject: [PATCH 2/2] dev: Add `--race` option to `make run` task Enables the race detector on locally running instances https://go.dev/doc/articles/race_detector. This will report useful information in the logs about race related issues. Signed-off-by: Michael Nairn --- Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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.