-
Notifications
You must be signed in to change notification settings - Fork 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
Added ability to override frontend priority for k8s ingress router #1874
Added ability to override frontend priority for k8s ingress router #1874
Conversation
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.
Thanks for the PR -- exposing the priority through an annotation should prove helpful to a bunch of people. 👏
Left a few comments. One general remark: We still need to extend the unit test suite.
provider/kubernetes/kubernetes.go
Outdated
priority := len(pa.Path) | ||
|
||
priorityString, ok := i.Annotations[types.LabelFrontendPriority] | ||
if ok { |
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.
You can merge this line with the previous one, thereby reducing the scope of priorityString
to the necessary minimum.
provider/kubernetes/kubernetes.go
Outdated
@@ -186,6 +186,17 @@ func (p *Provider) loadIngresses(k8sClient Client) (*types.Configuration, error) | |||
witelistSourceRangeAnnotation := i.Annotations[annotationKubernetesWhitelistSourceRange] | |||
whitelistSourceRange := provider.SplitAndTrimString(witelistSourceRangeAnnotation) | |||
|
|||
priority := len(pa.Path) | |||
|
|||
priorityString, ok := i.Annotations[types.LabelFrontendPriority] |
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.
I'd suggest to rename priorityString
to priorityRaw
to better convey its semantics.
provider/kubernetes/kubernetes.go
Outdated
if ok { | ||
priorityParsed, err := strconv.Atoi(priorityString) | ||
|
||
if err == 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.
We should not ignore errors.
When we encounter an error, we can either
- drop the associated frontend (by deleting it from the frontends map) or
- fall back to the default priority of 1.
We kind of do a mix in Traefik, so we cannot just point at any single pattern. Personally, I'm much more in favor of dropping the frontend as this makes the error explicit and way easier to identify. @dtomcej @errm thoughts?
In either case, the error should be logged.
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.
Actually in current logic it will fall back to the default priority of len(Path), as if no priority is set.
Personally I think that dropping the frontend is ok when it has wrong Auth (because we obviously don't wanna show something secret to all users), but here probably routing with default priority is fine.
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.
Yeah that does sound reasonable. 👍
provider/kubernetes/kubernetes.go
Outdated
priorityParsed, err := strconv.Atoi(priorityString) | ||
|
||
if err == nil { | ||
priority = priorityParsed; |
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.
No trailing semicolon needed here.
provider/kubernetes/kubernetes.go
Outdated
if err == nil { | ||
priority = priorityParsed; | ||
} | ||
} |
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.
Can you put the entire priority processing logic into a dedicated function?
|
@timoreimann , looks like I changed everything you asked, could you please check again? |
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.
Looking pretty good already. Some minor things left.
provider/kubernetes/kubernetes.go
Outdated
if err == nil { | ||
priority = priorityParsed | ||
} else { | ||
log.Errorf("Error in ingress: failed to parse '%q' value '%q'.", types.LabelFrontendPriority, priorityRaw) |
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.
We don't need the extra '
quotes if we already use %q
, do we?
actualJSON, _ := json.Marshal(actual) | ||
expectedJSON, _ := json.Marshal(expected) | ||
|
||
if !reflect.DeepEqual(actual, expected) { |
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.
Please use assert.Equal
(like here). Comparing with DeepEqual
and showing JSON can hide subtle differences, like nil
slices vs. empty slices.
I know we use it everywhere in our Kubernetes tests, which is something we need to get rid of. :-)
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.
LGTM, thanks. 👍
Sorry, I just realized something: Should the annotation actually go on the Ingress or rather the Service object? We do keep some annotations on Services. I know @dtomcej previously explained why, but I forgot... |
The annotation should go on the ingress in this case, because it affects how the controller handles the routing to the service. The ingress does not care how the services are configured or structured :) |
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.
LGTM
docs/user-guide/kubernetes.md
Outdated
## Specifying priority for routing | ||
|
||
Sometimes you need to specify priority for ingress route, especially when handling wildcard routes. | ||
This can be done by adding annotation ```traefik.frontend.priority```, i.e.: |
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.
could you replace ``` only one ` ?
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.
sure, replaced
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.
Great Job 👍
LGTM
Updated documentation Pull Request review fixes (renaming & code cleanup)
… JSON to assert.Equals in test.
Description
Added ability to specify
traefik.frontend.priority
priority from kubernetes ingress annotation.This is useful when you have following case:
Ingress A:
Host: *.mydomain
Ingress B:
Host: specific.mydomain