This looks like a great start!

The only really significant comment is the one about creating the security connector on a per-connection basis without plumbing it through the handshaker API.

Please let me know if you have any questions. Thanks!

Reviewed 32 of 32 files at r1.
Reviewable status: all files reviewed, 35 unresolved discussions (waiting on @yashykt)

src/core/ext/transport/chttp2/server/chttp2_server.cc, line 194 at r1 (raw file):

      interested_parties_(grpc_pollset_set_create()) {
  grpc_pollset_set_add_pollset(interested_parties_, accepting_pollset_);
  grpc_error* error = HandshakerRegistry::AddHandshakers(

It looks like the reason for the handshaker API change is to move the security connector creation into the security handshaker's factory. However, that seems like a fairly invasive change (as evidenced by the number of files that needed to be changed in this PR, and I believe it would also require internal changes), and it doesn't really seem like a natural conceptual fit, because creating the security connector for the connection doesn't really have anything to do with adding handshakers.

Instead, I suggest plumbing in a function of the following type:

// A function to modify channel args for an individual connection.
// Takes ownership of the args.  Caller takes ownership of returned args.
// On failure, the error parameter will be set.
using Chttp2ServerConnectionArgModifier = std::function<grpc_channel_args*(grpc_channel_args*, grpc_error**)>;

In server_secure_chttp2.cc, we can pass in a function that will create the security connector and add it to the args. In server_chttp2.cc (the insecure case), where no security connector is needed, we can use a no-op function that just returns the args that are passed to it.

The code here can invoke that function to modify before populating the handshake manager.

I think this approach will accomplish the same thing in a much cleaner and less invasive way.

src/core/ext/xds/xds_api.h, line 282 at r1 (raw file):

  // TODO(roth): When we can use absl::variant<>, consider using that
  // here, to enforce the fact that only one of the two fields can be set.
  struct LdsUpdate {

I think this struct should now have its own ToString() method, and we should use that method here:

src/core/ext/xds/xds_api.cc, line 625 at r1 (raw file):

bool XdsApi::DownstreamTlsContext::Empty() const {
  return common_tls_context.Empty();

Should it also be considered non-empty if require_client_certificate is set?

src/core/ext/xds/xds_api.cc, line 1545 at r1 (raw file):

grpc_error* LdsResponseParseClient(
    XdsClient* client, TraceFlag* tracer, upb_symtab* symtab, upb_arena* arena,
    XdsApi::LdsUpdate& lds_update,

lds_update is an output parameter, so it should be passed as a pointer instead of a reference, and it should be the last parameter.

src/core/ext/xds/xds_api.cc, line 1615 at r1 (raw file):

grpc_error* LdsResponseParseServer(
    upb_arena* arena, XdsApi::LdsUpdate& lds_update,

lds_update is an output parameter, so it should be passed as a pointer instead of a reference, and it should be the last parameter.

src/core/ext/xds/xds_api.cc, line 1619 at r1 (raw file):

    const std::string& listener_name,
    const envoy_config_core_v3_Address* address) {
  // Get address from listener name

As per discussion in the gRFC, I think we've decided not to check this, since we will be using a template from the bootstrap file to determine the resource name, which means that we can't know exactly what format the name will be in. So let's just remove this check.

src/core/ext/xds/xds_api.cc, line 1642 at r1 (raw file):

        "Addresses from listener name and listener address do not match");
  }
  // Right now, we are supporting and expecting only one entry in filter_chains.

As per the gRFC, we should actually look at every entry in filter_chains use the first one that matches, or else use default_filter_chain instead.

src/core/ext/xds/xds_api.cc, line 1650 at r1 (raw file):

        "Only one filter_chain supported.");
  }
  // Expect to find "h2" as an application protocol

I believe the gRFC says that we aren't supposed to look at this field at all.

src/core/ext/xds/xds_api.cc, line 1755 at r1 (raw file):

    }
    XdsApi::LdsUpdate& lds_update = (*lds_update_map)[listener_name];
    // Get api_listener and decode it to http_connection_manager.

I think this comment should say something like "Check whether it's a client or server listener.".

src/core/ext/xds/xds_certificate_provider.h, line 75 at r1 (raw file):

  void set_require_client_certificates(bool require_client_certificates) {
    require_client_certificates_ = require_client_certificates;

If require_client_certificates_ is an atomic, don't the setter and getter methods need to use atomic ops to access it?

src/core/ext/xds/xds_certificate_provider.h, line 118 at r1 (raw file):

  std::vector<XdsApi::StringMatcher> san_matchers_;
  // Not guarded by mu_ to avoid deadlocks.
  std::atomic<bool> require_client_certificates_{false};

Please use Atomic<bool> instead.

src/core/ext/xds/xds_server_config_fetcher.cc, line 102 at r1 (raw file):

            "[ListenerWatcher %p] Received LDS update from xds client %p: {%s}",
            this, xds_client_.get(),
            listener.downstream_tls_context.Empty()

This should just use LdsUpdate::ToString() (which doesn't currently exist, but should).

src/core/ext/xds/xds_server_config_fetcher.cc, line 110 at r1 (raw file):

      grpc_error* error = UpdateXdsCertificateProvider(listener);
      if (error != GRPC_ERROR_NONE) {
        gpr_log(GPR_ERROR, "XdsCertificateProvider update failed: %s",

This should probably just call OnError(), since we'll want to handle it the same way (including notifying the application somehow when we eventually address that TODO).

src/core/ext/xds/xds_server_config_fetcher.cc, line 147 at r1 (raw file):

          grpc_find_server_credentials_in_args(args_);
      if (server_creds == nullptr ||
          server_creds->type() != grpc_core::kCredentialsTypeXds) {

No need for grpc_core::, since we're already in that namespace.

src/core/ext/xds/xds_server_config_fetcher.cc, line 225 at r1 (raw file):

      } else {
        // No security configuration provided.
        xds_certificate_provider_ = nullptr;

Prefer .reset().

src/core/lib/security/credentials/xds/xds_credentials.cc, line 182 at r1 (raw file):

          GRPC_SSL_DONT_REQUEST_CLIENT_CERTIFICATE);
    }
    return MakeRefCounted<TlsServerCredentials>(

Prefer to write this as:

    auto tls_credentials =
        MakeRefCounted<TlsServerCredentials>(std::move(tls_credentials_options));
    return tls_credentials->create_security_connector(args);

src/proto/grpc/testing/xds/v3/listener.proto, line 100 at r1 (raw file):

  // Example using SNI for filter chain selection can be found in the
  // :ref:`FAQ entry <faq_how_to_setup_sni>`.
  repeated FilterChain filter_chains = 3;  

Please remove trailing whitespace.

test/cpp/end2end/xds_end2end_test.cc, line 2062 at r1 (raw file):

  class ServerThread {
   public:
    ServerThread(bool use_xds_enabled_server = false)

explicit

test/cpp/end2end/xds_end2end_test.cc, line 2120 at r1 (raw file):

    int port() const { return port_; }

    bool xds_enabled() const { return use_xds_enabled_server_; }

Please call this use_xds_enabled_server().

test/cpp/end2end/xds_end2end_test.cc, line 2138 at r1 (raw file):

  class BackendServerThread : public ServerThread {
   public:
    BackendServerThread(bool use_xds_enabled_server)

explicit

test/cpp/end2end/xds_end2end_test.cc, line 2155 at r1 (raw file):

    std::shared_ptr<ServerCredentials> Credentials() override {
      if (xds_enabled() && GetParam().use_xds_credentials()) {

Please structure as:

if (GetParam().use_xds_credentials()) {
  if (use_xds_enabled_server()) {
    // We are testing server's use of XdsServerCredentials.
    return experimental::XdsServerCredentials(InsecureServerCredentials());
  } else {
    // We are testing client's use of XdsCredentials.
    //...
    return grpc::experimental::TlsServerCredentials(options);
  }
  return ServerThread::Credentials();
}

test/cpp/end2end/xds_end2end_test.cc, line 5480 at r1 (raw file):

          break;
        } else {
          gpr_log(GPR_ERROR, "%d",

This error message could use some improvement. :)

test/cpp/end2end/xds_end2end_test.cc, line 5984 at r1 (raw file):

  }

  void TearDown() override { XdsEnd2endTest::TearDown(); }

This override is a no-op, so I don't think it's needed.

test/cpp/end2end/xds_end2end_test.cc, line 6145 at r1 (raw file):

        backends_[0]->port());
    auto* filter_chain = listener.add_filter_chains();
    if (!root_instance_name.empty() || !identity_instance_name.empty()) {

Shouldn't this check only the identity instance name, since we're adding that unconditionally?

test/cpp/end2end/xds_end2end_test.cc, line 6207 at r1 (raw file):

        grpc_tls_credentials_create(options));
    grpc_tls_server_authorization_check_config_release(check_config);
    auto channel = CreateCustomChannel(uri, channel_creds, args);

No need for this local variable; just directly return the result of CreateCustomChannel().

test/cpp/end2end/xds_end2end_test.cc, line 6239 at r1 (raw file):

        grpc_tls_credentials_create(options));
    grpc_tls_server_authorization_check_config_release(check_config);
    auto channel = CreateCustomChannel(uri, channel_creds, args);

Same here.

test/cpp/end2end/xds_end2end_test.cc, line 6255 at r1 (raw file):

  }

  template <typename Functor>

All 3 of the channel-creation functions have the same signature, so it seems like this doesn't need to be a template.

test/cpp/end2end/xds_end2end_test.cc, line 6313 at r1 (raw file):

      break;
    }
    EXPECT_TRUE(num_tries < kRetryCount);

EXPECT_LT(num_tries, kRetryCount);

test/cpp/end2end/xds_end2end_test.cc, line 6348 at r1 (raw file):

  socket_address->set_address("[::1]");
  balancers_[0]->ads_service()->SetLdsResource(listener);
  CheckRpcSendFailure(1, RpcOptions().set_wait_for_ready(true));

This is using an xDS-enabled client, not the machinery you defined in XdsServerSecurityTest::SendRpc() above. Is that what you intended here?

test/cpp/end2end/xds_end2end_test.cc, line 6360 at r1 (raw file):

  SetLdsUpdate("", "", "unknown", "", false);
  // Send RPC
  SendRpc([this]() { return CreateTlsChannel(); }, {}, {},

I don't think this lambda is needed. Just pass CreateTlsChannel by itself as the param.

Same for all of these tests.

test/cpp/end2end/xds_end2end_test.cc, line 6410 at r1 (raw file):

  SetLdsUpdate("fake_plugin1", "", "fake_plugin2", "", true);
  SendRpc([this]() { return CreateMtlsChannel(); },
          {server_authenticated_identity_2_}, {client_authenticated_identity_});

Why the braces around these two params? I'm surprised this compiles...

Same question in every test that uses server_authenticated_identity_2_.

test/cpp/end2end/xds_end2end_test.cc, line 6463 at r1 (raw file):

      {"good", {root_cert_, identity_pair_2_}}};
  g_fake1_cert_data_map = &fake1_cert_map;
  ;

Please remove.

test/cpp/end2end/xds_end2end_test.cc, line 6487 at r1 (raw file):

  g_fake1_cert_data_map = &fake1_cert_map;
  SetLdsUpdate("fake_plugin1", "", "fake_plugin1", "", false);
  // Send RPC on mTLS channel

s/mTLS/TLS/

test/cpp/end2end/xds_end2end_test.cc, line 6556 at r1 (raw file):

  SendRpc([this]() { return CreateTlsChannel(); },
          server_authenticated_identity_, {});
  /* Send update */

This comment isn't really useful.

Same in all of the remaining tests.

test/cpp/end2end/xds_end2end_test.cc, line 7943 at r1 (raw file):

// We are only testing the server here.
INSTANTIATE_TEST_SUITE_P(XdsTest, XdsEnabledServerTest,
                         ::testing::Values(TestType(false, false, false, false,

As discussed, please change use_xds_resolver to true here.

Reviewable status: 18 of 34 files reviewed, 35 unresolved discussions (waiting on @markdroth and @yashykt)

src/core/ext/transport/chttp2/server/chttp2_server.cc, line 194 at r1 (raw file):

I like it! and I think there was a related issue around where the security connector is created. Looks like the same security connector needs to be shared across connections to allow for ssl reuse.

src/core/ext/xds/xds_api.cc, line 625 at r1 (raw file):

Yes, require_client_certificate is of no consequence if there is no way to verify client certificates.

src/core/ext/xds/xds_api.cc, line 1545 at r1 (raw file):

Done.

src/core/ext/xds/xds_api.cc, line 1615 at r1 (raw file):

Done.

src/core/ext/xds/xds_api.cc, line 1619 at r1 (raw file):

Removed

src/core/ext/xds/xds_api.cc, line 1642 at r1 (raw file):

I agree that that's what we should be doing but I'm not sure our current infrastructure supports that. Let me confirm this.

src/core/ext/xds/xds_api.cc, line 1650 at r1 (raw file):

Removed.

src/core/ext/xds/xds_certificate_provider.h, line 75 at r1 (raw file):

std::atomic is convenient that way. You don't need to use explicit Store/Load

src/core/ext/xds/xds_certificate_provider.h, line 118 at r1 (raw file):

Do we want to avoid using std::atomic?

src/core/ext/xds/xds_server_config_fetcher.cc, line 102 at r1 (raw file):

This is slightly awkward since not only fields matter to the server, but I guess that's fine

src/core/ext/xds/xds_server_config_fetcher.cc, line 110 at r1 (raw file):

Done.

src/core/ext/xds/xds_server_config_fetcher.cc, line 147 at r1 (raw file):

Done.

src/core/ext/xds/xds_server_config_fetcher.cc, line 225 at r1 (raw file):

Done.

src/core/lib/security/credentials/xds/xds_credentials.cc, line 182 at r1 (raw file):

Done.

src/proto/grpc/testing/xds/v3/listener.proto, line 100 at r1 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 2062 at r1 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 2120 at r1 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 2138 at r1 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 2155 at r1 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 5480 at r1 (raw file):

Yeah, it looks like some of my debugging effort showed up here. Making the entire thing prettier.

test/cpp/end2end/xds_end2end_test.cc, line 5984 at r1 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 6145 at r1 (raw file):

You are right!

test/cpp/end2end/xds_end2end_test.cc, line 6207 at r1 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 6239 at r1 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 6255 at r1 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 6313 at r1 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 6348 at r1 (raw file):

Yeah, it does not really matter here

test/cpp/end2end/xds_end2end_test.cc, line 6360 at r1 (raw file):

We actually do. CreateTlsChannel and others are member functions and they need to be member functions to access some of the protected state like ipv6_only_ and backends_.

test/cpp/end2end/xds_end2end_test.cc, line 6410 at r1 (raw file):

I'm surprised too, but rules that C++ compilers need to follow are pretty complex nowadays anyway

test/cpp/end2end/xds_end2end_test.cc, line 6463 at r1 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 6487 at r1 (raw file):

removing these comments altogether

test/cpp/end2end/xds_end2end_test.cc, line 6556 at r1 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 7943 at r1 (raw file):

Done.

Reviewable status: 17 of 33 files reviewed, 35 unresolved discussions (waiting on @markdroth)

src/core/ext/xds/xds_api.h, line 282 at r1 (raw file):

It seems a bit awkward to have

src/core/ext/xds/xds_api.cc, line 1755 at r1 (raw file):

Done.

This looks pretty good! I think the security connector creation still needs a bit of work.

Please let me know if you have any questions. Thanks!

Reviewed 12 of 21 files at r2, 1 of 2 files at r3, 6 of 7 files at r4.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @markdroth and @yashykt)

src/core/ext/transport/chttp2/server/chttp2_server.cc, line 194 at r1 (raw file):

If we lose SSL state when switching security connectors, then that implies that we'll lose this state every time we get an LDS update. Is that going to cause any problems?

src/core/ext/transport/chttp2/server/insecure/server_chttp2.cc, line 32 at r3 (raw file):

namespace {

grpc_channel_args* connection_args_modifier(grpc_channel_args* args,

Please call this ModifyArgsForConnection().

src/core/ext/transport/chttp2/server/secure/server_secure_chttp2.cc, line 43 at r3 (raw file):

namespace {

grpc_channel_args* connection_args_modifier(grpc_channel_args* args,

ModifyArgsForConnection()

src/core/ext/transport/chttp2/server/secure/server_secure_chttp2.cc, line 87 at r4 (raw file):

    goto done;
  }
  if (server->core_server->config_fetcher() != nullptr) {

This approach would mean that callers are required to call grpc_server_set_config_fetcher() before calling grpc_server_add_secure_http2_port(). I don't think there is currently any such API requirement, and it seems better not to add that requirement. In general, the API for servers has always been that you can set all the parameters in any order, and nothing actually happens to put all of those parameters together until you call grpc_server_start().

Given this, I think it would be better to create the security connector via the connection arg modifier function even if there is no config fetcher.

src/core/ext/transport/chttp2/server/secure/server_secure_chttp2.cc, line 114 at r4 (raw file):

done:
  sc.reset(DEBUG_LOCATION, "server");

Please remove blank line.

src/core/ext/xds/xds_api.h, line 282 at r1 (raw file):

I assume you meant to finish that sentence, but I'm not sure what the end was going to be. :)

Anyway, this looks good.

src/core/ext/xds/xds_api.cc, line 642 at r4 (raw file):

        absl::StrFormat("route_config_name=%s", route_config_name.c_str()));
  }
  contents.push_back(absl::StrFormat("http_max_stream_duration=%s",

How about adding a ListenerType enum to the LdsUpdate struct, so that we can differentiate between TCP listeners and HTTP API listeners? That way, we can include only the relevant fields for each type.

src/core/ext/xds/xds_certificate_provider.h, line 75 at r1 (raw file):

What memory order gets used in this case for setting and getting? Is it the one we want?

src/core/ext/xds/xds_certificate_provider.h, line 118 at r1 (raw file):

Historically, we have preferred to use our own Atomic<> API. But it may be that there's less reason for that now that we can use STL.

I suggest checking with Esun and Soheil to see what they think about this. If everyone is okay with changing our code to directly use std::atomic<>, then we can probably just get rid of our own Atomic<> API.

src/core/ext/xds/xds_server_config_fetcher.cc, line 102 at r1 (raw file):

I assume that was meant to say "none of the other fields".

I think that LdsUpdate::ToString() should report different fields depending on whether it's a TCP listener or an API listener. So this should wind up being basically right for the server as well as the client.

src/core/lib/security/transport/security_handshaker.cc, line 27 at r4 (raw file):

#include <limits>

#include "absl/strings/str_cat.h"

No longer needed.

src/core/lib/security/transport/security_handshaker.cc, line 533 at r4 (raw file):

                      HandshakeManager* handshake_mgr) override {
    // Add the security connector handshakers
    grpc_server_credentials* creds = grpc_find_server_credentials_in_args(args);

Why check that the server creds are present in args? All we really care about here is the security connector.

I don't think this file should need any changes in this PR.

test/cpp/end2end/xds_end2end_test.cc, line 6255 at r1 (raw file):

I don't see this change. I think this can just use std::function<std::shared_ptr<grpc::Channel>()>.

test/cpp/end2end/xds_end2end_test.cc, line 6360 at r1 (raw file):

Good point. I think this could also be done without lambdas by using C++'s strange pointer-to-member-function syntax, but the lambda is a lot more readable. :)

test/cpp/end2end/xds_end2end_test.cc, line 5484 at r4 (raw file):

      break;
    }
    EXPECT_TRUE(num_tries < kRetryCount);

EXPECT_LT(num_tries, kRetryCount);

test/cpp/end2end/xds_end2end_test.cc, line 7975 at r4 (raw file):

  grpc::testing::WriteBootstrapFiles();
  grpc::testing::g_port_saver = new grpc::testing::PortSaver();
  // Make the backup poller poll very frequently in order to pick up

Why move all of this setup code here?

I'm particularly concerned about doing grpc_init() and grpc_shutdown() just once for the whole file instead of once per test. This could actually be a pretty significant change for the test, allowing more state to leak from one test case to the next.

Reviewable status: 25 of 33 files reviewed, 15 unresolved discussions (waiting on @jiangtaoli2016, @markdroth, and @ZhenLian)

src/core/ext/transport/chttp2/server/chttp2_server.cc, line 194 at r1 (raw file):

I don't believe that is going to be an issue. On the other hand, I do believe it would be a good to have if we can somehow share the SSL state between security connectors that did not have any relevant change. @ZhenLian @jiangtaoli2016

src/core/ext/transport/chttp2/server/insecure/server_chttp2.cc, line 32 at r3 (raw file):

Done.

src/core/ext/transport/chttp2/server/secure/server_secure_chttp2.cc, line 43 at r3 (raw file):

Done.

src/core/ext/transport/chttp2/server/secure/server_secure_chttp2.cc, line 87 at r4 (raw file):

This is the workaround to the problem of reloading ssl creds that we discussed on Friday. In an earlier version of the code, I had tried creating the security connector with the connection arg modifier even when there is no config fetcher but that ran into issues with h2_ssl_cred_reload.cc which assumes that there is only one security connector. After we decide on how the SSL creds fetcher API needs to look like in the future, we can clean this up, but in the mean time since this is an experimental Core API, I think it should be fine for now. What do you think?

src/core/ext/transport/chttp2/server/secure/server_secure_chttp2.cc, line 114 at r4 (raw file):

Done.

src/core/ext/xds/xds_api.h, line 282 at r1 (raw file):

Haha! It was supposed to - "It seems a bit awkward to have client specific fields in the server listener and vice versa"

src/core/ext/xds/xds_api.cc, line 642 at r4 (raw file):

Done!

src/core/ext/xds/xds_certificate_provider.h, line 75 at r1 (raw file):

The default is std::memory_order_seq_cst which is also the default for grpc_core::Atomic

src/core/ext/xds/xds_certificate_provider.h, line 118 at r1 (raw file):

Changed to Atomic

src/core/ext/xds/xds_server_config_fetcher.cc, line 102 at r1 (raw file):

I assume that was meant to say "none of the other fields".

Yeah, it did. Modified ToString() to use an enum

src/core/lib/security/transport/security_handshaker.cc, line 27 at r4 (raw file):

Done. Removed.

src/core/lib/security/transport/security_handshaker.cc, line 533 at r4 (raw file):

You are right. Reverting.

test/cpp/end2end/xds_end2end_test.cc, line 6255 at r1 (raw file):

Really done this time.

test/cpp/end2end/xds_end2end_test.cc, line 5484 at r4 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 7975 at r4 (raw file):

The reason for this is that grpc shutdown can potentially happen in the background which causes issues when we want to register certificate providers. It often happens that when the new test suite tries to run and register certificate providers, the shutdown from the previous suite hasn't finished. This actually means in normal test runs, grpc shutdown would never actually happen if the init from the next test suite runs before the asynchronous shutdown starts.
If we really want to, we can use grpc_shutdown_blocking()but that's deprecated.

Reviewable status: 23 of 33 files reviewed, 15 unresolved discussions (waiting on @jiangtaoli2016 and @markdroth)

src/core/ext/transport/chttp2/server/chttp2_server.cc, line 194 at r1 (raw file):

Hi Yash, can I see the code for the SSL state that is currently being shared across connections? I think ideally, it would be great if we can create new security connector only when needed, but I'd like to see exactly what's stored in SSL state, and what are the things being shared.
Also, does the client side share SSL state among connections?

Reviewable status: 23 of 33 files reviewed, 15 unresolved discussions (waiting on @jiangtaoli2016 and @markdroth)

src/core/ext/transport/chttp2/server/chttp2_server.cc, line 194 at r1 (raw file):
For now, I've updated the code to only create the new security connector when the grpc_tls_credentials_options would change. I guess if the options change then we can't really share the SSL session either.

Hi Yash, can I see the code for the SSL state that is currently being shared across connections?
I haven't dug deep enough in the security code to figure out how that happens yet, but the current state might be an optimal one, because even with regular LDS updates, changes that would affect the options would be less common anyway.

This is getting close! As soon as I merge #25120, you can merge those changes in here.

Please let me know if you have any questions. Thanks!

Reviewed 7 of 9 files at r5, 4 of 4 files at r6.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @markdroth and @yashykt)

src/core/ext/transport/chttp2/server/chttp2_server.cc, line 194 at r1 (raw file):

This change looks good, thanks!

src/core/ext/transport/chttp2/server/secure/server_secure_chttp2.cc, line 87 at r4 (raw file):

Ah, okay, I didn't realize that that was the reason for this change. Please add a detailed TODO comment to explain why this is being done, what the drawback is, and what we plan to do about it.

I don't want this to block this PR, but I would really prefer to find a way to address this before we de-experimentalize the server xDS API.

src/core/ext/xds/xds_api.h, line 285 at r6 (raw file):

    enum class ListenerType {
      kTcpListener = 0,
      kApiListener,

Let's call this kHttpApiListener.

src/core/ext/xds/xds_server_config_fetcher.cc, line 104 at r6 (raw file):

      }
      auto* prev_provider = xds_certificate_provider_.get();
      bool provides_root_certs = false, provides_identity_certs = false,

Please declare each variable on its own line.

src/core/ext/xds/xds_server_config_fetcher.cc, line 119 at r6 (raw file):

      }
      // Only send an update, if something changed.
      if (updated_once_ && xds_certificate_provider_.get() == prev_provider &&

Instead of checking the before and after conditions here, would it make sense to have UpdateXdsCertificateProvider() return a bool indicating if something changed?

src/core/ext/xds/xds_server_config_fetcher.cc, line 120 at r6 (raw file):

      // Only send an update, if something changed.
      if (updated_once_ && xds_certificate_provider_.get() == prev_provider &&
          ((prev_provider == nullptr) ||

No need for the parens around prev_provider == nullptr.

src/core/ext/xds/xds_server_config_fetcher.cc, line 120 at r6 (raw file):

      // Only send an update, if something changed.
      if (updated_once_ && xds_certificate_provider_.get() == prev_provider &&
          ((prev_provider == nullptr) ||

With the current logic in UpdateXdsCertificateProvider(), I don't think this part of the expression is actually necessary, because the cases where we'd need to change the tls_credentials_options are also the cases where UpdateXdsCertificateProvider() will create a new XdsCertificateProvider(), so the check on the line above should be sufficient by itself.

That having been said, once #25120 is merged, I think the logic in UpdateXdsCertificateProvider() will need to change such that it just updates the map inside of the XdsCertificateProvider and never replaces the object itself, in which case you will need the additional logic here.

src/core/ext/xds/xds_server_config_fetcher.cc, line 180 at r6 (raw file):

          listener.downstream_tls_context.common_tls_context
              .tls_certificate_certificate_provider_instance.certificate_name;
      RefCountedPtr<XdsCertificateProvider> new_root_provider;

I just noticed that this should use grpc_tls_certificate_provider, not XdsCertificateProvider.

src/core/ext/xds/xds_server_config_fetcher.cc, line 195 at r6 (raw file):

        root_certificate_provider_ = std::move(new_root_provider);
      }
      RefCountedPtr<XdsCertificateProvider> new_identity_provider;

Same here.

test/cpp/end2end/xds_end2end_test.cc, line 7975 at r4 (raw file):

Why is this problem showing up now and hasn't shown up before? For example, the XdsClient itself takes a while to shut down, and that gets created and destroyed between every test. Although admittedly, use of UnsetGlobalXdsClientForTest() forces that to work right even if the shutdown doesn't happen.

Can we convince ourselves that there's no state that is going to fail to be reset between tests by doing this that would affect the test outcomes?

Can you run the test suite 1000x on RBE to make sure this doesn't cause new flakes?

Reviewable status: 27 of 33 files reviewed, 10 unresolved discussions (waiting on @markdroth and @yashykt)

src/core/ext/xds/xds_api.h, line 285 at r6 (raw file):

Done.

src/core/ext/xds/xds_server_config_fetcher.cc, line 104 at r6 (raw file):

Outdated

src/core/ext/xds/xds_server_config_fetcher.cc, line 119 at r6 (raw file):

Outdated

src/core/ext/xds/xds_server_config_fetcher.cc, line 120 at r6 (raw file):

Outdated

src/core/ext/xds/xds_server_config_fetcher.cc, line 120 at r6 (raw file):

Updated and moved the logic to UpdateXdsCertificateProvider(). Note that earlier, we were not ALWAYS creating a different XdsCertificateProvider object.

src/core/ext/xds/xds_server_config_fetcher.cc, line 180 at r6 (raw file):

Done. Thanks!

src/core/ext/xds/xds_server_config_fetcher.cc, line 195 at r6 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 7975 at r4 (raw file):

Why is this problem showing up now and hasn't shown up before? For example, the XdsClient itself takes a while to shut down, and that gets created and destroyed between every test. Although admittedly, use of UnsetGlobalXdsClientForTest() forces that to work right even if the shutdown doesn't happen.

I don't think there were tests earlier which depended on grpc_shutdown() finishing deterministically. Now, since we have multiple suites that require registering certificate providers I can either forcefully do a shutdown() at the end of each suite with grpc_shutdown_blocking() or by simply not doing an init()/shutdown() per test suite. Since grpc_shutdown_blocking()

Can we convince ourselves that there's no state that is going to fail to be reset between tests by doing this that would affect the test outcomes?

Without an explicit grpc_init()/grpc_shutdown() there is going to be some state that does not get reset, but that should not affect test outcomes atleast currently. There is a chance that in the future that we might have such state that needs to be reset between tests and a blocking shutdown is the only solution I can think of for now (though it would potentially slow down our tests).

Reviewable status: 26 of 33 files reviewed, 10 unresolved discussions (waiting on @markdroth)

src/core/ext/transport/chttp2/server/secure/server_secure_chttp2.cc, line 87 at r4 (raw file):

TODO added. Note that this would only really impact the Core API. Wrapped language APIs should be able to get around this.

This looks really good! Only minor comments remaining.

Please let me know if you have any questions. Thanks!

Reviewed 6 of 6 files at r7, 1 of 1 files at r9.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @yashykt)

src/core/ext/transport/chttp2/server/secure/server_secure_chttp2.cc, line 87 at r4 (raw file):

It depends on how directly the wrapped language APIs expose the C-core API. If the specific semantics of the C-core API are abstracted away, then it's probably not an issue. But, for example, I know that the C# API pretty directly exposes the C-core API, so depending on how we design the xDS-enabled APIs, we may need to expose some constraints to users. We'll have to see how it goes.

Anyway, this looks good for now.

src/core/ext/transport/chttp2/server/secure/server_secure_chttp2.cc, line 21 at r9 (raw file):

#include <grpc/support/port_platform.h>

#include <grpc/grpc.h>

This include should actually be moved down to right above <grpc/support/alloc.h>.

src/core/ext/xds/xds_api.cc, line 1642 at r1 (raw file):

Are you going to do this as part of the current PR or defer this to a separate one?

src/core/ext/xds/xds_certificate_provider.h, line 57 at r9 (raw file):

      const std::string& cluster, std::vector<XdsApi::StringMatcher> matchers);

  bool GetRequireClientCertificate(const std::string& cert_name);

Please move these two up above the SAN matcher methods, so that all of the methods whose data is stored in certificate_state_map_ are next to each other.

src/core/ext/xds/xds_certificate_provider.h, line 113 at r9 (raw file):

   private:
    XdsCertificateProvider* xds_certificate_provider_;

Good catch -- you're right that this didn't need to be holding a ref. I'm not sure why I thought that was necessary.

Does this change mean that we can address the TODO I added?

src/core/ext/xds/xds_certificate_provider.cc, line 335 at r9 (raw file):

}

bool XdsCertificateProvider::GetRequireClientCertificate(

Same as in the .h file: Please move these up above the SAN matcher methods.

src/core/ext/xds/xds_certificate_provider.cc, line 348 at r9 (raw file):

  auto it = certificate_state_map_.find(cert_name);
  if (it == certificate_state_map_.end()) return;
  return it->second->set_require_client_certificate(require_client_certificate);

No need for return.

src/core/lib/security/credentials/xds/xds_credentials.cc, line 214 at r9 (raw file):

      tls_credentials_options->set_watch_root_cert(true);
    }
    if (xds_certificate_provider->ProvidesRootCerts("")) {

This is exactly the same condition as the previous block, so let's just merge the two.

test/cpp/end2end/xds_end2end_test.cc, line 7975 at r4 (raw file):

Okay. Did you run the test suite 1000x on RBE to verify that it's not more flaky?

Reviewable status: 29 of 34 files reviewed, 8 unresolved discussions (waiting on @markdroth)

src/core/ext/transport/chttp2/server/secure/server_secure_chttp2.cc, line 21 at r9 (raw file):

Done.

src/core/ext/xds/xds_api.cc, line 1642 at r1 (raw file):

As a separate one

src/core/ext/xds/xds_certificate_provider.h, line 57 at r9 (raw file):

Done.

src/core/ext/xds/xds_certificate_provider.h, line 113 at r9 (raw file):

Yeah, removed it

src/core/ext/xds/xds_certificate_provider.cc, line 335 at r9 (raw file):

Done.

src/core/ext/xds/xds_certificate_provider.cc, line 348 at r9 (raw file):

Done.

src/core/lib/security/credentials/xds/xds_credentials.cc, line 214 at r9 (raw file):

Done.

test/cpp/end2end/xds_end2end_test.cc, line 7975 at r4 (raw file):

Yes, I did. For the sanitizers, I could just 100x runs in. This test takes a long time

This looks great!

Reviewed 5 of 5 files at r10.
Reviewable status: complete! all files reviewed, all discussions resolved