This looks good overall! Comments are all fairly minor.

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

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

BUILD, line 1338 at r1 (raw file):

grpc_cc_library(
    name = "grpc_google_mesh_ca_certificate_provider",

This is not the certificate provider itself; it's just the factory. The provider itself will probably be in a separate target in src/core/lib/security, so that it can be used with TlsCreds if we want.

Please rename accordingly, both the target name and the filenames.

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

namespace grpc_core {

class GoogleMeshCaCertificateProviderFactory : CertificateProviderFactory {

Inheritence must be public, as per style guide.

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

  class Config : public CertificateProviderFactory::Config {
   public:
    struct StsConfig {

Instead of redeclaring this, I suggest using the existing struct here:

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

    };

    const char* name() const override { return "meshCA"; }

This string exists in two places. Suggest defining it as a constant (preferably in the .cc file) and using it in both places.

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

//
template <typename NumericType, typename ErrorVectorType>
bool ExtractJsonType(const Json& json, const std::string& field_name,

General API suggestion for all of these helper functions: Instead of returning a bool to indicate success and returning the value by parameter, I suggest having the function return absl::optional<NumericValue> (or whatever type is appropriate for the function). If the result has no value, then it was an error.

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

// proto message, as per:
//   https://developers.google.com/protocol-buffers/docs/proto3#json
bool ParseDuration(const Json& field, grpc_millis* duration) {

This function already exists in the client channel code. Instead of duplicating it, how about moving it into its own module in src/core/lib/json, so that it can be used in both places?

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

GoogleMeshCaCertificateProviderFactory::Config::Parse(const Json& config_json,
                                                      grpc_error** error) {
  std::vector<grpc_error*> error_list;

It looks like you're currently using the same error_list vector for all errors. This can make it hard to figure out where the error is actually coming from, because error messages include the inner-most field name only.

I suggest doing the same sort of recursive parsing we do elsewhere (e.g., in the LB policy code and in the xDS bootstrap parser). Every time you enter a new scope in the JSON, recurse into a new parsing helper function. Each parsing function has its own error list, and upon return, any errors in that list get grouped underneath one error that indicates the field name from the enclosing scope. Thus, the structure of the error tree makes it easy to see exactly where in the tree the parsing errors were.

test/core/client_channel/BUILD, line 34 at r1 (raw file):

grpc_cc_test(
    name = "google_mesh_ca_certificate_provider_test",

Please add "factory" to both the target name and filename.

test/core/client_channel/BUILD, line 34 at r1 (raw file):

grpc_cc_test(
    name = "google_mesh_ca_certificate_provider_test",

This isn't really client-channel-specific functionality (it will also be used in the server), so this seems like the wrong directory for it. Suggest creating a new directory test/core/xds.

test/core/client_channel/google_mesh_ca_certificate_provider_test.cc, line 32 at r1 (raw file):

namespace {

void VerifyRegexMatch(grpc_error* error, const std::regex& e) {

Can we just use the gmock ContainsRegex() matcher instead?

test/core/client_channel/google_mesh_ca_certificate_provider_test.cc, line 258 at r1 (raw file):

}

TEST(GoogleMeshCaConfigTest, WrongTypes2) {

Suggest calling this GrpcServicesNotAnArray.

test/core/client_channel/google_mesh_ca_certificate_provider_test.cc, line 280 at r1 (raw file):

}

TEST(GoogleMeshCaConfigTest, WrongTypes3) {

GoogleGrpcNotAnObject

test/core/client_channel/google_mesh_ca_certificate_provider_test.cc, line 304 at r1 (raw file):

}

TEST(GoogleMeshCaConfigTest, WrongTypes4) {

CallCredentialsNotAnArray

test/core/client_channel/google_mesh_ca_certificate_provider_test.cc, line 331 at r1 (raw file):

}

TEST(GoogleMeshCaConfigTest, WrongTypes5) {

StsServiceNotAnObject

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

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

I thought of this earlier and went back and forth between the pointer as a parameter and an absl::optional return. The thing I liked about providing the pointer to the helpers is that the main parsing logic is really concise and does not get bloated by if conditions where we check has_value() and then assign the field to value().

What do you think?

So,
ParsedJsonObjectField(json, "field_name", &config->field, ...);

becomes

absl::optional<std::string> parsed_field = ParsedJsonObjectField(...); if(parsed_field.has_value()) { config->field = parsed_field.value(); }

what do you think?

Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @yashykt)

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

Yeah, that's a good point. This is counter to our normal style guidance (prefer to return values directly instead of via parameters), but I guess this makes more sense in this case.

Okay, this is fine.

Reviewable status: 1 of 24 files reviewed, 13 unresolved discussions (waiting on @markdroth)

BUILD, line 1338 at r1 (raw file):

Done.

test/core/client_channel/BUILD, line 34 at r1 (raw file):

Done.

test/core/client_channel/BUILD, line 34 at r1 (raw file):

Done.

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

Done.

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

StsCredentialsOptions
I don't think we want a dependency on a C++ header from Core

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

Done.

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

Done. Moved to src/core/lib/json/json_util.h

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

Done.

test/core/client_channel/google_mesh_ca_certificate_provider_test.cc, line 32 at r1 (raw file):

Yes, we can! Replaced.

test/core/client_channel/google_mesh_ca_certificate_provider_test.cc, line 258 at r1 (raw file):

Done.

test/core/client_channel/google_mesh_ca_certificate_provider_test.cc, line 280 at r1 (raw file):

Done.

test/core/client_channel/google_mesh_ca_certificate_provider_test.cc, line 304 at r1 (raw file):

Done.

test/core/client_channel/google_mesh_ca_certificate_provider_test.cc, line 331 at r1 (raw file):

Done.

Looking better and better! Comments remaining minor.

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

Reviewed 24 of 24 files at r2.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @yashykt)

src/core/lib/json/json_util.h, line 24 at r2 (raw file):

#include <grpc/support/port_platform.h>

#include <grpc/support/string_util.h>

This include and the next one should be moved to the .cc file.

src/core/lib/json/json_util.h, line 35 at r2 (raw file):

// proto message, as per:
//   https://developers.google.com/protocol-buffers/docs/proto3#json
bool ParseDurationFromJson(const Json& field, grpc_millis* duration);

Please document that it returns true on success.

src/core/lib/json/json_util.cc, line 54 at r2 (raw file):

}

}  // namespace grpc_core

Please add trailing newline.

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

Good point. Then I guess we should use this instead:

This is the form that we'll need anyway in order to create the STS call creds.

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

Can you split up the parsing code for each scope into its own helper method? I think that will make it a lot easier to read and maintain.

src/core/ext/xds/google_mesh_ca_certificate_provider_factory.cc, line 38 at r2 (raw file):

namespace {

const char* kMeshCaPlugin = "meshCA";

Have we standardized on this string across languages? It will appear in the bootstrap file, so we'll need to be consistent.

If we haven't yet standardized, I'd prefer to call it MeshCA. But if we have, then this is fine.

test/core/xds/google_mesh_ca_certificate_provider_factory_test.cc, line 19 at r2 (raw file):

//

#include <regex>

I don't think this include is needed anymore.

Reviewable status: 19 of 24 files reviewed, 7 unresolved discussions (waiting on @markdroth)

src/core/lib/json/json_util.h, line 24 at r2 (raw file):

You are right. Moved

src/core/lib/json/json_util.h, line 35 at r2 (raw file):

Done

src/core/lib/json/json_util.cc, line 54 at r2 (raw file):

Done

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

Yeah, unfortunately this takes in const char*. We could potentially store the config to have the strings backing available to guarantee lifetime, but then we have cases where we need to store defaults and compute the values in some cases so we need them to be modifiable.

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

Done.

src/core/ext/xds/google_mesh_ca_certificate_provider_factory.cc, line 38 at r2 (raw file):

This is what the docs say presently but it is not set in stone, we can change it.

test/core/xds/google_mesh_ca_certificate_provider_factory_test.cc, line 19 at r2 (raw file):

Removed.

Just a couple of remaining comments about structuring the parsing recursion.

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

Reviewed 4 of 5 files at r3, 3 of 3 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @yashykt)

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

Ugh, that's really unfortunate. I hate the fact that we will now have this struct in 3 different places. But okay.

src/core/ext/xds/google_mesh_ca_certificate_provider_factory.h, line 68 at r4 (raw file):

   private:
    // Helpers for parsing the config
    void ParseJsonObjectStsService(const Json::Object& parent_object,

Instead of passing in the parent object, please pass in only the value that this function is responsible for parsing. It can be passed in as a const Json&, and the function can validate its type.

src/core/ext/xds/google_mesh_ca_certificate_provider_factory.h, line 69 at r4 (raw file):

    // Helpers for parsing the config
    void ParseJsonObjectStsService(const Json::Object& parent_object,
                                   std::vector<grpc_error*>* parent_error_list);

Instead of passing in the error vector from the parent and having the child populate it, please just have the child return an error vector, which the parent can add to its own.

https://abseil.io/tips/176

Reviewable status: 22 of 24 files reviewed, 2 unresolved discussions (waiting on @markdroth)

src/core/ext/xds/google_mesh_ca_certificate_provider_factory.h, line 68 at r4 (raw file):

Restructured, though I'm passing in Json::Object instead of a Json letting the helper functions deal with the Json instead

src/core/ext/xds/google_mesh_ca_certificate_provider_factory.h, line 69 at r4 (raw file):

Done

This looks great! Feel free to merge after addressing the one remaining nit.

Reviewed 2 of 2 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yashykt)

src/core/ext/xds/google_mesh_ca_certificate_provider_factory.cc, line 310 at r5 (raw file):

GoogleMeshCaCertificateProviderFactory::Config::Parse(const Json& config_json,
                                                      grpc_error** error) {
  std::vector<grpc_error*> error_list;

Please move this down to line 318, since it's not needed before then.