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.

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

Ignore unknown provider specific properties #4669

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
61 changes: 61 additions & 0 deletions endpoint/provider_specific_property_filter.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move this instead to pkg/filters ? Example pull request https://github.com/gofogo/k8s-sigs-external-dns-fork/pull/3/files, is not officially a pull request against this repository, as there is another work that needs doing as well

Copyright 2017 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 endpoint

import (
"strings"

log "github.com/sirupsen/logrus"
)

type ProviderSpecificPropertyFilter struct {
// Names of the provider specific properties to match
Names []string
// Prefixes of the provider specific properties to match
Prefixes []string
}

// Match checks whether a ProviderSpecificProperty.Name can be found in the
// ProviderSpecificPropertyFilter.
func (pf ProviderSpecificPropertyFilter) Match(name string) bool {
for _, pfName := range pf.Names {
if pfName == name {
return true
}
}
for _, prefix := range pf.Prefixes {
if strings.HasPrefix(name, prefix) {
return true
}
}
return false
}

// Filter removes all ProviderSpecificProperty's that don't match from every endpoint.
func (pf ProviderSpecificPropertyFilter) Filter(endpoints []*Endpoint) {
for _, ep := range endpoints {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to loosely couple this filter and endpoints with some sort of an interface or pass a map, interface{} etc?

for _, providerSpecific := range ep.ProviderSpecific {
if !pf.Match(providerSpecific.Name) {
log.WithFields(log.Fields{
"dnsName": ep.DNSName,
"targets": ep.Targets,
}).Debugf("Provider specific property ignored by provider: %s", providerSpecific.Name)
ep.DeleteProviderSpecificProperty(providerSpecific.Name)
}
}
}
}
175 changes: 175 additions & 0 deletions endpoint/provider_specific_property_filter_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,175 @@
/*
Copyright 2017 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 endpoint

import (
"fmt"
"testing"

"github.com/stretchr/testify/assert"
)

type providerSpecificPropertyFilterTest struct {
names []string
prefixes []string
endpoints []*Endpoint
expectedEndpoints []*Endpoint
}

var providerSpecificPropertyFilterTests = []providerSpecificPropertyFilterTest{
// 0. No Names or Prefixes
{
[]string{},
[]string{},
[]*Endpoint{
{
DNSName: "a.example.com",
ProviderSpecific: []ProviderSpecificProperty{
{Name: "prefix1/name1"},
{Name: "name1"},
},
},
{
DNSName: "b.example.com",
ProviderSpecific: []ProviderSpecificProperty{
{Name: "prefix2/name1"},
{Name: "name2"},
},
},
},
[]*Endpoint{
{
DNSName: "a.example.com",
ProviderSpecific: []ProviderSpecificProperty{},
},
{
DNSName: "b.example.com",
ProviderSpecific: []ProviderSpecificProperty{},
},
},
},
// 1. Only Names
{
[]string{"name1", "name2"},
[]string{},
[]*Endpoint{
{
DNSName: "a.example.com",
ProviderSpecific: []ProviderSpecificProperty{
{Name: "name1"},
{Name: "name2"},
{Name: "name3"},
{Name: "prefix1/name1"},
},
},
},
[]*Endpoint{
{
DNSName: "a.example.com",
ProviderSpecific: []ProviderSpecificProperty{
{Name: "name1"},
{Name: "name2"},
},
},
},
},
// 2. Only Prefixes
{
[]string{},
[]string{"prefix1/", "prefix2"},
[]*Endpoint{
{
DNSName: "a.example.com",
ProviderSpecific: []ProviderSpecificProperty{
{Name: "prefix1/foo"},
{Name: "prefix1-bar"},
{Name: "prefix2"},
{Name: "prefix3"},
},
},
},
[]*Endpoint{
{
DNSName: "a.example.com",
ProviderSpecific: []ProviderSpecificProperty{
{Name: "prefix1/foo"},
{Name: "prefix2"},
},
},
},
},
// 3. No Endpoints
{
[]string{"name1"},
[]string{"prefix1/"},
[]*Endpoint{},
[]*Endpoint{},
},
// 4. Both Names and Prefixes
{
[]string{"name1"},
[]string{"prefix1/"},
[]*Endpoint{
{
DNSName: "a.example.com",
ProviderSpecific: []ProviderSpecificProperty{
{Name: "prefix1/property"},
{Name: "prefix2/property"},
{Name: "name10"},
},
},
{
DNSName: "b.example.com",
ProviderSpecific: []ProviderSpecificProperty{
{Name: "prefix1/property"},
{Name: "name1"},
{Name: "name2"},
},
},
},
[]*Endpoint{
{
DNSName: "a.example.com",
ProviderSpecific: []ProviderSpecificProperty{
{Name: "prefix1/property"},
},
},
{
DNSName: "b.example.com",
ProviderSpecific: []ProviderSpecificProperty{
{Name: "prefix1/property"},
{Name: "name1"},
},
},
},
},
}

func TestPropertyFilter(t *testing.T) {
for i, tt := range providerSpecificPropertyFilterTests {
t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
propertyFilter := ProviderSpecificPropertyFilter{
Names: tt.names,
Prefixes: tt.prefixes,
}

propertyFilter.Filter(tt.endpoints)

assert.Equal(t, tt.expectedEndpoints, tt.endpoints)
})
}
}
13 changes: 13 additions & 0 deletions provider/aws/aws.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,11 @@ const (
sameZoneAlias = "same-zone"
)

var awsProviderSpecificPropertyFilter = endpoint.ProviderSpecificPropertyFilter{
Names: []string{providerSpecificAlias},
Prefixes: []string{"aws/"},
}

// see: https://docs.aws.amazon.com/general/latest/gr/elb.html
var canonicalHostedZones = map[string]string{
// Application Load Balancers and Classic Load Balancers
Expand Down Expand Up @@ -296,6 +301,9 @@ func NewAWSProvider(awsConfig AWSConfig, clients map[string]Route53API) (*AWSPro
dryRun: awsConfig.DryRun,
zonesCache: &zonesListCache{duration: awsConfig.ZoneCacheDuration},
failedChangesQueue: make(map[string]Route53Changes),
BaseProvider: provider.BaseProvider{
ProviderSpecificPropertyFilter: awsProviderSpecificPropertyFilter,
},
}

return provider, nil
Expand Down Expand Up @@ -749,6 +757,11 @@ func (p *AWSProvider) newChanges(action route53types.ChangeAction, endpoints []*
// Example: CNAME endpoints pointing to ELBs will have a `alias` provider-specific property
// added to match the endpoints generated from existing alias records in Route53.
func (p *AWSProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) ([]*endpoint.Endpoint, error) {
endpoints, err := p.BaseProvider.AdjustEndpoints(endpoints)
if err != nil {
return endpoints, err
}

for _, ep := range endpoints {
alias := false

Expand Down
5 changes: 5 additions & 0 deletions provider/aws/aws_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -568,6 +568,7 @@ func TestAWSAdjustEndpoints(t *testing.T) {
endpoint.NewEndpoint("cname-test-elb-no-alias.zone-2.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeCNAME, "foo.eu-central-1.elb.amazonaws.com").WithProviderSpecific(providerSpecificAlias, "false"),
endpoint.NewEndpoint("cname-test-elb-no-eth.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeCNAME, "foo.eu-central-1.elb.amazonaws.com").WithProviderSpecific(providerSpecificEvaluateTargetHealth, "false"), // eth = evaluate target health
endpoint.NewEndpoint("cname-test-elb-alias.zone-2.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeCNAME, "foo.eu-central-1.elb.amazonaws.com").WithProviderSpecific(providerSpecificAlias, "true").WithProviderSpecific(providerSpecificEvaluateTargetHealth, "true"),
endpoint.NewEndpoint("a-test-other-provider-specific.zone-1.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeA, "8.8.8.8").WithProviderSpecific(providerSpecificAlias, "true").WithProviderSpecific(providerSpecificEvaluateTargetHealth, "true").WithProviderSpecific("external-dns.alpha.kubernetes.io/cloudflare-proxied", "false"),
}

records, err := provider.AdjustEndpoints(records)
Expand All @@ -581,6 +582,7 @@ func TestAWSAdjustEndpoints(t *testing.T) {
endpoint.NewEndpoint("cname-test-elb-no-alias.zone-2.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeCNAME, "foo.eu-central-1.elb.amazonaws.com").WithProviderSpecific(providerSpecificAlias, "false"),
endpoint.NewEndpoint("cname-test-elb-no-eth.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeA, "foo.eu-central-1.elb.amazonaws.com").WithProviderSpecific(providerSpecificAlias, "true").WithProviderSpecific(providerSpecificEvaluateTargetHealth, "false"), // eth = evaluate target health
endpoint.NewEndpoint("cname-test-elb-alias.zone-2.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeA, "foo.eu-central-1.elb.amazonaws.com").WithProviderSpecific(providerSpecificAlias, "true").WithProviderSpecific(providerSpecificEvaluateTargetHealth, "true"),
endpoint.NewEndpoint("a-test-other-provider-specific.zone-1.ext-dns-test-2.teapot.zalan.do", endpoint.RecordTypeA, "8.8.8.8").WithProviderSpecific(providerSpecificAlias, "true").WithProviderSpecific(providerSpecificEvaluateTargetHealth, "true"),
})
}

Expand Down Expand Up @@ -1970,6 +1972,9 @@ func newAWSProviderWithTagFilter(t *testing.T, domainFilter endpoint.DomainFilte
dryRun: false,
zonesCache: &zonesListCache{duration: 1 * time.Minute},
failedChangesQueue: make(map[string]Route53Changes),
BaseProvider: provider.BaseProvider{
ProviderSpecificPropertyFilter: awsProviderSpecificPropertyFilter,
},
}

createAWSZone(t, provider, &route53types.HostedZone{
Expand Down
13 changes: 13 additions & 0 deletions provider/cloudflare/cloudflare.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,13 @@ const (
cloudFlareUpdate = "UPDATE"
// defaultCloudFlareRecordTTL 1 = automatic
defaultCloudFlareRecordTTL = 1
providerSpecificProxied = "external-dns.alpha.kubernetes.io/cloudflare-proxied"
)

var cloudflareProviderSpecificPropertyFilter = endpoint.ProviderSpecificPropertyFilter{
Names: []string{providerSpecificProxied},
}

// We have to use pointers to bools now, as the upstream cloudflare-go library requires them
// see: https://github.com/cloudflare/cloudflare-go/pull/595

Expand Down Expand Up @@ -210,6 +215,9 @@ func NewCloudFlareProvider(domainFilter endpoint.DomainFilter, zoneIDFilter prov
DryRun: dryRun,
DNSRecordsPerPage: dnsRecordsPerPage,
RegionKey: regionKey,
BaseProvider: provider.BaseProvider{
ProviderSpecificPropertyFilter: cloudflareProviderSpecificPropertyFilter,
},
}
return provider, nil
}
Expand Down Expand Up @@ -416,6 +424,11 @@ func (p *CloudFlareProvider) submitChanges(ctx context.Context, changes []*cloud

// AdjustEndpoints modifies the endpoints as needed by the specific provider
func (p *CloudFlareProvider) AdjustEndpoints(endpoints []*endpoint.Endpoint) ([]*endpoint.Endpoint, error) {
endpoints, err := p.BaseProvider.AdjustEndpoints(endpoints)
if err != nil {
return endpoints, err
}

adjustedEndpoints := []*endpoint.Endpoint{}
for _, e := range endpoints {
proxied := shouldBeProxied(e, p.proxiedByDefault)
Expand Down
Loading
Loading