diff --git a/controller/controller.go b/controller/controller.go index 4353fe44f..2b9a633f0 100644 --- a/controller/controller.go +++ b/controller/controller.go @@ -72,6 +72,14 @@ var ( Help: "Timestamp of last successful sync with the DNS provider", }, ) + controllerNoChangesTotal = prometheus.NewCounter( + prometheus.CounterOpts{ + Namespace: "external_dns", + Subsystem: "controller", + Name: "no_op_runs_total", + Help: "Number of reconcile loops ending up with no changes on the DNS provider side.", + }, + ) deprecatedRegistryErrors = prometheus.NewCounter( prometheus.CounterOpts{ Subsystem: "registry", @@ -96,6 +104,7 @@ func init() { prometheus.MustRegister(lastSyncTimestamp) prometheus.MustRegister(deprecatedRegistryErrors) prometheus.MustRegister(deprecatedSourceErrors) + prometheus.MustRegister(controllerNoChangesTotal) } // Controller is responsible for orchestrating the different components. @@ -112,7 +121,7 @@ type Controller struct { // The interval between individual synchronizations Interval time.Duration // The DomainFilter defines which DNS records to keep or exclude - DomainFilter endpoint.DomainFilter + DomainFilter endpoint.DomainFilterInterface // The nextRunAt used for throttling and batching reconciliation nextRunAt time.Time // The nextRunAtMux is for atomic updating of nextRunAt @@ -144,23 +153,29 @@ func (c *Controller) RunOnce(ctx context.Context) error { sourceEndpointsTotal.Set(float64(len(endpoints))) endpoints = c.Registry.AdjustEndpoints(endpoints) + //filter := plan := &plan.Plan{ Policies: []plan.Policy{c.Policy}, Current: records, Desired: endpoints, - DomainFilter: c.DomainFilter, + DomainFilter: endpoint.MatchAllDomainFilters{c.DomainFilter, c.Registry.GetDomainFilter()}, PropertyComparator: c.Registry.PropertyValuesEqual, ManagedRecords: []string{endpoint.RecordTypeA, endpoint.RecordTypeCNAME}, } plan = plan.Calculate() - err = c.Registry.ApplyChanges(ctx, plan.Changes) - if err != nil { - registryErrorsTotal.Inc() - deprecatedRegistryErrors.Inc() - return err + if plan.Changes.HasChanges() { + err = c.Registry.ApplyChanges(ctx, plan.Changes) + if err != nil { + registryErrorsTotal.Inc() + deprecatedRegistryErrors.Inc() + return err + } + } else { + controllerNoChangesTotal.Inc() + log.Info("All records are already up to date") } lastSyncTimestamp.SetToCurrentTime() diff --git a/controller/controller_test.go b/controller/controller_test.go index 8c042d085..89ac590f0 100644 --- a/controller/controller_test.go +++ b/controller/controller_test.go @@ -40,6 +40,30 @@ type mockProvider struct { ExpectChanges *plan.Changes } +type filteredMockProvider struct { + provider.BaseProvider + domainFilter endpoint.DomainFilterInterface + RecordsStore []*endpoint.Endpoint + RecordsCallCount int + ApplyChangesCalls []*plan.Changes +} + +func (p *filteredMockProvider) GetDomainFilter() endpoint.DomainFilterInterface { + return p.domainFilter +} + +// Records returns the desired mock endpoints. +func (p *filteredMockProvider) Records(ctx context.Context) ([]*endpoint.Endpoint, error) { + p.RecordsCallCount++ + return p.RecordsStore, nil +} + +// ApplyChanges stores all calls for later check +func (p *filteredMockProvider) ApplyChanges(ctx context.Context, changes *plan.Changes) error { + p.ApplyChangesCalls = append(p.ApplyChangesCalls, changes) + return nil +} + // Records returns the desired mock endpoints. func (p *mockProvider) Records(ctx context.Context) ([]*endpoint.Endpoint, error) { return p.RecordsStore, nil @@ -192,3 +216,155 @@ func TestShouldRunOnce(t *testing.T) { // But not two times assert.False(t, ctrl.ShouldRunOnce(now)) } + +func testControllerFiltersDomains(t *testing.T, configuredEndpoints []*endpoint.Endpoint, domainFilter endpoint.DomainFilterInterface, providerEndpoints []*endpoint.Endpoint, expectedChanges []*plan.Changes) { + t.Helper() + source := new(testutils.MockSource) + source.On("Endpoints").Return(configuredEndpoints, nil) + + // Fake some existing records in our DNS provider and validate some desired changes. + provider := &filteredMockProvider{ + RecordsStore: providerEndpoints, + } + r, err := registry.NewNoopRegistry(provider) + + require.NoError(t, err) + + ctrl := &Controller{ + Source: source, + Registry: r, + Policy: &plan.SyncPolicy{}, + DomainFilter: domainFilter, + } + + assert.NoError(t, ctrl.RunOnce(context.Background())) + assert.Equal(t, 1, provider.RecordsCallCount) + require.Len(t, provider.ApplyChangesCalls, len(expectedChanges)) + for i, change := range expectedChanges { + assert.Equal(t, *change, *provider.ApplyChangesCalls[i]) + } +} + +func TestControllerSkipsEmptyChanges(t *testing.T) { + testControllerFiltersDomains( + t, + []*endpoint.Endpoint{ + { + DNSName: "create-record.other.tld", + RecordType: endpoint.RecordTypeA, + Targets: endpoint.Targets{"1.2.3.4"}, + }, + { + DNSName: "some-record.used.tld", + RecordType: endpoint.RecordTypeA, + Targets: endpoint.Targets{"8.8.8.8"}, + }, + }, + endpoint.NewDomainFilter([]string{"used.tld"}), + []*endpoint.Endpoint{ + { + DNSName: "some-record.used.tld", + RecordType: endpoint.RecordTypeA, + Targets: endpoint.Targets{"8.8.8.8"}, + }, + }, + []*plan.Changes{}, + ) +} + +func TestWhenNoFilterControllerConsidersAllComain(t *testing.T) { + testControllerFiltersDomains( + t, + []*endpoint.Endpoint{ + { + DNSName: "create-record.other.tld", + RecordType: endpoint.RecordTypeA, + Targets: endpoint.Targets{"1.2.3.4"}, + }, + { + DNSName: "some-record.used.tld", + RecordType: endpoint.RecordTypeA, + Targets: endpoint.Targets{"8.8.8.8"}, + }, + }, + nil, + []*endpoint.Endpoint{ + { + DNSName: "some-record.used.tld", + RecordType: endpoint.RecordTypeA, + Targets: endpoint.Targets{"8.8.8.8"}, + }, + }, + []*plan.Changes{ + { + Create: []*endpoint.Endpoint{ + { + DNSName: "create-record.other.tld", + RecordType: endpoint.RecordTypeA, + Targets: endpoint.Targets{"1.2.3.4"}, + }, + }, + }, + }, + ) +} + +func TestWhenMultipleControllerConsidersAllFilteredComain(t *testing.T) { + testControllerFiltersDomains( + t, + []*endpoint.Endpoint{ + { + DNSName: "create-record.other.tld", + RecordType: endpoint.RecordTypeA, + Targets: endpoint.Targets{"1.2.3.4"}, + }, + { + DNSName: "some-record.used.tld", + RecordType: endpoint.RecordTypeA, + Targets: endpoint.Targets{"1.1.1.1"}, + }, + { + DNSName: "create-record.unused.tld", + RecordType: endpoint.RecordTypeA, + Targets: endpoint.Targets{"1.2.3.4"}, + }, + }, + endpoint.NewDomainFilter([]string{"used.tld", "other.tld"}), + []*endpoint.Endpoint{ + { + DNSName: "some-record.used.tld", + RecordType: endpoint.RecordTypeA, + Targets: endpoint.Targets{"8.8.8.8"}, + }, + }, + []*plan.Changes{ + { + Create: []*endpoint.Endpoint{ + { + DNSName: "create-record.other.tld", + RecordType: endpoint.RecordTypeA, + Targets: endpoint.Targets{"1.2.3.4"}, + }, + }, + UpdateOld: []*endpoint.Endpoint{ + { + DNSName: "some-record.used.tld", + RecordType: endpoint.RecordTypeA, + Targets: endpoint.Targets{"8.8.8.8"}, + Labels: endpoint.Labels{}, + }, + }, + UpdateNew: []*endpoint.Endpoint{ + { + DNSName: "some-record.used.tld", + RecordType: endpoint.RecordTypeA, + Targets: endpoint.Targets{"1.1.1.1"}, + Labels: endpoint.Labels{ + "owner": "", + }, + }, + }, + }, + }, + ) +} diff --git a/endpoint/domain_filter.go b/endpoint/domain_filter.go index 91445e862..dcec5ba09 100644 --- a/endpoint/domain_filter.go +++ b/endpoint/domain_filter.go @@ -21,6 +21,44 @@ import ( "strings" ) +// DomainFilterInterface defines the interface to select matching domains for a specific provider or runtime +type DomainFilterInterface interface { + Match(domain string) bool + IsConfigured() bool +} + +type MatchAllDomainFilters []DomainFilterInterface + +func (f MatchAllDomainFilters) Match(domain string) bool { + if !f.IsConfigured() { + return true + } + for _, filter := range f { + if filter == nil { + continue + } + if filter.IsConfigured() && !filter.Match(domain) { + return false + } + } + return true +} + +func (f MatchAllDomainFilters) IsConfigured() bool { + if f == nil { + return false + } + for _, filter := range f { + if filter == nil { + continue + } + if filter.IsConfigured() { + return true + } + } + return len(f) > 0 +} + // DomainFilter holds a lists of valid domain names type DomainFilter struct { // Filters define what domains to match diff --git a/plan/plan.go b/plan/plan.go index 3ae346fcc..30ee39931 100644 --- a/plan/plan.go +++ b/plan/plan.go @@ -21,6 +21,9 @@ import ( "strconv" "strings" + "github.com/google/go-cmp/cmp" + log "github.com/sirupsen/logrus" + "sigs.k8s.io/external-dns/endpoint" ) @@ -40,7 +43,7 @@ type Plan struct { // Populated after calling Calculate() Changes *Changes // DomainFilter matches DNS names - DomainFilter endpoint.DomainFilter + DomainFilter endpoint.DomainFilterInterface // Property comparator compares custom properties of providers PropertyComparator PropertyComparator // DNS record types that will be considered for management @@ -115,12 +118,23 @@ func (t planTable) addCandidate(e *endpoint.Endpoint) { t.rows[dnsName][e.SetIdentifier].candidates = append(t.rows[dnsName][e.SetIdentifier].candidates, e) } +func (c *Changes) HasChanges() bool { + if len(c.Create) > 0 || len(c.Delete) > 0 { + return true + } + return !cmp.Equal(c.UpdateNew, c.UpdateOld) +} + // Calculate computes the actions needed to move current state towards desired // 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() + if p.DomainFilter == nil { + p.DomainFilter = endpoint.MatchAllDomainFilters(nil) + } + for _, current := range filterRecordsForPlan(p.Current, p.DomainFilter, p.ManagedRecords) { t.addCurrent(current) } @@ -227,12 +241,13 @@ func (p *Plan) shouldUpdateProviderSpecific(desired, current *endpoint.Endpoint) // Per RFC 1034, CNAME records conflict with all other records - it is the // only record with this property. The behavior of the planner may need to be // made more sophisticated to codify this. -func filterRecordsForPlan(records []*endpoint.Endpoint, domainFilter endpoint.DomainFilter, managedRecords []string) []*endpoint.Endpoint { +func filterRecordsForPlan(records []*endpoint.Endpoint, domainFilter endpoint.DomainFilterInterface, managedRecords []string) []*endpoint.Endpoint { filtered := []*endpoint.Endpoint{} for _, record := range records { // Ignore records that do not match the domain filter provided if !domainFilter.Match(record.DNSName) { + log.Debugf("ignoring record %s that does not match domain filter", record.DNSName) continue } if isManagedRecord(record.RecordType, managedRecords) { diff --git a/provider/aws/aws.go b/provider/aws/aws.go index 2fb253e85..51bce1f43 100644 --- a/provider/aws/aws.go +++ b/provider/aws/aws.go @@ -465,6 +465,21 @@ func (p *AWSProvider) createUpdateChanges(newEndpoints, oldEndpoints []*endpoint return combined } +// GetDomainFilter generates a filter to exclude any domain that is not controlled by the provider +func (p *AWSProvider) GetDomainFilter() endpoint.DomainFilterInterface { + zones, err := p.Zones(context.Background()) + if err != nil { + log.Errorf("failed to list zones: %v", err) + return &endpoint.DomainFilter{} + } + zoneNames := []string(nil) + for _, z := range zones { + zoneNames = append(zoneNames, aws.StringValue(z.Name), "."+aws.StringValue(z.Name)) + } + log.Infof("Applying provider record filter for domains: %v", zoneNames) + return endpoint.NewDomainFilter(zoneNames) +} + // ApplyChanges applies a given set of changes in a given zone. func (p *AWSProvider) ApplyChanges(ctx context.Context, changes *plan.Changes) error { zones, err := p.Zones(ctx) diff --git a/provider/aws/aws_test.go b/provider/aws/aws_test.go index 563914c47..1eb810573 100644 --- a/provider/aws/aws_test.go +++ b/provider/aws/aws_test.go @@ -313,6 +313,29 @@ func TestAWSZones(t *testing.T) { } } +func TestAWSRecordsFilter(t *testing.T) { + provider, _ := newAWSProvider(t, endpoint.DomainFilter{}, provider.ZoneIDFilter{}, provider.ZoneTypeFilter{}, false, false, nil) + domainFilter := provider.GetDomainFilter() + assert.NotNil(t, domainFilter) + require.IsType(t, endpoint.DomainFilter{}, domainFilter) + count := 0 + filters := domainFilter.(endpoint.DomainFilter).Filters + for _, tld := range []string{ + "zone-4.ext-dns-test-3.teapot.zalan.do", + ".zone-4.ext-dns-test-3.teapot.zalan.do", + "zone-2.ext-dns-test-2.teapot.zalan.do", + ".zone-2.ext-dns-test-2.teapot.zalan.do", + "zone-3.ext-dns-test-2.teapot.zalan.do", + ".zone-3.ext-dns-test-2.teapot.zalan.do", + "zone-4.ext-dns-test-3.teapot.zalan.do", + ".zone-4.ext-dns-test-3.teapot.zalan.do", + } { + assert.Contains(t, filters, tld) + count++ + } + assert.Len(t, filters, count) +} + func TestAWSRecords(t *testing.T) { provider, _ := newAWSProvider(t, endpoint.NewDomainFilter([]string{"ext-dns-test-2.teapot.zalan.do."}), provider.NewZoneIDFilter([]string{}), provider.NewZoneTypeFilter(""), false, false, []*endpoint.Endpoint{ endpoint.NewEndpointWithTTL("list-test.zone-1.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeA, endpoint.TTL(recordTTL), "1.2.3.4"), diff --git a/provider/provider.go b/provider/provider.go index c0c66cff0..ff9c40210 100644 --- a/provider/provider.go +++ b/provider/provider.go @@ -31,6 +31,7 @@ type Provider interface { ApplyChanges(ctx context.Context, changes *plan.Changes) error PropertyValuesEqual(name string, previous string, current string) bool AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint + GetDomainFilter() endpoint.DomainFilterInterface } type BaseProvider struct { @@ -44,6 +45,10 @@ func (b BaseProvider) PropertyValuesEqual(name, previous, current string) bool { return previous == current } +func (b BaseProvider) GetDomainFilter() endpoint.DomainFilterInterface { + return endpoint.DomainFilter{} +} + type contextKey struct { name string } diff --git a/registry/aws_sd_registry.go b/registry/aws_sd_registry.go index e08537419..9a87da07f 100644 --- a/registry/aws_sd_registry.go +++ b/registry/aws_sd_registry.go @@ -42,6 +42,10 @@ func NewAWSSDRegistry(provider provider.Provider, ownerID string) (*AWSSDRegistr }, nil } +func (sdr *AWSSDRegistry) GetDomainFilter() endpoint.DomainFilterInterface { + return sdr.provider.GetDomainFilter() +} + // Records calls AWS SD API and expects AWS SD provider to provider Owner/Resource information as a serialized // value in the AWSSDDescriptionLabel value in the Labels map func (sdr *AWSSDRegistry) Records(ctx context.Context) ([]*endpoint.Endpoint, error) { diff --git a/registry/noop.go b/registry/noop.go index 34c2b4610..73257730c 100644 --- a/registry/noop.go +++ b/registry/noop.go @@ -36,6 +36,10 @@ func NewNoopRegistry(provider provider.Provider) (*NoopRegistry, error) { }, nil } +func (im *NoopRegistry) GetDomainFilter() endpoint.DomainFilterInterface { + return im.provider.GetDomainFilter() +} + // Records returns the current records from the dns provider func (im *NoopRegistry) Records(ctx context.Context) ([]*endpoint.Endpoint, error) { return im.provider.Records(ctx) diff --git a/registry/registry.go b/registry/registry.go index 3367355d5..d80890b0c 100644 --- a/registry/registry.go +++ b/registry/registry.go @@ -34,6 +34,7 @@ type Registry interface { ApplyChanges(ctx context.Context, changes *plan.Changes) error PropertyValuesEqual(attribute string, previous string, current string) bool AdjustEndpoints(endpoints []*endpoint.Endpoint) []*endpoint.Endpoint + GetDomainFilter() endpoint.DomainFilterInterface } //TODO(ideahitme): consider moving this to Plan diff --git a/registry/txt.go b/registry/txt.go index eb58fc6af..42944615c 100644 --- a/registry/txt.go +++ b/registry/txt.go @@ -68,6 +68,10 @@ func NewTXTRegistry(provider provider.Provider, txtPrefix, txtSuffix, ownerID st }, nil } +func (im *TXTRegistry) GetDomainFilter() endpoint.DomainFilterInterface { + return im.provider.GetDomainFilter() +} + // Records returns the current records from the registry excluding TXT Records // If TXT records was created previously to indicate ownership its corresponding value // will be added to the endpoints Labels map