feat: add 5xx error page #3883

starsz · 2021-03-23T01:07:45Z

What this PR does / why we need it:

Show our own HTML page, when the apisix self returns 5xx.
fix #3251

Pre-submission checklist:

Did you explain what problem does this PR solves? Or what new features have been added?
Have you added corresponding test cases?
Have you modified the corresponding document?
Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

t/cli/test_error_page.sh

apisix/error_handler.lua

spacewander · 2021-03-24T02:00:24Z

t/cli/test_error_page.sh

+. ./t/cli/common.sh
+
+git checkout conf/config.yaml
+make init


We don't need it if we call make run later.

membphis · 2021-03-24T04:57:02Z

you can merge the master branch

t/cli/test_error_page.sh

tokers · 2021-03-24T06:25:02Z

t/cli/test_error_page.sh

+         echo "failed: the error page is not customized"
+         exit 1
+    fi
+done


Should also add some test cases that an upstream returns 5xx but these error pages are not shown.

membphis · 2021-03-29T15:41:52Z

ci failed now

moonming · 2021-03-30T04:14:24Z

apisix/cli/ngx_tpl.lua

@@ -233,6 +233,9 @@ http {
    {% end %}
    {% end %}

+    # error_page
+    error_page 500 502 503 504 @apisix_error_handler;


what about the 4xx response codes?

Yes, we do need it.

<html> <head><title>403 Forbidden</title></head> <body> <center><h1>403 Forbidden</h1></center> <hr><center>openresty</center> </body> </html>

Maybe we can add a link in the response first.

apisix/error_handler.lua

moonming · 2021-03-30T04:17:48Z

apisix/error_handler.lua

+<p>If you are the system administrator of this resource then you should check
+the <a href="https://github.com/apache/apisix/blob/master/conf/config-default.yaml#L135">error log
+</a> for details.</p>
+<p>If you need any help, click <a href="https://apisix.apache.org/">need help</a>.</em></p>


I don't think this link can help users

How about

https://apisix.apache.org/help/

same to #3883 (comment)

tokers · 2021-03-31T02:09:08Z

ci failed now

Please fix it when you have time @starsz .

starsz · 2021-03-31T03:38:58Z

ci failed now

Please fix it when you have time @starsz .

Need some time.A little busy lately.

moonming · 2021-04-01T04:49:42Z

Of course, we need the 4xx page. Thanks, Ming Wen Twitter: _WenMing Peter Zhu ***@***.***> 于2021年4月1日周四下午12:45写道：

…

***@***.**** commented on this pull request. ------------------------------ In apisix/cli/ngx_tpl.lua <#3883 (comment)>: > @@ -233,6 +233,9 @@ http { {% end %} {% end %} + # error_page + error_page 500 502 503 504 @apisix_error_handler; Now the 4xx situation like: 404: {"error_msg":"404 Route Not Found"} 401: {"code":4107,"message":"request unauthorized","data":null,"request_id":""} We will return JSON, do this need to add an error page? IMO, it doesn't need now. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3883 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGJZBK23EDVVWLBE5EIVAJLTGP26BANCNFSM4ZUF3MSA> .

tokers · 2021-04-01T12:15:37Z

apisix/error_handler.lua

@@ -50,5 +50,4 @@ return function()
    if ngx.status >= 500 and ngx.status <= 599 then
        ngx.say(html_5xx)
    end
-end
-
+end


Please check your editor, end of line symbol is required.

apisix/error_handler.lua

moonming · 2021-04-04T02:43:04Z

ping @starsz

tokers · 2021-04-06T00:53:48Z

t/cli/test_error_page.sh

+sleep 0.1
+
+# set route
+code=$(curl -XPUT -k -i -m 20 -o /dev/null -s -w %{http_code} http://127.0.0.1:9080/apisix/admin/routes/1 -H 'X-API-KEY: edd1c9f034335f136f87ad84b625c8f1' -d '{


I'm confused why you don't test them by Test::Nginx.

moonming · 2021-04-17T01:29:07Z

ping @starsz

I am going to close this pr, there is no feedback for too long

starsz · 2021-04-17T01:32:35Z

ping @starsz

I am going to close this pr, there is no feedback for too long

Better not, I will finish it this weekend.

t/error_handler/error_handler.t

moonming · 2021-04-19T01:23:19Z

apisix/error_handler.lua

+local ngx_status_line = {
+    [400] = "400 Bad Request",
+    [401] = "401 Unauthorized",
+    [403] = "403 Forbidden",
+    [404] = "404 Not Found",
+    [405] = "405 Not Allowed",
+}
+


why we need this?

Because we should add it into 4xx html.
I can't find any API that contains this info.
@tokers @spacewander Do you have any opinions?

Show status code in body is useful in the browser, I think we can reserve it.

moonming · 2021-04-19T01:31:31Z

t/error_handler/error_handler.t

+=== TEST 12: test apisix with upstream error code 400
+--- request
+GET /specific_status
+--- more_headers
+X-Test-Upstream-Status: 400
+--- error_code: 400
+--- response_body
+upstream status: 400
+
+
+
+=== TEST 11: test apisix with upstream error code 500
+--- request
+GET /specific_status
+--- more_headers
+X-Test-Upstream-Status: 500
+--- error_code: 500
+--- response_body
+upstream status: 500


I am a little confused, why do special treatments for upstream status?

Here is a mock that I mock the upstream status.
You can see t/lib/server.lua.

I mean, why doesn’t the upstream 5xx show the 5xx page of Apache APISIX?

I mean, why doesn’t the upstream 5xx show the 5xx page of Apache APISIX?

The upstream 5xx has its own mean.There maybe contains the error info like `{"code": 5xx, "error_msg": "xxx"}.
So IMO we shouldn't modify it.

Do users need to care about the error of the Apache APISIX or the upstream error? I remember that there are already corresponding HTTP headers to distinguish

In some situations. The info in the response body is useful. The user will need the upstream error info.
cc @tokers

@moonming We should keep the upstream 4xx/5xx pages intact by default, but we may add a switch like:

proxy_intercept_errors on | off;

This is a standard Nginx directive.

moonming

Before this PR, what was the data returned by 5xx and 4xx?

tokers · 2021-04-19T05:37:27Z

I have no other questions but one:

Make sure it won't overwrite the 4xx and 5xx pages generated by upstream.
For those 4xx and 5xx codes generated by APISIX, we may need more information like "IP is forbidden" (ip-restriction plugin).

For 1, we may have a switch to control whether or not to override the upstream.

moonming · 2021-04-19T15:32:45Z

Before this PR, what was the data returned by 5xx and 4xx?

any response? @starsz

starsz · 2021-04-19T15:33:51Z

Before this PR, what was the data returned by 5xx and 4xx?

any response? @starsz

Let me test and list the response.

starsz · 2021-04-19T15:59:40Z

Before this PR, what was the data returned by 5xx and 4xx?

any response? @starsz

Let me test and list the response.

The internal error:
500:

<html>
<head><title>500 Internal Server Error</title></head>
<body>
<center><h1>500 Internal Server Error</h1></center>
<hr><center>openresty</center>
</body>
</html>

502:

<html>
<head><title>502 Bad Gateway</title></head>
<body>
<center><h1>502 Bad Gateway</h1></center>
<hr><center>openresty</center>
</body>
</html>

503:

<html>
<head><title>503 Service Temporarily Unavailable</title></head>
<body>
<center><h1>503 Service Temporarily Unavailable</h1></center>
<hr><center>openresty</center>
</body>
</html>

504:

<html>
<head><title>504 Gateway Time-out</title></head>
<body>
<center><h1>504 Gateway Time-out</h1></center>
<hr><center>openresty</center>
</body>
</html>

400:

<head><title>400 Bad Request</title></head>
<body>
<center><h1>400 Bad Request</h1></center>
<hr><center>openresty</center>
</body>
</html>

401:

<html>
<head><title>401 Authorization Required</title></head>
<body>
<center><h1>401 Authorization Required</h1></center>
<hr><center>openresty</center>
</body>
</html>

404:

<html>
<head><title>404 Not Found</title></head>
<body>
<center><h1>404 Not Found</h1></center>
<hr><center>openresty</center>
</body>
</html>

moonming · 2021-04-19T23:03:52Z

Then your PR did not solve the issue @starsz

moonming · 2021-04-19T23:04:16Z

please read the issue again

starsz · 2021-04-20T13:48:58Z

Then your PR did not solve the issue @starsz

I am confused. This PR can solve the issue. The response I give is before this PR.
Can you give more details about why this PR can't solve the issue?

By the way, I will fix the CI first.

moonming · 2021-04-20T14:48:10Z

Then your PR did not solve the issue @starsz

I am confused. This PR can solve the issue. The response I give is before this PR.

Can you give more details about why this PR can't solve the issue?

By the way, I will fix the CI first.

please make sure you read the issue and thread in mailinglist

tokers · 2021-04-21T00:34:13Z

@starsz Conflicts should be resolved.

starsz · 2021-04-22T16:09:15Z

Then your PR did not solve the issue @starsz

I am confused. This PR can solve the issue. The response I give is before this PR.
Can you give more details about why this PR can't solve the issue?
By the way, I will fix the CI first.

please make sure you read the issue and thread in mailing list

Hi, @moonming.I read the email again.

The openresty updated the 50x.html.
If we use the openresty's default conf or nginx default conf. There is the following configuration:

error_page   500 502 503 504  /50x.html;
        location = /50x.html {
            root   html;
        }

So that we can see the HTML that updated by the openresty.

But in apisix, we don't set the error page in config.See the ngx_tpl.lua.So we wouldn't see the 50x.html updated by the openresty.

BTW, I think my implementation is too complicated. Can we create html directory and put our html in it? Then we can get the html if an error occurred.

moonming · 2021-04-29T09:38:35Z

any update?

moonming · 2021-04-29T09:39:01Z

@starsz ping

starsz · 2021-04-29T16:54:12Z

any update?

Use another PR #4164 to fix this issue.
So this PR can be closed.

starsz added 6 commits March 23, 2021 08:58

feat: add 5xx error page (apache#3251)

e32286b

update: add cli test

016be59

update: local ngx

efe2497

update: add eol

f27d5dc

update: code 201

cf2e848

update: add sleep time

c27f70b

starsz marked this pull request as ready for review March 24, 2021 01:07

starsz requested review from spacewander and membphis and removed request for spacewander March 24, 2021 01:50

spacewander reviewed Mar 24, 2021

View reviewed changes

tokers reviewed Mar 24, 2021

View reviewed changes

update: error page

5eb4eda

moonming reviewed Mar 30, 2021

View reviewed changes

tokers reviewed Apr 1, 2021

View reviewed changes

membphis reviewed Apr 4, 2021

View reviewed changes

apisix/error_handler.lua Outdated Show resolved Hide resolved

tokers reviewed Apr 6, 2021

View reviewed changes

starsz added 3 commits April 18, 2021 00:04

update: 4xx code

594de48

update: fix review

c9b92e6

update: add error handler test

265cce1

tokers reviewed Apr 19, 2021

View reviewed changes

t/error_handler/error_handler.t Outdated Show resolved Hide resolved

moonming reviewed Apr 19, 2021

View reviewed changes

starsz added 2 commits April 19, 2021 10:42

update: fix code style

b9b9d79

upodate: fix reindex

cbe7602

update: fix code style

6db104b

update: fix apisix.pm

e2c977e

starsz closed this Apr 29, 2021

feat: add 5xx error page #3883

feat: add 5xx error page #3883

Conversation

starsz commented Mar 23, 2021

What this PR does / why we need it:

Pre-submission checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

membphis commented Mar 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

membphis commented Mar 29, 2021

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

tokers commented Mar 31, 2021

starsz commented Mar 31, 2021

moonming commented Apr 1, 2021 via email

Choose a reason for hiding this comment

moonming commented Apr 4, 2021

Choose a reason for hiding this comment

moonming commented Apr 17, 2021

starsz commented Apr 17, 2021

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

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

moonming left a comment

Choose a reason for hiding this comment

tokers commented Apr 19, 2021

moonming commented Apr 19, 2021

starsz commented Apr 19, 2021

starsz commented Apr 19, 2021

moonming commented Apr 19, 2021

moonming commented Apr 19, 2021

starsz commented Apr 20, 2021 • edited Loading

moonming commented Apr 20, 2021

tokers commented Apr 21, 2021

starsz commented Apr 22, 2021

moonming commented Apr 29, 2021

moonming commented Apr 29, 2021

starsz commented Apr 29, 2021 • edited Loading

starsz commented Apr 20, 2021 •

edited

Loading

starsz commented Apr 29, 2021 •

edited

Loading