feat: support configurating the node listening address by wayne-cheng · Pull Request #4856 · apache/apisix

wayne-cheng · 2021-08-19T10:47:34Z

Signed-off-by: wayne-cheng [email protected]

What this PR does / why we need it:

support configurating the node listening address,
The config.yaml example as below：

apisix:
  node_listen:
  - 9080
  - port: 9380
    ip: 127.0.0.2            # If not given, the default is 0.0.0.0
    allow_ssl: True        # if False, the ip connot be ued to config ssl.

Fix #4853

Pre-submission checklist:

Did you explain what problem does this PR solve? Or what new features have been added?
Have you added corresponding test cases?
Have you modified the corresponding document?
Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

wayne-cheng · 2021-08-19T10:50:27Z

@spacewander Hi, PTAL

nic-chen

@wayne-cheng tanks.need test cases

spacewander

A test under t/cli is in need.

spacewander · 2021-08-19T11:12:30Z

-        {% end %}
-        {% if ssl.enable then %}
+        listen {* item.ip *}:{* item.port *} default_server {% if enable_reuseport then %} reuseport {% end %} {% if item.enable_http2 then %} http2 {% end %};
+        {% if item.allow_ssl and ssl.enable then %}


Better to use enable_ssl.

ok，and should I explain these params in the doc FAQ.md ?

apisix: node_listen: - 9080 - port: 9380 ip: 127.0.0.1 # If not given, the default is `0.0.0.0`. enable_ssl: True # If `False`, the ip connot be ued to enable ssl. If not given, the default is `True`.

IMHO, document it in the conf/config-default.yaml as a comment is enough.

And the default value of "enable_ssl" should not be true, since 9080 doesn't have "enable_ssl", and it doesn't enable SSL by default.

Actually, we are planning to reduce the length of the current FAQ, let each answer go to suitable places, instead of putting them together.

It is better to modify the config apisix.ssl.listen_port ,like this:

apisix: ssl: listen_port: - ip: 127.0.0.1 port: 9443

The ssl config in the param apisix.node_listen[] could be confusing，for example:

apisix: node_listen: - ip: 127.0.0.2 port: 9082 enable_ssl: true - ip: 127.0.0.2 port: 9083 enable_ssl: false

What do you think? @spacewander

Agree. Personally, I prefer to use a new key "listen" since it is not just with port. Maybe we need to move "enable_http2" under "listen" too. Like this one:

apisix: ssl: # enable_http2: false # listen_port: ... listen: - ip: 127.0.0.1 port: 9443 enable_http2: true # per listen configuration

@wayne-cheng
What's your opinion?

@spacewander
I cannot agree any more. BTW, do I need to consider compatibility with the old configuration about ssl? There are something troublesome to deal with, the new config need to be merged with the old config.

Yes. We need to keep backward compatibility.

@spacewander I have updated the code, please take a look.

spacewander · 2021-08-20T14:45:30Z

-  node_listen: 9080                # APISIX listening port
+  node_listen:                     # APISIX listening port
+  - 9080
+  - ip: 127.0.0.2                  # Specific IP, If not set, the default value is `0.0.0.0`.


Please comment out the example

spacewander · 2021-08-20T14:45:45Z

-    listen_port: 9443
+    listen:
+    - 9443
+    - ip: 127.0.0.3               # Specific IP, If not set, the default value is `0.0.0.0`.


spacewander · 2021-08-20T14:47:41Z

+    - ip: 127.0.0.3               # Specific IP, If not set, the default value is `0.0.0.0`.
+      port: 9444
+      enable_http2: true          # If not set, the default value is `false`.
+    #port: 9081


Please remove the irrelative line

spacewander · 2021-08-20T14:54:40Z

+                local addr = ip .. ":" .. port
+
+                if ip_port_to_check[addr] == nil then
+                    table_insert(total_ssl_listen, {ip = ip, port = port, enable_http2 = yaml_conf.apisix.ssl.enable_http2})


We can refactor the repeated code in a function, which accepts enable_http2, enable_ipv6 and other arguments

Thx, the code is much clearer now

spacewander · 2021-08-20T14:57:34Z

+      enable_http2: true          # If not set, the default value is `false`.
+    #port: 9081
+    #enable_http2: true
+    #listen_port: 9443


Need to add a comment to show that this way still supports, but is no longer recommended

ok，I will comment it like this:

apisix: node_listen: 9080 # APISIX listening port # node_listen: # This style support multiple ports # - 9080 # - ip: 127.0.0.2 # Specific IP, If not set, the default value is `0.0.0.0`. # port: 9082 ssl: enable: true # listen: # APISIX listening port in https. # - 9443 # - ip: 127.0.0.3 # Specific IP, If not set, the default value is `0.0.0.0`. # port: 9444 # enable_http2: true # If not set, the default value is `false`. enable_http2: true # Not recommend: This parameter should be set via the `listen`. listen_port: 9443 # Not recommend: This parameter should be set via the `listen`.

I prefer to:

apisix: # node_listen: 9080 # APISIX listening port node_listen: # This style support multiple ports - 9080 # - ip: 127.0.0.2 # Specific IP, If not set, the default value is `0.0.0.0`. # port: 9082 ssl: enable: true listen: # APISIX listening port in https. - 9443 # - ip: 127.0.0.3 # Specific IP, If not set, the default value is `0.0.0.0`. # port: 9444 # enable_http2: true # If not set, the default value is `false`. enable_http2: true # Not recommend: This parameter should be set via the `listen`. # listen_port: 9443 # Not recommend: This parameter should be set via the `listen`.

Let's encourage users to use the new style.

spacewander · 2021-08-20T14:57:54Z

 #

 apisix:
-  node_listen: 9080                # APISIX listening port


wayne-cheng · 2021-08-21T04:09:58Z

@spacewander A new commit has been pushed, PTAL

spacewander · 2021-08-21T04:18:30Z

    server {
        {% for _, item in ipairs(node_listen) do %}
-        listen {* item.port *} default_server {% if enable_reuseport then %} reuseport {% end %} {% if item.enable_http2 then %} http2 {% end %};
+        listen {* item.ip *}:{* item.port *} default_server {% if enable_reuseport then %} reuseport {% end %};


http port also need to enable_http2

OK, and it means that the config apisix.node_listen[] also has the property enable_http2 , right?

apisix: # node_listen: 9080 # APISIX listening port node_listen: # This style support multiple ports - 9080 # - ip: 127.0.0.2 # Specific IP, If not set, the default value is `0.0.0.0`. # port: 9082 # enable_http2: true

it means that the config apisix.node_listen[] also has the property enable_http2 , right?

Yes.

spacewander · 2021-08-21T04:19:25Z


-        local node_listen = {{port = yaml_conf.apisix.node_listen}}
-        yaml_conf.apisix.node_listen = node_listen
+        local addr = ip .. ":" .. port


We can add scheme to this key so that we can reduce a branch

Since the config apisix.node_listen[] also has the property enable_http2, the branch can be reduced.
Port conflicts should be checked at the TCP layer, not the scheme http/https, so the scheme would not be add to this key.

wayne-cheng · 2021-08-22T03:29:06Z

The conifg apisix.port_admin also need to be compatible, now all changes have been pushed.
PTAL @spacewander

spacewander · 2021-08-23T01:11:38Z

    server {
        {%if https_admin then%}
-        listen {* port_admin *} ssl;
+        {% for _, item in ipairs(ssl.listen) do %}


I don't like the idea that the admin requires multiple ports.

Multiple admin ports are useless. You only need to set one in the API client.

I did not change the config port_admin. It is still the same as before, only one port_admin can be set.
The idea is to match the specific ip in the config node_listen，not for multiple ports.

spacewander · 2021-08-23T01:12:37Z

Please don't change the port_admin, thanks!

wayne-cheng · 2021-08-23T01:42:33Z

@spacewander
Please see this after that code:

{% if item.only_admin then %}
listen {* item.ip *}:{* item.port *} ssl;
{% end %}

In my environment, the apisix bind the VIP. If the port_admin is bound to all IP addresses, those programs with the same port but different IP will not work.

spacewander · 2021-08-23T02:05:32Z

Maybe we can add an "admin_listen" section like what we have done with ssl.listen_port? We can do it in the next PR. This PR is big enough and should pass the CI first.

wayne-cheng · 2021-08-23T02:17:08Z

Agree，Of course this way is better. Next, Do I undo the changes about port_admin and push commit again？

spacewander · 2021-08-23T02:29:22Z

Yes

wayne-cheng · 2021-08-23T03:17:24Z

OK，PTAL @spacewander

wayne-cheng · 2021-08-23T08:16:22Z

@spacewander
The config apisix.node_listen in previous config-default.yaml is the type number, but the new has been the type table.
If a user still uses the type number to set the config apisix.node_listen like this:

apisix:
  node_listen: 9081

The error will occur when exec make init，and it also appeared in the CI test:

./bin/apisix init
/usr/local/openresty/luajit/bin/luajit ./apisix/cli/apisix.lua init
failed to read local yaml config of apisix: failed to merge, path[apisix->ssl->listen] expect: table, but got: number
make: *** [Makefile:127: init] Error 1

This bug should have existed before, but it was missed, since the previous file config-default.yaml used the number type.
Should I fix the function merge_conf in the code file apisix/cli/file.lua(+151) ?

 -- ...           
            elseif type(base[key]) ~= type_val then
                if (ppath == "nginx_config" or str_sub(ppath, 1, 14) == "nginx_config->") and
                    (type_val == "number" or type_val == "string")
                then
                    base[key] = val
                else
                    local path = ppath == "" and key or ppath .. "->" .. key
                    return nil, "failed to merge, path[" .. path ..  "] expect: " ..
                                type(base[key]) .. ", but got: " .. type_val
                end
            else
                base[key] = val
            end
-- ...

spacewander · 2021-08-23T10:26:47Z

Thanks for your report. Let's add it to the allow list.

I think you can submit it via a separate PR. I can merge it first, so that you are no longer a first-time contributor, then you can see the CI result without approval.

Signed-off-by: wayne-cheng <[email protected]>

spacewander · 2021-08-29T11:16:11Z

    enable: true
-    enable_http2: true
-    listen_port: 9443
+    # listen: 9443                # APISIX listening port in https.


I think we should only support table format for the new configuration, this will make it easier to understand.

ok，I have removed it in the config-default.yaml.

Signed-off-by: wayne-cheng <[email protected]>

spacewander · 2021-08-30T02:40:30Z

        return true
    end

+    if path == "apisix->ssl->listen" and type_val == "number" then


Could we remove it now?

spacewander · 2021-08-30T02:40:47Z

+
+    local ssl_listen = {}
+    -- listen in https, support multiple ports, support specific IP
+    if type(yaml_conf.apisix.ssl.listen) == "number" then


Got it, the related code has also been removed.

…format Signed-off-by: wayne-cheng <[email protected]>

wayne-cheng · 2021-09-02T03:44:41Z

@spacewander Thanks for your patience to review, I'll submit next PR to add the config admin_listen.

jagerzhang · 2021-09-26T11:08:51Z

Hi, When will this feature be released to the docker image?

nic-chen reviewed Aug 19, 2021

View reviewed changes

spacewander changed the title ~~feat:support configurating the node listening address (#4853)~~ feat: support configurating the node listening address (#4853) Aug 19, 2021

spacewander requested changes Aug 19, 2021

View reviewed changes

spacewander reviewed Aug 20, 2021

View reviewed changes

spacewander reviewed Aug 21, 2021

View reviewed changes

spacewander reviewed Aug 23, 2021

View reviewed changes

wayne-cheng mentioned this pull request Aug 24, 2021

fix: validation during merging node_listen configuration #4881

Merged

4 tasks

wayne-cheng added 8 commits August 27, 2021 17:07

feat:support configurating the node listening address (apache#4853)

c0cb6c4

Signed-off-by: wayne-cheng <[email protected]>

support specific ip about ssl

bc80dd8

Signed-off-by: wayne-cheng <[email protected]>

refactor repeated code and comment config-default.yaml

36349f2

add enable_http2 config in apisix.node_listen

51725ba

Signed-off-by: wayne-cheng <[email protected]>

compatibility with the config apisix.port_admin

7183796

Signed-off-by: wayne-cheng <[email protected]>

undo the changes about port_admin

72f0b9f

Signed-off-by: wayne-cheng <[email protected]>

compatible with the ssl old config enbale_http2

589eed8

Signed-off-by: wayne-cheng <[email protected]>

add apisix.ssl.listen in merge_conf validation

8fbce3c

Signed-off-by: wayne-cheng <[email protected]>

wayne-cheng force-pushed the 4853-2.8-zhengwei branch from 8f69513 to 8fbce3c Compare August 27, 2021 09:23

wayne-cheng added 2 commits August 27, 2021 17:51

adjust the port conflict test case for new listen config

8427e03

Signed-off-by: wayne-cheng <[email protected]>

adjust the node_listen port test case for new listen config

e645a1d

add a little delay in TEST-6 of plugins-reload.t

19fd1fa

spacewander approved these changes Aug 29, 2021

View reviewed changes

remove apisix.ssl.listen number format in config-default.yaml

8de3c80

Signed-off-by: wayne-cheng <[email protected]>

spacewander reviewed Aug 30, 2021

View reviewed changes

remove the code about the config apisix.ssl.listen supporting number …

b3a4e1b

…format Signed-off-by: wayne-cheng <[email protected]>

spacewander approved these changes Aug 30, 2021

View reviewed changes

tzssangglass reviewed Sep 1, 2021

View reviewed changes

Comment thread t/cli/test_main.sh

tzssangglass approved these changes Sep 1, 2021

View reviewed changes

spacewander changed the title ~~feat: support configurating the node listening address (#4853)~~ feat: support configurating the node listening address Sep 2, 2021

spacewander merged commit c72700e into apache:master Sep 2, 2021

wayne-cheng deleted the 4853-2.8-zhengwei branch September 2, 2021 03:50

wayne-cheng mentioned this pull request Sep 2, 2021

feat: support configuring admin specific IP #4967

Merged

4 tasks

Uh oh!

Conversation

wayne-cheng commented Aug 19, 2021 • edited by spacewander Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does / why we need it:

Pre-submission checklist:

Uh oh!

wayne-cheng commented Aug 19, 2021

Uh oh!

nic-chen left a comment

Choose a reason for hiding this comment

Uh oh!

spacewander left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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!

wayne-cheng commented Aug 21, 2021

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!

Choose a reason for hiding this comment

Uh oh!

wayne-cheng commented Aug 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wayne-cheng commented Aug 19, 2021 •

edited by spacewander

Loading