-
Notifications
You must be signed in to change notification settings - Fork 26.4k
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
Triple protocol http1 upgrade support #14026
Conversation
3b48127
to
8238e49
Compare
...riple/src/main/java/org/apache/dubbo/rpc/protocol/tri/h12/HttpServerAfterUpgradeHandler.java
Outdated
Show resolved
Hide resolved
@oxsean PTAL |
8238e49
to
6d62b56
Compare
Please look into whether Application-Layer Protocol Negotiation is supported in addition to h2c: |
...pc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/TripleHttp2Protocol.java
Outdated
Show resolved
Hide resolved
Because you specified a http scheme (and not https), and asked curl to use HTTP/2, then curl will attempt to perform a HTTP/1.1 upgrade to HTTP/2, as can be seen from the logs. Typical HTTP/1.1 upgrades are performed using GET, not POST, notably the HTTP/1.1 upgrade to WebSocket. The server does not seem to be prepared to accept a POST with a body as an attempt to upgrade and replies with 413 because it does not expect a body. If you try a GET without body it will likely succeed. Alternatively, if you know that port 8889 accepts prior-knowledge clear-text HTTP/2 (that is, you can send directly HTTP/2 bytes to that port without having to perform a HTTP/1.1 upgrade), you can try:
`$ curl -v --http2-prior-knowledge -X POST -k -H "content-type:application/json" -d '["myFirstParameter"]' http://127.0.0.1:50052/org.apache.dubbo.springboot.demo.DemoService/sayHello
< HTTP/2 200
|
There is no documentation mentioned that methods other than GET are not supported. I think it might be because you haven't set maxContentLength --http2-prior-knowledge will skip negotiation, therefore this test is meaningless. |
The current common implementation of ALPN is through extended TLS. TLS processing in Dubbo is in ‘org.apache.dubbo.remoting.transport.netty4.NettyPortUnificationServerHandler’, before confirming the RPC protocol. Extensions to this implementation will have an impact on all RPC protocols. Could you please confirm whether support is needed at this level? |
I think so, but we need to confirm whether adding ALPN support cause a regression in protocols other than Triple, such as the Dubbo protocol. |
6d62b56
to
2ac7b09
Compare
You are right, thanks for the guidance. I have submitted a fix |
OK, I will submit a commit try to implement it. |
Submitted a commit. Please take a look, thanks. My test: |
LGTM, but have you tested no impact to dubbo protocol? |
Submitted some test cases to apache/dubbo-integration-cases#22
|
@AlbumenJ The problem has been solved, PTAL. |
.../main/java/org/apache/dubbo/remoting/transport/netty4/NettyPortUnificationServerHandler.java
Outdated
Show resolved
Hide resolved
...pc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/TripleHttp2Protocol.java
Outdated
Show resolved
Hide resolved
...pc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/TripleHttp2Protocol.java
Outdated
Show resolved
Hide resolved
...pc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/TripleHttp2Protocol.java
Outdated
Show resolved
Hide resolved
@walklown Sorry, I forgot commit the comments, please take a look. |
Submitted, thanks for your guidance ! |
.../main/java/org/apache/dubbo/remoting/transport/netty4/NettyPortUnificationServerHandler.java
Show resolved
Hide resolved
aab8c3e
to
6016080
Compare
...pc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/TripleHttp2Protocol.java
Outdated
Show resolved
Hide resolved
|
What is the purpose of the change
Triple protocol http1 upgrade support. For issue [Feature][3.3] Triple protocol http1 upgrade support
Brief changelog
Add Http2ServerUpgradeCodec to http1 channel handlers.
Verifying this change
https://github.com/walklown/dubbo-pr-test/tree/main/pr-test-triple