Adds CustomHTTPErrors ingress annotation and test #3344

jasongwartz · 2018-11-01T23:34:00Z

This is a reopen of #3340 (after some rebasing trouble)

What this PR does / why we need it:

This PR introduces a new annotation called custom-http-errors, which functions in a similar way to the config value custom-http-errors, but performs the proxy_intercept overriding on the location blocks specified by that Ingress only.

In our Kubernetes cluster, we have one single ingress controller which serves both frontend website services and APIs - and we want to send back 'pretty' error pages for the web services, but not the APIs.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

#2532
#3246 - when using this PR, it's possible to cancel out the global custom-http-errors setting by adding the Ingress annotation and setting an unused status code only, like "nginx.ingress.kubernetes.io/custom-http-errors": "418".

Special notes for your reviewer:

We've been using this annotation in our development/test cluster for a while now and it's working well. I'm open to feedback on how to improve!

jasongwartz · 2018-11-01T23:34:35Z

@aledbf had some trouble during the rebase and accidentally closed #3340, sorry about that!

rootfs/etc/nginx/template/nginx.tmpl

ElvinEfendi · 2018-11-02T08:35:04Z

internal/ingress/types.go

@@ -148,6 +148,8 @@ type Server struct {
 	SSLCiphers string `json:"sslCiphers,omitempty"`
 	// AuthTLSError contains the reason why the access to a server should be denied
 	AuthTLSError string `json:"authTLSError,omitempty"`
+	// AllCustomHTTPErrors contains an array of custom error codes for all locations
+	AllCustomHTTPErrors []int `json:allCustomHTTPErrors,omitempty`


Is this really necessary? A server has locations you can access when rendering at https://github.com/kubernetes/ingress-nginx/pull/3344/files#diff-980db9e4b88704f12338bd074839f94eR821 and aggregate the custom HTTP codes per location. Also IMO template.go is a better place for doing the aggregation rather than controller.go.

@ElvinEfendi IIRC I set this up when I intended to do deduplication (for example - two ingresses for the same host but different paths can both intercept 404, but only one @custom_404 should be created). I looked into doing this directly in the gotemplate but it seemed tricky - I'll look into moving this into template.go as you suggest

@ElvinEfendi as per your comment I removed the AllCustomHTTPErrors field and replaced it with a template function to do the collation of all error routes for a server block, and added deduplication (so both ingresses can specify 404 and only one @custom_404 location is created, for example).

Let me know if this is more in line with what you were thinking!

Edit: if it looks better, I'll squash the new commit into the earlier squashed commits

jasongwartz · 2018-11-03T14:10:59Z

Rebasing from master seems to have fixed the CI build.

ElvinEfendi · 2018-11-05T05:43:03Z

internal/ingress/controller/controller.go

-				SSLCiphers:     anns.SSLCiphers,
+					SSLPassthrough: anns.SSLPassthrough,
+					SSLCiphers:     anns.SSLCiphers,
+				}


Can you revert this nop change? I think you left it from your original commit and you don't need it anymore.

yep, can do

aledbf · 2018-11-06T12:36:29Z

internal/ingress/annotations/customhttperrors/main.go

+	"strconv"
+	"strings"
+
+	// "github.com/pkg/errors"


Please remove this comment

Done (in with latest squash commit)

aledbf · 2018-11-06T12:37:39Z

@jasongwartz please squash the commits and this lgtm

aledbf · 2018-11-06T12:39:01Z

@jasongwartz how hard is to allow custom annotations for the same error code in different paths?
(this implies multiple ingresses with different paths for the same hostname using the annotation)

Adds per-server/location error-catch functionality to nginx template Adds documentation Reduces template duplication with helper function for CUSTOM_ERRORS data Updates documentation Adds e2e test for customerrors Removes AllCustomHTTPErrors, replaces with template function with deduplication and adds e2e test of deduplication Fixes copy-paste error in test, adds additional test cases Reverts noop change in controller.go (unused now)

jasongwartz · 2018-11-06T15:51:12Z

@aledbf cheers! squashed and pushed

I think I understand your question - are you asking in the case that there are two ingresses with the same hostname but different paths? In that case, each ingress can set its own set of error codes in an annotation - if both ingresses want to intercept the same error codes, they can both set the annotation the same (or the global config value can be set).

aledbf · 2018-11-06T16:01:24Z

if both ingresses want to intercept the same error codes, they can both set the annotation the same (or the global config value can be set).

I am sorry for the lack of context. This PR allows custom error codes using the "default backend". What I "want" is custom error codes with different default backends

jasongwartz · 2018-11-06T16:15:09Z

@aledbf I actually thought about that but was planning on doing it as a follow-up, I think I need to learn more about how the default-backends are used/assigned first. I considered both sending the custom errors to the annotation default-backend or to a separate backend (maybe specifiable by a secondary annotation), but I figured the current state of the PR is already an improvement on the existing functionality.
I assume it's a matter of passing the right value here but I haven't been able to look into that in more detail yet.
I'm also not sure how the annotations would have to be structured to potentially allow for n-to-n matching (any number of sets of error codes matching any number of backends), would need to think more about that.

aledbf · 2018-11-06T16:17:56Z

I actually thought about that but was planning on doing it as a follow-up

👍 :)

I considered both sending the custom errors to the annotation default-backend or to a separate backend

I think would be ideal to avoid new annotations

aledbf · 2018-11-06T16:19:03Z

/lgtm
/approve

k8s-ci-robot · 2018-11-06T16:19:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, 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:

~~OWNERS~~ [aledbf]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ElvinEfendi · 2018-11-06T16:27:13Z

👍

jasongwartz · 2018-11-06T16:52:29Z

/retest

If this doesn't work, is there something I can do to rerun the build?

aledbf · 2018-11-06T16:58:46Z

If this doesn't work, is there something I can do to rerun the build?

No. The bot does not trigger travis-ci builds.

jasongwartz · 2018-11-06T17:01:52Z

@aledbf my mistake, I wasn’t sure. The build seems to be failing on unrelated e2e tests

jasongwartz · 2018-11-06T17:25:46Z

@aledbf @ElvinEfendi cheers, and thanks for the help!

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 1, 2018

k8s-ci-robot requested review from aledbf and ElvinEfendi November 1, 2018 23:34

jasongwartz mentioned this pull request Nov 1, 2018

Adds custom-http-errors per Ingress #3340

Closed

aledbf reviewed Nov 1, 2018

View reviewed changes

rootfs/etc/nginx/template/nginx.tmpl Outdated Show resolved Hide resolved

jasongwartz force-pushed the jg-customerrors-per-ingress branch 2 times, most recently from 508878a to ed56533 Compare November 2, 2018 08:17

ElvinEfendi reviewed Nov 2, 2018

View reviewed changes

jasongwartz force-pushed the jg-customerrors-per-ingress branch 3 times, most recently from 92d0f71 to ae49fd5 Compare November 3, 2018 13:32

jasongwartz force-pushed the jg-customerrors-per-ingress branch 2 times, most recently from ee39db2 to 1662c78 Compare November 3, 2018 22:01

ElvinEfendi reviewed Nov 5, 2018

View reviewed changes

aledbf reviewed Nov 6, 2018

View reviewed changes

jasongwartz force-pushed the jg-customerrors-per-ingress branch from 8c10eef to 0ebf035 Compare November 6, 2018 15:48

k8s-ci-robot assigned aledbf Nov 6, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 6, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 6, 2018

k8s-ci-robot merged commit 265f96b into kubernetes:master Nov 6, 2018

aledbf mentioned this pull request Nov 14, 2018

Allow override of custom error pages status codes per ingress configuration #2532

Closed

This was referenced Feb 1, 2019

Default Backend Annotation Incompatible with custom-http-errors #3700

Closed

Disable custom-http-errors on a per Ingress basis #3246

Closed

Changes CustomHTTPErrors annotation to use custom default backend #3730

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds CustomHTTPErrors ingress annotation and test #3344

Adds CustomHTTPErrors ingress annotation and test #3344

jasongwartz commented Nov 1, 2018

jasongwartz commented Nov 1, 2018

ElvinEfendi Nov 2, 2018 •

edited

Loading

jasongwartz Nov 2, 2018

jasongwartz Nov 2, 2018 •

edited

Loading

jasongwartz commented Nov 3, 2018

ElvinEfendi Nov 5, 2018 •

edited

Loading

jasongwartz Nov 5, 2018

aledbf Nov 6, 2018

jasongwartz Nov 6, 2018

aledbf commented Nov 6, 2018

aledbf commented Nov 6, 2018 •

edited

Loading

jasongwartz commented Nov 6, 2018

aledbf commented Nov 6, 2018

jasongwartz commented Nov 6, 2018 •

edited

Loading

aledbf commented Nov 6, 2018

aledbf commented Nov 6, 2018

k8s-ci-robot commented Nov 6, 2018

ElvinEfendi commented Nov 6, 2018

jasongwartz commented Nov 6, 2018 •

edited

Loading

aledbf commented Nov 6, 2018

jasongwartz commented Nov 6, 2018

jasongwartz commented Nov 6, 2018

Adds CustomHTTPErrors ingress annotation and test #3344

Adds CustomHTTPErrors ingress annotation and test #3344

Conversation

jasongwartz commented Nov 1, 2018

jasongwartz commented Nov 1, 2018

ElvinEfendi Nov 2, 2018 • edited Loading

Choose a reason for hiding this comment

jasongwartz Nov 2, 2018

Choose a reason for hiding this comment

jasongwartz Nov 2, 2018 • edited Loading

Choose a reason for hiding this comment

jasongwartz commented Nov 3, 2018

ElvinEfendi Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

jasongwartz Nov 5, 2018

Choose a reason for hiding this comment

aledbf Nov 6, 2018

Choose a reason for hiding this comment

jasongwartz Nov 6, 2018

Choose a reason for hiding this comment

aledbf commented Nov 6, 2018

aledbf commented Nov 6, 2018 • edited Loading

jasongwartz commented Nov 6, 2018

aledbf commented Nov 6, 2018

jasongwartz commented Nov 6, 2018 • edited Loading

aledbf commented Nov 6, 2018

aledbf commented Nov 6, 2018

k8s-ci-robot commented Nov 6, 2018

ElvinEfendi commented Nov 6, 2018

jasongwartz commented Nov 6, 2018 • edited Loading

aledbf commented Nov 6, 2018

jasongwartz commented Nov 6, 2018

jasongwartz commented Nov 6, 2018

ElvinEfendi Nov 2, 2018 •

edited

Loading

jasongwartz Nov 2, 2018 •

edited

Loading

ElvinEfendi Nov 5, 2018 •

edited

Loading

aledbf commented Nov 6, 2018 •

edited

Loading

jasongwartz commented Nov 6, 2018 •

edited

Loading

jasongwartz commented Nov 6, 2018 •

edited

Loading