Custom Error Pages #1675

bparli · 2017-05-26T21:43:03Z

Description

Following the above design and discussion:

New ErrorPage type to capture frontend error pages configurations
New error_pages middleware containing a response recorder
Documentation/example
Unit tests to ensure middleware is being initialized and rendering correctly
Basic integration tests

Custom error pages

nmengin · 2017-06-09T17:30:30Z

@bparli, your PR seems good for me.

But, I believe the issue 1634 forgets an Use Case : how to specify an error backend for only one error code?
Of course, it's possible to set a range of one code [404-404] but it's a bit ugly. It would be better to allow single host by using configuration as below :

 [frontends]
   [frontends.website]
   backend = "website"
   [errors]
     [error.network]
     status = "404"
     backend = "error"
     query = "/{status}.html"
   [frontends.website.routes.website]
   rule = "Host: website.mydomain.com"

 [backends]
   [backends.website]
     [backends.website.servers.website]
     url = "https://1.2.3.4"
   [backends.error]
     [backends.error.servers.error]
     url = "http://2.3.4.5"

Moreover, the best solution may be to specify a list of single code(s) and range (as printed page into printer windows) and configure it as below :

 [frontends]
   [frontends.website]
   backend = "website"
   [errors]
     [error.network]
     status = "401,404,[500-600]"
     backend = "error"
     query = "/{status}.html"
   [frontends.website.routes.website]
   rule = "Host: website.mydomain.com"

 [backends]
   [backends.website]
     [backends.website.servers.website]
     url = "https://1.2.3.4"
   [backends.error]
     [backends.error.servers.error]
     url = "http://2.3.4.5"

I guess the second suggestion is a little bit harder to develop.
It can be good to allow, at least, configuring a single code in a first time and maybe open a PR to improve the behavior with the list as described in the second solution.

@emilevauge, @bparli, what is your mind about this?

bparli · 2017-06-09T21:02:32Z

Thanks for taking a look @nmengin

The concern you raise did actually cross my mind, but I stuck to the original design and made sure the one-off HTTP code was still covered (albeit not very aesthetically as you point out). Also, I felt the one-off HTTP code may not be a common case; I've usually seen, or configured myself, for a range (or even an entire class) of codes. Of course, you all have a good vantage point so I can work on addressing your concern if you feel its helpful or necessary.

Also, did you mean to ask @emilevauge above instead of that other person?

emilevauge · 2017-06-10T00:17:05Z

@containous/traefik do you think we should implement the one-off HTTP code on top of ranges? IMHO, ranges only may be OK at first.

nmengin · 2017-06-12T11:53:35Z

@bparli @emilevauge , I am OK with you.

LGTM

re-gofmt types.go

* use renamed http recorder

emilevauge

Hey @bparli !
Thanks for this great PR, and sorry for being slow on reviewing.
I made few comments and just wanted to know what you think of adding the possibility to manage variables in query: /{status}.html (like what we proposed in #1634). It may be too restricting not being able to configure the query. WDYT?
But other than these little things, looks very good to me ❤️

emilevauge · 2017-06-23T22:12:45Z

middlewares/error_pages.go

+			log.Debugf("Caught HTTP Status Code %d, returning error page", recorder.Code)
+			w.WriteHeader(recorder.Code)
+			if newReq, err := http.NewRequest(http.MethodGet, ep.BackendURL, nil); err != nil {
+				w.Write([]byte(http.StatusText(recorder.Code)))


We may log err here

emilevauge · 2017-06-23T22:16:50Z

docs/basics.md

+    [error.network]
+    status = ["500-599"]
+    backend = "error"
+    query = "/500.html"


/{status}.html ?

It seems I originally misunderstood this point. I like it though and will extend to include the {status} query

emilevauge · 2017-06-23T22:19:42Z

integration/error_pages_test.go

+	c.Assert(err, checker.IsNil)
+}
+
+func (ep *ErrorPagesSuite) TestErrorPage(c *check.C) {


It would be even better to test using a non empty query IMHO

* use renamed http recorder * use renamed http recorder * updates based on feedback * grammar

nmengin

Many Thanks for this great PR!

LGTM

juliens

Great PR
Thanks

juliens · 2017-06-29T08:29:12Z

middlewares/error_pages.go

+	for _, block := range ep.HTTPCodeRanges {
+		if recorder.Code >= block[0] && recorder.Code <= block[1] {
+			log.Errorf("Caught HTTP Status Code %d, returning error page", recorder.Code)
+			re := regexp.MustCompile("{status}")


Maybe we can allocate the compiled regex in the construction of ErrorPagesHandler ?

I think you don't need a regex:

finalURL := strings.Replace(vli, "{status}", "400", -1)

Thanks @juliens, good point

and you're right @ldez, strings package will do the trick here

juliens · 2017-06-29T08:39:34Z

middlewares/error_pages.go

+		if err != nil {
+			return nil, err
+		}
+		highCode, err := strconv.Atoi(codes[1])


If we haven't "-", it panic here with out of range
Can you handle the case when there is no "-" (with a good error)
And add a test for this

thx

I think there are actually two options here, both simple. 1) catch it and log an error or 2) catch it and assume the user means to serve the error page for just the single code. In this case the single entry can still be used to create the correct array for checking at runtime. So, for example if ["503"] (instead of ["503-503"]) is configured, the array ["503", "503"] is created and we continue on. We essentially create the correct configuration on the user's behalf.

This was actually raised by @nmengin above, I should have just addressed it then

WDYT @juliens ?

I agree that handle the "no range status code" would be better, but a log could be enough

juliens · 2017-06-30T13:06:34Z

server/server.go

+
+					if len(frontend.Errors) > 0 {
+						for _, errorPage := range frontend.Errors {
+							if configuration.Backends[errorPage.Backend] != nil && configuration.Backends[errorPage.Backend].Servers["error"].URL != "" {


Could you add a log if the backend doesn't exist and/or if the URL is empty

* use renamed http recorder * use renamed http recorder * updates based on feedback * grammar * use string instead of regex, handle single code * add error message for mis-configured error backend *Use string instead of regex, handle single code

bparli · 2017-06-30T17:20:58Z

Thanks everyone for the reviews and feedback. I think its all addressed to this point

juliens

LGTM
Thanks

ldez · 2017-06-30T12:29:07Z

docs/basics.md

+    url = "http://2.3.4.5"
+```
+
+In the above example, the error page rendered was based on the status code.  Instead, the query parameter can also be set to some generic error page like so:  ```query = "/500s.html"```


could you replace ``` by `

ldez · 2017-06-30T19:09:41Z

@bparli It's time to squash and rebase 😃

ldez

LGTM

pjeby · 2017-09-01T05:17:08Z

docs/basics.md

+[frontends]
+  [frontends.website]
+  backend = "website"
+  [errors]


This bit of documentation looks wrong: the test fixture that resembles this has [frontends.frontend1.errors]
[frontends.frontend1.errors.networks], but the doc here just says [errors] and [errors.networks].

Thanks for noticing! Could you please file a new issue or ideally submit a PR? This one has been closed a while ago.

Thank you.

Ben Parli and others added 6 commits May 24, 2017 11:24

custom error pages

7dc04c5

custom error pages

d6fc5be

feedback updates

3c8fe12

error page integration tests

3218770

fix integration test port

58d51cd

Merge pull request #2 from bparli/custom-error-pages

0fb5104

Custom error pages

ldez added area/middleware kind/enhancement a new or improved feature. status/1-needs-design-review labels May 26, 2017

emilevauge added status/2-needs-review and removed status/1-needs-design-review labels Jun 6, 2017

nmengin added the contributor/needs-resolve-conflicts label Jun 13, 2017

Merge branch 'master' into main

8950516

ldez removed the contributor/needs-resolve-conflicts label Jun 13, 2017

Ben Parli and others added 3 commits June 13, 2017 10:31

re-gofmt types.go

4b15bc6

Merge pull request #3 from bparli/bp-main

6a16ef3

re-gofmt types.go

Use renamed http recorder (#4)

31c5410

* use renamed http recorder

emilevauge reviewed Jun 23, 2017

View reviewed changes

emilevauge added contributor/waiting-for-corrections status/2-needs-review and removed status/2-needs-review labels Jun 24, 2017

ldez added this to the 1.4 milestone Jun 26, 2017

bparli added 2 commits June 26, 2017 11:45

updates based on feedback (#5)

d8ade2d

* use renamed http recorder * use renamed http recorder * updates based on feedback * grammar

Merge branch 'master' into main

ede2eae

ldez removed the contributor/waiting-for-corrections label Jun 26, 2017

nmengin approved these changes Jun 27, 2017

View reviewed changes

juliens requested changes Jun 29, 2017

View reviewed changes

ldez added the contributor/waiting-for-corrections label Jun 29, 2017

juliens reviewed Jun 30, 2017

View reviewed changes

bparli added 2 commits June 30, 2017 09:56

Updates based on feedback

bda0698

* use renamed http recorder * use renamed http recorder * updates based on feedback * grammar * use string instead of regex, handle single code * add error message for mis-configured error backend *Use string instead of regex, handle single code

Merge branch 'master' into main

3129b36

juliens approved these changes Jun 30, 2017

View reviewed changes

ldez suggested changes Jun 30, 2017

View reviewed changes

bparli added 2 commits June 30, 2017 13:46

Update basics.md

d5f8e66

Merge branch 'master' into main

39dfcbc

ldez removed the contributor/waiting-for-corrections label Jun 30, 2017

ldez approved these changes Jun 30, 2017

View reviewed changes

doc: format.

f2c7941

ldez merged commit 121c057 into traefik:master Jun 30, 2017

ldez removed the status/2-needs-review label Jul 10, 2017

timoreimann mentioned this pull request Aug 19, 2017

Custom error pages #1634

Closed

pjeby reviewed Sep 1, 2017

View reviewed changes

kvncrw mentioned this pull request Sep 1, 2017

Add documentation for Error Pages on various configurations #2035

Closed

ldez mentioned this pull request Sep 1, 2017

Fix error pages configuration. #2038

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom Error Pages #1675

Custom Error Pages #1675

bparli commented May 26, 2017

nmengin commented Jun 9, 2017 •

edited by ldez

Loading

bparli commented Jun 9, 2017

emilevauge commented Jun 10, 2017

nmengin commented Jun 12, 2017

emilevauge left a comment

emilevauge Jun 23, 2017

emilevauge Jun 23, 2017

bparli Jun 26, 2017

emilevauge Jun 23, 2017

bparli Jun 26, 2017

nmengin left a comment

juliens left a comment

juliens Jun 29, 2017

ldez Jun 29, 2017 •

edited

Loading

bparli Jun 29, 2017

juliens Jun 29, 2017

bparli Jun 29, 2017 •

edited

Loading

juliens Jun 30, 2017 •

edited

Loading

juliens Jun 30, 2017

bparli commented Jun 30, 2017

juliens left a comment

ldez Jun 30, 2017

ldez commented Jun 30, 2017 •

edited

Loading

ldez left a comment

pjeby Sep 1, 2017

timoreimann Sep 1, 2017

Custom Error Pages #1675

Custom Error Pages #1675

Conversation

bparli commented May 26, 2017

Description

nmengin commented Jun 9, 2017 • edited by ldez Loading

bparli commented Jun 9, 2017

emilevauge commented Jun 10, 2017

nmengin commented Jun 12, 2017

emilevauge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmengin left a comment

Choose a reason for hiding this comment

juliens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldez Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparli Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

juliens Jun 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparli commented Jun 30, 2017

juliens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldez commented Jun 30, 2017 • edited Loading

ldez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmengin commented Jun 9, 2017 •

edited by ldez

Loading

ldez Jun 29, 2017 •

edited

Loading

bparli Jun 29, 2017 •

edited

Loading

juliens Jun 30, 2017 •

edited

Loading

ldez commented Jun 30, 2017 •

edited

Loading