fix(redirect): set redirect server port when enable http_to_https

[kwanhur]

Description

Support set redirect server port and retrieve it from header x-forwarded-port, instead of only the well-known port 443.

1. When enable APISIX default https server, its port 9443, x-forwarded-port default value $server_port equals to 9443, redirect to 9443 could meet the expectation.
   So support to fetch the redirect server port on specified x-forwarded-port, it only works on enable http_to_https to true.

   • x-forwarded-port should be number and range 1 to 65535.

2. Fixed example Test Plugin code demo.

Fixes #4400

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)

[tokers]

@kwanhur IMHO specify a port is really hacky, what if users say we need to customize the host in the Location, instead of the one passed from the client? Do we need to add another ret_host field?

Why not extend the uri field, let it also be effective in the http_to_https mode, so that we can custom the Location.

[kwanhur]

From plugin redirect docs, rule 1

Only one of http_to_https, uri or regex_uri can be specified.

1. Under uri mode, it had supported to specify redirect port or host, like test case 15-16.

   • specify redirect port just update plugin's uri like(the other field is also the same solution)

 "plugins": {
        "redirect": {
        "uri": "https://$host:9443$request_uri",
        "ret_code": 301
     }
}

2. Under http_to_https mode, extend uri field, it seems to break the rule 1 and bring a big change.

To extend the other field, what about new attribute location?

• If empty, its default value https://$host$request_uri.
• If not empty, it goes to the same with uri.

[tokers]

Under http_to_https mode, extend uri field, it seems to break the rule 1 and bring a big change.

It's a big change but not broken? Let's hear more voices from the community :)

[leslie-tsang]

I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

Looks like a breaking change here, discussion in mail list first in needed.

[tokers]

@kwanhur Any updates?

[kwanhur]

@kwanhur Any updates?

Not yet. I'm not agree with reusing uri under http_to_https mode. It breaks the original design and had supported to specify redirect port under uri mode.

Hopefully hear other voices.

[tokers]

Indeed it's not a broken change, URI is an identifier for a resource, not a component of a URL (e.g., the path). Could you explain the details about the broken change?

Oh, by the way, maybe we should talk about this on the mailing list.

[kwanhur]

test case 21 shows the incompatibility

apisix/t/plugin/redirect.t

Line 487 in dbe7eee

=== TEST 21: wrong configure, enable http_to_https with uri

apisix/t/plugin/redirect.t

Lines 515 to 516 in dbe7eee

	--- response_body eval
	qr/error_msg":"failed to check the configuration of plugin redirect err: value should match only one schema, but matches both schemas 1 and 3/

[tokers]

Thanks, that really change the rule, but on the other hand, even if we remove this limitation, all configurations set before can still be applied successfully in the new version. @spacewander What do you think?

[kwanhur]

We also need to take some compatibility points on admin-api dashboard ingress etc when removing the limitation.

[tokers]

We also need to take some compatibility points on admin-api dashboard ingress etc when removing the limitation.

All of them are on the caller's side, could you give a specific example of the incompatibility?

[spacewander]

Thanks, that really change the rule, but on the other hand, even if we remove this limitation, all configurations set before can still be applied successfully in the new version. @spacewander What do you think?

I can't catch up with the point. The new field is optional and it is not a break change?

[tokers]

@kwanhur Another way is that we can check the X-Forwarded-Port in this plugin.

[kwanhur]

@kwanhur Another way is that we can check the X-Forwarded-Port in this plugin.

Ok, I'll move on at X-Forwarded-Port.

[kwanhur]

Have a glance at x-forwarded-port definition, it's default value $server_port which default is 9443, using it means that the default redirect server port will be $server_port, not now the well-known port 443.

Using it means need to update the existed test cases, please note that.

apisix/apisix/cli/ngx_tpl.lua

Line 668 in 4afc8a7

set $var_x_forwarded_port $server_port;

[kwanhur]

Just like the test log here, many cases failed with inconsistent port, X-Forwarded-Port from $server_port in t/APISIX.pm is 1984 but the original expected well-known 443.

How about updating these test cases' expected redirect server port? @tokers

[tokers]

Just like the test log here, many cases failed with inconsistent port, X-Forwarded-Port from $server_port in t/APISIX.pm is 1984 but the original expected well-known 443.

How about updating these test cases' expected redirect server port? @tokers

I think so, if the downstream really carries the X-Forwarded-Port.

[kwanhur]

Three scenarios will force x-forwarded-port value into well-known port 443:

1. non-number
2. less than and zero
3. greater than 65535

[kwanhur]

Workflow failed on t/plugin/api-breaker.t, not the plugin redirect.
The administrator, please trigger it to rerun the cases, thanks.

[tangzhenhuang]

Recently, I upgraded apisix with the latest code, and found that there was an error in converting http to https. When redirecting, I always bring a port 80. It should be caused by this change, because I am listening on port 80 and port 443, but ngx_tpl There is a piece of code in it that is "set $var_x_forwarded_port $server_port;", which means that the port from http to https is always the same. I don't think this change is very reasonable?

[kwanhur]

Oh, seems I got what you mean. The server port 80 and then enable http_to_https, it'll always redirect to https://host:80 then get 400 bad request. cc @tokers @spacewander @tzssangglass

We need to take more consideration about #6686 (comment)

[tzssangglass]

Can we set a default value for this case?
If there is no X-Forwarded-Port in clinet reuqest headers, then the redirect plugin use 443 as the default https port?

[kwanhur]

It has default as the server_port

apisix/apisix/cli/ngx_tpl.lua

Line 668 in 1b5c190

set $var_x_forwarded_port $server_port;

As mentioned before #6686 (comment)

[tzssangglass]

It has default as the server_port

I mean in the redirect plugin, if someone enable http_to_https and the client request doesn't pass X-Forwarded-Port, use 443 as the default port in Location and don't care about the server_port variable.

[tangzhenhuang]

In my opinion, it is definitely impossible to know the listening port of https for this domain name if it is not configured from http to https, so it can be considered that the port can be configured, but the default is 443?

[kwanhur]

As the conversation before #6686 (comment)

The original implement with an extra attribute ret_port, but no acceptable.

[spacewander]

Maybe we can read the correct port from config.yaml?

[atlassianss]

把这个set $var_x_forwarded_port $server_port; 改成set $var_x_forwarded_port 443;