fix(Traefik Proxy): missing IgnoreIngressRulesSpec check #5114
Conversation
When setting the `ingress-hostname-source` annotation to `annotation-only`, ExternalDNS still inspects the IngressRoute spec for hostnames. This commit adds a check for the `IgnoreIngressRulesSpec` configuration value and, if set to true, will skip inspecting the IngressRoute spec.
The committers listed above are authorized under a signed CLA. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
k8s-ci-robot
added
the
cncf-cla: no
|
Welcome @linux2647! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @linux2647. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
k8s-ci-robot
added
size/L
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
I have some concerns about this pull request. Could you please explain how you identified the underlying issue? Also, could you provide the Kubernetes manifests we need to reproduce and validate the issue? |
|
@linux2647 Would you please complete documentation on Traefik proxy source with this possibility ? |
Yeah. I have a few apps that are independently deployed, but share a common hostname. I want to have the shared hostname to be managed by a Traefik IngressRoute independent of any of the apps hosted on it. That way, any ExternalDNS configuration on an app's IngressRoute doesn't clobber the shared hostname. Something like: apiVersion: traefik.io/v1alpha1
kind: IngressRoute
metadata:
name: test-app
annotations:
external-dns.alpha.kubernetes.io/hostname: test-app.prod.example.com
external-dns.alpha.kubernetes.io/ingress-hostname-source: annotation-only
spec:
entryPoints:
- websecure
routes:
- match: (Host(`shared-app.prod.example.com`) && PathPrefix(`/test-app`)) || Host(`test-app.prod.example.com`)
kind: Rule
services:
- kind: Service
name: hello-kubernetes-svcWhat I want is for |
@mloiseleur I can, but this is covered by ExternalDNS's documentation on annotations, so it doesn't feel like something that's Traefik specific. To me, it feels like a bug that needs fixing. |
|
ExternalDNS's documentation on annotations says:
It does not speak about |
|
/retitle fix(Traefik Proxy): missing IgnoreIngressRulesSpec check |
k8s-ci-robot
changed the title
| for _, route := range ingressRoute.Spec.Routes { | ||
| match := route.Match | ||
|
|
||
| for _, hostEntry := range traefikHostExtractor.FindAllString(match, -1) { | ||
| for _, host := range traefikValueProcessor.FindAllString(hostEntry, -1) { | ||
| host = strings.TrimPrefix(host, "`") | ||
| host = strings.TrimSuffix(host, "`") | ||
| for _, hostEntry := range traefikHostExtractor.FindAllString(match, -1) { | ||
| for _, host := range traefikValueProcessor.FindAllString(hostEntry, -1) { | ||
| host = strings.TrimPrefix(host, "`") | ||
| host = strings.TrimSuffix(host, "`") | ||
|
|
||
| // Checking for host = * is required, as Host(`*`) can be set | ||
| if host != "*" && host != "" { | ||
| endpoints = append(endpoints, endpointsForHostname(host, targets, ttl, providerSpecific, setIdentifier, resource)...) | ||
| // Checking for host = * is required, as Host(`*`) can be set | ||
| if host != "*" && host != "" { | ||
| endpoints = append(endpoints, endpointsForHostname(host, targets, ttl, providerSpecific, setIdentifier, resource)...) | ||
| } |
There was a problem hiding this comment.
this looks edentical to lines 723-735. Could we have a private method for that as well?
Most likely not part of this change, but I leave it to you to decide, if what I'll open a pull request.
There are triple nested looping
The time complexity of the selected code is O(n * m * k), where:
- n is the number of routes in ingressRoute.Spec.Routes.
- m is the number of matches found by traefikHostExtractor.FindAllString.
- k is the number of hosts found by traefikValueProcessor.FindAllString.
This is because the code iterates over each route, then for each route, it iterates over each match found, and for each match, it iterates over each host found.
it would be desirable to have something like that
var hosts []string
for _, route := range ingressRoute.Spec.Routes {
match := route.Match
h := traefikHostExtractor.FindAllString(match, -1)
hosts = append(hosts, h...)
}
var processedHosts []string
for _, entry := range hosts {
pHosts := traefikValueProcessor.FindAllString(entry, -1)
processedHosts = append(processedHosts, pHosts...)
}
for _, host := range processedHosts {
host = strings.Trim(host, "`")
if host != "*" && host != "" {
endpoints = append(endpoints, endpointsForHostname(host, targets, ttl, providerSpecific, setIdentifier, resource)...)
}
}aka time complexity of O(n + m + p) same as O(n) vs O(n * m * p)
Description
When setting the
ingress-hostname-sourceannotation toannotation-only, ExternalDNS still inspects the IngressRoute spec for hostnames. This change adds a check for theIgnoreIngressRulesSpecconfiguration value and, if set to true, will skip inspecting the IngressRoute spec.Checklist