Support filtering ingresses on `ingressClassName` as well as deprecated kubernetes.io/ingress.class annotation

[dsalisbury]

Description

Adds the ability to filter ingresses based on their IngressClassName attribute. There's an --ingress-class-filter argument that's now exposed through the CLI and some additional filtering in source/ingress.go

Fixes #1975 #1792

Checklist

• [x] Unit tests updated
• [x] End user documentation updated

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dsalisbury To complete the pull request process, please assign raffo after the PR has been reviewed. You can assign the PR to them by writing /assign @raffo in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

[dsalisbury]

~So one thing I have outstanding is detecting the current default IngressClass (as per https://kubernetes.io/docs/concepts/services-networking/ingress/#default-ingress-class)~

~Here's my thinking:~

1. ~in the setup of source/ingress.go, load the IngressClasses, find any with the special annotation, and store this away~
2. ~the default class is used for any ingresses which are found and don't have a class explicitly~
3. ~if more than 1 default ingress class is found then assume no default ingress?~

   Caution: If you have more than one IngressClass marked as the default for your cluster, the admission controller prevents creating new Ingress objects that don't have an ingressClassName specified. You can resolve this by ensuring that at most 1 IngressClass is marked as default in your cluster.

4. ~We probably need to watch for changes to the ingress classes and reconcile if/when the default changes~
5. ~In an RBAC cluster, I think this will require some extra permissions so doc updates and things will be needed~

~Any thoughts on this @Raffo? ~

[seanmalloy]

/kind feature

[FalconerTC]

Any updates here? Would really like to see this feature get through

[wagnst]

@dsalisbury can you fix the merge conflicts, maybe then someone will review?

[dsalisbury]

Update on things:

1. detecting default ingress class isn't necessary; if there's a default and a new ingress is created with no class name set, the admission controller sets it explicitly.
2. code is updated and rebased on latest master

[dsalisbury]

I think this is ready for review!

I've done some manual verification locally with a kind cluster connected to pdns in the following scenarios:

• two IngressClass defined (with one as default) and one Ingress of each class; checked with zero, one, and two --ingress-class args
• Two IngressClass and neither is default, with an Ingress of each class
• use of --ingress-class to match deprecated annotation-based ingress class specification
• use of mixed ingress.class annotation and ingressClassName

All scenarios worked correctly as far as I could tell

[sergeyshevch]

I also interested on this feature. Can someone review it?

[sergeyshevch]

@dsalisbury Can you rebase this PR? or if you have no time for it i can do it and create separate PR

[dsalisbury]

@dsalisbury Can you rebase this PR? or if you have no time for it i can do it and create separate PR

Yep, all done!

[dsalisbury]

@Raffo @njuettner would you be able to take a look at this?

[sorenmat]

This would be really nice to get in, since the annotation is now deprecated. And an upgrade is impossible since external-dns deletes the resource when you are only using the ingressClassName and not the annotation

[dsalisbury]

Hey @njuettner @Raffo

Are you able to take a look at this? I'd love to get this merged so I can get back to running on proper releases rather than my locally-built docker image which keeps on drifting from master (and recently broke with an upgrade to k8s 1.22 :( ).

I've recently merged master back in to get things all synced

[onedr0p]

World be great if a maintainer could see this PR thru to being merged. Where they at?

[FalconerTC]

This is a pretty important feature for this project to have support for 🙏

[dsalisbury]

@lambdanis

There is also a tutorial for setting up both public and private Route53 zones using nginx-ingress-controller. Would be great to update it too to use --ingress-class filter. But I guess that should be done after your change is released (?).

I've updated that example too now!

[valentin-stefan-popa]

Can we merge this PR?

[midu-git]

I guess there is a corner case not handled by this PR adequately, as you can define default ingress classes.

Assume, you have two ingress classes, internal and external, say. Moreover, assume internal has the annotation ingressclass.kubernetes.io/is-default-class: true. Thus, it is not required to specify ingressClassName at the ingress resource at all, if the corresponding entry should go into the associated Private DNS zone. Now, how do you want to configure external-dnses as in the example public-private-route53 such that the internal external-dns will catch up the ingress resource without ingressClassName specified to create an entry in the private DNS zone?

Moreover, assume you have two services of type LoadBalancer. One is for internal and the other for internet-facing traffic. As documented

Beware when using multiple sources, e.g. --source=service --source=ingress, --annotation-filter
will filter every given source objects. If you need to filter only one specific source you have to run
a separated external dns service containing only the wanted --source and --annotation-filter.

the annotation filter is working for Ingress as well as for Service resources. ingressClassName, however, is only valid for Ingress resources.

[dsalisbury]

I guess there is a corner case not handled by this PR adequately, as you can define default ingress classes.

Assume, you have two ingress classes, internal and external, say. Moreover, assume internal has the annotation ingressclass.kubernetes.io/is-default-class: true. Thus, it is not required to specify ingressClassName at the ingress resource at all, if the corresponding entry should go into the associated Private DNS zone.

The DefaultIngressClass admission controller manages this for us automatically. When a new ingress is created, it'll automatically set the spec.IngressClassName attribute to the default ingress class at that point in time. I've verified this in my own testing.

https://github.com/kubernetes/kubernetes/blob/1a845ccd076bbf1b03420fe694c85a5cd3bd6bed/plugin/pkg/admission/network/defaultingressclass/admission.go

If you change your default ingress class later on, it won't be applied retroactively to existing Ingresses

Moreover, assume you have two services of type LoadBalancer. One is for internal and the other for internet-facing traffic. As documented ... the annotation filter is working for Ingress as well as for Service resources. ingressClassName, however, is only valid for Ingress resources.

Are you saying this should be better documented in some way (and if so how?) or that there's some feature gap here?

[midu-git]

Hi @dsalisbury.

Thanks for answering.

The DefaultIngressClass admission controller manages this for us automatically. When a new ingress is created, it'll automatically set the spec.IngressClassName attribute to the default ingress class at that point in time. I've verified this in my own testing.

https://github.com/kubernetes/kubernetes/blob/1a845ccd076bbf1b03420fe694c85a5cd3bd6bed/plugin/pkg/admission/network/defaultingressclass/admission.go

If you change your default ingress class later on, it won't be applied retroactively to existing Ingresses

Exactly, this will be handled exactly once and the admission controller won't update it. In this particular case, it's handled by the admission controller (thus a dependency is introduced on the behaviour of an external tool). My argument is, that using annotations this is perfectly possible to configure in the general case, since there exists a not in operation for filters in Kubernetes. The proposed PR can't handle this case and thus introduces a breaking change that can't be mitigated by only configuring external-dns. I'd go for a solution that enables the exact same configuration.

Why can't IngressClassName be filtered exactly the same as an annotation? This would imply to write custom code for the definition of the operators (in, not in, = ...), as this can't be just passed to the Kubernetes SDK (I'd be happy to be wrong).

Nevertheless, if you decide to merge this PR, you should explicitly state this in the documentation.

Are you saying this should be better documented in some way (and if so how?) or that there's some feature gap here?

I had a quick glance into the code, and it seems that both, annotation-filter and ingress-class, are still possible to define and the logic for Service is not affected. If this is true, then, without taking the changed behaviour for Ingress resources into account, it's possible to configure external-dns as before for Services.

If I'm wrong and this proposed PR forbids to have both specified, then you are forced to deploy even more instances of external-dnses, since for a service it's not possible to state ingressClassName as a filter, as it's not part of a Services' specification.

[mateuszdrab]

Hi @midu-git

Why can't IngressClassName be filtered exactly the same as an annotation? This would imply to write custom code for the definition of the operators (in, not in, = ...), as this can't be just passed to the Kubernetes SDK (I'd be happy to be wrong).

It would be nice, but I think this is unnecessary. With annotations/labels you're filtering keys and values, with ingressClassName you're only dealing with the value.

I think this will pick up more heat and more issues will be raised as latest K8s versions are adopted by those who used the old label filter.

[mateuszdrab]

@Raffo @njuettner @lambdanis Any chance to get this one merged?

[midu-git]

Do you expect having a decision on this PR somewhen soon? Either way, it'd be nice to have a solution be merged.

[midu-git]

Can someone honestly answer why this is taking that long? I mean, it seems that all discussions have been done and are resolved. There are no open comments. C'mon, this already lasts 15 months.

[zerodayyy]

What's holding this PR from being merged? The annotation has been deprecated over 2 years ago.

[mateuszdrab]

What's holding this PR from being merged? The annotation has been deprecated over 2 years ago.

I call this FOSS politics 😅

[ivanaguilario]

I think this should be merged asap, it's a basic feature and the PR has been ready for months at this point

[avestuk]

Would be super to see this get merged given that ingressClassName is going to be the defacto way of specifying which ingress class to use going forward.

[midu-git]

@Hen-Itzhaki-TwistBioscience Catching up on your review comment

Add backward compatible.

(https://github.com/kubernetes-sigs/external-dns/pull/2054#pullrequestreview-993384431)

Does this mean there should be something done before you'll approve it? Can you give more context?

Can someone of the responsible guys comment what is required to get this merged? Any sign of life? @lambdanis @njuettner @Raffo

[ChristianSpiess]

Please? Nobody here?

[flickers]

This feature would be really useful for my organization. Can the reviewers give this the final review and approve this PR? @lambdanis @Hen-Itzhaki-TwistBioscience @njuettner @Raffo