This looks like a great start! Please let me know if you have any questions. Thanks!

Reviewed 9 of 9 files at r1.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @apolcyn)

src/core/lib/security/credentials/credentials.h, line 96 at r1 (raw file):

#define GRPC_ARG_CHANNEL_CREDENTIALS "grpc.channel_credentials"

extern grpc_core::Map<const char*,

These global variables should not be exposed outside of the .cc file.

src/core/lib/security/credentials/credentials.h, line 96 at r1 (raw file):

#define GRPC_ARG_CHANNEL_CREDENTIALS "grpc.channel_credentials"

extern grpc_core::Map<const char*,

The style guide prohibits global variables that are not trivially destructible.

An alternative is to have the global map variable be a pointer, have grpc_init() dynamically allocate the map and set the global pointer, and then have grpc_shutdown() free it and reset it to null.

src/core/lib/security/credentials/credentials.h, line 145 at r1 (raw file):

  // if no other creds are currently registered under authority. Returns
  // true if registered successfully and false if not.
  bool attach_credentials(

The implementation of these methods should move to the .cc file.

src/core/lib/security/credentials/credentials.h, line 153 at r1 (raw file):

      return false;
    }
    local_control_plane_creds_[authority] = control_plane_creds;

I think we need to make our own copy of authority here, since there's no guarantee that the passed-in string will remain alive after this call returns.

src/core/lib/security/credentials/credentials.h, line 165 at r1 (raw file):

  get_control_plane_credentials(const char* authority) {
    {
      grpc_core::MutexLock lock(&g_control_plane_creds_mu);

The mutex protects the global map only, not the local map. We don't need to acquire it unless the lookup in the local map fails.

src/core/lib/security/credentials/credentials.h, line 184 at r1 (raw file):

 private:
  const char* type_;
  grpc_core::Map<const char*,

The map key should probably be a UniquePtr<char>.

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

grpc_core::Mutex g_control_plane_creds_mu;

bool grpc_channel_credentials_attach_credentials(

This code should probably move up to line 48, so that it's next to the other channel creds methods.

test/core/security/control_plane_credentials_test.cc, line 86 at r1 (raw file):

// authorization metadata sent to the server, if any. Return
// nullptr if no authorization metadata was sent to the server.
char* perform_call_and_get_authorization_header(

Suggest having this return a UniquePtr<char>.

test/core/security/control_plane_credentials_test.cc, line 299 at r1 (raw file):

  GPR_ASSERT(grpc_channel_credentials_attach_credentials(main_creds, "foo",
                                                         foo_creds) == true);
  GPR_ASSERT(grpc_channel_credentials_attach_credentials(main_creds, "foo.",

The trailing '.' isn't obvious enough. Suggest using '2' instead.

test/core/security/control_plane_credentials_test.cc, line 325 at r1 (raw file):

}

void test_attach_and_get_with_global_registry() {

I don't actually see anywhere in this test where we're calling grpc_control_plane_credentials_register().

Update: I just noticed that this is happening in main(). That makes the test a lot harder to understand. Instead, I suggest doing that registration inside of this test. If you're worried about losing state between tests, you can have each test call grpc_init() at the start and grpc_shutdown() at the end.

test/core/security/control_plane_credentials_test.cc, line 432 at r1 (raw file):

    grpc::testing::TestEnvironment env(argc, argv);
    grpc_init();
    // The entries in the global registry must still persist through

Why? That's not the behavior I'd expect. I think shutting down the library should clear the map.

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @apolcyn, @jiangtaoli2016, and @markdroth)

test/core/security/control_plane_credentials_test.cc, line 432 at r1 (raw file):

My concern here was that clearing the global map between library restarts may cause surprising behavior. In e.g. wrapped languages for one, the init/shutdown lifetimes of the grpc library is an implementation detail managed by the wrapper, which users don't have visibility into. For example in ruby the library has an init/shutdown pair per object alloc/GC (though there's a couple of threads that will just happen to keep the library initialized). I could imagine other cases though we there's a disconnect between the code that cares about registering something in the global map and the code that manages grpc library init/shutdown.

WDYT?

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @apolcyn and @jiangtaoli2016)

test/core/security/control_plane_credentials_test.cc, line 432 at r1 (raw file):

Why doesn't Ruby simply call grpc_init() when the module is loaded and grpc_shutdown() when it gets unloaded?

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @apolcyn and @jiangtaoli2016)

test/core/security/control_plane_credentials_test.cc, line 432 at r1 (raw file):

Hmm, actually, this might wind up being more of a problem for C++ than for the wrapped languages, since C++ calls grpc_init() and grpc_shutdown() when each type of object is instantiated (e.g., channels, calls, etc) and then grpc_shutdown() when they are destroyed.

The problem is, given my comment elsewhere about the style guide prohibiting global variables that are not trivially destructible, it's not obvious how to make this data survive across shutdown and subsequent re-init. I guess we could just never destroy the map, but that seems ugly, and we'd probably still need some internal way to clear the map for testing purposes.

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @apolcyn, @jiangtaoli2016, and @markdroth)

test/core/security/control_plane_credentials_test.cc, line 432 at r1 (raw file):

Re ruby question: the main problem is that only "module unloaded" hook we have access to is atexit, but using atexit is broken on Windows for complications related to the way the different Windows shared libraries are loaded and unloaded (#17997 for background)

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @apolcyn, @jiangtaoli2016, and @markdroth)

test/core/security/control_plane_credentials_test.cc, line 432 at r1 (raw file):

The problem is, given my comment elsewhere about the style guide prohibiting global variables that are not trivially destructible, it's not obvious how to make this data survive across shutdown and subsequent re-init. I guess we could just never destroy the map, but that seems ugly, and we'd probably still need some internal way to clear the map for testing purposes.

I see, the only thing I can think of to work around that would be to expose something along the lines of test_only_clear_global_control_plane_creds_map

In gRPC tests, we could call that perhaps from the TestEnvironment dtor. In 3rd-party test code that use gRPC and also run under a memory leak detector, they could perhaps call that directly.

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

src/core/lib/security/credentials/credentials.h, line 96 at r1 (raw file):

Done.

src/core/lib/security/credentials/credentials.h, line 96 at r1 (raw file):

Per offline conversation, went with this approach, but avoided freeing it during grpc_shutdown

src/core/lib/security/credentials/credentials.h, line 145 at r1 (raw file):

Done.

src/core/lib/security/credentials/credentials.h, line 153 at r1 (raw file):

Whoops good catch. Fixed and tweaked tests so that ASAN would catch the previous error

src/core/lib/security/credentials/credentials.h, line 165 at r1 (raw file):

AFAICS the local maps, and in general the credentials objects, still need to be thread safe, even though such API usage is unlikely. So keeping it all under one lock seemed simplest. PLMK

src/core/lib/security/credentials/credentials.h, line 184 at r1 (raw file):

Done.

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

Done.

test/core/security/control_plane_credentials_test.cc, line 86 at r1 (raw file):

Done.

test/core/security/control_plane_credentials_test.cc, line 299 at r1 (raw file):

Done.

test/core/security/control_plane_credentials_test.cc, line 325 at r1 (raw file):

Per the offline conversation about keeping the global map state around between library restarts, I left this but just tried to point it out in a comment on the the test that uses it. PLMK

This looks pretty good! Remaining comments are mostly minor.

Please let me know if you have any questions.

Reviewed 4 of 4 files at r2.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @apolcyn and @jiangtaoli2016)

src/core/lib/security/credentials/credentials.h, line 165 at r1 (raw file):

I'm concerned about scalability in the case where the control-plane creds are all attached to the channel credentials (as opposed to being in the global registry). The lock we're using is global. This means that if there are 1000 different credentials objects being used by 1000 different channels, only one of them can do a lookup at any one time. This seems like a surprising and unnecessary restriction.

If we actually need a lock here, it would be better to have a global lock for the global registry and then a separate lock as a member of the credentials object to guide its local map.

That having been said, it's really not clear to me that we need to offer thread-safety for the local credential map in the first place. I think it would be fine to document that only one thread may add control plane creds at any one time and that new control plane creds must not be added after the credential has been used to create a channel.

I do agree that the lock for the global registry is a good idea, because there is no way for a given piece of code that wants to add an entry to the global map to know whether there is already a gRPC channel in use someplace that may be accessing it. But when attaching to the creds that the application is about to pass to a channel, that problem should not exist, which is why I think the lock on the local map is unnecessary.

src/core/lib/security/credentials/credentials.cc, line 52 at r2 (raw file):

                      grpc_core::StringLess>* g_grpc_control_plane_creds;
static gpr_mu g_control_plane_creds_mu;
gpr_once once_init_control_plane_creds;

I don't think this needs to be global. We can move it inside of grpc_control_plane_credentials_init() and make it static.

Also, this needs to be initialized to GPR_ONCE_INIT.

src/core/lib/security/credentials/credentials.cc, line 80 at r2 (raw file):

    grpc_core::MutexLock lock(&g_control_plane_creds_mu);
    if (g_grpc_control_plane_creds->find(grpc_core::UniquePtr<char>(
            gpr_strdup(authority))) != g_grpc_control_plane_creds->end()) {

Let's not duplicate the string twice. We can do it once and then use the same string in both the lookup here and the insertion below.

src/core/lib/security/credentials/credentials.cc, line 94 at r2 (raw file):

  grpc_core::MutexLock lock(&g_control_plane_creds_mu);
  auto local_lookup = local_control_plane_creds_.find(
      grpc_core::UniquePtr<char>(gpr_strdup(authority)));

Same here.

src/core/lib/security/credentials/credentials.cc, line 108 at r2 (raw file):

    grpc_core::MutexLock lock(&g_control_plane_creds_mu);
    auto local_lookup = local_control_plane_creds_.find(
        grpc_core::UniquePtr<char>(gpr_strdup(authority)));

Same here.

test/core/security/control_plane_credentials_test.cc, line 199 at r2 (raw file):

  GPR_ASSERT(status == GRPC_STATUS_OK);
  // Extract the ascii value of the authorization header, if present
  grpc_core::UniquePtr<char> authorization_header_val = nullptr;

No need for = nullptr.

Reviewable status: 7 of 10 files reviewed, 6 unresolved discussions (waiting on @jiangtaoli2016 and @markdroth)

src/core/lib/security/credentials/credentials.h, line 165 at r1 (raw file):

Yeah I agree that this API shouldn't need to offer thread-safety, since it presumably should only be used at credential construction time. I've updated to add that mentioned comment in credentials.h

src/core/lib/security/credentials/credentials.cc, line 52 at r2 (raw file):

Done

src/core/lib/security/credentials/credentials.cc, line 80 at r2 (raw file):

Done

src/core/lib/security/credentials/credentials.cc, line 94 at r2 (raw file):

Done.

src/core/lib/security/credentials/credentials.cc, line 108 at r2 (raw file):

Done.

test/core/security/control_plane_credentials_test.cc, line 199 at r2 (raw file):

Done.

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

Reviewed 3 of 3 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @apolcyn and @jiangtaoli2016)

src/core/lib/security/credentials/credentials.h, line 181 at r3 (raw file):

    attach control plane creds to a given credentials object at
    any one time, and new control plane creds must not be
    attached after \a credentials has been used to create a channel.*/

Nit: Please add a space after the end of the sentence.

Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @jiangtaoli2016 and @markdroth)

src/core/lib/security/credentials/credentials.h, line 181 at r3 (raw file):

Done. Thanks for the review!

This looks great!

Reviewed 1 of 1 files at r4, 7 of 7 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jiangtaoli2016)

Looks great! Thanks Alex for implementation!