From d8284253f5359bbab95e0736f0d7b146be740554 Mon Sep 17 00:00:00 2001
From: Douglas Mayle <dmayle@dmayle.com>
Date: Mon, 1 Jun 2020 15:16:45 +0200
Subject: [PATCH] Fix handling of DNS updates for RFC2136 provider.

When receiving an update, we should be removing the old record and
adding the new.  Previously, we were removing and readding the new
record, which would cause the old DNS record to be left lying around.
---
 provider/rfc2136/rfc2136.go      | 10 ++---
 provider/rfc2136/rfc2136_test.go | 69 ++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 5 deletions(-)

diff --git a/provider/rfc2136/rfc2136.go b/provider/rfc2136/rfc2136.go
index 2fb0327506..e716b6d818 100644
--- a/provider/rfc2136/rfc2136.go
+++ b/provider/rfc2136/rfc2136.go
@@ -221,14 +221,14 @@ func (r rfc2136Provider) ApplyChanges(ctx context.Context, changes *plan.Changes
 
 		r.AddRecord(m, ep)
 	}
-	for _, ep := range changes.UpdateNew {
+	for index, ep := range changes.UpdateNew {
 
 		if !r.domainFilter.Match(ep.DNSName) {
 			log.Debugf("Skipping record %s because it was filtered out by the specified --domain-filter", ep.DNSName)
 			continue
 		}
 
-		r.UpdateRecord(m, ep)
+		r.UpdateRecord(m, changes.UpdateOld[index], ep)
 	}
 	for _, ep := range changes.Delete {
 
@@ -251,13 +251,13 @@ func (r rfc2136Provider) ApplyChanges(ctx context.Context, changes *plan.Changes
 	return nil
 }
 
-func (r rfc2136Provider) UpdateRecord(m *dns.Msg, ep *endpoint.Endpoint) error {
-	err := r.RemoveRecord(m, ep)
+func (r rfc2136Provider) UpdateRecord(m *dns.Msg, oldEp *endpoint.Endpoint, newEp *endpoint.Endpoint) error {
+	err := r.RemoveRecord(m, oldEp)
 	if err != nil {
 		return err
 	}
 
-	return r.AddRecord(m, ep)
+	return r.AddRecord(m, newEp)
 }
 
 func (r rfc2136Provider) AddRecord(m *dns.Msg, ep *endpoint.Endpoint) error {
diff --git a/provider/rfc2136/rfc2136_test.go b/provider/rfc2136/rfc2136_test.go
index 87c0725efd..fa977a3afe 100644
--- a/provider/rfc2136/rfc2136_test.go
+++ b/provider/rfc2136/rfc2136_test.go
@@ -248,6 +248,75 @@ func TestRfc2136ApplyChangesWithDifferentTTLs(t *testing.T) {
 
 }
 
+func TestRfc2136ApplyChangesWithUpdate(t *testing.T) {
+	stub := newStub()
+
+	provider, err := createRfc2136StubProvider(stub)
+	assert.NoError(t, err)
+
+	p := &plan.Changes{
+		Create: []*endpoint.Endpoint{
+			{
+				DNSName:    "v1.foo.com",
+				RecordType: "A",
+				Targets:    []string{"1.2.3.4"},
+				RecordTTL:  endpoint.TTL(400),
+			},
+			{
+				DNSName:    "v1.foobar.com",
+				RecordType: "TXT",
+				Targets:    []string{"boom"},
+			},
+		},
+	}
+
+	err = provider.ApplyChanges(context.Background(), p)
+	assert.NoError(t, err)
+
+	p = &plan.Changes{
+		UpdateOld: []*endpoint.Endpoint{
+			{
+				DNSName:    "v1.foo.com",
+				RecordType: "A",
+				Targets:    []string{"1.2.3.4"},
+				RecordTTL:  endpoint.TTL(400),
+			},
+			{
+				DNSName:    "v1.foobar.com",
+				RecordType: "TXT",
+				Targets:    []string{"boom"},
+			},
+		},
+		UpdateNew: []*endpoint.Endpoint{
+			{
+				DNSName:    "v1.foo.com",
+				RecordType: "A",
+				Targets:    []string{"1.2.3.5"},
+				RecordTTL:  endpoint.TTL(400),
+			},
+			{
+				DNSName:    "v1.foobar.com",
+				RecordType: "TXT",
+				Targets:    []string{"kablui"},
+			},
+		},
+	}
+
+	err = provider.ApplyChanges(context.Background(), p)
+	assert.NoError(t, err)
+
+	createRecords := extractAuthoritySectionFromMessage(stub.createMsgs[0])
+	assert.Equal(t, 2, len(createRecords))
+	assert.True(t, strings.Contains(createRecords[0], "v1.foo.com"))
+	assert.False(t, strings.Contains(createRecords[0], "1.2.3.4"))
+	assert.True(t, strings.Contains(createRecords[0], "1.2.3.5"))
+	assert.True(t, strings.Contains(createRecords[0], "400"))
+	assert.True(t, strings.Contains(createRecords[1], "v1.foobar.com"))
+	assert.False(t, strings.Contains(createRecords[1], "boom"))
+	assert.True(t, strings.Contains(createRecords[1], "kablui"))
+
+}
+
 func contains(arr []*endpoint.Endpoint, name string) bool {
 	for _, a := range arr {
 		if a.DNSName == name {