Add client ALPN support #2251
Merged
Add client ALPN support #2251
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The tests succeeded in my fork's CI. https://github.com/Menci/brpc/actions/runs/5030791101/jobs/9023448641 Maybe rerun it? |
wwbmmm
reviewed
Jun 2, 2023
| 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: " |
There was a problem hiding this comment.
这里把alpn_protocol.size()打印一下？
| if (!BuildALPNProtocolList(options.alpn_protocols, alpn_list)) { | ||
| return NULL; | ||
| } | ||
| SSL_CTX_set_alpn_protos(ssl_ctx.get(), alpn_list.data(), alpn_list.size()); |
There was a problem hiding this comment.
mesalink是否支持这个功能？
mesalock-linux/mesalink#36
There was a problem hiding this comment.
好问题，要不我就把 mesalink 的部分删掉吧，顺便我应该怎么测试 mesalink 呢
|
LGTM |
|
和master分支冲突了 @Menci |
|
@wwbmmm resolved 了 conflict，添加了文档~ 看到 server side 的 ALPN 已经进了，思考了一下没有和它一样用 AdaptiveProtocolType，因为 client 端并不知道目标的服务器端是什么，所以就不做相关的检查了。 |
|
LGTM |
|
谁可以来 merge 一下呢 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
curlsendshttp1.1andh2. A standard client should send H2 requests in TLS payload only if the server choosesh2in ALPN.We tried a deployment with Nginx (TLS termination and
grpc_passto 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_protocolsis 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 setalpn_protocolsto{"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.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: