Merge pull request #8464 from justinsb/google_clouddns_delete_records

DNS: Don't try to apply empty changesets
kubernetes · May 17, 2020 · 2e5d476 · 2e5d476
2 parents bda2a15 + 3306549
commit 2e5d476
Show file tree

Hide file tree

Showing 19 changed files with 296 additions and 84 deletions.
diff --git a/dns-controller/pkg/dns/dnscontroller.go b/dns-controller/pkg/dns/dnscontroller.go
@@ -17,6 +17,7 @@ limitations under the License.
 package dns
 
 import (
+	"context"
 	"fmt"
 	"time"
 
@@ -189,6 +190,8 @@ type recordKey struct {
 }
 
 func (c *DNSController) runOnce() error {
+	ctx := context.TODO()
+
 	snapshot := c.snapshotIfChangedAndReady()
 	if snapshot == nil {
 		// Unchanged / not ready
@@ -302,8 +305,12 @@ func (c *DNSController) runOnce() error {
 	}
 
 	for key, changeset := range op.changesets {
+		if changeset.IsEmpty() {
+			continue
+		}
+
 		klog.V(2).Infof("applying DNS changeset for zone %s", key)
-		if err := changeset.Apply(); err != nil {
+		if err := changeset.Apply(ctx); err != nil {
 			klog.Warningf("error applying DNS changeset for zone %s: %v", key, err)
 			errors = append(errors, fmt.Errorf("error applying DNS changeset for zone %s: %v", key, err))
 		}
@@ -321,6 +328,8 @@ func (c *DNSController) runOnce() error {
 }
 
 func (c *DNSController) RemoveRecordsImmediate(records []Record) error {
+	ctx := context.TODO()
+
 	op, err := newDNSOp(c.zoneRules, c.dnsCache)
 	if err != nil {
 		return err
@@ -344,7 +353,7 @@ func (c *DNSController) RemoveRecordsImmediate(records []Record) error {
 
 	for key, changeset := range op.changesets {
 		klog.V(2).Infof("applying DNS changeset for zone %s", key)
-		if err := changeset.Apply(); err != nil {
+		if err := changeset.Apply(ctx); err != nil {
 			klog.Warningf("error applying DNS changeset for zone %s: %v", key, err)
 			errors = append(errors, fmt.Errorf("error applying DNS changeset for zone %s: %v", key, err))
 		}

diff --git a/dnsprovider/pkg/dnsprovider/dns.go b/dnsprovider/pkg/dnsprovider/dns.go
@@ -17,6 +17,7 @@ limitations under the License.
 package dnsprovider
 
 import (
+	"context"
 	"reflect"
 
 	"k8s.io/kops/dnsprovider/pkg/dnsprovider/rrstype"
@@ -79,7 +80,8 @@ type ResourceRecordChangeset interface {
 	// If you have the pre-image, it will likely be more efficient to call Remove and Add.
 	Upsert(ResourceRecordSet) ResourceRecordChangeset
 	// Apply applies the accumulated operations to the Zone.
-	Apply() error
+	// Implementations should tolerate an empty changeset, and be a relatively quick no-op.
+	Apply(ctx context.Context) error
 	// IsEmpty returns true if there are no accumulated operations.
 	IsEmpty() bool
 	// ResourceRecordSets returns the parent ResourceRecordSets

diff --git a/dnsprovider/pkg/dnsprovider/providers/aws/route53/route53_test.go b/dnsprovider/pkg/dnsprovider/providers/aws/route53/route53_test.go
@@ -17,6 +17,7 @@ limitations under the License.
 package route53
 
 import (
+	"context"
 	"flag"
 	"fmt"
 	"os"
@@ -128,8 +129,8 @@ func getExampleRrs(zone dnsprovider.Zone) dnsprovider.ResourceRecordSet {
 	return rrsets.New("www11."+zone.Name(), []string{"10.10.10.10", "169.20.20.20"}, 180, rrstype.A)
 }
 
-func addRrsetOrFail(t *testing.T, rrsets dnsprovider.ResourceRecordSets, rrset dnsprovider.ResourceRecordSet) {
-	err := rrsets.StartChangeset().Add(rrset).Apply()
+func addRrsetOrFail(ctx context.Context, t *testing.T, rrsets dnsprovider.ResourceRecordSets, rrset dnsprovider.ResourceRecordSet) {
+	err := rrsets.StartChangeset().Add(rrset).Apply(ctx)
 	if err != nil {
 		t.Fatalf("Failed to add recordsets: %v", err)
 	}
@@ -180,21 +181,25 @@ func TestResourceRecordSetsList(t *testing.T) {
 
 /* TestResourceRecordSetsAddSuccess verifies that addition of a valid RRS succeeds */
 func TestResourceRecordSetsAddSuccess(t *testing.T) {
+	ctx := context.Background()
+
 	zone := firstZone(t)
 	sets := rrs(t, zone)
 	set := getExampleRrs(zone)
-	addRrsetOrFail(t, sets, set)
-	defer sets.StartChangeset().Remove(set).Apply()
+	addRrsetOrFail(ctx, t, sets, set)
+	defer sets.StartChangeset().Remove(set).Apply(ctx)
 	t.Logf("Successfully added resource record set: %v", set)
 }
 
 /* TestResourceRecordSetsAdditionVisible verifies that added RRS is visible after addition */
 func TestResourceRecordSetsAdditionVisible(t *testing.T) {
+	ctx := context.Background()
+
 	zone := firstZone(t)
 	sets := rrs(t, zone)
 	rrset := getExampleRrs(zone)
-	addRrsetOrFail(t, sets, rrset)
-	defer sets.StartChangeset().Remove(rrset).Apply()
+	addRrsetOrFail(ctx, t, sets, rrset)
+	defer sets.StartChangeset().Remove(rrset).Apply(ctx)
 	t.Logf("Successfully added resource record set: %v", rrset)
 	found := false
 	for _, record := range listRrsOrFail(t, sets) {
@@ -208,18 +213,20 @@ func TestResourceRecordSetsAdditionVisible(t *testing.T) {
 	}
 }
 
-/* TestResourceRecordSetsAddDuplicateFail verifies that addition of a duplicate RRS fails */
+/* TestResourceRecordSetsAddDuplicateFailure verifies that addition of a duplicate RRS fails */
 func TestResourceRecordSetsAddDuplicateFailure(t *testing.T) {
+	ctx := context.Background()
+
 	zone := firstZone(t)
 	sets := rrs(t, zone)
 	rrset := getExampleRrs(zone)
-	addRrsetOrFail(t, sets, rrset)
-	defer sets.StartChangeset().Remove(rrset).Apply()
+	addRrsetOrFail(ctx, t, sets, rrset)
+	defer sets.StartChangeset().Remove(rrset).Apply(ctx)
 	t.Logf("Successfully added resource record set: %v", rrset)
 	// Try to add it again, and verify that the call fails.
-	err := sets.StartChangeset().Add(rrset).Apply()
+	err := sets.StartChangeset().Add(rrset).Apply(ctx)
 	if err == nil {
-		defer sets.StartChangeset().Remove(rrset).Apply()
+		defer sets.StartChangeset().Remove(rrset).Apply(ctx)
 		t.Errorf("Should have failed to add duplicate resource record %v, but succeeded instead.", rrset)
 	} else {
 		t.Logf("Correctly failed to add duplicate resource record %v: %v", rrset, err)
@@ -228,14 +235,16 @@ func TestResourceRecordSetsAddDuplicateFailure(t *testing.T) {
 
 /* TestResourceRecordSetsRemove verifies that the removal of an existing RRS succeeds */
 func TestResourceRecordSetsRemove(t *testing.T) {
+	ctx := context.Background()
+
 	zone := firstZone(t)
 	sets := rrs(t, zone)
 	rrset := getExampleRrs(zone)
-	addRrsetOrFail(t, sets, rrset)
-	err := sets.StartChangeset().Remove(rrset).Apply()
+	addRrsetOrFail(ctx, t, sets, rrset)
+	err := sets.StartChangeset().Remove(rrset).Apply(ctx)
 	if err != nil {
 		// Try again to clean up.
-		defer sets.StartChangeset().Remove(rrset).Apply()
+		defer sets.StartChangeset().Remove(rrset).Apply(ctx)
 		t.Errorf("Failed to remove resource record set %v after adding", rrset)
 	} else {
 		t.Logf("Successfully removed resource set %v after adding", rrset)
@@ -244,14 +253,16 @@ func TestResourceRecordSetsRemove(t *testing.T) {
 
 /* TestResourceRecordSetsRemoveGone verifies that a removed RRS no longer exists */
 func TestResourceRecordSetsRemoveGone(t *testing.T) {
+	ctx := context.Background()
+
 	zone := firstZone(t)
 	sets := rrs(t, zone)
 	rrset := getExampleRrs(zone)
-	addRrsetOrFail(t, sets, rrset)
-	err := sets.StartChangeset().Remove(rrset).Apply()
+	addRrsetOrFail(ctx, t, sets, rrset)
+	err := sets.StartChangeset().Remove(rrset).Apply(ctx)
 	if err != nil {
 		// Try again to clean up.
-		defer sets.StartChangeset().Remove(rrset).Apply()
+		defer sets.StartChangeset().Remove(rrset).Apply(ctx)
 		t.Errorf("Failed to remove resource record set %v after adding", rrset)
 	} else {
 		t.Logf("Successfully removed resource set %v after adding", rrset)
@@ -287,3 +298,11 @@ func TestResourceRecordSetsDifferentTypes(t *testing.T) {
 	zone := firstZone(t)
 	tests.CommonTestResourceRecordSetsDifferentTypes(t, zone)
 }
+
+// TestContract verifies the general interface contract
+func TestContract(t *testing.T) {
+	zone := firstZone(t)
+	sets := rrs(t, zone)
+
+	tests.TestContract(t, sets)
+}
diff --git a/dnsprovider/pkg/dnsprovider/providers/aws/route53/rrchangeset.go b/dnsprovider/pkg/dnsprovider/providers/aws/route53/rrchangeset.go
@@ -18,6 +18,7 @@ package route53
 
 import (
 	"bytes"
+	"context"
 	"fmt"
 
 	"github.com/aws/aws-sdk-go/aws"
@@ -73,7 +74,12 @@ func buildChange(action string, rrs dnsprovider.ResourceRecordSet) *route53.Chan
 	return change
 }
 
-func (c *ResourceRecordChangeset) Apply() error {
+func (c *ResourceRecordChangeset) Apply(ctx context.Context) error {
+	// Empty changesets should be a relatively quick no-op
+	if c.IsEmpty() {
+		return nil
+	}
+
 	hostedZoneID := c.zone.impl.Id
 
 	removals := make(map[string]*route53.Change)

diff --git a/dnsprovider/pkg/dnsprovider/providers/coredns/coredns_test.go b/dnsprovider/pkg/dnsprovider/providers/coredns/coredns_test.go
@@ -17,6 +17,7 @@ limitations under the License.
 package coredns
 
 import (
+	"context"
 	"flag"
 	"fmt"
 	"os"
@@ -125,8 +126,8 @@ func getExampleRrs(zone dnsprovider.Zone) dnsprovider.ResourceRecordSet {
 	return rrsets.New("www11."+zone.Name(), []string{"10.10.10.10", "169.20.20.20"}, 180, rrstype.A)
 }
 
-func addRrsetOrFail(t *testing.T, rrsets dnsprovider.ResourceRecordSets, rrset dnsprovider.ResourceRecordSet) {
-	err := rrsets.StartChangeset().Add(rrset).Apply()
+func addRrsetOrFail(ctx context.Context, t *testing.T, rrsets dnsprovider.ResourceRecordSets, rrset dnsprovider.ResourceRecordSet) {
+	err := rrsets.StartChangeset().Add(rrset).Apply(ctx)
 	if err != nil {
 		t.Fatalf("Failed to add recordsets: %v", err)
 	}
@@ -154,11 +155,13 @@ func TestResourceRecordSetsGet(t *testing.T) {
 
 /* TestResourceRecordSetsAddSuccess verifies that addition of a valid RRS succeeds */
 func TestResourceRecordSetsAddSuccess(t *testing.T) {
+	ctx := context.Background()
+
 	zone := firstZone(t)
 	sets := rrs(t, zone)
 	set := getExampleRrs(zone)
-	addRrsetOrFail(t, sets, set)
-	defer sets.StartChangeset().Remove(set).Apply()
+	addRrsetOrFail(ctx, t, sets, set)
+	defer sets.StartChangeset().Remove(set).Apply(ctx)
 	t.Logf("Successfully added resource record set: %v", set)
 	if sets.Zone().ID() != zone.ID() {
 		t.Errorf("Zone for rrset does not match expected")
@@ -167,11 +170,13 @@ func TestResourceRecordSetsAddSuccess(t *testing.T) {
 
 /* TestResourceRecordSetsAdditionVisible verifies that added RRS is visible after addition */
 func TestResourceRecordSetsAdditionVisible(t *testing.T) {
+	ctx := context.Background()
+
 	zone := firstZone(t)
 	sets := rrs(t, zone)
 	rrset := getExampleRrs(zone)
-	addRrsetOrFail(t, sets, rrset)
-	defer sets.StartChangeset().Remove(rrset).Apply()
+	addRrsetOrFail(ctx, t, sets, rrset)
+	defer sets.StartChangeset().Remove(rrset).Apply(ctx)
 	t.Logf("Successfully added resource record set: %v", rrset)
 
 	record := getRrOrFail(t, sets, rrset.Name())
@@ -180,18 +185,20 @@ func TestResourceRecordSetsAdditionVisible(t *testing.T) {
 	}
 }
 
-/* TestResourceRecordSetsAddDuplicateFail verifies that addition of a duplicate RRS fails */
+/* TestResourceRecordSetsAddDuplicateFailure verifies that addition of a duplicate RRS fails */
 func TestResourceRecordSetsAddDuplicateFailure(t *testing.T) {
+	ctx := context.Background()
+
 	zone := firstZone(t)
 	sets := rrs(t, zone)
 	rrset := getExampleRrs(zone)
-	addRrsetOrFail(t, sets, rrset)
-	defer sets.StartChangeset().Remove(rrset).Apply()
+	addRrsetOrFail(ctx, t, sets, rrset)
+	defer sets.StartChangeset().Remove(rrset).Apply(ctx)
 	t.Logf("Successfully added resource record set: %v", rrset)
 	// Try to add it again, and verify that the call fails.
-	err := sets.StartChangeset().Add(rrset).Apply()
+	err := sets.StartChangeset().Add(rrset).Apply(ctx)
 	if err == nil {
-		defer sets.StartChangeset().Remove(rrset).Apply()
+		defer sets.StartChangeset().Remove(rrset).Apply(ctx)
 		t.Errorf("Should have failed to add duplicate resource record %v, but succeeded instead.", rrset)
 	} else {
 		t.Logf("Correctly failed to add duplicate resource record %v: %v", rrset, err)
@@ -200,14 +207,16 @@ func TestResourceRecordSetsAddDuplicateFailure(t *testing.T) {
 
 /* TestResourceRecordSetsRemove verifies that the removal of an existing RRS succeeds */
 func TestResourceRecordSetsRemove(t *testing.T) {
+	ctx := context.Background()
+
 	zone := firstZone(t)
 	sets := rrs(t, zone)
 	rrset := getExampleRrs(zone)
-	addRrsetOrFail(t, sets, rrset)
-	err := sets.StartChangeset().Remove(rrset).Apply()
+	addRrsetOrFail(ctx, t, sets, rrset)
+	err := sets.StartChangeset().Remove(rrset).Apply(ctx)
 	if err != nil {
 		// Try again to clean up.
-		defer sets.StartChangeset().Remove(rrset).Apply()
+		defer sets.StartChangeset().Remove(rrset).Apply(ctx)
 		t.Errorf("Failed to remove resource record set %v after adding", rrset)
 	} else {
 		t.Logf("Successfully removed resource set %v after adding", rrset)
@@ -216,14 +225,16 @@ func TestResourceRecordSetsRemove(t *testing.T) {
 
 /* TestResourceRecordSetsRemoveGone verifies that a removed RRS no longer exists */
 func TestResourceRecordSetsRemoveGone(t *testing.T) {
+	ctx := context.Background()
+
 	zone := firstZone(t)
 	sets := rrs(t, zone)
 	rrset := getExampleRrs(zone)
-	addRrsetOrFail(t, sets, rrset)
-	err := sets.StartChangeset().Remove(rrset).Apply()
+	addRrsetOrFail(ctx, t, sets, rrset)
+	err := sets.StartChangeset().Remove(rrset).Apply(ctx)
 	if err != nil {
 		// Try again to clean up.
-		defer sets.StartChangeset().Remove(rrset).Apply()
+		defer sets.StartChangeset().Remove(rrset).Apply(ctx)
 		t.Errorf("Failed to remove resource record set %v after adding", rrset)
 	} else {
 		t.Logf("Successfully removed resource set %v after adding", rrset)
@@ -252,3 +263,11 @@ func TestResourceRecordSetsDifferentTypes(t *testing.T) {
 	zone := firstZone(t)
 	tests.CommonTestResourceRecordSetsDifferentTypes(t, zone)
 }
+
+// TestContract verifies the general interface contract
+func TestContract(t *testing.T) {
+	zone := firstZone(t)
+	sets := rrs(t, zone)
+
+	tests.TestContract(t, sets)
+}
diff --git a/dnsprovider/pkg/dnsprovider/providers/coredns/rrchangeset.go b/dnsprovider/pkg/dnsprovider/providers/coredns/rrchangeset.go
@@ -69,8 +69,12 @@ func (c *ResourceRecordChangeset) Upsert(rrset dnsprovider.ResourceRecordSet) dn
 	return c
 }
 
-func (c *ResourceRecordChangeset) Apply() error {
-	ctx := context.Background()
+func (c *ResourceRecordChangeset) Apply(ctx context.Context) error {
+	// Empty changesets should be a relatively quick no-op
+	if c.IsEmpty() {
+		return nil
+	}
+
 	etcdPathPrefix := c.zone.zones.intf.etcdPathPrefix
 	getOpts := &etcdc.GetOptions{}
 	setOpts := &etcdc.SetOptions{}