Merge pull request #8464 from justinsb/google_clouddns_delete_records

DNS: Don't try to apply empty changesets

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))
 		}

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

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) {
+func addRrsetOrFail(ctx context.Context, t *testing.T, rrsets dnsprovider.ResourceRecordSets, rrset dnsprovider.ResourceRecordSet) {
-	err := rrsets.StartChangeset().Add(rrset).Apply()
+	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)
+	addRrsetOrFail(ctx, t, sets, set)
-	defer sets.StartChangeset().Remove(set).Apply()
+	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)
+	addRrsetOrFail(ctx, t, sets, rrset)
-	defer sets.StartChangeset().Remove(rrset).Apply()
+	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)
+	addRrsetOrFail(ctx, t, sets, rrset)
-	defer sets.StartChangeset().Remove(rrset).Apply()
+	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)
+	addRrsetOrFail(ctx, t, sets, rrset)
-	err := sets.StartChangeset().Remove(rrset).Apply()
+	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)
+	addRrsetOrFail(ctx, t, sets, rrset)
-	err := sets.StartChangeset().Remove(rrset).Apply()
+	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)
+}

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)

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) {
+func addRrsetOrFail(ctx context.Context, t *testing.T, rrsets dnsprovider.ResourceRecordSets, rrset dnsprovider.ResourceRecordSet) {
-	err := rrsets.StartChangeset().Add(rrset).Apply()
+	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)
+	addRrsetOrFail(ctx, t, sets, set)
-	defer sets.StartChangeset().Remove(set).Apply()
+	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)
+	addRrsetOrFail(ctx, t, sets, rrset)
-	defer sets.StartChangeset().Remove(rrset).Apply()
+	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)
+	addRrsetOrFail(ctx, t, sets, rrset)
-	defer sets.StartChangeset().Remove(rrset).Apply()
+	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)
+	addRrsetOrFail(ctx, t, sets, rrset)
-	err := sets.StartChangeset().Remove(rrset).Apply()
+	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)
+	addRrsetOrFail(ctx, t, sets, rrset)
-	err := sets.StartChangeset().Remove(rrset).Apply()
+	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)
+}

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 {
+func (c *ResourceRecordChangeset) Apply(ctx context.Context) error {
-	ctx := context.Background()
+	// 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{}

dnsprovider/pkg/dnsprovider/providers/google/clouddns/clouddns_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package clouddns
 
 import (
+	"context"
 	"flag"
 	"fmt"
 	"os"
@@ -106,8 +107,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) {
+func addRrsetOrFail(ctx context.Context, t *testing.T, rrsets dnsprovider.ResourceRecordSets, rrset dnsprovider.ResourceRecordSet) {
-	err := rrsets.StartChangeset().Add(rrset).Apply()
+	err := rrsets.StartChangeset().Add(rrset).Apply(ctx)
 	if err != nil {
 		t.Fatalf("Failed to add recordsets: %v", err)
 	}
@@ -159,21 +160,23 @@ 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)
+	addRrsetOrFail(ctx, t, sets, set)
-	defer sets.StartChangeset().Remove(set).Apply()
+	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)
+	addRrsetOrFail(ctx, t, sets, rrset)
-	defer sets.StartChangeset().Remove(rrset).Apply()
+	defer sets.StartChangeset().Remove(rrset).Apply(ctx)
 	t.Logf("Successfully added resource record set: %v", rrset)
 	found := false
 	for _, record := range listRrsOrFail(t, sets) {
@@ -187,18 +190,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)
+	addRrsetOrFail(ctx, t, sets, rrset)
-	defer sets.StartChangeset().Remove(rrset).Apply()
+	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)
@@ -207,14 +212,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)
+	addRrsetOrFail(ctx, t, sets, rrset)
-	err := sets.StartChangeset().Remove(rrset).Apply()
+	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: %v", rrset, err)
 	} else {
 		t.Logf("Successfully removed resource set %v after adding", rrset)
@@ -223,14 +230,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)
+	addRrsetOrFail(ctx, t, sets, rrset)
-	err := sets.StartChangeset().Remove(rrset).Apply()
+	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: %v", rrset, err)
 	} else {
 		t.Logf("Successfully removed resource set %v after adding", rrset)
@@ -266,3 +275,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)
+}

dnsprovider/pkg/dnsprovider/providers/google/clouddns/rrchangeset.go

@@ -17,6 +17,7 @@ limitations under the License.
 package clouddns
 
 import (
+	"context"
 	"fmt"
 
 	"k8s.io/kops/dnsprovider/pkg/dnsprovider"
@@ -49,7 +50,12 @@ func (c *ResourceRecordChangeset) Upsert(rrset dnsprovider.ResourceRecordSet) dn
 	return c
 }
 
-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
+	}
+
 	rrsets := c.rrsets
 
 	service := rrsets.zone.zones.interface_.service.Changes()
@@ -115,7 +121,7 @@ func (c *ResourceRecordChangeset) Apply() error {
 }
 
 func (c *ResourceRecordChangeset) IsEmpty() bool {
-	return len(c.additions) == 0 && len(c.removals) == 0
+	return len(c.additions) == 0 && len(c.removals) == 0 && len(c.upserts) == 0
 }
 
 // ResourceRecordSets returns the parent ResourceRecordSets

dnsprovider/pkg/dnsprovider/providers/openstack/designate/BUILD.bazel

@@ -1,4 +1,4 @@
-load("@io_bazel_rules_go//go:def.bzl", "go_library")
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 
 go_library(
     name = "go_default_library",
@@ -25,3 +25,10 @@ go_library(
         "//vendor/k8s.io/klog:go_default_library",
     ],
 )
+
+go_test(
+    name = "go_default_test",
+    srcs = ["designate_test.go"],
+    embed = [":go_default_library"],
+    deps = ["//dnsprovider/pkg/dnsprovider/tests:go_default_library"],
+)

dnsprovider/pkg/dnsprovider/providers/openstack/designate/designate_test.go

@@ -0,0 +1,30 @@
+/*
+Copyright 2020 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package designate
+
+import (
+	"testing"
+
+	"k8s.io/kops/dnsprovider/pkg/dnsprovider/tests"
+)
+
+// TestContract verifies the general interface contract
+func TestContract(t *testing.T) {
+	sets := &ResourceRecordSets{}
+
+	tests.TestContract(t, sets)
+}

dnsprovider/pkg/dnsprovider/providers/openstack/designate/rrchangeset.go

@@ -17,6 +17,7 @@ limitations under the License.
 package designate
 
 import (
+	"context"
 	"fmt"
 
 	"github.com/gophercloud/gophercloud/openstack/dns/v2/recordsets"
@@ -50,7 +51,12 @@ func (c *ResourceRecordChangeset) Upsert(rrset dnsprovider.ResourceRecordSet) dn
 	return c
 }
 
-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
+	}
+
 	zoneID := c.zone.impl.ID
 
 	for _, removal := range c.removals {

dnsprovider/pkg/dnsprovider/tests/BUILD.bazel

@@ -2,7 +2,10 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library")
 
 go_library(
     name = "go_default_library",
-    srcs = ["commontests.go"],
+    srcs = [
+        "commontests.go",
+        "contract.go",
+    ],
     importpath = "k8s.io/kops/dnsprovider/pkg/dnsprovider/tests",
     visibility = ["//visibility:public"],
     deps = [

dnsprovider/pkg/dnsprovider/tests/commontests.go

@@ -17,6 +17,7 @@ limitations under the License.
 package tests
 
 import (
+	"context"
 	"reflect"
 	"testing"
 
@@ -26,20 +27,21 @@ import (
 
 /* CommonTestResourceRecordSetsReplace verifies that replacing an RRS works */
 func CommonTestResourceRecordSetsReplace(t *testing.T, zone dnsprovider.Zone) {
+	ctx := context.Background()
 	rrsets, _ := zone.ResourceRecordSets()
 
 	sets := rrs(t, zone)
 	rrset := rrsets.New("alpha.test.com", []string{"8.8.4.4"}, 40, rrstype.A)
-	addRrsetOrFail(t, sets, rrset)
+	addRrsetOrFail(ctx, t, sets, rrset)
-	defer sets.StartChangeset().Remove(rrset).Apply()
+	defer sets.StartChangeset().Remove(rrset).Apply(ctx)
 
 	// Replace the record (change ttl and rrdatas)
 	newRrset := rrsets.New("alpha.test.com", []string{"8.8.8.8"}, 80, rrstype.A)
-	err := sets.StartChangeset().Add(newRrset).Remove(rrset).Apply()
+	err := sets.StartChangeset().Add(newRrset).Remove(rrset).Apply(ctx)
 	if err != nil {
 		t.Errorf("Failed to replace resource record set %v -> %v: %v", rrset, newRrset, err)
 	} else {
-		defer sets.StartChangeset().Remove(newRrset).Apply()
+		defer sets.StartChangeset().Remove(newRrset).Apply(ctx)
 		t.Logf("Correctly replaced resource record %v -> %v", rrset, newRrset)
 	}
 
@@ -49,21 +51,22 @@ func CommonTestResourceRecordSetsReplace(t *testing.T, zone dnsprovider.Zone) {
 
 /* CommonTestResourceRecordSetsReplaceAll verifies that we can remove an RRS and create one with a different name*/
 func CommonTestResourceRecordSetsReplaceAll(t *testing.T, zone dnsprovider.Zone) {
+	ctx := context.Background()
 	rrsets, _ := zone.ResourceRecordSets()
 
 	sets := rrs(t, zone)
 	rrset := rrsets.New("alpha.test.com", []string{"8.8.4.4"}, 40, rrstype.A)
-	addRrsetOrFail(t, sets, rrset)
+	addRrsetOrFail(ctx, t, sets, rrset)
-	defer sets.StartChangeset().Remove(rrset).Apply()
+	defer sets.StartChangeset().Remove(rrset).Apply(ctx)
 
 	newRrset := rrsets.New("beta.test.com", []string{"8.8.8.8"}, 80, rrstype.A)
 
 	// Try to add it again, and verify that the call fails.
-	err := sets.StartChangeset().Add(newRrset).Remove(rrset).Apply()
+	err := sets.StartChangeset().Add(newRrset).Remove(rrset).Apply(ctx)
 	if err != nil {
 		t.Errorf("Failed to replace resource record set %v -> %v: %v", rrset, newRrset, err)
 	} else {
-		defer sets.StartChangeset().Remove(newRrset).Apply()
+		defer sets.StartChangeset().Remove(newRrset).Apply(ctx)
 		t.Logf("Correctly replaced resource record %v -> %v", rrset, newRrset)
 	}
 
@@ -74,21 +77,22 @@ func CommonTestResourceRecordSetsReplaceAll(t *testing.T, zone dnsprovider.Zone)
 
 /* CommonTestResourceRecordSetsDifferentType verifies that we can add records of the same name but different types */
 func CommonTestResourceRecordSetsDifferentTypes(t *testing.T, zone dnsprovider.Zone) {
+	ctx := context.Background()
 	rrsets, _ := zone.ResourceRecordSets()
 
 	sets := rrs(t, zone)
 	rrset := rrsets.New("alpha.test.com", []string{"8.8.4.4"}, 40, rrstype.A)
-	addRrsetOrFail(t, sets, rrset)
+	addRrsetOrFail(ctx, t, sets, rrset)
-	defer sets.StartChangeset().Remove(rrset).Apply()
+	defer sets.StartChangeset().Remove(rrset).Apply(ctx)
 
 	aaaaRrset := rrsets.New("alpha.test.com", []string{"2001:4860:4860::8888"}, 80, rrstype.AAAA)
 
 	// Add the resource with the same name but different type
-	err := sets.StartChangeset().Add(aaaaRrset).Apply()
+	err := sets.StartChangeset().Add(aaaaRrset).Apply(ctx)
 	if err != nil {
 		t.Errorf("Failed to add resource record set %v: %v", aaaaRrset, err)
 	}
-	defer sets.StartChangeset().Remove(aaaaRrset).Apply()
+	defer sets.StartChangeset().Remove(aaaaRrset).Apply(ctx)
 
 	// Check that both records exist
 	assertHasRecord(t, sets, aaaaRrset)
@@ -184,8 +188,8 @@ func assertEquivalent(t *testing.T, l, r dnsprovider.ResourceRecordSet) {
 	}
 }
 
-func addRrsetOrFail(t *testing.T, rrsets dnsprovider.ResourceRecordSets, rrset dnsprovider.ResourceRecordSet) {
+func addRrsetOrFail(ctx context.Context, t *testing.T, rrsets dnsprovider.ResourceRecordSets, rrset dnsprovider.ResourceRecordSet) {
-	err := rrsets.StartChangeset().Add(rrset).Apply()
+	err := rrsets.StartChangeset().Add(rrset).Apply(ctx)
 	if err != nil {
 		t.Fatalf("Failed to add recordset %v: %v", rrset, err)
 	} else {

dnsprovider/pkg/dnsprovider/tests/contract.go

@@ -0,0 +1,68 @@
+/*
+Copyright 2020 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package tests
+
+import (
+	"testing"
+
+	"k8s.io/kops/dnsprovider/pkg/dnsprovider"
+	"k8s.io/kops/dnsprovider/pkg/dnsprovider/rrstype"
+)
+
+// TestContract verifies the general ResourceRecordChangeset contract
+func TestContract(t *testing.T, rrsets dnsprovider.ResourceRecordSets) {
+
+	{
+		changeset := rrsets.StartChangeset()
+		if !changeset.IsEmpty() {
+			t.Fatalf("expected new changeset to be empty")
+		}
+
+		rrs := changeset.ResourceRecordSets().New("foo", []string{"192.168.0.1"}, 1, rrstype.A)
+		changeset.Add(rrs)
+		if changeset.IsEmpty() {
+			t.Fatalf("expected changeset not to be empty after add")
+		}
+	}
+
+	{
+		changeset := rrsets.StartChangeset()
+		if !changeset.IsEmpty() {
+			t.Fatalf("expected new changeset to be empty")
+		}
+
+		rrs := changeset.ResourceRecordSets().New("foo", []string{"192.168.0.1"}, 1, rrstype.A)
+		changeset.Remove(rrs)
+		if changeset.IsEmpty() {
+			t.Fatalf("expected changeset not to be empty after remove")
+		}
+	}
+
+	{
+		changeset := rrsets.StartChangeset()
+		if !changeset.IsEmpty() {
+			t.Fatalf("expected new changeset to be empty")
+		}
+
+		rrs := changeset.ResourceRecordSets().New("foo", []string{"192.168.0.1"}, 1, rrstype.A)
+		changeset.Upsert(rrs)
+		if changeset.IsEmpty() {
+			t.Fatalf("expected changeset not to be empty after upsert")
+		}
+	}
+
+}

pkg/resources/digitalocean/dns/dns.go

@@ -313,13 +313,15 @@ func (r *resourceRecordChangeset) Upsert(rrset dnsprovider.ResourceRecordSet) dn
 
 // Apply adds new records stored in r.additions, updates records stored
 // in r.upserts and deletes records stored in r.removals
-func (r *resourceRecordChangeset) Apply() error {
+func (r *resourceRecordChangeset) Apply(ctx context.Context) error {
-	klog.V(2).Info("applying changes in record change set")
+	// Empty changesets should be a relatively quick no-op
 	if r.IsEmpty() {
-		klog.V(2).Info("record change set is empty")
+		klog.V(4).Info("record change set is empty")
 		return nil
 	}
 
+	klog.V(2).Info("applying changes in record change set")
+
 	if len(r.additions) > 0 {
 		for _, rrset := range r.additions {
 			err := r.applyResourceRecordSet(rrset)

pkg/resources/digitalocean/dns/dns_test.go

@@ -369,6 +369,8 @@ func TestNewResourceRecordSet(t *testing.T) {
 }
 
 func TestResourceRecordChangeset(t *testing.T) {
+	ctx := context.Background()
+
 	fake := &fakeDomainService{}
 	fake.recordsFunc = func(ctx context.Context, domain string, listOpts *godo.ListOptions) ([]godo.DomainRecord, *godo.Response, error) {
 		domainRecords := []godo.DomainRecord{
@@ -456,7 +458,7 @@ func TestResourceRecordChangeset(t *testing.T) {
 	record = rrset.New("to-upsert", []string{"127.0.0.1"}, 3600, rrstype.A)
 	changeset.Upsert(record)
 
-	err = changeset.Apply()
+	err = changeset.Apply(ctx)
 	if err != nil {
 		t.Errorf("error applying changeset: %v", err)
 	}

protokube/pkg/gossip/dns/provider/changeset.go

@@ -17,6 +17,8 @@ limitations under the License.
 package provider
 
 import (
+	"context"
+
 	"k8s.io/kops/dnsprovider/pkg/dnsprovider"
 )
 
@@ -52,7 +54,12 @@ func (c *resourceRecordChangeset) Upsert(rrs dnsprovider.ResourceRecordSet) dnsp
 }
 
 // Apply applies the accumulated operations to the Zone.
-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
+	}
+
 	return c.zone.applyChangeset(c)
 }
 

upup/pkg/fi/cloudup/apply_cluster.go

@@ -960,7 +960,7 @@ func (c *ApplyClusterCmd) Run(ctx context.Context) error {
 	}
 
 	if shouldPrecreateDNS {
-		if err := precreateDNS(cluster, cloud); err != nil {
+		if err := precreateDNS(ctx, cluster, cloud); err != nil {
 			klog.Warningf("unable to pre-create DNS records - cluster startup may be slower: %v", err)
 		}
 	}

upup/pkg/fi/cloudup/dns.go

@@ -17,6 +17,7 @@ limitations under the License.
 package cloudup
 
 import (
+	"context"
 	"fmt"
 	"net"
 	"os"
@@ -124,7 +125,7 @@ func validateDNS(cluster *kops.Cluster, cloud fi.Cloud) error {
 	return nil
 }
 
-func precreateDNS(cluster *kops.Cluster, cloud fi.Cloud) error {
+func precreateDNS(ctx context.Context, cluster *kops.Cluster, cloud fi.Cloud) error {
 	// TODO: Move to update
 	if !featureflag.DNSPreCreate.Enabled() {
 		klog.V(4).Infof("Skipping DNS record pre-creation because feature flag not enabled")
@@ -240,7 +241,7 @@ func precreateDNS(cluster *kops.Cluster, cloud fi.Cloud) error {
 	}
 
 	if len(created) != 0 {
-		err := changeset.Apply()
+		err := changeset.Apply(ctx)
 		if err != nil {
 			return fmt.Errorf("error pre-creating DNS records: %v", err)
 		}