This looks pretty good! My comments are mostly minor, except for the one about updating the tests. I think it's important that our tests cover the fields actually described in the gRFC.

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

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @donnadionne and @yashykt)

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

}

// CertificateProviderInstance is deprecated but we are still supporting it for

Please add a TODO to remove this as soon as TD is updated to populate the new fields.

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

      envoy_extensions_transport_sockets_tls_v3_CommonTlsContext_combined_validation_context(
          common_tls_context_proto);
  // The validation context is derived from the oneof in

Please move this comment up above the preceding statement.

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

    // 'validation_context_certificate_provider_instance' inside
    // 'combined_validation_context'. Note that this way of fetching root
    // certificates is deprecated and might be removed in the future.

s/might/will/. Please make this a TODO.

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

  } else {
    // Fall back onto 'tls_certificate_certificate_provider_instance'. Note that
    // this way of fetching identity certificates is deprecated and might be

s/might/will/. Please make this a TODO.

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

        "provider instance specified for validation."));
  }
  return GRPC_ERROR_CREATE_FROM_VECTOR("Error parsing DownstreamTlsContext",

If we're not going to support match_subject_alt_names on the server side, then we need an additional check here to add an error if that field is set.

Please thoroughly check the gRFC to make sure that there are no other edge cases we're missing here.

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

}

message SdsSecretConfig {}

Doesn't look like this is needed here, since you've added it inside of CommonTlsContext below.

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

      transport_socket->set_name("envoy.transport_sockets.tls");
      UpstreamTlsContext upstream_tls_context;
      // TODO(yashykt): Modify this to use the new security fields once the

I think we should go ahead and do this now, and just add a separate test or two that use the old fields. That way, when we remove the legacy code, we can just remove those separate tests and everything else will already be correct.

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

    filter_chain->add_filters()->mutable_typed_config()->PackFrom(
        HttpConnectionManager());
    // TODO(yashykt): Modify this to use the new security fields once the actual

Same here.

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @donnadionne, @markdroth, and @yashykt)

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

On the server side an implementation may support this field and its semantics, or NACK the update it if it doesn't. Ah, I missed this earlier

Reviewable status: 3 of 6 files reviewed, 8 unresolved discussions (waiting on @donnadionne and @markdroth)

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

Done.

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

Done.

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

Done.

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

Done.

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

Good eye!

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

Done.

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

Done.

Overall, this looks really good. There's just one problem with the tests to address.

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

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @donnadionne and @yashykt)

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

}

TEST_P(XdsServerSecurityTest, NacksFieldTlsCertificates) {

These two tests seem wrong.

On the server side, the TLS cert provider is required, so if it's not present, we should be getting an error that says that the cert provider is not present, and it does not matter whether the tls_certificates or tls_certificate_sds_secret_configs fields are specified. (Technically, we don't even need to check these fields on the server side. It's okay if we do, and it's okay if that error gets added to the NACK message, but what we really care about on the server side is that we get an error message that says that the TLS cert provider is not specified, not whether it also includes errors about these two irrelevant fields.)

Conversely, I think we do need these tests on the client side, because there the TLS cert provider is optional, so the checks for these two fields are relevant.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @donnadionne and @markdroth)

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

Moved to the client side

Looks great!

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @donnadionne)