quiche: support downstream using SDS by danzh2010 · Pull Request #15821 · envoyproxy/envoy

danzh2010 · 2021-04-02T04:24:21Z

Commit Message: Update quic server transport socket factory upon SDS updates. Add stats for SDS updates. Add checking of nullptr

Additional Description: fix UDP listener to register xDS with server init_manager.
Risk Level: low
Testing: enable downstream HTTP3 in sds_dynamic_integration_test.cc

Fixes #15034
Part of #12930 #2557

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk · 2021-04-05T12:52:42Z

I'll wait until you have CI sorted.
Since you mentioned offline this is downstream only can you add a TODO(14829) somewhere appropriate for upstream support?

lambdai · 2021-04-05T17:30:43Z

source/common/quic/quic_transport_socket_factory.h

+  QuicTransportSocketFactoryStats stats_;
+
+private:
+  std::unique_ptr<Ssl::ContextConfig> context_config_;


I don't have the full picture regarding the client context. However, if you can put context_config_ to derived QuicServerTransportSocketFactory and QuicClientTransportSocketFactory, the dynamic cast can be eliminated.

Actually I moved it from subclasses to the base class so that they can have the SDS update callback setup in the base class :)
Doing so is just to have the same behavior on both endpoints. Indeed it brings up the need of dynamic_cast in subclasses. I don't have strong preference for either way.

After thinking about it more, I changed the ownership to subclasses as the client subclass may need to do extra work in the future. Thus avoiding the dynamic_cast. Thanks for pointing out the issue here!

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2021-04-08T00:17:22Z

/retest

repokitteh-read-only · 2021-04-08T00:17:25Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15821 (comment) was created by @danzh2010.

see: more, trace.

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk

main merge should fix verify_examples, sorry for the breakage

test/config/utility.cc

alyssawilk · 2021-04-08T12:59:02Z

test/integration/sds_dynamic_integration_test.cc

+getSdsTestsParams(bool test_quic) {
+  std::vector<std::tuple<Network::Address::IpVersion, Grpc::ClientType, bool>> ret;
+  for (auto ip_version : TestEnvironment::getIpVersionsForTest()) {
+    for (auto grpc_type : {Grpc::ClientType::EnvoyGrpc, Grpc::ClientType::GoogleGrpc}) {


Let's add a utility for this, like getIpVersionsFor test, which getsGrpcVersionsForTest based on what's compiled in.

done, also updated grpc integration tests to use that.

alyssawilk · 2021-04-08T13:00:24Z

test/integration/sds_dynamic_integration_test.cc

+
+      ret.push_back({ip_version, grpc_type, false});
+      if (test_quic) {
+#ifdef ENVOY_ENABLE_QUIC


I also think we should have one for client / upstream protocols but I can take that on, since I didn't add the correct utility over in https://github.com/envoyproxy/envoy/pull/15853/files

test/integration/sds_dynamic_integration_test.cc

alyssawilk · 2021-04-08T13:04:56Z

test/integration/sds_dynamic_integration_test.cc

+            transport_socket_config.mutable_downstream_tls_context()->mutable_common_tls_context();
+        configInlinedCerts(common_tls_context);
+        transport_socket->set_name("envoy.transport_sockets.quic");
+        transport_socket->mutable_typed_config()->PackFrom(transport_socket_config);


I feel like we're getting the quic vs tls stuff in a lot of places and I'd like to reduce that. Can you think of something cleaner? Passing in the context to a lambda and having it fill in the right one? Setting the h2 tls context by default and having a convertIfHttp3 function which moved it over and set the correct transport socket name?

I added a utility function in ConfigHelper to distinguish ssl v.s quic.

Should we use the new test helper here?

We should! Done

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk

LGTM modulo that one typo. Passing to Snow for second pass

alyssawilk · 2021-04-12T13:43:02Z

test/config/utility.cc

  }
 }

+void ConfigHelper::configDownstreamTransportSocketWitTls(


WitTls -> WithTls

Signed-off-by: Dan Zhang <danzh@google.com>

snowp

Overall looks reasonable to me, just a few suggestions/questions

snowp · 2021-04-13T01:15:01Z

source/common/quic/quic_transport_socket_factory.h

-  const Ssl::ClientContextConfig& clientContextConfig() const { return *config_; }
+  const Ssl::ClientContextConfig& clientContextConfig() const { return config_; }


I think an alternative way to do this would be to have this getter do

ASSERT(dynamic_cast<...> != nullptr); return static_cast<...>(contextConfig());

That way we avoid storing a separate ref on the derived classes while still avoiding a dynamic cast in prod builds?

snowp · 2021-04-13T01:23:18Z

source/common/quic/quic_transport_socket_factory.h

+  QuicTransportSocketFactoryStats stats_;
+
+private:
+  std::unique_ptr<Ssl::ContextConfig> context_config_;


It shouldn't be const. setSecretUpdateCallback() is non-const method. But all these stuff has been moved to subclasses.

I was thinking more about const unique_ptr, not unique_ptr<const ...>, since the pointer doesn't change

snowp · 2021-04-13T01:32:51Z

test/config/utility.cc

+      transport_socket->set_name("envoy.transport_sockets.quic");
+      envoy::extensions::transport_sockets::quic::v3::QuicDownstreamTransport
+          quic_transport_socket_config;
+      fill_tls_stuff(*quic_transport_socket_config.mutable_downstream_tls_context()


maybe make this name a bit more explicit? configureTlsContext?

snowp · 2021-04-13T01:37:08Z

test/integration/sds_dynamic_integration_test.cc

 public:
  SdsDynamicIntegrationBaseTest()
-      : HttpIntegrationTest(Http::CodecClient::Type::HTTP1, ipVersion()),
+      : HttpIntegrationTest(Http::CodecClient::Type::HTTP1, std::get<0>(GetParam())),


seems easier to read when this was ipVersion()?

ipVersion() is a base class member. We haven't construct the base class here yet. Switched to use TestParams.ip_version for readability.

snowp · 2021-04-13T01:37:46Z

test/integration/sds_dynamic_integration_test.cc

+    : public Grpc::BaseGrpcClientIntegrationParamTest,
+      public HttpIntegrationTest,
+      public testing::TestWithParam<
+          std::tuple<Network::Address::IpVersion, Grpc::ClientType, bool>> {


Maybe document what the gRPC test parameter is used for? Also what the bool refer to

I change the tuple to a struct. Hope it helps readability.

snowp · 2021-04-13T01:42:12Z

test/integration/sds_dynamic_integration_test.cc

+            transport_socket_config.mutable_downstream_tls_context()->mutable_common_tls_context();
+        configInlinedCerts(common_tls_context);
+        transport_socket->set_name("envoy.transport_sockets.quic");
+        transport_socket->mutable_typed_config()->PackFrom(transport_socket_config);


Should we use the new test helper here?

Signed-off-by: Dan Zhang <danzh@google.com>

snowp · 2021-04-13T21:36:20Z

source/common/quic/quic_transport_socket_factory.h

-      stats_.context_config_update_by_sds_.inc();
+      : stats_(generateStats(store, perspective)) {
+    context_config.setSecretUpdateCallback([this]() {
+      // The callback also triggers updating config_ with the new secret.


also updates

snowp · 2021-04-13T21:37:10Z

source/common/quic/quic_transport_socket_factory.h

-      // TODO(danzh) Client transport socket factory may also need to update quic crypto.
-      stats_.context_config_update_by_sds_.inc();
+      : stats_(generateStats(store, perspective)) {
+    context_config.setSecretUpdateCallback([this]() {


will this ever be immediately triggered? we'd call a virtual function in the ctor in that case

Yes, if triggerred in the callstack, it will call base class version of the virtual function which makes sense as the derived class hasn't use context_config to setup anything yet. Is this not desired in Envoy?

https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors

It tends to lead to surprising behavior and is often not resilient to refactors, so I would avoid it if possible

Got it! I added initialize() to add the callback after construction.

snowp · 2021-04-13T21:40:51Z

test/integration/sds_dynamic_integration_test.cc

-      std::get<2>(p.param) ? "UsesQuic" : "UsesTcp");
+struct TestParams {
+  Network::Address::IpVersion ip_version;
+  Grpc::ClientType grpc_type;


sds_grpc_type? to make it clear that this relates to the SDS client, not the downstream client of the integration test

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2021-04-15T14:27:52Z

/retest

repokitteh-read-only · 2021-04-15T14:27:57Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15821 (comment) was created by @danzh2010.

see: more, trace.

danzh2010 · 2021-04-16T16:11:20Z

Ping @snowp

Signed-off-by: Dan Zhang <danzh@google.com>

snowp

Thanks this is looking pretty good! A few more comments

source/common/quic/quic_transport_socket_factory.h

snowp · 2021-04-19T14:28:09Z

source/common/quic/quic_transport_socket_factory.h

+    if (!config_->isReady()) {
+      ENVOY_LOG(warn, "SDS hasn't finished updating Ssl context config yet.");
+      stats_.downstream_context_secrets_not_ready_.inc();
+      return {};
+    }
+    return config_->tlsCertificates();


For my understanding when would this happen? Do we not block the usage of this on the SDS config being ready?

I think we do block. But I saw similar logic in SSL socket to create a dummy socket so might as well added this.

test/common/http/conn_pool_grid_test.cc

snowp · 2021-04-19T14:30:09Z

test/common/quic/envoy_quic_proof_source_test.cc

@@ -1,5 +1,6 @@
 #include <memory>
 #include <string>
+#include <type_traits>


I don't see anything from type_traits being introduced in this PR, can this be removed?

snowp · 2021-04-19T14:32:52Z

test/integration/sds_dynamic_integration_test.cc

+    common_tls_context.add_alpn_protocols(downstream_protocol_ == Http::CodecClient::Type::HTTP1
+                                              ? Http::Utility::AlpnNames::get().Http11
+                                              : Http::Utility::AlpnNames::get().Http3);


Should this use test_quic instead? I think this logic just matches the logic above that uses test_quic? Not clear without that why we do this remapping

Yes, test_quic_ and downstream_protocol_ are consistent. Switched to use test_quic_ as other places in this test do.

snowp · 2021-04-19T14:34:06Z

test/integration/sds_dynamic_integration_test.cc

+                               ? "quic_server_transport_socket_factory.context_config_update_by_sds"
+                               : "server_ssl_socket_factory.ssl_context_update_by_sds"),


This makes me wonder whether the stats name should match? ie make the stat introduced in this PR ssl_context_update_by_sds? Or does that not fit well with the quic naming of things?

ssl_context is a SslSocket thing. Quic transport socket factory uses its config directly instead of generating an ssl_context object. So I think use ssl_context in the stats name is confusing.

snowp · 2021-04-19T14:34:56Z

test/integration/sds_dynamic_integration_test.cc

    client_ssl_ctx_ = createClientSslTransportSocketFactory({}, context_manager_, *api_);
  }

+  void configInlinedCerts(


nit config -> configure

snowp · 2021-04-19T14:35:31Z

test/integration/sds_dynamic_integration_test.cc


+  void configInlinedCerts(
+      envoy::extensions::transport_sockets::tls::v3::CommonTlsContext* common_tls_context) {
+    common_tls_context->add_alpn_protocols(Http::Utility::AlpnNames::get().Http11);


Maybe pull this out or rename function to make it clear that we're also setting the ALPN?

moved it out.

Signed-off-by: Dan Zhang <danzh@google.com>

snowp

LGTM! I see @antoniovicente assigned himself so I'll let him take a look as well

antoniovicente · 2021-04-20T16:56:49Z

LGTM! I see @antoniovicente assigned himself so I'll let him take a look as well

I mostly added myself as a reviewer due to Alyssa being OOO this week. I did a quick pass and think that the changes in this PR look good.

Signed-off-by: Dan Zhang <danzh@google.com> Signed-off-by: Gokul Nair <gnair@twitter.com>

danzh1989 added 7 commits February 19, 2021 01:00

add secret update callback

720858b

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into sdscallback

7b83f18

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into sdscallback

90a48c6

Signed-off-by: Dan Zhang <danzh@google.com>

build sds integration test

5152488

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into sdscallback

65d0903

Signed-off-by: Dan Zhang <danzh@google.com>

remove client connection factory

0ceea97

Signed-off-by: Dan Zhang <danzh@google.com>

enable quic downstream with SDS

d4acccd

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 requested review from asraa, ggreenway and lizan as code owners April 2, 2021 04:24

danzh1989 added 2 commits April 2, 2021 00:25

change quic transport socket factory interface

e9a50b8

Signed-off-by: Dan Zhang <danzh@google.com>

fix unit tests failure because of interface change

0162b9f

Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123 assigned alyssawilk Apr 2, 2021

fix protocol test

28824b6

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk added the waiting label Apr 5, 2021

lambdai reviewed Apr 5, 2021

View reviewed changes

danzh1989 added 2 commits April 7, 2021 13:14

Merge branch 'master' into sdscallback

86a40b2

Signed-off-by: Dan Zhang <danzh@google.com>

fix udp config

e07ed93

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot removed the waiting label Apr 7, 2021

fix clang-tidy

ae87d6f

Signed-off-by: Dan Zhang <danzh@google.com>

checkENVOY_GOOGLE_GRPC

6a91c0a

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk reviewed Apr 8, 2021

View reviewed changes

danzh1989 added 3 commits April 8, 2021 15:52

more utility helpers

0dc3250

Signed-off-by: Dan Zhang <danzh@google.com>

fix grpc test

1b235f5

Signed-off-by: Dan Zhang <danzh@google.com>

fix clang-tidy

fe42891

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk assigned snowp Apr 12, 2021

alyssawilk reviewed Apr 12, 2021

View reviewed changes

fix typo in function name

c4f3f61

Signed-off-by: Dan Zhang <danzh@google.com>

snowp suggested changes Apr 13, 2021

View reviewed changes

address comments

f098356

Signed-off-by: Dan Zhang <danzh@google.com>

snowp suggested changes Apr 13, 2021

View reviewed changes

snowp added the waiting label Apr 14, 2021

address comment

f82e877

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot removed the waiting label Apr 15, 2021

typo

1215b88

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into sdscallback

4363c40

Signed-off-by: Dan Zhang <danzh@google.com>

snowp suggested changes Apr 19, 2021

View reviewed changes

snowp added the waiting label Apr 19, 2021

address comment

8811f45

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot removed the waiting label Apr 19, 2021

antoniovicente self-assigned this Apr 19, 2021

snowp approved these changes Apr 20, 2021

View reviewed changes

antoniovicente approved these changes Apr 20, 2021

View reviewed changes

snowp merged commit d841924 into envoyproxy:main Apr 20, 2021

gokulnair pushed a commit to gokulnair/envoy that referenced this pull request May 6, 2021

quiche: support downstream using SDS (envoyproxy#15821)

dadfa9d

Signed-off-by: Dan Zhang <danzh@google.com> Signed-off-by: Gokul Nair <gnair@twitter.com>

		const Ssl::ClientContextConfig& clientContextConfig() const { return *config_; }
		const Ssl::ClientContextConfig& clientContextConfig() const { return config_; }

		? "quic_server_transport_socket_factory.context_config_update_by_sds"
		: "server_ssl_socket_factory.ssl_context_update_by_sds"),

Conversation

danzh2010 commented Apr 2, 2021

Uh oh!

alyssawilk commented Apr 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danzh2010 commented Apr 8, 2021

Uh oh!

repokitteh-read-only bot commented Apr 8, 2021

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk Apr 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

snowp Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danzh2010 Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk Apr 12, 2021 •

edited

Loading

snowp Apr 13, 2021 •

edited

Loading

danzh2010 Apr 13, 2021 •

edited

Loading

danzh2010 Apr 14, 2021 •

edited

Loading