Skip to content
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

Merged
merged 13 commits into from
May 18, 2022
29 changes: 27 additions & 2 deletions apisix/plugins/redirect.lua
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
-- limitations under the License.
--
local core = require("apisix.core")
local plugin = require("apisix.plugin")
local tab_insert = table.insert
local tab_concat = table.concat
local string_format = string.format
Expand All @@ -24,7 +25,8 @@ local ipairs = ipairs
local ngx = ngx
local str_find = core.string.find
local str_sub = string.sub
local tonumber = tonumber
local type = type
local math_random = math.random

local lrucache = core.lrucache.new({
ttl = 300, count = 100
Expand Down Expand Up @@ -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)
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here:

local plugin_name = "redirect"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was defined before :)

if attr then
ret_port = attr.https_port
end

if not ret_port then
local local_conf = core.config.local_conf()
local ssl = core.table.try_read_attr(local_conf, "apisix", "ssl")
if ssl and ssl["enable"] then
ret_port = ssl["listen_port"]
if not ret_port then
local ret_ports = ssl["listen"]
if ret_ports and #ret_ports > 0 then
local idx = math_random(1, #ret_ports)
ret_port = ret_ports[idx]
if type(ret_port) == "table" then
ret_port = ret_port.port
end
end
end
end
end

Copy link
Contributor

@soulbird soulbird May 18, 2022

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

local uri = conf.uri
local regex_uri = conf.regex_uri

Expand Down
2 changes: 2 additions & 0 deletions conf/config-default.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -470,3 +470,5 @@ plugin_attr:
connect: 60s
read: 60s
send: 60s
# redirect:
# https_port: 8443 # the default port for use by HTTP redirects to HTTPS
5 changes: 4 additions & 1 deletion docs/en/latest/plugins/redirect.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`).
* If `apisix.ssl` is enabled, read `apisix.ssl.listen_port` first, and if it does not exist, read `apisix.ssl.listen` and select a port randomly from it.
* Use 443 as the default https port.

:::

Expand Down
5 changes: 4 additions & 1 deletion docs/zh/latest/plugins/redirect.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`
* 如果 `apisix.ssl` 处于开启状态,先读取 `apisix.ssl.listen_port`,如果没有,再读取 `apisix.ssl.listen` 并从中随机选一个 `port`
* 使用 443 作为默认 `https port`

:::

Expand Down
64 changes: 44 additions & 20 deletions t/plugin/redirect.t
Original file line number Diff line number Diff line change
Expand Up @@ -428,59 +428,83 @@ passed



=== TEST 18: redirect
=== TEST 18: redirect(port using `plugin_attr.redirect.https_port`)
--- extra_yaml_config
plugin_attr:
redirect:
https_port: 8443
--- request
GET /hello
--- more_headers
Host: foo.com
--- error_code: 301
--- response_headers
Location: https://foo.com:1984/hello
Location: https://foo.com:8443/hello



=== TEST 19: redirect(pass well-known port 443 to x-forwarded-port)
=== TEST 19: redirect(port using `apisix.ssl.listen_port`)
--- yaml_config
apisix:
ssl:
enable: true
listen_port: 9445
--- extra_yaml_config
plugin_attr:
redirect_https_port: null
--- request
GET /hello
--- more_headers
Host: foo.com
x-forwarded-port: 443
--- error_code: 301
--- response_headers
Location: https://foo.com/hello
Location: https://foo.com:9445/hello



=== TEST 20: redirect(pass negative number to x-forwarded-port)
=== TEST 20: redirect(port using `apisix.ssl.listen` when listen length is one)
--- request
GET /hello
--- more_headers
Host: foo.com
x-forwarded-port: -443
--- error_code: 301
--- response_headers
Location: https://foo.com/hello
Location: https://foo.com:9443/hello



=== TEST 21: redirect(pass number more than 65535 to x-forwarded-port)
=== TEST 21: redirect(port using `apisix.ssl.listen` when listen length more than one)
--- yaml_config
apisix:
ssl:
enable: true
listen:
- 6443
- 7443
- port: 8443
- port: 9443
--- request
GET /hello
--- more_headers
Host: foo.com
x-forwarded-port: 65536
--- error_code: 301
--- response_headers
Location: https://foo.com/hello
--- response_headers_like
Location: https://foo.com:[6-9]443/hello



=== TEST 22: redirect(pass invalid non-number to x-forwarded-port)
=== TEST 22: redirect(port using `https default port`)
--- yaml_config
apisix:
ssl:
enable: null
Copy link
Member

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:

  1. use listen_port
  2. use listen and get port in the array

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

--- extra_yaml_config
plugin_attr:
redirect: null
--- request
GET /hello
--- more_headers
Host: foo.com
x-forwarded-port: ok
--- error_code: 301
--- response_headers
Location: https://foo.com/hello
Expand Down Expand Up @@ -528,7 +552,7 @@ GET /hello
Host: foo.com
--- error_code: 301
--- response_headers
Location: https://foo.com:1984/hello
Location: https://foo.com:9443/hello



Expand Down Expand Up @@ -613,7 +637,7 @@ GET /hello
Host: test.com
--- error_code: 301
--- response_headers
Location: https://test.com:1984/hello
Location: https://test.com:9443/hello



Expand Down Expand Up @@ -763,7 +787,7 @@ POST /hello-https
--- more_headers
Host: test.com
--- response_headers
Location: https://test.com:1984/hello-https
Location: https://test.com:9443/hello-https
--- error_code: 308
--- no_error_log
[error]
Expand All @@ -776,7 +800,7 @@ GET /hello-https
--- more_headers
Host: test.com
--- response_headers
Location: https://test.com:1984/hello-https
Location: https://test.com:9443/hello-https
--- error_code: 301
--- no_error_log
[error]
Expand All @@ -789,7 +813,7 @@ HEAD /hello-https
--- more_headers
Host: test.com
--- response_headers
Location: https://test.com:1984/hello-https
Location: https://test.com:9443/hello-https
--- error_code: 301
--- no_error_log
[error]
Expand Down Expand Up @@ -1092,4 +1116,4 @@ Host: foo.com
X-Forwarded-Proto: http
--- error_code: 301
--- response_headers
Location: https://foo.com:1984/hello
Location: https://foo.com:9443/hello