-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Changes CustomHTTPErrors annotation to use custom default backend #3730
Changes CustomHTTPErrors annotation to use custom default backend #3730
Conversation
@@ -591,11 +591,13 @@ func (n *NGINXController) getBackendServers(ingresses []*ingress.Ingress) ([]*in | |||
location.Path, server.Hostname, location.DefaultBackend.Namespace, location.DefaultBackend.Name) | |||
|
|||
nb := upstream.DeepCopy() | |||
name := fmt.Sprintf("custom-default-backend-%v", upstream.Name) | |||
name := fmt.Sprintf("custom-default-backend-%v", location.DefaultBackend.GetName()) |
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 change means that the backend name of custom-error locations is custom-default-backend-<service_name>
where service_name
is the name of the default-backend service. It used to be that the service_name
would be the name of the upstream for which this default-backend is substituted. Eg. where it used to be custom-default-backend-my-app
, it is now custom-default-backend-app-errorhandler
.
This is necessary since multiple locations within a server block can share the same 'default-backend' - so now, the name of the error-handler-location clearly signifies which service it will route to.
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.
Is this a necessary change? I wonder what was the rationale to design that way initially? Could it be maybe because it would explicitly mention what upstream it is replacing due to the fact that upstream has no endpoints?
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.
well it's necessary if multiple ingress objects (ie locations) for a single upstream can have different error blocks. if I don't make this change, all locations that point to a given upstream have to share the same default-backend.
I assume it was that way initially since the default-backend was only for when the upstream had no endpoints, thus this upstream was replacing the regular upstream entirely (whereas now, if endpoints > 0, the default backend is just another upstream).
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.
Given two ingresses one with location /one pointing to upstream X and another with location /two pointing to upstream X as well. And upstream X has no endpoints. Then without this change we can not use different default-backend
annotation per location, right?
Given that this change is not necessarily about custom errors, do you think we should introduce it in a different PR with its own tests and documentation adjustments?
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 case I was more targeting is two ingresses /one and /two, same upstream, both with custom error annotations but different default backends - but yeah, that case you mentioned is also 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.
It might be tricky to pull it out into another PR - I'll look and see how tightly I integrated the rest. I don't think it will have any user-facing change, but we can definitely add tests and update the docs (https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/annotations/#default-backend is out-of-date anyway)
return struct { | ||
ProxySetHeaders map[string]string |
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 far as I could see, the ProxySetHeaders map was unused in the template. If this is a bug, we can re-add it and fix.
/assign bowei |
@jasongwartz please don't assign PRs manually |
@aledbf sorry, my bad. can we remove that line from the k8s-ci-robot message somehow? |
No, because the bot reads the owners files and tries to suggest a name 😞 |
@jasongwartz please squash the commits |
8bb1700
to
22a89cf
Compare
@aledbf cool, have done so |
/approve |
@ElvinEfendi please review (and lgtm) |
@@ -580,7 +580,7 @@ func (n *NGINXController) getBackendServers(ingresses []*ingress.Ingress) ([]*in | |||
for _, server := range servers { | |||
for _, location := range server.Locations { | |||
if upstream.Name == location.Backend { | |||
if len(upstream.Endpoints) == 0 { | |||
if len(upstream.Endpoints) == 0 || len(location.CustomHTTPErrors) != 0 { | |||
klog.V(3).Infof("Upstream %q has no active Endpoint", upstream.Name) |
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 think this need to be replaced - it is not going to be true when len(upstream.Endpoints) != 0 and len(location.CustomHTTPErrors) != 0
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.
it should be true in that case, right? maybe I'm not following, but if you mean false || true
it should eval to true
@@ -591,11 +591,13 @@ func (n *NGINXController) getBackendServers(ingresses []*ingress.Ingress) ([]*in | |||
location.Path, server.Hostname, location.DefaultBackend.Namespace, location.DefaultBackend.Name) |
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.
similar to https://github.com/kubernetes/ingress-nginx/pull/3730/files#r256678420, this log message can be lie, maybe move it to https://github.com/kubernetes/ingress-nginx/pull/3730/files#diff-86ac9ff9d75a0c5005c116e766a6127dR598?
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.
done as well, good catch - I had missed that entirely
@@ -580,7 +580,7 @@ func (n *NGINXController) getBackendServers(ingresses []*ingress.Ingress) ([]*in | |||
for _, server := range servers { | |||
for _, location := range server.Locations { | |||
if upstream.Name == location.Backend { | |||
if len(upstream.Endpoints) == 0 { | |||
if len(upstream.Endpoints) == 0 || len(location.CustomHTTPErrors) != 0 { | |||
klog.V(3).Infof("Upstream %q has no active Endpoint", upstream.Name) | |||
// check if the location contains endpoints and a custom default backend | |||
if location.DefaultBackend != 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.
What do you think of merging everything until https://github.com/kubernetes/ingress-nginx/pull/3730/files#diff-86ac9ff9d75a0c5005c116e766a6127dR590 and extracting them all together into a helper function something called shouldCreateUpstreamForLocationDefaultBackend
?
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.
done, let me know if that's what you had in mind
if location.DefaultBackend != nil { | ||
return fmt.Sprintf("custom-default-backend-%v", location.DefaultBackend.GetName()) | ||
} | ||
return "upstream-default-backend" |
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 think these values are used in controller.go as well, how hard would it be to put them into a helper function and constant respectively and reuse here?
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 be happy to do that. package controller has a utils file already, but importing the controller package here doesn't quite seem right - is there somewhere with shared utils (across both controller and template)?
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.
Maybe we can make this function a property of Location
struct?
I don't mind leaving this as is for now.
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 tried an implementation of making it a property of Location struct, let me know if it's what you had in mind?
|
||
for upstream := range codesMap { | ||
codesForUpstream := []int{} | ||
for code := range codesMap[upstream] { |
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 rewrite it as
for code, _ := range codesMap[upstream] {
otherwise it's really confusing to read
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.
yup, will do
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.
have done so now, but gofmt throws a warning 'should omit 2nd value'
// and returns a unique set (for the template to create @custom_xxx locations) | ||
func collectCustomErrorsPerServer(input interface{}) []int { | ||
// and returns a set which is unique by default-upstream and error code. The resulting | ||
// data structure is a map of upstreamName => []errorCodes, so that sufficiently unique |
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 I read the code correctly this is an intermediary data structure you're using not what the function returns.
41ef917
to
3dedcd8
Compare
@aledbf thanks for the reminder! I addressed the comments and am still working on the e2e tests (I haven't had a lot of time to dive into this but I'll keep at it) |
930a557
to
19ca056
Compare
@jasongwartz just comparing |
I believe following patch should fix the issue:
The reason why I'm not comparing the whole |
55d41c2
to
8693fe8
Compare
@ElvinEfendi I should be able to use the new |
eeb54be
to
cdd1245
Compare
I'm thinking the failure of the e2e test is because the service of the custom-default-backend doesn't really exist and therefore has no endpoints (so the upstream doesn't get created). I'll try adding a deployment + service and see if that works |
Updates e2e test Removes focus from e2e test Fixes renamed function Adds tests for new template funcs Addresses gofmt Updates e2e test, fixes custom-default-backend test by creating service Updates docs
cdd1245
to
3865e30
Compare
Looks like that was it. @aledbf @ElvinEfendi I think this is ready for review again |
@@ -479,6 +479,10 @@ func (l1 *Location) Equal(l2 *Location) bool { | |||
return false | |||
} | |||
|
|||
if l1.DefaultBackendUpstreamName != l2.DefaultBackendUpstreamName { | |||
return false | |||
} |
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.
Was this the only change that fixed e2e test failure?
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, and adding an actual deployment so that endpoints will exist (here: https://github.com/kubernetes/ingress-nginx/pull/3730/files#diff-e82a2942a511f5a12803acc64b3fc676R107)
extensions "k8s.io/api/extensions/v1beta1" | ||
|
||
"k8s.io/ingress-nginx/test/e2e/framework" | ||
) | ||
|
||
const defaultBackendName = "upstream-default-backend" |
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 is not being used
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'll put up a quick follow-up to remove this, wasn't quick enough before merging. thanks for catching it
/lgtm I'll defer Thank you @jasongwartz ! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aledbf, ElvinEfendi, jasongwartz 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:
Approvers can indicate their approval by writing |
/hold oops the PR already has |
/hold cancel |
What this PR does / why we need it:
This PR makes it possible to use a default-backend annotation together with a custom-http-errors annotation. When the default-backend annotation is set, the upstream of the
@custom
error locations is set to the upstream name of the default-backend annotation's service.Cases covered:
This involved making the name of the error-handling location
@custom_<upstream-name>_<code>
. This is needed for the case where, within one server block, different paths (locations) are capturing the same error code but sending it to different default-backends. This means that the new 'default' error location name is@custom_upstream-default-backend_<code>
.Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close that issue when PR gets merged):This PR is a follow-up to #3344, specifically addressing comment: #3344 (comment). This also resolves #3700
Special notes:
I've had some trouble running the e2e-tests locally so I might need to revisit those. Thanks for reviewing!