From 1bf0417b7991983dcc528ab994f5226f147e27f5 Mon Sep 17 00:00:00 2001 From: Adam Stankiewicz Date: Wed, 28 Oct 2020 18:46:21 +0100 Subject: [PATCH] Allow for normalizing any field of desired endpoint --- controller/controller.go | 10 ++-- endpoint/endpoint.go | 43 ++++++++++++++++ plan/plan.go | 59 +++++----------------- plan/plan_test.go | 16 ++++-- provider/cloudflare/cloudflare.go | 20 +++++--- provider/cloudflare/cloudflare_test.go | 70 +++++++++++++++++++++++--- provider/provider.go | 5 +- provider/provider_test.go | 9 ---- registry/aws_sd_registry.go | 5 +- registry/noop.go | 6 +-- registry/registry.go | 2 +- registry/txt.go | 6 +-- 12 files changed, 160 insertions(+), 91 deletions(-) diff --git a/controller/controller.go b/controller/controller.go index f9355875eb..c59d0ca484 100644 --- a/controller/controller.go +++ b/controller/controller.go @@ -140,11 +140,11 @@ func (c *Controller) RunOnce(ctx context.Context) error { sourceEndpointsTotal.Set(float64(len(endpoints))) plan := &plan.Plan{ - Policies: []plan.Policy{c.Policy}, - Current: records, - Desired: endpoints, - DomainFilter: c.DomainFilter, - PropertyComparator: c.Registry.PropertyValuesEqual, + Policies: []plan.Policy{c.Policy}, + Current: records, + Desired: endpoints, + DomainFilter: c.DomainFilter, + NormalizeDesiredEndpoint: c.Registry.NormalizeDesiredEndpoint, } plan = plan.Calculate() diff --git a/endpoint/endpoint.go b/endpoint/endpoint.go index 5f5ef14882..1b07d5599e 100644 --- a/endpoint/endpoint.go +++ b/endpoint/endpoint.go @@ -19,6 +19,7 @@ package endpoint import ( "fmt" "sort" + "strconv" "strings" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -191,10 +192,52 @@ func (e *Endpoint) GetProviderSpecificProperty(key string) (ProviderSpecificProp return ProviderSpecificProperty{}, false } +type NormalizeProviderSpecificConfig map[string]func(string) string + +// Selects and normalizes fields specified by config +func (e *Endpoint) NormalizeProviderSpecific(config NormalizeProviderSpecificConfig) { + normalized := ProviderSpecific{} + + for name, normalize := range config { + if property, ok := e.GetProviderSpecificProperty(name); ok { + normalized = append(normalized, ProviderSpecificProperty{ + Name: name, + Value: normalize(property.Value), + }) + } else { + normalized = append(normalized, ProviderSpecificProperty{ + Name: name, + Value: normalize(""), + }) + } + } + + e.ProviderSpecific = normalized +} + func (e *Endpoint) String() string { return fmt.Sprintf("%s %d IN %s %s %s %s", e.DNSName, e.RecordTTL, e.RecordType, e.SetIdentifier, e.Targets, e.ProviderSpecific) } +// Normalizes boolean value of custom property +func NormalizeBoolean(value string, defaultValue bool) string { + val, err := strconv.ParseBool(value) + + if err != nil { + if defaultValue { + return "true" + } else { + return "false" + } + } + + if val { + return "true" + } + + return "false" +} + // DNSEndpointSpec defines the desired state of DNSEndpoint type DNSEndpointSpec struct { Endpoints []*Endpoint `json:"endpoints,omitempty"` diff --git a/plan/plan.go b/plan/plan.go index 32040cb576..935e19fb20 100644 --- a/plan/plan.go +++ b/plan/plan.go @@ -18,15 +18,11 @@ package plan import ( "fmt" - "strconv" "strings" "sigs.k8s.io/external-dns/endpoint" ) -// PropertyComparator is used in Plan for comparing the previous and current custom annotations. -type PropertyComparator func(name string, previous string, current string) bool - // Plan can convert a list of desired and current records to a series of create, // update and delete actions. type Plan struct { @@ -41,8 +37,8 @@ type Plan struct { Changes *Changes // DomainFilter matches DNS names DomainFilter endpoint.DomainFilter - // Property comparator compares custom properties of providers - PropertyComparator PropertyComparator + // Function normalizing desired endpoint (e.g. applying defaults) + NormalizeDesiredEndpoint func(endpoint *endpoint.Endpoint) } // Changes holds lists of actions to be executed by dns providers @@ -71,12 +67,13 @@ bar.com | | [->191.1.1.1, ->190.1.1.1] | = create (bar.com -> 1 "=", i.e. result of calculation relies on supplied ConflictResolver */ type planTable struct { + plan *Plan rows map[string]map[string]*planTableRow resolver ConflictResolver } -func newPlanTable() planTable { //TODO: make resolver configurable - return planTable{map[string]map[string]*planTableRow{}, PerResource{}} +func newPlanTable(p *Plan) planTable { //TODO: make resolver configurable + return planTable{p, map[string]map[string]*planTableRow{}, PerResource{}} } // planTableRow @@ -102,8 +99,11 @@ func (t planTable) addCurrent(e *endpoint.Endpoint) { t.rows[dnsName][e.SetIdentifier].current = e } -func (t planTable) addCandidate(e *endpoint.Endpoint) { +func (t planTable) addDesired(e *endpoint.Endpoint) { dnsName := normalizeDNSName(e.DNSName) + if t.plan.NormalizeDesiredEndpoint != nil { + t.plan.NormalizeDesiredEndpoint(e) + } if _, ok := t.rows[dnsName]; !ok { t.rows[dnsName] = make(map[string]*planTableRow) } @@ -117,13 +117,13 @@ func (t planTable) addCandidate(e *endpoint.Endpoint) { // state. It then passes those changes to the current policy for further // processing. It returns a copy of Plan with the changes populated. func (p *Plan) Calculate() *Plan { - t := newPlanTable() + t := newPlanTable(p) for _, current := range filterRecordsForPlan(p.Current, p.DomainFilter) { t.addCurrent(current) } for _, desired := range filterRecordsForPlan(p.Desired, p.DomainFilter) { - t.addCandidate(desired) + t.addDesired(desired) } changes := &Changes{} @@ -201,19 +201,11 @@ func (p *Plan) shouldUpdateProviderSpecific(desired, current *endpoint.Endpoint) } if d, ok := desiredProperties[c.Name]; ok { - if p.PropertyComparator != nil { - if !p.PropertyComparator(c.Name, c.Value, d.Value) { - return true - } - } else if c.Value != d.Value { + if c.Value != d.Value { return true } } else { - if p.PropertyComparator != nil { - if !p.PropertyComparator(c.Name, c.Value, "") { - return true - } - } else if c.Value != "" { + if c.Value != "" { return true } } @@ -261,28 +253,3 @@ func normalizeDNSName(dnsName string) string { } return s } - -// CompareBoolean is an implementation of PropertyComparator for comparing boolean-line values -// For example external-dns.alpha.kubernetes.io/cloudflare-proxied: "true" -// If value doesn't parse as boolean, the defaultValue is used -func CompareBoolean(defaultValue bool, name, current, previous string) bool { - var err error - - v1, v2 := defaultValue, defaultValue - - if previous != "" { - v1, err = strconv.ParseBool(previous) - if err != nil { - v1 = defaultValue - } - } - - if current != "" { - v2, err = strconv.ParseBool(current) - if err != nil { - v2 = defaultValue - } - } - - return v1 == v2 -} diff --git a/plan/plan_test.go b/plan/plan_test.go index 1a34161968..b5c888bb5e 100644 --- a/plan/plan_test.go +++ b/plan/plan_test.go @@ -313,8 +313,12 @@ func (suite *PlanTestSuite) TestSyncSecondRoundWithProviderSpecificDefaultFalse( Policies: []Policy{&SyncPolicy{}}, Current: current, Desired: desired, - PropertyComparator: func(name, previous, current string) bool { - return CompareBoolean(false, name, previous, current) + NormalizeDesiredEndpoint: func(e *endpoint.Endpoint) { + e.NormalizeProviderSpecific(endpoint.NormalizeProviderSpecificConfig{ + "external-dns.alpha.kubernetes.io/cloudflare-proxied": func(value string) string { + return endpoint.NormalizeBoolean(value, false) + }, + }) }, } @@ -337,8 +341,12 @@ func (suite *PlanTestSuite) TestSyncSecondRoundWithProviderSpecificDefualtTrue() Policies: []Policy{&SyncPolicy{}}, Current: current, Desired: desired, - PropertyComparator: func(name, previous, current string) bool { - return CompareBoolean(true, name, previous, current) + NormalizeDesiredEndpoint: func(e *endpoint.Endpoint) { + e.NormalizeProviderSpecific(endpoint.NormalizeProviderSpecificConfig{ + "external-dns.alpha.kubernetes.io/cloudflare-proxied": func(value string) string { + return endpoint.NormalizeBoolean(value, true) + }, + }) }, } diff --git a/provider/cloudflare/cloudflare.go b/provider/cloudflare/cloudflare.go index d78764acd2..c53457b4e3 100644 --- a/provider/cloudflare/cloudflare.go +++ b/provider/cloudflare/cloudflare.go @@ -258,14 +258,6 @@ func (p *CloudFlareProvider) ApplyChanges(ctx context.Context, changes *plan.Cha return p.submitChanges(ctx, cloudflareChanges) } -func (p *CloudFlareProvider) PropertyValuesEqual(name string, previous string, current string) bool { - if name == source.CloudflareProxiedKey { - return plan.CompareBoolean(p.proxiedByDefault, name, previous, current) - } - - return p.BaseProvider.PropertyValuesEqual(name, previous, current) -} - // submitChanges takes a zone and a collection of Changes and sends them as a single transaction. func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloudFlareChange) error { // return early if there is nothing to change @@ -386,6 +378,18 @@ func (p *CloudFlareProvider) newCloudFlareChange(action string, endpoint *endpoi } } +func (p *CloudFlareProvider) NormalizeDesiredEndpoint(e *endpoint.Endpoint) { + if shouldBeProxied(e, p.proxiedByDefault) { + e.RecordTTL = 0 + } + + e.NormalizeProviderSpecific(endpoint.NormalizeProviderSpecificConfig{ + source.CloudflareProxiedKey: func(value string) string { + return endpoint.NormalizeBoolean(value, p.proxiedByDefault) + }, + }) +} + func shouldBeProxied(endpoint *endpoint.Endpoint, proxiedByDefault bool) bool { proxied := proxiedByDefault diff --git a/provider/cloudflare/cloudflare_test.go b/provider/cloudflare/cloudflare_test.go index 49bde44f71..c69ac828f9 100644 --- a/provider/cloudflare/cloudflare_test.go +++ b/provider/cloudflare/cloudflare_test.go @@ -250,9 +250,10 @@ func AssertActions(t *testing.T, provider *CloudFlareProvider, endpoints []*endp } plan := &plan.Plan{ - Current: records, - Desired: endpoints, - DomainFilter: endpoint.NewDomainFilter([]string{"bar.com"}), + Current: records, + Desired: endpoints, + DomainFilter: endpoint.NewDomainFilter([]string{"bar.com"}), + NormalizeDesiredEndpoint: provider.NormalizeDesiredEndpoint, } changes := plan.Calculate().Changes @@ -1035,9 +1036,9 @@ func TestProviderPropertiesIdempotency(t *testing.T) { } plan := plan.Plan{ - Current: current, - Desired: desired, - PropertyComparator: provider.PropertyValuesEqual, + Current: current, + Desired: desired, + NormalizeDesiredEndpoint: provider.NormalizeDesiredEndpoint, } plan = *plan.Calculate() @@ -1091,7 +1092,8 @@ func TestCloudflareComplexUpdate(t *testing.T) { }, }, }, - DomainFilter: endpoint.NewDomainFilter([]string{"bar.com"}), + DomainFilter: endpoint.NewDomainFilter([]string{"bar.com"}), + NormalizeDesiredEndpoint: provider.NormalizeDesiredEndpoint, } planned := plan.Calculate() @@ -1133,3 +1135,57 @@ func TestCloudflareComplexUpdate(t *testing.T) { }, }) } + +func TestCustomTTLWithEnabeledProxyNotChanged(t *testing.T) { + client := NewMockCloudFlareClientWithRecords(map[string][]cloudflare.DNSRecord{ + "001": []cloudflare.DNSRecord{ + { + ID: "1234567890", + ZoneID: "001", + Name: "foobar.bar.com", + Type: endpoint.RecordTypeA, + TTL: 1, + Content: "1.2.3.4", + Proxied: true, + }, + }, + }) + + provider := &CloudFlareProvider{ + Client: client, + } + + records, err := provider.Records(context.Background()) + + if err != nil { + t.Errorf("should not fail, %s", err) + } + + plan := &plan.Plan{ + Current: records, + Desired: []*endpoint.Endpoint{ + { + DNSName: "foobar.bar.com", + Targets: endpoint.Targets{"1.2.3.4"}, + RecordType: endpoint.RecordTypeA, + RecordTTL: 300, + Labels: endpoint.Labels{}, + ProviderSpecific: endpoint.ProviderSpecific{ + { + Name: "external-dns.alpha.kubernetes.io/cloudflare-proxied", + Value: "true", + }, + }, + }, + }, + DomainFilter: endpoint.NewDomainFilter([]string{"bar.com"}), + NormalizeDesiredEndpoint: provider.NormalizeDesiredEndpoint, + } + + planned := plan.Calculate() + + assert.Equal(t, 0, len(planned.Changes.Create), "no new changes should be here") + assert.Equal(t, 0, len(planned.Changes.UpdateNew), "no new changes should be here") + assert.Equal(t, 0, len(planned.Changes.UpdateOld), "no new changes should be here") + assert.Equal(t, 0, len(planned.Changes.Delete), "no new changes should be here") +} diff --git a/provider/provider.go b/provider/provider.go index f2b8cd5e6b..17d591d872 100644 --- a/provider/provider.go +++ b/provider/provider.go @@ -29,14 +29,13 @@ import ( type Provider interface { Records(ctx context.Context) ([]*endpoint.Endpoint, error) ApplyChanges(ctx context.Context, changes *plan.Changes) error - PropertyValuesEqual(name string, previous string, current string) bool + NormalizeDesiredEndpoint(endpoint *endpoint.Endpoint) } type BaseProvider struct { } -func (b BaseProvider) PropertyValuesEqual(name, previous, current string) bool { - return previous == current +func (b BaseProvider) NormalizeDesiredEndpoint(endpoint *endpoint.Endpoint) { } type contextKey struct { diff --git a/provider/provider_test.go b/provider/provider_test.go index 0e24ca4f01..89598919a3 100644 --- a/provider/provider_test.go +++ b/provider/provider_test.go @@ -54,12 +54,3 @@ func TestDifference(t *testing.T) { assert.Equal(t, remove, []string{"foo"}) assert.Equal(t, leave, []string{"bar"}) } - -func TestBaseProviderPropertyEquality(t *testing.T) { - p := BaseProvider{} - assert.True(t, p.PropertyValuesEqual("some.property", "", ""), "Both properties not present") - assert.False(t, p.PropertyValuesEqual("some.property", "", "Foo"), "First property missing") - assert.False(t, p.PropertyValuesEqual("some.property", "Foo", ""), "Second property missing") - assert.True(t, p.PropertyValuesEqual("some.property", "Foo", "Foo"), "Properties the same") - assert.False(t, p.PropertyValuesEqual("some.property", "Foo", "Bar"), "Attributes differ") -} diff --git a/registry/aws_sd_registry.go b/registry/aws_sd_registry.go index 2e4823f71d..8fd74fb0b6 100644 --- a/registry/aws_sd_registry.go +++ b/registry/aws_sd_registry.go @@ -88,6 +88,7 @@ func (sdr *AWSSDRegistry) updateLabels(endpoints []*endpoint.Endpoint) { } } -func (sdr *AWSSDRegistry) PropertyValuesEqual(name string, previous string, current string) bool { - return sdr.provider.PropertyValuesEqual(name, previous, current) +// NormalizeDesiredEndpoint normalizes desired endpoint (e.g. applies defaults) +func (sdr *AWSSDRegistry) NormalizeDesiredEndpoint(endpoint *endpoint.Endpoint) { + sdr.provider.NormalizeDesiredEndpoint(endpoint) } diff --git a/registry/noop.go b/registry/noop.go index 10e54adb22..d9f382ee33 100644 --- a/registry/noop.go +++ b/registry/noop.go @@ -46,7 +46,7 @@ func (im *NoopRegistry) ApplyChanges(ctx context.Context, changes *plan.Changes) return im.provider.ApplyChanges(ctx, changes) } -// PropertyValuesEqual compares two property values for equality -func (im *NoopRegistry) PropertyValuesEqual(attribute string, previous string, current string) bool { - return im.provider.PropertyValuesEqual(attribute, previous, current) +// NormalizeDesiredEndpoint normalizes desired endpoint (e.g. applies defaults) +func (im *NoopRegistry) NormalizeDesiredEndpoint(endpoint *endpoint.Endpoint) { + im.provider.NormalizeDesiredEndpoint(endpoint) } diff --git a/registry/registry.go b/registry/registry.go index 1155411d63..e61f30b05b 100644 --- a/registry/registry.go +++ b/registry/registry.go @@ -32,7 +32,7 @@ import ( type Registry interface { Records(ctx context.Context) ([]*endpoint.Endpoint, error) ApplyChanges(ctx context.Context, changes *plan.Changes) error - PropertyValuesEqual(attribute string, previous string, current string) bool + NormalizeDesiredEndpoint(endpoint *endpoint.Endpoint) } //TODO(ideahitme): consider moving this to Plan diff --git a/registry/txt.go b/registry/txt.go index ebf4020212..2cbb939389 100644 --- a/registry/txt.go +++ b/registry/txt.go @@ -191,9 +191,9 @@ func (im *TXTRegistry) ApplyChanges(ctx context.Context, changes *plan.Changes) return im.provider.ApplyChanges(ctx, filteredChanges) } -// PropertyValuesEqual compares two attribute values for equality -func (im *TXTRegistry) PropertyValuesEqual(name string, previous string, current string) bool { - return im.provider.PropertyValuesEqual(name, previous, current) +// NormalizeDesiredEndpoint normalizes desired endpoint (e.g. applies defaults) +func (im *TXTRegistry) NormalizeDesiredEndpoint(endpoint *endpoint.Endpoint) { + im.provider.NormalizeDesiredEndpoint(endpoint) } /**