-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: redirect http to https but port not change #7065
Conversation
Signed-off-by: Wei Jiang <machowei01@gmail.com>
Signed-off-by: Wei Jiang <machowei01@gmail.com>
Signed-off-by: Wei Jiang <machowei01@gmail.com>
conf/config-default.yaml
Outdated
@@ -470,3 +470,4 @@ plugin_attr: | |||
connect: 60s | |||
read: 60s | |||
send: 60s | |||
redirect_https_port: 8443 # the default port for use by HTTP redirects to HTTPS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use a structure like this:
redirect:
https_port: ...
and comment it out as nobody wants to be redirected to 8443 by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
apisix/plugins/redirect.lua
Outdated
@@ -148,7 +149,30 @@ function _M.rewrite(conf, ctx) | |||
core.log.info("plugin rewrite phase, conf: ", core.json.delay_encode(conf)) | |||
|
|||
local ret_code = conf.ret_code | |||
local ret_port = tonumber(ctx.var["var_x_forwarded_port"]) | |||
local local_conf = core.config.local_conf() | |||
local ret_port = core.table.try_read_attr(local_conf, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a wrapper to get the attribute of a plugin:
apisix/apisix/plugins/prometheus.lua
Line 58 in 6ebe027
local attr = plugin.plugin_attr(plugin_name) |
We need to use it instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
--- yaml_config | ||
apisix: | ||
ssl: | ||
enable: null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add two more test:
- use listen_port
- use listen and get port in the array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Co-authored-by: 罗泽轩 <spacewanderlzx@gmail.com>
Signed-off-by: Wei Jiang <machowei01@gmail.com>
docs/en/latest/plugins/redirect.md
Outdated
@@ -45,7 +45,10 @@ The `redirect` Plugin can be used to configure redirects. | |||
|
|||
Only one of `http_to_https`, `uri` and `regex_uri` can be configured. | |||
|
|||
* When enabling `http_to_https`, the port in the redirect URL will be the value of header `X-Forwarded-Port` or the port of the server. | |||
* When enabling `http_to_https`, the ports in the redirect URL will pick a value in the following order (in descending order of priority) | |||
* Read `plugin_attr.redirect_https_port` from the configuration file (`conf/config.yaml`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redirect_https_port should be updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
docs/zh/latest/plugins/redirect.md
Outdated
@@ -45,7 +45,10 @@ description: 本文介绍了关于 Apache APISIX `redirect` 插件的基本信 | |||
|
|||
`http_to_https`、`uri` 和 `regex_uri` 只能配置其中一个属性。 | |||
|
|||
* 当开启 `http_to_https` 时,重定向 URL 中的端口将是 `X-Forwarded-Port` 请求头的值或服务器的端口。 | |||
* 当开启 `http_to_https` 时,重定向 URL 中的端口将按如下顺序选取一个值(按优先级从高到低排列) | |||
* 从配置文件(`conf/config.yaml`)中读取 `plugin_attr.redirect_https_port`。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
t/plugin/redirect.t
Outdated
@@ -428,59 +428,83 @@ passed | |||
|
|||
|
|||
|
|||
=== TEST 18: redirect | |||
=== TEST 18: redirect(port using `plugin_attr.redirect_https_port`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Co-authored-by: tzssangglass <tzssangglass@gmail.com> Co-authored-by: Alex Zhang <tokers@apache.org>
Co-authored-by: tzssangglass <tzssangglass@gmail.com>
Signed-off-by: Wei Jiang <machowei01@gmail.com>
Signed-off-by: Wei Jiang <machowei01@gmail.com>
end | ||
end | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I feel that this code is too inelegant. Suggest:
local function get_port(attr)
local port
if attr then
port = attr.https.port
end
if port then
return port
end
local local_conf = core.config.local_conf()
local ssl = core.table.try_read_attr(local_conf, "apisix", "ssl")
if not ssl or ssl["enable"] then
return port
end
port = ssl["listen_port"]
if port then
return port
end
local ports = ssl["listen"]
if ports and #ports > 0 then
local idx = math_random(1, #ports)
port = ports[idx]
if type(port) == "table" then
port = port.port
end
end
return port
end
local ret_port = get_port(attr)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
apisix/plugins/redirect.lua
Outdated
@@ -148,7 +150,30 @@ function _M.rewrite(conf, ctx) | |||
core.log.info("plugin rewrite phase, conf: ", core.json.delay_encode(conf)) | |||
|
|||
local ret_code = conf.ret_code | |||
local ret_port = tonumber(ctx.var["var_x_forwarded_port"]) | |||
local ret_port = nil | |||
local attr = plugin.plugin_attr(plugin_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where did you define the variable plugin_name
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here:
apisix/apisix/plugins/redirect.lua
Lines 65 to 67 in 6ebe027
local plugin_name = "redirect" | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was defined before :)
Signed-off-by: Wei Jiang <machowei01@gmail.com>
Let's merge master to make CI pass. |
Signed-off-by: Wei Jiang <machowei01@gmail.com> Co-authored-by: 罗泽轩 <spacewanderlzx@gmail.com> Co-authored-by: tzssangglass <tzssangglass@gmail.com> Co-authored-by: Alex Zhang <tokers@apache.org>
Signed-off-by: Wei Jiang <machowei01@gmail.com> Co-authored-by: 罗泽轩 <spacewanderlzx@gmail.com> Co-authored-by: tzssangglass <tzssangglass@gmail.com> Co-authored-by: Alex Zhang <tokers@apache.org>
* upsgrteam/master: (351 commits) fix(proxy-cache): bypass when method mismatch cache_method (apache#7111) chore(script): support to install dependencies under arm64 (apache#7091) chore(ci): use the latest build script for apisix-base (apache#7090) fix(batch-requests): ignore "unix:" in the configuration (apache#7106) fix: install dependencies issues (apache#7092) feat(ops): use lua libs to backup config file insteadof shell command (apache#7048) test: reduce CI failure caused by flaky tests (apache#7085) chore(ci): move set_dns.sh to ci dir (apache#7089) feat: release 2.14.0 (apache#7057) docs: update "Tracers" Plugins (apache#7086) docs: update "Traffic" Plugin docs 3 (apache#7064) docs: update "Serverless" Plugins (apache#7076) feat(ops): check process running with posix.signal insteadof lsof (apache#7006) docs: modify how-to-build filename (apache#7087) docs: fix link of hot-reload in docs (apache#7081) chore(ci): apt update before install (apache#7080) docs: add pubsub develop example for kafka (apache#7059) ci: enable rebase in some situation (apache#7074) fix: redirect http to https but port not change (apache#7065) ci: make it pass under OpenResty 1.21 (apache#7067) ...
Signed-off-by: Wei Jiang <machowei01@gmail.com> Co-authored-by: 罗泽轩 <spacewanderlzx@gmail.com> Co-authored-by: tzssangglass <tzssangglass@gmail.com> Co-authored-by: Alex Zhang <tokers@apache.org>
Description
When http redirects to htpps, we use a correct port
Fixes #7011
Checklist