Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DNS: Don't try to apply empty changesets #8464

Merged
merged 3 commits into from
May 17, 2020
Merged

DNS: Don't try to apply empty changesets #8464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5204744
DNS: Don't try to apply empty changesets
justinsb Feb 3, 2020
7e710e8
DNS: Apply should be a no-op when empty
justinsb Apr 9, 2020
3306549
Add general contract test for IsEmpty
justinsb Apr 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions dns-controller/pkg/dns/dnscontroller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package dns

import (
"context"
"fmt"
"time"

Expand Down Expand Up @@ -189,6 +190,8 @@ type recordKey struct {
}

func (c *DNSController) runOnce() error {
ctx := context.TODO()

snapshot := c.snapshotIfChangedAndReady()
if snapshot == nil {
// Unchanged / not ready
Expand Down Expand Up @@ -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))
}
Expand All @@ -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
Expand All @@ -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))
}
Expand Down
4 changes: 3 additions & 1 deletion dnsprovider/pkg/dnsprovider/dns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package dnsprovider

import (
"context"
"reflect"

"k8s.io/kops/dnsprovider/pkg/dnsprovider/rrstype"
Expand Down Expand Up @@ -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
Expand Down
53 changes: 36 additions & 17 deletions dnsprovider/pkg/dnsprovider/providers/aws/route53/route53_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package route53

import (
"context"
"flag"
"fmt"
"os"
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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) {
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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)
}
8 changes: 7 additions & 1 deletion dnsprovider/pkg/dnsprovider/providers/aws/route53/rrchangeset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package route53

import (
"bytes"
"context"
"fmt"

"github.com/aws/aws-sdk-go/aws"
Expand Down Expand Up @@ -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)
Expand Down
53 changes: 36 additions & 17 deletions dnsprovider/pkg/dnsprovider/providers/coredns/coredns_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package coredns

import (
"context"
"flag"
"fmt"
"os"
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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")
Expand All @@ -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())
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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)
}
8 changes: 6 additions & 2 deletions dnsprovider/pkg/dnsprovider/providers/coredns/rrchangeset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand Down
Loading