conn pool: use hostnames of endpoints as SNI values #34898
Conversation
Signed-off-by: Dmitriy Ilin <[email protected]>
|
Hi @dmitriyilin, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Dmitriy Ilin <[email protected]>
|
I see that "Publish and verify" check fails in other PRs as well. Is anything required from me? |
|
Just realized that "auto_san_validation" functionality is not consistent with the new mechanism. I'll fix it. |
| // Alternative mechanism for derivation of the SNI value. It uses endpoint's :ref:`hostname <envoy_v3_api_field_config.endpoint.v3.Endpoint.hostname>` as the value, if ``hostname`` is set. | ||
| // If set, takes precedence over ``auto_sni``. | ||
| bool auto_sni_from_upstream = 4; | ||
| } |
There was a problem hiding this comment.
It's a minor point, but for consistency I think I would require auto_ani to be set as well, vs. just override it. Optimally this would have been a oneof but it's too late for that.
There was a problem hiding this comment.
Just a note: oneof is now discouraged by the API style guide as of #30851
There was a problem hiding this comment.
@abeyad, @mattklein123, API style guide also discourages boolean overloads and advises usage of multiple fields with defined precedence.
There was a problem hiding this comment.
@dmitriyilin I think the way you have it set up now with precedence of auto_sni_from_upstream over auto_sni comports with my understanding of the API style guide.
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
Risk Level: Low
Testing: integration
Docs Changes: added information about new mechanism of SNI derivation
Release Notes: https://github.com/dmitriyilin/envoy/blob/b8e8a4537e537da66925f442cd5aeb45094cc3c9/changelogs/current.yaml#L377
Platform Specific Features: N/A
Fixes #15839