-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Ignore Ingresses with empty Endpoint subsets. #1604
Ignore Ingresses with empty Endpoint subsets. #1604
Conversation
If there is an ingress, with no endpoints, shouldn't we create an empty backend to serve a 503? |
In that case, we create a backend with no servers configured. Do you think that won't be enough? Should we make it "completely empty"? |
3260632
to
97bd2a6
Compare
Hmm, I'm having a hard time getting this to work as desired: I either get 500s or 404s, but never 503s. Marking as WIP for the time being. |
@errm how did you verify the behavior you described in #1077 (comment), especially this part:
I have removed the Pods and even the Endpoints, but I never manage to get a 502; only ever a 500. |
I have no idea, but I would expect that if I did get that to occur, it would have been as a result of whatever was going on with network timeouts... or perhaps I just misremembered and really I was just seeing 500 errors rather than 503... |
provider/kubernetes/kubernetes.go
Outdated
continue | ||
} | ||
|
||
for _, subset := range endpoints.Subsets { |
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 won't execute when endpoints.Subsets
is empty, thats why there is no else?
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.
As discussed on Slack, the else
got replaced by the continue
effectively, though the else
body is still there.
97bd2a6
to
0071542
Compare
It took me a truckload of debug log messages sprinkled around the k8s provider, but I finally figured out why I would only be able to see 404s (as opposed to 50x) on missing/empty Endpoints: The k8s template relies on at least one configuration parameter to get rendered successfully in order to include the (I also changed the log level to WARNING (because the previous ERROR and DEBUG seemed too strict and loose, respectively), and used Now, the one thing left is the concrete 50x error code: Right now I'm only able to get a 500 when the backend is empty. This seems to be a matter out-of-scope of the k8s provider, however, and my tests with a file provider support my assumption. If we really want to make Treafik return 503s, I believe it should happen in a different PR. @dtomcej WDYT on the current state of the PR and the 500/503 question? |
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
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.
@timoreimann If it returns a 500 instead of a 503, thats fine IMO for this PR.
Creating the empty backend provides the correct behavior, with a 5XX code returned instead of a 4XX.
We can address the error code in another PR.
LGTM
We previously fell back to using ClusterIPs. However, the approach can lead to all kinds of problems since Ingresses rely on being able to talk to Endpoints directly. For instance, it can break stickiness and retries.
0071542
to
54c9a26
Compare
We previously fell back to using ClusterIPs. However, the approach can lead to all kinds of problems since Ingresses rely on being able to talk to Endpoints directly. For instance, it can break stickiness and retries.
I believe the issue should still go into 1.3 since the implications can be quite severe and users won't have much of a chance to spot the misconfigurations easily.
Fixes #1462.