fix: redirect http to https but port not change by jwrookie · Pull Request #7065 · apache/apisix

jwrookie · 2022-05-17T11:03:43Z

Description

When http redirects to htpps, we use a correct port

Fixes #7011

Checklist

I have explained the need for this PR and the problem it solves
I have explained the changes or the new features added to this PR
I have added tests corresponding to this change
I have updated the documentation to reflect this change
I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

Signed-off-by: Wei Jiang <[email protected]>

spacewander · 2022-05-17T11:35:08Z

      connect: 60s
      read: 60s
      send: 60s
+  redirect_https_port: 8443   # the default port for use by HTTP redirects to HTTPS


We should use a structure like this:

redirect: https_port: ...

and comment it out as nobody wants to be redirected to 8443 by default.

spacewander · 2022-05-17T11:36:48Z

    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 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.

spacewander · 2022-05-17T11:37:44Z

+--- yaml_config
+apisix:
+    ssl:
+        enable: null


Let's add two more test:

use listen_port

use listen and get port in the array

Co-authored-by: 罗泽轩 <[email protected]>

Signed-off-by: Wei Jiang <[email protected]>

…ix/http_to_https

spacewander · 2022-05-17T14:04:18Z


-* 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`).


redirect_https_port should be updated

spacewander · 2022-05-17T14:04:25Z


-* 当开启 `http_to_https` 时，重定向 URL 中的端口将是 `X-Forwarded-Port` 请求头的值或服务器的端口。
+* 当开启 `http_to_https` 时，重定向 URL 中的端口将按如下顺序选取一个值（按优先级从高到低排列）
+  * 从配置文件（`conf/config.yaml`）中读取 `plugin_attr.redirect_https_port`。


spacewander · 2022-05-17T14:04:31Z



-=== TEST 18: redirect
+=== TEST 18: redirect(port using `plugin_attr.redirect_https_port`)


Co-authored-by: tzssangglass <[email protected]> Co-authored-by: Alex Zhang <[email protected]>

Co-authored-by: tzssangglass <[email protected]>

Signed-off-by: Wei Jiang <[email protected]>

…ix/http_to_https

Signed-off-by: Wei Jiang <[email protected]>

soulbird · 2022-05-18T01:42:59Z

+            end
+        end
+    end
+


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)

tokers · 2022-05-18T01:43:56Z

    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)


Where did you define the variable plugin_name?

here:

apisix/apisix/plugins/redirect.lua

Lines 65 to 67 in 6ebe027

local plugin_name = "redirect"

Was defined before :)

Signed-off-by: Wei Jiang <[email protected]>

spacewander · 2022-05-18T03:18:59Z

Let's merge master to make CI pass.

Signed-off-by: Wei Jiang <[email protected]> Co-authored-by: 罗泽轩 <[email protected]> Co-authored-by: tzssangglass <[email protected]> Co-authored-by: Alex Zhang <[email protected]>

* 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 <[email protected]> Co-authored-by: 罗泽轩 <[email protected]> Co-authored-by: tzssangglass <[email protected]> Co-authored-by: Alex Zhang <[email protected]>

jwrookie added 3 commits May 17, 2022 17:55

fix: redirect http to https but port not change

9617d41

Signed-off-by: Wei Jiang <[email protected]>

fix: redirect http to https but port not change

096129b

Signed-off-by: Wei Jiang <[email protected]>

fix: redirect http to https but port not change

06aa401

Signed-off-by: Wei Jiang <[email protected]>

spacewander reviewed May 17, 2022

View reviewed changes

Comment thread docs/en/latest/plugins/redirect.md Outdated

Comment thread docs/zh/latest/plugins/redirect.md Outdated

jwrookie and others added 3 commits May 17, 2022 20:53

Apply suggestions from code review

9b159e1

Co-authored-by: 罗泽轩 <[email protected]>

fix: review

c8d2f55

Signed-off-by: Wei Jiang <[email protected]>

Merge branch 'fix/http_to_https' of github.com:jwrookie/apisix into f…

155b5b3

…ix/http_to_https

spacewander previously approved these changes May 17, 2022

View reviewed changes

tzssangglass reviewed May 17, 2022

View reviewed changes

Comment thread apisix/plugins/redirect.lua Outdated

Comment thread apisix/plugins/redirect.lua Outdated

tokers reviewed May 18, 2022

View reviewed changes

Comment thread docs/en/latest/plugins/redirect.md Outdated

Apply suggestions from code review

6b38bfe

Co-authored-by: tzssangglass <[email protected]> Co-authored-by: Alex Zhang <[email protected]>

jwrookie dismissed spacewander’s stale review via 6b38bfe May 18, 2022 01:19

jwrookie and others added 3 commits May 18, 2022 09:25

Update apisix/plugins/redirect.lua

b9449f9

Co-authored-by: tzssangglass <[email protected]>

fix: review

1ab81b7

Signed-off-by: Wei Jiang <[email protected]>

Merge branch 'fix/http_to_https' of github.com:jwrookie/apisix into f…

9c151e5

…ix/http_to_https

spacewander previously approved these changes May 18, 2022

View reviewed changes

fix: review

d965ff8

Signed-off-by: Wei Jiang <[email protected]>

jwrookie dismissed spacewander’s stale review via d965ff8 May 18, 2022 01:40

soulbird reviewed May 18, 2022

View reviewed changes

tokers reviewed May 18, 2022

View reviewed changes

fix: review

215e66b

Signed-off-by: Wei Jiang <[email protected]>

Merge remote-tracking branch 'upstream/master' into fix/http_to_https

afe5124

spacewander approved these changes May 18, 2022

View reviewed changes

tzssangglass approved these changes May 18, 2022

View reviewed changes

jwrookie requested a review from tokers May 18, 2022 04:38

spacewander merged commit 435565d into apache:master May 18, 2022

spacewander mentioned this pull request Jun 6, 2022

request help: use https #7172

Closed



		=== TEST 18: redirect
		=== TEST 18: redirect(port using `plugin_attr.redirect_https_port`)

Uh oh!

Conversation

jwrookie commented May 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

soulbird May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacewander commented May 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jwrookie commented May 17, 2022 •

edited

Loading

soulbird May 18, 2022 •

edited

Loading