(V2) Support filtering ingresses on ingressClassName as well as deprecated kubernetes.io/ingress.class annotation

[alefray]

⚠️ Disclaimer

This is an attempt to finalize the work started by @dsalisbury on #2054
All credits goes to him.

I've just implemented unresolved comments by @sftim @johngmyers @szuecs and other contributors.

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

• Unit tests updated
• End user documentation updated

Open questions

I'm not sure about the behavior of --ingress-class when both ingressClassName field and kubernetes.io/ingress.class annotation exist:

• ingressClassName takes precedence over kubernetes.io/ingress.class ? (or the opposite)
• --ingress-class must match ingressClassName if defined and kubernetes.io/ingress.class annotation is ignored ?
• --ingress-class can match any of ingressClassName or kubernetes.io/ingress.class annotation ?

Currently, the last case is implemented. Thus considering the comment https://github.com/kubernetes-sigs/external-dns/pull/2054/files#r1064085316, test cases are:

• annotation dmz and ingressClassName internal. Should not be in expected. Should be in expected.
• annotation internal ingressClassName dmz. Should be in expected.
• neither annotation nor ingressClassName. Should not be in expected.

Other questions, there is many mention of kubernetes.io/ingress.class in the documentation, should they all be removed in favor of ingressClassName ?

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: dsalisbury / name: David Salisbury (c4898b7, 115e250, c823eba, 0b6c67f, da71b3f, 8da6f99, ac0c4be, 71a672f, 901effb, 807e213, 8ec342a, a24217a, 0aa3d55, 6c4a450, 31bc5bb, b8cbd4b, 097df5c, 1ceec80, 99c10f5, 15f27aa, f76382a)
• ✅ login: alefray / name: Arnaud Lefray (4a2b9c6, 04d7ed8, 56773c0, e9fd860)

[k8s-ci-robot]

Welcome @alefray!

It looks like this is your first PR to kubernetes-sigs/external-dns 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/external-dns has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[johngmyers]

/lgtm
/assign njuettner

[szuecs]

@alefray thanks for your work!
To respond to your questions as best as I know:

Open questions

I'm not sure about the behavior of --ingress-class when both ingressClassName field and kubernetes.io/ingress.class annotation exist:

1.    ingressClassName takes precedence over kubernetes.io/ingress.class ? (or the opposite)
2.   --ingress-class must match ingressClassName if defined and kubernetes.io/ingress.class annotation is ignored ?
3.   --ingress-class can match any of ingressClassName or kubernetes.io/ingress.class annotation ?

1. IngressClass should take precedence and fallback to the old annotation. For ingress an ingressClassName only specifies which https://kubernetes.io/docs/concepts/services-networking/ingress/#ingress-class ingressclass to query from kube-apiserver. It's not yet another string match. For us it likely means we have to do the string match as you wrote. (@robscott just FYI ingressclass is string match by name for external-dns, likely we can't do better, but maybe you have an idea).
2. No you want also to check the annotation, otherwise a migration from annotations to ingressclass won't work or I have no idea how this should be reasonably work with one controller.
3. Yes or we have to have 2 flags, one ingress-class and one for the annotation. I don't mind, but needs to be clear in the docs.

[szuecs]

/ok-to-test

[robscott]

@szuecs I think string matching on IngressClassName is probably fine here. As far as when both annotation and field are set on Ingress resource, I think it depends on the history of this config in external-dns.

1. If this is a new feature/flag for external-dns, I'd favor ingressClassName over the annotation.
2. If this is a preexisting feature/flag that only supported the annotation previously, I think that would have to take precedence.

Assuming this is 1 here, but let me know if I missed something.

[johngmyers]

ingressClassName should take precedence per https://kubernetes.io/docs/concepts/services-networking/ingress/#deprecated-annotation

[alefray]

Thanks for you remarks. All comments have been taken into account. 👍
The implementation is now: ingressClassName takes precedence and fallback to the annotation, if ingressClassName is nil or empty.

[szuecs]

@robscott thanks for your input!
@alefray thanks for your work!
@johngmyers thanks for your review!

[szuecs]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alefray, szuecs

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [szuecs]

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

[szuecs]

/lgtm