conn pool: use hostnames of endpoints as SNI values#35380
conn pool: use hostnames of endpoints as SNI values#35380dmitriyilin wants to merge 6 commits intoenvoyproxy:mainfrom
Conversation
…nvoyproxy#34898)" (envoyproxy#35212)" This reverts commit d84f707. Signed-off-by: Dmitriy Ilin <dmitry.m.ilyin@gmail.com>
…o remove flakiness Signed-off-by: Dmitriy Ilin <dmitry.m.ilyin@gmail.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Dmitriy Ilin <dmitry.m.ilyin@gmail.com>
|
fyi, I took "ProxyFilterIntegrationTest, UpstreamTlsInvalidSAN" as basis for the test that was showing flaky behavior "AutoSniIntegrationTest, AutoSniFromUpstreamAndAutoSanValidationFailureTest". |
Signed-off-by: Dmitriy Ilin <dmitry.m.ilyin@gmail.com>
Signed-off-by: Dmitriy Ilin <dmitry.m.ilyin@gmail.com>
Signed-off-by: Dmitriy Ilin <dmitry.m.ilyin@gmail.com>
|
@adisuissa , could you "rereview" this change? |
|
Assigning Matt who also reviewed #34898. Can you provide more info about how the flaky test was fixed? |
| !upstream_http_protocol_options->auto_sni_from_upstream()) { | ||
| return transport_socket_options; | ||
| } | ||
| const absl::string_view hostname = host->hostname(); |
There was a problem hiding this comment.
I believe host->hostname() can include :port if it was resolved by the Dynamic Forward Proxy. You can end up with invalid SNI.
There was a problem hiding this comment.
According to the comments hostname is a hostname and there is also address available in the HostDescription, so it would be very confusing to also include a port in hostname. I also can not spot where a port can be added to a hostname of a HostImpl or a LogicalHost.
I had used As flaky test validates behaviour in case of failure, I've added explicit cleanup: |
|
@yanavlasov, @adisuissa, @mattklein123, just a reminder |
|
Hey! This is exactly the feature I'm needing. Do you think it's possible to do the same using current Envoy's release configuration? |
It's a new functionality, so new config is needed. |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
github-actions
Bot
added
the
stale
|
@yanavlasov, @adisuissa, @mattklein123 do you see any issues with the changes? |
github-actions
Bot
removed
the
stale
|
@mattklein123 @yanavlasov @adisuissa any feedback? |
|
I too would like this to be merged |
|
Can you please merge main and I can take a look? /wait |
|
@ggreenway , thanks for the info. I am closing this PR then |
8 participants
Commit Message: conn pool: use hostnames of endpoints as SNI values
Additional Description: optional support for usage of upstream cluster endpoints' hostnames as SNI values. This PR is the successor of 34898 with expected improvement of test flakiness. Original PR was reverted by 35212.
Risk Level: Low
Testing: integration
Docs Changes: added information about new mechanism of SNI derivation
Release Notes: https://github.com/dmitriyilin/envoy/blob/0f70012a2383dd56cb070199664be44dbd8bbd93/changelogs/current.yaml#L16
Platform Specific Features: N/A
Fixes #15839