You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "Menci (via GitHub)" <gi...@apache.org> on 2023/05/16 18:44:16 UTC
[PR] Add client ALPN support (brpc)
Menci opened a new pull request, #2251:
URL: https://github.com/apache/brpc/pull/2251
### What problem does this PR solve?
Issue Number: #1991
Problem Summary:
### What is changed and the side effects?
Changed:
In HTTP/2 standard a client should negotiate the protocol with server with ALPN. The client should send a list of protocols it support and the server will choose one. Normally `curl` sends `http1.1` and `h2`. A standard client should send H2 requests in TLS payload only if the server chooses `h2` in ALPN.
We tried a deployment with Nginx (TLS termination and `grpc_pass` to backend) in front of BRPC and found Nginx is treating BRPC client's H2 payload as H1 since there's no ALPN. It's confirmed that adding client side ALPN fixes it.
In this PR, if `options.alpn_protocols` is set in SSL options, the client will send ALPN extension during SSL handshake and check if the server responded with a acceptable protocol name. Normally we could set `alpn_protocols` to `{"h2"}` to only use H2. Note that the implementation will raise an error if the server returns no ALPN selection or unacceptable ALPN selection. Currently BRPC has no server side ALPN support (until #2102 is merged) so using this option with no HTTPS reverse proxy in front of BRPC server will not work.
```cpp
// Example to work with "h2:grpc" protocol
channel_options.mutable_ssl_options()->alpn_protocols = {"h2"};
```
Client side ALPN must be set manually since it requires server side ALPN support. By default it's unset and the behavior is like without this feature.
Side effects:
- Performance effects(性能影响): If the client side ALPN feature is used, a little more memory allocations are happened during client side SSL connection setup and handshake.
- Breaking backward compatibility(向后兼容性): No effect
---
### Check List:
- Please make sure your changes are compilable(请确保你的更改可以通过编译).
- When providing us with a new feature, it is best to add related tests(如果你向我们增加一个新的功能, 请添加相关测试).
- Please follow [Contributor Covenant Code of Conduct](../../master/CODE_OF_CONDUCT.md).(请遵循贡献者准则).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Add client ALPN support (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #2251:
URL: https://github.com/apache/brpc/pull/2251#issuecomment-1862026203
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Add client ALPN support (brpc)
Posted by "Menci (via GitHub)" <gi...@apache.org>.
Menci commented on PR #2251:
URL: https://github.com/apache/brpc/pull/2251#issuecomment-1867930599
谁可以来 merge 一下呢
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Add client ALPN support (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm merged PR #2251:
URL: https://github.com/apache/brpc/pull/2251
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Add client ALPN support (brpc)
Posted by "Menci (via GitHub)" <gi...@apache.org>.
Menci commented on code in PR #2251:
URL: https://github.com/apache/brpc/pull/2251#discussion_r1214173729
##########
src/brpc/details/mesalink_ssl_helper.cpp:
##########
@@ -280,6 +280,14 @@ SSL_CTX* CreateClientSSLContext(const ChannelSSLOptions& options) {
return NULL;
}
+ if (!options.alpn_protocols.empty()) {
+ std::vector<unsigned char> alpn_list;
+ if (!BuildALPNProtocolList(options.alpn_protocols, alpn_list)) {
+ return NULL;
+ }
+ SSL_CTX_set_alpn_protos(ssl_ctx.get(), alpn_list.data(), alpn_list.size());
Review Comment:
好问题，要不我就把 mesalink 的部分删掉吧，顺便我应该怎么测试 mesalink 呢
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Add client ALPN support (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #2251:
URL: https://github.com/apache/brpc/pull/2251#issuecomment-1605893202
LGTM
@Menci 能否更新一下用户文档
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Add client ALPN support (brpc)
Posted by "Menci (via GitHub)" <gi...@apache.org>.
Menci commented on PR #2251:
URL: https://github.com/apache/brpc/pull/2251#issuecomment-1555934266
The tests succeeded in my fork's CI. https://github.com/Menci/brpc/actions/runs/5030791101/jobs/9023448641
Maybe rerun it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Add client ALPN support (brpc)
Posted by "Menci (via GitHub)" <gi...@apache.org>.
Menci commented on PR #2251:
URL: https://github.com/apache/brpc/pull/2251#issuecomment-1848952485
@wwbmmm resolved 了 conflict，添加了文档~
看到 server side 的 ALPN 已经进了，思考了一下没有和它一样用 AdaptiveProtocolType，因为 client 端并不知道目标的服务器端是什么，所以就不做相关的检查了。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Add client ALPN support (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on code in PR #2251:
URL: https://github.com/apache/brpc/pull/2251#discussion_r1214037618
##########
src/brpc/details/ssl_helper.cpp:
##########
@@ -833,6 +841,35 @@ void Print(std::ostream& os, X509* cert, const char* sep) {
os << butil::StringPiece(bufp, len);
}
+bool BuildALPNProtocolList(
+ const std::vector<std::string>& alpn_protocols,
+ std::vector<unsigned char>& result
+) {
+ size_t alpn_list_length = 0;
+ for (const auto& alpn_protocol : alpn_protocols) {
+ if (alpn_protocol.size() > UCHAR_MAX) {
+ LOG(ERROR) << "Fail to build ALPN procotol list: "
Review Comment:
这里把alpn_protocol.size()打印一下？
##########
src/brpc/details/mesalink_ssl_helper.cpp:
##########
@@ -280,6 +280,14 @@ SSL_CTX* CreateClientSSLContext(const ChannelSSLOptions& options) {
return NULL;
}
+ if (!options.alpn_protocols.empty()) {
+ std::vector<unsigned char> alpn_list;
+ if (!BuildALPNProtocolList(options.alpn_protocols, alpn_list)) {
+ return NULL;
+ }
+ SSL_CTX_set_alpn_protos(ssl_ctx.get(), alpn_list.data(), alpn_list.size());
Review Comment:
mesalink是否支持这个功能？
https://github.com/mesalock-linux/mesalink/issues/36
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Add client ALPN support (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #2251:
URL: https://github.com/apache/brpc/pull/2251#issuecomment-1778632267
和master分支冲突了 @Menci
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org