allow filtering by source annotation #354

khrisrichardson · 2017-10-09T20:14:02Z

In certain scenarios, there may be multiple ingress controllers running in the same Kubernetes cluster with different ingress classes (e.g. - ALB + Tectonic ingress), and it would be desirable to toggle which controllers' resources had their DNS records managed by external-dns. The intention of this PR is to manage that scenario via pattern matching on the ingress class annotation: kubernetes.io/ingress.class.

linki · 2017-10-10T16:23:53Z

@khrisrichardson Thanks for your contribution.

I would like to avoid tying ExternalDNS to a specific annotation that's used by another project. However, these two projects are related to each other so I'm not opposed to the idea.

Though, we could solve the issue more generic by allowing people to filter Ingresses (and Services etc.) by arbitrary annotation selectors. This way people could invoke ExternalDNS with:

$ ./external-dns --source=Ingress --source-annotation-filter=kubernetes.io/ingress.class=foo --provider=...

This would allow people to run ExternalDNS on a subset of Ingresses for any reason and wouldn't tie us to the particular ingress.class annotation.

We did the same thing in mate to work in combination with ExternalDNS: https://github.com/linki/mate#compatibility-with-other-controllers

I see, though, that ingress.class is a common case so it could justify a separate flag (like this PR).

Let me know what you think.

khrisrichardson · 2017-10-10T17:24:21Z

Hi @linki. I had similar reservations about using a filter on an annotation outside the project's scope. I think we can make this generic pretty easily.

linki · 2017-10-11T10:33:42Z

main.go

+		FQDNTemplate:           cfg.FQDNTemplate,
+		Compatibility:          cfg.Compatibility,
+		PublishInternal:        cfg.PublishInternal,
+		SourceAnnotationFilter: cfg.SourceAnnotationFilter,


Let's omit the Source prefix because we're already using the Config from the source package.

linki · 2017-10-11T10:35:36Z

pkg/apis/externaldns/types.go

+	LogFormat              string
+	MetricsAddress         string
+	LogLevel               string
+	SourceAnnotationFilter string


I tried to maintain a cosistent ordering. Let's add this under/behind the source's namespace parameter. So, in all the structs, tests and kingpin definitionions.

linki · 2017-10-11T10:39:33Z

pkg/apis/externaldns/types.go

@@ -139,6 +141,7 @@ func (cfg *Config) ParseFlags(args []string) error {
 	app.Flag("log-format", "The format in which log messages are printed (default: text, options: text, json)").Default(defaultConfig.LogFormat).EnumVar(&cfg.LogFormat, "text", "json")
 	app.Flag("metrics-address", "Specify where to serve the metrics and health check endpoint (default: :7979)").Default(defaultConfig.MetricsAddress).StringVar(&cfg.MetricsAddress)
 	app.Flag("log-level", "Set the level of logging. (default: info, options: panic, debug, info, warn, error, fatal").Default(defaultConfig.LogLevel).EnumVar(&cfg.LogLevel, allLogLevelsAsStrings()...)
+	app.Flag("source-annotation-filter", "Filter sources managed by external-dns via annotation regexp pattern (default: all sources)").Default(defaultConfig.SourceAnnotationFilter).StringVar(&cfg.SourceAnnotationFilter)


Let's just use --annotation-filter. It currently fits better with --namespace which is also a filter on the source side. I think we won't have annotation filtering stuff on the provider side for a while.

linki · 2017-10-11T10:47:24Z

source/ingress.go

+			log.Debugf("Skipping ingress %s/%s because '%s' annotation does not match pattern, found: %s, expected: %s",
+				ing.Namespace, ing.Name, annotation, string, pattern)
+			continue
+		}


I recommend adopting this pattern: https://github.com/linki/chaoskube/blob/v0.6.0/chaoskube/chaoskube.go#L180-L199

With this you get all the matching functionality from Kubernetes itself for free, including matching multiple labels, checking for existence (--filter=foo) as well as exclusion (--filter=ingress.class!=nginx).

…tern

khrisrichardson · 2017-10-19T19:59:42Z

@linki, are there any other changes you would propose following my latest updates. Do you think we should rename annotationFilter to annotationSelector?

szuecs · 2017-10-28T07:06:56Z

source/ingress.go

@@ -70,6 +73,10 @@ func (sc *ingressSource) Endpoints() ([]*endpoint.Endpoint, error) {
 	if err != nil {
 		return nil, err
 	}
+	ingresses.Items, err = sc.filterByAnnotations(ingresses.Items, sc.annotationFilter)


if you call a method of "sc", you should not pass sc.annotationFilter as parameter, but use it in the method from your receiver.

szuecs · 2017-10-28T07:12:05Z

source/service.go

@@ -75,6 +78,10 @@ func (sc *serviceSource) Endpoints() ([]*endpoint.Endpoint, error) {
 	if err != nil {
 		return nil, err
 	}
+	services.Items, err = sc.filterByAnnotations(services.Items, sc.annotationFilter)


here is the same case, passing sc.annotationFilter to a function receiver of sc.

szuecs · 2017-10-28T07:13:08Z

source/service.go

@@ -148,6 +155,37 @@ func (sc *serviceSource) endpoints(svc *v1.Service) []*endpoint.Endpoint {
 	return endpoints
 }

+// filterByAnnotations filters a list of services by a given annotation selector.
+func (sc *serviceSource) filterByAnnotations(services []v1.Service, annotationFilter string) ([]v1.Service, error) {


I think there was no case to pass the filter as parameter, because it's already member of serviceSource.

szuecs · 2017-10-28T07:14:11Z

For me this looks good. Only a tiny nitpick and we can merge it.
@linki @ideahitme

…ternal-dns into ingress-class-pattern

linki · 2017-10-29T10:48:21Z

LGTM 👍

Thanks @khrisrichardson and @szuecs for the implementation and reviews.

hjacobs · 2017-11-08T21:49:12Z

Should we merge? Any reason not to?

szuecs · 2017-11-09T08:29:11Z

please merge @hjacobs , I just don't have the right to do so

szuecs · 2017-11-09T08:29:26Z

@ideahitme can you merge this please?

* allow filtering by ingress class * generic source annotation filter as opposed to ingress class filter * rename and fix argument ordering, switch to label selector semantics * remove redundant parameters

allow filtering by ingress class

406d452

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 9, 2017

generic source annotation filter as opposed to ingress class filter

34c692c

khrisrichardson changed the title ~~allow filtering by ingress class~~ allow filtering by source annotation Oct 10, 2017

linki reviewed Oct 11, 2017

View reviewed changes

khrisrichardson added 3 commits October 11, 2017 15:29

rename and fix argument ordering, switch to label selector semantics

43fc61e

Merge remote-tracking branch 'upstream/master' into ingress-class-pat…

aa4621f

…tern

Merge branch 'master' into ingress-class-pattern

e3e1ddd

mikkeloscar mentioned this pull request Oct 16, 2017

handle multiple ingresses of the same type #348

Closed

szuecs reviewed Oct 28, 2017

View reviewed changes

khrisrichardson added 2 commits October 28, 2017 09:36

remove redundant parameters

fd39ae1

Merge branch 'ingress-class-pattern' of github.com:khrisrichardson/ex…

14280ca

…ternal-dns into ingress-class-pattern

linki added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 29, 2017

ideahitme merged commit b23765e into kubernetes-sigs:master Nov 9, 2017

khrisrichardson deleted the ingress-class-pattern branch November 27, 2017 17:51

hjacobs mentioned this pull request Jan 17, 2018

Ingress - exclude host rules #440

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow filtering by source annotation #354

allow filtering by source annotation #354

khrisrichardson commented Oct 9, 2017

linki commented Oct 10, 2017

khrisrichardson commented Oct 10, 2017

linki Oct 11, 2017

linki Oct 11, 2017

linki Oct 11, 2017 •

edited

Loading

linki Oct 11, 2017 •

edited

Loading

khrisrichardson commented Oct 19, 2017

szuecs Oct 28, 2017

szuecs Oct 28, 2017

szuecs Oct 28, 2017

szuecs commented Oct 28, 2017 •

edited

Loading

linki commented Oct 29, 2017

hjacobs commented Nov 8, 2017

szuecs commented Nov 9, 2017

szuecs commented Nov 9, 2017

allow filtering by source annotation #354

allow filtering by source annotation #354

Conversation

khrisrichardson commented Oct 9, 2017

linki commented Oct 10, 2017

khrisrichardson commented Oct 10, 2017

linki Oct 11, 2017

Choose a reason for hiding this comment

linki Oct 11, 2017

Choose a reason for hiding this comment

linki Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

linki Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

khrisrichardson commented Oct 19, 2017

szuecs Oct 28, 2017

Choose a reason for hiding this comment

szuecs Oct 28, 2017

Choose a reason for hiding this comment

szuecs Oct 28, 2017

Choose a reason for hiding this comment

szuecs commented Oct 28, 2017 • edited Loading

linki commented Oct 29, 2017

hjacobs commented Nov 8, 2017

szuecs commented Nov 9, 2017

szuecs commented Nov 9, 2017

linki Oct 11, 2017 •

edited

Loading

linki Oct 11, 2017 •

edited

Loading

szuecs commented Oct 28, 2017 •

edited

Loading