feat: support listen http2 with plaintext by sysulq · Pull Request #3547 · apache/apisix

sysulq · 2021-02-07T07:19:37Z

What this PR does / why we need it:

Details can be found in #3496.

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

tokers · 2021-02-07T11:55:41Z

+        local node_listen = {{port = yaml_conf.apisix.node_listen}}
        yaml_conf.apisix.node_listen = node_listen
+    elseif type(yaml_conf.apisix.node_listen) == "table" then
+        if type(yaml_conf.apisix.node_listen[1]) == "number" then


Cannot capture your mean for this if.

I think we should check every item, what if the user mixes the integer and the table?

Only support port with number parameter?

spacewander · 2021-02-07T12:02:57Z

+
+
+
+=== TEST 5: support listen multiple ports with array


Need to add tests in t/grpc-proxy-test.sh.
And need to update the doc/grpc-proxy.md

spacewander · 2021-02-08T03:08:10Z

 # If the configured environment variable can't be found, an error will be thrown.
 apisix:
+  node_listen:
+    - port: 9080                     # APISIX listening port


We should provide the configuration separately in:

apisix/.travis/linux_openresty_common_runner.sh

Line 125 in 26a4ed4

res=`./bin/apisix start`

spacewander · 2021-02-08T03:09:15Z

+        local node_listen = {{port = yaml_conf.apisix.node_listen}}
        yaml_conf.apisix.node_listen = node_listen
+    elseif type(yaml_conf.apisix.node_listen) == "table" then
+        if type(yaml_conf.apisix.node_listen[1]) == "number" then


I think we should check every item, what if the user mixes the integer and the table?

spacewander · 2021-02-08T03:26:58Z

 * attention: the `scheme` of the route's upstream must be `grpc` or `grpcs`.
-* attention: APISIX use TLS‑encrypted HTTP/2 to expose gRPC service, so need to [config SSL certificate](https.md)
+* attention: APISIX use TLS‑encrypted HTTP/2 to expose gRPC service (default port 9443), so need to [config SSL certificate](https.md)
+* attention: APISIX also support to expose gRPC service with plaintext HTTP/2 (default port 9081), which do not need to config SSL certificate, usually used to proxy gRPC service in intranet environment


Need a configuration to show how we can do it.

spacewander · 2021-02-08T03:28:08Z

+    - port: 9080                     # APISIX listening port
+      enable_http2: false            # Disable to support HTTP proxy, nginx currently do not support HTTP and HTTP/2 with plaintext on the same port
+                                     # Details can be found in https://trac.nginx.org/nginx/ticket/816
+    - port: 9081                     # APISIX listening port for HTTP/2 with plaintext


Don't open a port for HTTP/2 by default.

spacewander · 2021-02-08T06:45:00Z

Need to resolve the conflict, so that CI can start.

tokers · 2021-02-08T07:13:37Z


 * attention: the `scheme` of the route's upstream must be `grpc` or `grpcs`.
 * attention: APISIX use TLS‑encrypted HTTP/2 to expose gRPC service, so need to [config SSL certificate](https.md)
+* attention: APISIX also support to expose gRPC service with plaintext HTTP/2, which do not need to config SSL certificate, usually used to proxy gRPC service in intranet environment


which do not need to config SSL certificate => which doesn't rely on ALPN/NPN extensions in TLS/SSL protocol.

Maybe this is a little hard to understand for user...? Maybe it's enough to mention in doc that this feature is supported, and show how to configure it?

What about "which doesn't rely on TLS/SSL"?

Alright, done :-)

tokers · 2021-02-08T07:16:21Z

+    elseif type(yaml_conf.apisix.node_listen) == "table" then
+        local node_listen = {}
+        for index, value in ipairs(yaml_conf.apisix.node_listen) do
+            if type(value) == "number" then


Is the number type really needed? I think the following way is enough.

node_listen: - port: 9080 - port: 9180 enable_http2: true

We need to be backward compatibility unless it is a bugfix.

I mean the element type inside node_listen, not the node_listen itself.

We can also be compatible with this format:

node_listen: - 9080 - 9081

sysulq changed the title ~~feat: support grpc proxy with insecure mode~~ feat: support listen http2 with plaintext Feb 7, 2021

tokers reviewed Feb 7, 2021

View reviewed changes

spacewander reviewed Feb 7, 2021

View reviewed changes

spacewander reviewed Feb 8, 2021

View reviewed changes

sysulq added 11 commits February 8, 2021 15:02

add new listen config for supporting plaintext grpc

9d4a82d

add test cases

1e25f2a

format

e082cb0

improve

1684eb5

fix

ae60601

improve doc, add test

760666b

improve doc

95b413a

doc: improve

91980e9

update

c2090f9

remove

a183628

update

6d6cbf7

sysulq force-pushed the feature/grpc-proxy-plaintext branch from e3f90e5 to 6d6cbf7 Compare February 8, 2021 07:04

remove dir

5be4e93

tokers reviewed Feb 8, 2021

View reviewed changes

spacewander approved these changes Feb 8, 2021

View reviewed changes

improve doc

854df6f

sysulq requested a review from tokers February 8, 2021 09:54

tokers approved these changes Feb 19, 2021

View reviewed changes

spacewander merged commit 5f4e8df into apache:master Feb 19, 2021

spacewander mentioned this pull request Mar 28, 2021

request help: nginx: [emerg] invalid port in "table:" of the "listen" directive in ... #3920

Closed

Uh oh!

Conversation

sysulq commented Feb 7, 2021

What this PR does / why we need it:

Pre-submission 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!

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!

spacewander commented Feb 8, 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!

sysulq Feb 8, 2021 • 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!

spacewander Feb 8, 2021 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sysulq Feb 8, 2021 •

edited

Loading

spacewander Feb 8, 2021 •

edited

Loading