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
28 changes: 26 additions & 2 deletions apisix/plugins/redirect.lua
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,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 +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,
Copy link
Member

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:

local attr = plugin.plugin_attr(plugin_name)

We need to use it instead.

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

"plugin_attr",
"redirect_https_port")

if not ret_port then
local ssl = core.table.try_read_attr(local_conf,
"apisix",
"ssl")
jwrookie marked this conversation as resolved.
Show resolved Hide resolved
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
1 change: 1 addition & 0 deletions conf/config-default.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

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.

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

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.
jwrookie marked this conversation as resolved.
Show resolved Hide resolved
* If `apisix.ssl` is enabling, read `apisix.ssl.listen_port` first, and if it does not exist, read `apisix.ssl.listen` and select a random port from it.
jwrookie marked this conversation as resolved.
Show resolved Hide resolved
* 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 中的端口将按如下顺序选取一个值(按优先级从高到低排列)
* 从配置文件中读取 `plugin_attr.redirect_https_port`。
jwrookie marked this conversation as resolved.
Show resolved Hide resolved
* 如果 `apisix.ssl` 处于开启状态,先读取 `apisix.ssl.listen_port`,如果没有,再读取 `apisix.ssl.listen` 并从中随机选一个 `port`。
* 使用 443 作为默认 `https port`。

:::

Expand Down
Loading