hds: add support for cluster transport_socket_matches.#12905
hds: add support for cluster transport_socket_matches.#12905htuch merged 27 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Drew S. Ortega <drewortega@google.com>
Signed-off-by: Drew S. Ortega <drewortega@google.com>
Signed-off-by: Drew S. Ortega <drewortega@google.com>
Signed-off-by: Drew S. Ortega <drewortega@google.com>
Signed-off-by: Drew S. Ortega <drewortega@google.com>
Signed-off-by: Drew S. Ortega <drewortega@google.com>
Signed-off-by: Drew S. Ortega <drewortega@google.com>
Signed-off-by: Drew S. Ortega <drewortega@google.com>
Signed-off-by: Drew S. Ortega <drewortega@google.com>
Signed-off-by: Drew S. Ortega <drewortega@google.com>
Signed-off-by: Drew S. Ortega <drewortega@google.com>
|
Retrying Azure Pipelines, to retry CircleCI checks, use |
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
Generally looks great, a few items of feedback.
/wait
| tls_context, factory_context_); | ||
| static Stats::Scope* upstream_stats_store = new Stats::IsolatedStoreImpl(); | ||
| return std::make_unique<Extensions::TransportSockets::Tls::ServerSslSocketFactory>( | ||
| std::move(cfg), context_manager_, *upstream_stats_store, std::vector<std::string>{}); |
There was a problem hiding this comment.
Possibly a refactor candidate with the same method in transport_socket_match_integration_test.cc.
There was a problem hiding this comment.
Hi Harvey, I went ahead and did a small refactor, moving this function up into HttpIntegration test, because I noticed a lot of tests that inherit from this class use this. You mentioned specifically transport_socket_match_integration_test so I reflected this change in this test as well. However, in my search I also found the following tests or files use the same or similar function:
alpn_selection_integration_test.cc
sds_dynamic_integration_test.cc and sds_static_integration_test.cc
xfcc_integration_test.cc
I think fixing all of these is a lot of extra code in this PR, would you like me to submit another PR that refactors this?
There was a problem hiding this comment.
Separate PR would be a bit cleaner, but up to you, the present PR isn't too big right now.
There was a problem hiding this comment.
@htuch I think I'll do a separate PR, after even more investigation it seems like there is a bit more cleanup needed here than I originally thought, because there are a few more places where this code exists. There is an available implementation in ssl_utility.cc it seems, and a few more tests use their own implementation. I want to create a PR that localizes all of these calls to the ssl_utility.cc version, and the HttpIntegrationTest version I just added directing its call to the ssl_utility version using its member variables.
Signed-off-by: Drew S. Ortega <drewortega@google.com>
Signed-off-by: Drew S. Ortega <drewortega@google.com>
Signed-off-by: Drew S. Ortega <drewortega@google.com>
Signed-off-by: Drew S. Ortega <drewortega@google.com>
Signed-off-by: Drew S. Ortega <drewortega@google.com>
Signed-off-by: Drew S. Ortega <drewortega@google.com>
Signed-off-by: Drew S. Ortega <drewortega@google.com>
Signed-off-by: Drew S. Ortega <drewortega@google.com>
Signed-off-by: Drew S. Ortega <drewortega@google.com>
htuch
left a comment
htuch
left a comment
There was a problem hiding this comment.
LGTM, just one question. Can you run all tests with https://github.com/envoyproxy/envoy/tree/master/test/integration#reproducing-test-flakes to validate we're not adding new flakes? Thanks.
Signed-off-by: Drew S. Ortega <drewortega@google.com>
3 participants
Commit Message: hds: add transport_socket_matches to HDS specifier
Additional Description:
In order to support TLS in a health check connection, a TransportSocket proto must be matched to build the proper TLS connection factory. These are matched by the repeated field transport_socket_matches in the cluster proto, which HDS is currently leaving blank when building this proto. As a result, there is not way to specify a TLS transport socket or any transport socket listed in the docs.
This change adds the transport_socket_matches field to the HDS health check specifier, and adds it to the Cluster config generated by HDS, to support transport socket matches per-health check.
Risk Level: Low
Testing: HDS Unit tests and integration tests pass. Added unit test to test that the transport socket matcher receives the correct fields. Added two integration tests with a TLS configuration, one over HTTP and one over HTTP/2.
Docs Changes: Added comments about the new transport_socket_maches field in the HDS specifier proto.
Release Notes: Included