-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Support for multiple domains within --domain-filter #252
Conversation
Thanks for the PR! |
main.go
Outdated
@@ -108,20 +109,22 @@ func main() { | |||
|
|||
endpointsSource := source.NewDedupSource(source.NewMultiSource(sources)) | |||
|
|||
domainFilter := domains.NewDomainFilter(cfg.DomainFilter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 very nice!
pkg/apis/externaldns/types.go
Outdated
@@ -99,7 +99,7 @@ func (cfg *Config) ParseFlags(args []string) error { | |||
// Flags related to providers | |||
app.Flag("provider", "The DNS provider where the DNS records will be created (required, options: aws, google, azure, cloudflare, digitalocean, inmemory)").Required().PlaceHolder("provider").EnumVar(&cfg.Provider, "aws", "google", "azure", "cloudflare", "digitalocean", "inmemory") | |||
app.Flag("google-project", "When using the Google provider, specify the Google project (required when --provider=google)").Default(defaultConfig.GoogleProject).StringVar(&cfg.GoogleProject) | |||
app.Flag("domain-filter", "Limit possible target zones by a domain suffix (optional)").Default(defaultConfig.DomainFilter).StringVar(&cfg.DomainFilter) | |||
app.Flag("domain-filter", "Limit possible target zones by a comma separeted list of domain suffixes (optional)").Default(defaultConfig.DomainFilter).StringVar(&cfg.DomainFilter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to change this to use StringVars
, note the traling s
. This already supports processing multiple strings into a single string slice. This avoids strings.Split(",")
in your code.
Furthermore, it changes the usage to provide the flag --domain-filter
multiple times. Your example would become:
external-dns \
--domain-filter=example.com \
--domain-filter=company.org \
--domain-filter=staging-company.org
This approach is also used by helm
and kubernetes
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason doing this way is, it's much easier to inject multiple domains as a single environmental variable into the pod. But might we work with a variable like this DOMAINS="--domain-filter foobar.org --domain-filter bla.org"
..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, but I would like to push this problem to kingpin. Ideally kingpin would allow to support both modes when using StringVars
. FWIW kingpin's env var for this flag is DOMAIN_FILTER
and you can specify multiple values by separating them with a line break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol, you already know that 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but it's ugly.
pkg/util/domains/domain_test.go
Outdated
for i, tt := range domainFilterTests { | ||
domainFilter := NewDomainFilter(tt.domainFilter) | ||
for _, domain := range tt.domains { | ||
require.Equal(t, tt.expected, domainFilter.Match(domain), "should not fail: %v in test-case #%v", domain, i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer assert.Equal
here.
pkg/util/domains/domain_test.go
Outdated
for _, tt := range domainFilterTests { | ||
domainFilter := DomainFilter{} | ||
for i, domain := range tt.domains { | ||
require.True(t, domainFilter.Match(domain), "should not fail: %v in test-case #%v", domain, i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer assert.True
here.
pkg/util/domains/domains.go
Outdated
filters = append(filters[:i], filters[i+1:]...) | ||
i-- | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks quite complicated. Maybe we can add some comments on what it's doing.
pkg/util/domains/domains.go
Outdated
|
||
for _, filter := range df.filters { | ||
|
||
// user can define domains either with trailing dot or without. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to sanitize both domains and then have only one suffix check. E.g. you could append or strip it from the provided filters before storing them. And then do the same with domain
here before going through the filters.
We settled with stripping it from the domain in several parts of the code. But yeah, it's a little annoying.
Please take a look. Changed the implementation to StringsVar() and change the code regarding your comments. |
main.go
Outdated
@@ -35,6 +35,7 @@ import ( | |||
"github.com/kubernetes-incubator/external-dns/controller" | |||
"github.com/kubernetes-incubator/external-dns/pkg/apis/externaldns" | |||
"github.com/kubernetes-incubator/external-dns/pkg/apis/externaldns/validation" | |||
"github.com/kubernetes-incubator/external-dns/pkg/util/domains" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this in the util
? it is generally not a good practice to put things into util
, if it is part of the provider, it should be in the provider package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I put it directly into provider package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the domain filter only to be used by providers then yes, it should be either in provider package directory, or it's child subdirectory.
If we find that provider
exposes too many types/functions we can think of refactoring it later on
pkg/util/domains/domains_test.go
Outdated
} | ||
} | ||
|
||
func TestDomainFilter_Match_default_Filter_always_matches(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove underscores and use camelcase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree
@@ -84,7 +84,7 @@ spec: | |||
args: | |||
- --source=service | |||
- --source=ingress | |||
- --domain-filter=external-dns-test.my-org.com. # will make ExternalDNS see only the hosted zones matching provided domain, omit to process all available hosted zones | |||
- --domain-filter=external-dns-test.my-org.com # will make ExternalDNS see only the hosted zones matching provided domain, omit to process all available hosted zones |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not change tutorials before the new release, the tutorial is anyway tied to the previous release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, is there a branch that is setup for those types of changes that PRs could be created against to allow PR authors the ability to update the docs with a stable 'release' process and limit your need to update them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jrnt30 no, but would be interesting to hear if you don't mind explaining the process in the docs (maybe create a PR with modified contribution guidelines) ?
pkg/util/domains/domains.go
Outdated
|
||
// NewDomainFilter returns a new DomainFilter given a comma separated list of domains | ||
func NewDomainFilter(filters []string) DomainFilter { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: it is better to make
a new slice and copy the strings into it without retaining pointer from SliceHeader
please update the changelog, and it is good to be merged |
Updated the changelog. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
@totallyunknown This is also a fix for #229 right? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great!
Thanks for merging! |
@totallyunknown Thanks! |
) * Support for multiple domains within --domain-filter The parameter accepts a comma separated list of domains with or without trailing dot. Example: --domain-filter="example.org, company.test.,staging.com". Closes kubernetes-sigs#247 and kubernetes-sigs#229 * Add boilerplate header * Add documentation for methods and structs * use StringsVar for the domain-filter flag * go fmt * Remove camel case from tests * Revert changes in README.md * Move DomainFilter to provider package * Make a new slice and copy elements to it * Update CHANGELOG.md * docs: change minor spelling mistake
The parameter accepts a comma separated list of domains with or without trailing dot. Example:
--domain-filter="example.org, company.test.,staging.com"
. Closes #247 and #229