feat: add 50x html for error page #4164
Conversation
starsz
requested review from
spacewander,
membphis,
tokers and
moonming
and removed request for
spacewander and
membphis
apisix/init.lua
Outdated
| @@ -620,6 +619,15 @@ end | |||
|
|
|||
| function _M.http_log_phase() | |||
| local api_ctx = common_phase("log") | |||
| if not api_ctx then | |||
| local ctx = ctxdump.apply_ngx_ctx(ngx_var.ctx_ref) | |||
There was a problem hiding this comment.
Why not reapply the ctx in http_header_filter_phase?
There was a problem hiding this comment.
Good. Let me fix it.
There was a problem hiding this comment.
Fixed. Please review it again.
apisix/cli/ngx_tpl.lua
Outdated
| @@ -617,6 +642,17 @@ http { | |||
| proxy_pass $upstream_mirror_host$request_uri; | |||
| } | |||
| {% end %} | |||
|
|
|||
| location = /50x.html { | |||
There was a problem hiding this comment.
Would it be better to use named location?
There was a problem hiding this comment.
Not really good to do that.Because we will use "50x.html" to search in the "html" directory.
There was a problem hiding this comment.
So the log phase might get a rewritten URL instead of the original one? This is not good news...
There was a problem hiding this comment.
Oh, I found a new way. We can use "try_files" to get the html.
There was a problem hiding this comment.
Fixed. Please review it again.
apisix/cli/ngx_tpl.lua
Outdated
| @@ -290,6 +293,17 @@ http { | |||
| apisix.http_control() | |||
| } | |||
| } | |||
|
|
|||
| location = /50x.html { | |||
There was a problem hiding this comment.
This should be a named location or internal location.
apisix/cli/ngx_tpl.lua
Outdated
| @@ -367,6 +381,17 @@ http { | |||
| apisix.http_admin() | |||
| } | |||
| } | |||
|
|
|||
| location = /50x.html { | |||
html/50x.html
Outdated
| @@ -0,0 +1,38 @@ | |||
| <!-- | |||
There was a problem hiding this comment.
Can we put this file into a Lua script like ngx_tpl.lua? So that there is no need to create an extra directory.
There was a problem hiding this comment.
Fixed. Please review it again.
apisix/cli/ops.lua
Outdated
| @@ -605,6 +611,9 @@ local function start(env, ...) | |||
| local cmd_logs = "mkdir -p " .. env.apisix_home .. "/logs" | |||
| util.execute_cmd(cmd_logs) | |||
|
|
|||
| local cmd_html = "mkdir -p " .. env.apisix_home .. "/html" | |||
There was a problem hiding this comment.
Need to add it to gitignore:
Line 48 in 504cd1b
There was a problem hiding this comment.
Fixed. Please review it again.
apisix/cli/ops.lua
Outdated
| @@ -605,6 +614,7 @@ local function start(env, ...) | |||
| local cmd_logs = "mkdir -p " .. env.apisix_home .. "/logs" | |||
| util.execute_cmd(cmd_logs) | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Why adding an empty line here?
There was a problem hiding this comment.
Fixed. Please review it again.
apisix/init.lua
Outdated
| @@ -534,6 +533,10 @@ end | |||
|
|
|||
|
|
|||
| function _M.http_header_filter_phase() | |||
| if not ngx.ctx or not ngx.ctx.api_ctx then | |||
| ngx.ctx = ctxdump.apply_ngx_ctx(ngx_var.ctx_ref) | |||
There was a problem hiding this comment.
Stash ngx.ctx unconditionally but checking the existence of ngx.ctx , this behavior leaks some keys for ngx.ctx tables (for those requests which doesn't make internal redirect).
There was a problem hiding this comment.
Fixed. Please review it again.
t/APISIX.pm
Outdated
| @@ -426,6 +429,18 @@ _EOC_ | |||
| more_clear_headers Date; | |||
| } | |||
|
|
|||
| location \@50x.html { | |||
| set \$error_page 'true'; | |||
There was a problem hiding this comment.
Better to use from_error_page instead
| @@ -74,3 +74,4 @@ ci/openwhisk-utilities/ | |||
| # release tar package | |||
| *.tgz | |||
| release/* | |||
| html/* | |||
There was a problem hiding this comment.
To make the license check pass, also need to change
Line 82 in 504cd1b
What this PR does / why we need it:
As we discussed in the email, we should have our own 50x HTML and index.html.Index.html maybe not necessary because of apisix is a grateway, there is no opportunity to show the index.html.
This PR is about to add 50x html into error_page. So when we had an error in apisix, we will show the html.
If the upstream returns 5xx error, we will show the upstream response rather than the 50x.html.
Fix: #3251
Pre-submission checklist: