feat: add 50x html for error page#4164
Conversation
| 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.
| } | ||
| {% 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.
| } | ||
| } | ||
|
|
||
| location = /50x.html { |
There was a problem hiding this comment.
This should be a named location or internal location.
| } | ||
| } | ||
|
|
||
| location = /50x.html { |
| @@ -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.
| 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.
Fixed. Please review it again.
| 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.
|
|
||
| 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.
| } | ||
|
|
||
| location \@50x.html { | ||
| set \$error_page 'true'; |
There was a problem hiding this comment.
Better to use from_error_page instead
| # 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
3 participants
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: