-
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 filtering ingresses on ingressClassName
as well as deprecated kubernetes.io/ingress.class annotation
#2054
Support filtering ingresses on ingressClassName
as well as deprecated kubernetes.io/ingress.class annotation
#2054
Conversation
~Any thoughts on this @Raffo? ~ |
/kind feature |
Any updates here? Would really like to see this feature get through |
@dsalisbury can you fix the merge conflicts, maybe then someone will review? |
...and update the help text to specify use more clearly
Update on things:
|
we don't want to get incompatible restrictions by letting someone set "--ingress-class" and --annotation-filter="kubernetes.io/ingress.class" at the same time
4b1e521
to
807e213
Compare
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:
All scenarios worked correctly as far as I could tell |
for _, nameFilter := range sc.ingressClassNames { | ||
if ingress.Spec.IngressClassName != nil && nameFilter == *ingress.Spec.IngressClassName { | ||
matched = true | ||
} else if matchLabelSelector(selector, ingress.Annotations) { |
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.
this block does the filtering with fallback
Maybe to add more context. I joined a couple of months ago to help shrinking the backlog. @ryan-dyer-sp TBH I think you and other commenters show not a very nice community communication style. Naturally I would give more priority to people who are more friendly, because I don't want to spend time with unfriendly people. You can just scroll the comments and see for example (others are not better and I don't want to pick one particular user here):
Examples like this make me re-thinking if I want to spend time maintaining this project, because it's not respectful IMO. Please also understand that as a maintainer I am free to review any PR that I feel is interesting for the project. Maybe I looked at it already, but think it's wrong, but did not care because I don't use the feature and think the feature should be rather abandoned (just hypothetical not really true, because I am aware that people run multiple instances of external-dns). |
@szuecs thanks for your work inside the OSS world! And someone used this project, tested it and has found something to improve and even submitted a PR, all for $0 and without even being a maintainer. Few people spend time on this problem and just felt left alone. If there are any open tasks to get this feature to a production stage, I am also willing to implement. |
@KarstenSiemer thanks and I fully agree let's make it. Yesterday I added my questions, it's not that I have string opinions on them, but maybe it would be great to have some mixed config possible for migration purposes. |
To do this with ExternalDNS you can use the `--ingress-class` to specifically tie an instance of ExternalDNS to | ||
an instance of a ingress controller. Let's assume you have two ingress controllers `nginx-internal` and `nginx-external` | ||
then you can start two ExternalDNS providers one with `--ingress-class=nginx-internal` and one with `--ingress-class=nginx-external`. |
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.
To do this with ExternalDNS you can use the `--ingress-class` to specifically tie an instance of ExternalDNS to | |
an instance of a ingress controller. Let's assume you have two ingress controllers `nginx-internal` and `nginx-external` | |
then you can start two ExternalDNS providers one with `--ingress-class=nginx-internal` and one with `--ingress-class=nginx-external`. | |
To do this with ExternalDNS you can use the `--ingress-class` flag to specifically tie an instance of ExternalDNS to | |
an instance of a ingress controller. Let's assume you have two ingress controllers, `nginx-internal` and `nginx-external`. | |
You can then start two ExternalDNS providers, one with `--ingress-class=nginx-internal` and one with `--ingress-class=nginx-external`. |
|
||
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 `--ingress-class` argument will check both the `ingressClassName` field as well as the deprecated `kubernetes.io/ingress.class` annotation. |
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 `--ingress-class` argument will check both the `ingressClassName` field as well as the deprecated `kubernetes.io/ingress.class` annotation. | |
The `--ingress-class` flag will check both the `ingressClassName` field and the deprecated `kubernetes.io/ingress.class` annotation. |
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.
“argument” is preferable. Golang calls command line arguments “flags” whether the type is a boolean or not, but the rest of the world of IT typically uses “flag” just for booleans (maybe tristates).
|
||
**Note:** Filtering based on annotation means that the external-dns controller will receive all resources of that kind and then filter on the client-side. | ||
Note: the `--ingress-class` argument cannot be used at the same time as a `kubernetes.io/ingress.class` annotation filter; if you do this an error will be raised. |
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.
Note: the `--ingress-class` argument cannot be used at the same time as a `kubernetes.io/ingress.class` annotation filter; if you do this an error will be raised. | |
Note: the `--ingress-class` flag cannot be used at the same time as a `kubernetes.io/ingress.class` annotation filter; if you do this an error will be raised. |
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.
// class | ||
func (sc *ingressSource) filterByIngressClass(ingresses []*networkv1.Ingress) ([]*networkv1.Ingress, error) { | ||
// if no class filter is specified then there's nothing to do | ||
if sc.ingressClassNames == nil { |
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 sc.ingressClassNames == nil { | |
if len(sc.ingressClassNames) == 0 { |
|
||
requirements, _ := selector.Requirements() | ||
for _, requirement := range requirements { | ||
if requirement.Key() == "kubernetes.io/ingress.class" { |
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.
filterByIngressClass()
does the fallback logic when considering whether an Ingress
matches the requirement set by the --ingress-class
flag. For that reason, passing both an --ingress-class
flag and a kubernetes.io/ingress.class
annotation filter doesn't make sense: it would exclude those ingresses that don't match the annotation filter. Thus, this check is a reasonable validation against the admin passing nonsensical flag combinations.
annotations: map[string]string{ | ||
"kubernetes.io/ingress.class": "dmz", | ||
}, | ||
}, |
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.
Suggest test case with annotation dmz and ingressClassName internal. Should not be in expected.
Possibly also annotation internal ingressClassName dmz. Should be in expected.
Suggest test case with neither annotation nor ingressClassName. Should not be in expected.
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dsalisbury The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
Does this need to change too? It mentions kubernetes.io/ingress.class
Additionally, you may leverage [cert-manager](https://github.com/jetstack/cert-manager) to automatically issue SSL certificates from [Let's Encrypt](https://letsencrypt.org/). To do that, request a certificate in public service definition: ```yaml apiVersion: networking.k8s.io/v1 kind: Ingress metadata: annotations: certmanager.k8s.io/acme-challenge-type: "dns01" certmanager.k8s.io/acme-dns01-provider: "route53" certmanager.k8s.io/cluster-issuer: "letsencrypt-production" kubernetes.io/ingress.class: "external-ingress" kubernetes.io/tls-acme: "true"
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.
Does the FAQ question I have a Service/Ingress but it's ignored by ExternalDNS. Why?“ need a new answer?
return ingresses, nil | ||
} | ||
|
||
classNameReq, err := labels.NewRequirement("kubernetes.io/ingress.class", selection.In, sc.ingressClassNames) |
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.
Labels? Is that right? The deprecated kubernetes.io/ingress.class
annotation is an annotation, not a label.
However, beware when using annotation filters with multiple sources, e.g. `--source=service --source=ingress`, since `--annotation-filter` will filter every given source objects. | ||
If you need to use annotation filters against a specific source you have to run a separated external dns service containing only the wanted `--source` and `--annotation-filter`. | ||
|
||
**Note:** Filtering based on annotation or ingress class name means that the external-dns controller will receive all resources of that kind and then filter on the client-side. | ||
In larger clusters with many resources which change frequently this can cause performance issues. If only some resources need to be managed by an instance | ||
of external-dns then label filtering can be used instead of annotation filtering. This means that only those resources which match the selector specified |
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.
of external-dns then label filtering can be used instead of annotation filtering. This means that only those resources which match the selector specified | |
of external-dns then label filtering can be used instead of ingress class filtering (or legacy annotation filtering). This means that only those resources which match the selector specified |
?
whats preventing this PR to be merged ? |
Not sure, this PR is notoriously ignored by the maintainers. I made a workaround that works and fits into my design well so this feature is not needed so critically but would still be useful to address a few edge cases. |
how did you worked this around without annotations ? |
It was with annotations, but not the |
Yeah i did the same, but is annoying when you already have that meta in the ingressClassName |
There are more than a couple of suggestions to change by 2 different members and as a maintainer I wait such that these are addressed. |
I understand and I feel bad for making it sound like this. It is just how I feel having seen someone put the effort into making this PR and I'd have thought the maintainers are here to drive progression and not just approve/deny changes. A lot of conversation is about ethics and little is done to progress with merging this. I don't like political discussion so I generally don't comment when unnecessary. It sort of appears to me that the maintainers get defensive as soon as people start asking for attention on this PR. I see new releases come out without this feature and it's been a long time since this was opened - it personally doesn't affect me as I only run this in a home lab. From a perspective of other users, it could be more significant. However, I have a right to express my opinion, and my opinion is that bugs/issues should be addressed before new features and deprecation of the ingress annotation is quite a fundamental change, a breaking change for some people who might be heavily dependent on external DNS and filtering by ingress classes. Kubernetes will not allow you to have both ingressclassname and the annotation in place and therefore the lack of support here could be potentially a blocker in migration to higher versions forcing some to stay on unsupported versions. We're now on 1.27 almost and this was deprecated on 1.18 so almost 10 releases ago... If I was the person who made a PR and couldn't get it merged for 2 years despite it being realtively small in size I'd just give up and it might be why the OP isn't responding. |
As I wrote the pr was reviewed by two kubernetes contributors and there was no change and you did just bike shedding @mateuszdrab which is not ok. |
I understand that the PR has been reviewed but let me highlight one fact that it appears that @dsalisbury has not made any contributions on GitHub for the past 6 months. Can we make suggestions to his code and you maintainers can merge those in? (did he allow maintainer edits?) |
@mateuszdrab take ownership if you want to have the pr merged and commit the changes then we will review the pr as people show here interest. |
Yep hi, that's me. Apologies for lack of engagement, but I don't have time or capacity to see this through any more. Anyone who wants is welcome to take the branch and run with it themselves or just throw it away and start over. |
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Thanks to @dsalisbury and to all the reviewers for their work 🙏 I've commit the changes asked in #3589 I hope we can finish this 😉 ! |
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 insource/ingress.go
Fixes #1975 #1792
Checklist