[3PI Support] Add base external account credentials class

[renkelvin]

Structure of 3PI Support

The implementation of 3PI support is organized as follows:
1. Base external account credentials class [This PR]
2. Subclasses to base external account credentials
3. Internal and public apis of external account credentials

Description of this PR

This PR adds the base external account credentials.

• The base class is similar to STSTokenFetcherCredentials, which inherits from grpc_oauth2_token_fetcher_credentials and override fetch_oauth2 to provide an access token.
• The main difference between the base class andSTSTokenFetcherCredentials is that the base class makes a sequence of HTTP calls while STSTokenFetcherCredentials makes only one. So there is additional TokenFetchContext struct and multiple static methods implemented. Please see go/grpc-sequenced-https-calls for more details.

@ZhenLian Could you please help review this PR at language level? Please let me know if you have any questions. Thanks!

---

This change is 

[ZhenLian]

LGTM overall. I don't have much context so my review would be limited to the language level. You can find someone who might be more familiar with the context to review now :)

[renkelvin]

Thanks @ZhenLian !

Hey @markdroth, this PR should be ready to review. Please take a look and let me if you have any questions!

[renkelvin]

Reviewable status: 0 of 19 files reviewed, 2 unresolved discussions (waiting on @markdroth and @ZhenLian)

---

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

Previously, ZhenLian wrote…

s/2019/2020

Done.

[markdroth]

I'm afraid that I'm not really seeing any significant improvement from the last PR. This still seems very disorganized and I don't see any structure that tells me how what parts of the code are supposed to be doing what.

I've attempted to add some comments to indicate what you should be thinking about here, but I'm going to once again request that you find someone more senior who is fluent in C++ and can help you with these design issues. If you can't do that, this is going to take a long time, because I don't have the cycles to put this kind of time into every review iteration.

I haven't reviewed the tests, since there were so many issues with the code itself. I can do that later, once the code is in good shape.

I hope these comments are helpful.

Reviewed 16 of 19 files at r1, 2 of 2 files at r2.
Reviewable status: 18 of 19 files reviewed, 25 unresolved discussions (waiting on @markdroth, @renkelvin, and @ZhenLian)

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 28 at r2 (raw file):

#include "src/core/lib/security/credentials/oauth2/oauth2_credentials.h"

using grpc_core::Json;

This is not allowed by the style guide:

https://google.github.io/styleguide/cppguide.html#Namespaces (see last bullet in that section)

It's also not actually necessary here, because all of your code is inside the grpc_core namespace anyway.

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 34 at r2 (raw file):

// External account credentials json interface.
struct ExternalAccountCredentialsOptions {

This struct can be in the public section of ExternalAccountCredentials.

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 49 at r2 (raw file):

// This is a helper struct to pass information between multiple callback based
// asynchronous calls.
struct TokenFetchContext {

I think the design still needs work here. It still appears to be structured as a single flat struct with a whole slew of fields shoved into it and a bunch of standalone functions that operate on that struct. That approach would make sense for C code, but C++ code is expected to provide better encapsulation. In general, you should seek to avoid exposing details where they aren't needed, both in terms of namespace and in terms of access control.

For example, the API currently appears to be exposing this entire struct to subclasses. Is that actually necessary? What functionality is going to be the same for all subclasses and what functionality will differ between them? Can we provide an interface for subclasses to implement that gives the subclass only the information that it needs to do its subclass-specific work, and have the parent class or some other shared code take care of the rest of it? I'm guessing that the only things that subclasses really need to do are populating the initial HTTP request and specifying the URI to talk to. Do they actually need access to all of this information in order to do that? If not, let's provide an interface that lets them do what they need without exposing them to all of the state that they don't care about.

On a related note, this struct seems to actually represent an in-flight token-fetch request, and most of the standalone functions in the .cc file are effectively methods to execute the request. Given that, I think this should be a class, and I think those standalone functions should be methods of this class.

One option to consider would be to define this class as the thing that subclasses will define, with virtual methods to perform the parts that they need to do. Another option would be to have the subclass implement a separate interface that is used by this class, and keep this class private within the .cc file.

If this struct does stay as part of the subclass API, it should be a member of ExternalAccountCredentials, in the protected section.

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 58 at r2 (raw file):

  grpc_millis deadline;
  // The options and scopes carried from external account credentials.
  ExternalAccountCredentialsOptions options;

There is no need to copy these two fields. Instead, these can be const pointers to the fields in the ExternalAccountCredentials object, since we know that object will outlive this one.

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 83 at r2 (raw file):

 protected:
  // Subclasses of base external account credentials need to override this
  // method to implement the specific subject token retrieval logic.

This needs to document exactly what subclasses are required to do. For example, if there are particular fields that need to be populated or callbacks that need to be invoked when the token is retrieved, please indicate what those are. The explanation should be clear enough that a reader can tell whether a given subclass implementation meets the requirements.

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 86 at r2 (raw file):

  virtual void RetrieveSubjectToken(TokenFetchContext* ctx) = 0;

  ExternalAccountCredentialsOptions options_;

Data members must be private, as per the style guide.

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

If you want to make these accessible to subclasses, you should provide accessor methods. Although from the implementation, it's not clear to me why these need to be exposed to begin with.

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 97 at r2 (raw file):

                    grpc_millis deadline) override;

  TokenFetchContext* ctx_;

This should be initialized to nullptr.

---

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

Previously, renkelvin (Chuan Ren) wrote…

I think technically, it can be moved but I'm not sure if it's necessary.

Why make a copy if you don't need to? This seems like needless inefficiency.

---

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

  }
  scopes_ = scopes;
  ctx_ = new TokenFetchContext;

Why allocate this automatically if we aren't actually sending a request?

Instead, I suggest allocating it when you start a request in fetch_oauth2(), and then de-allocating it as soon as the request is finished. There's no reason to hold on to memory that we're not actually using.

---

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

ExternalAccountCredentials::~ExternalAccountCredentials() {
  grpc_http_response_destroy(&ctx_->token_exchange_response);

These two lines should be in the TokenFetchContex dtor.

---

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

  grpc_http_response_destroy(&ctx_->token_exchange_response);
  grpc_http_response_destroy(&ctx_->service_account_impersonate_response);
  delete ctx_;

This should be done in FinishTokenFetch(), which should also reset the ctx_ field in ExternalAccountCredentials to null to indicate that it no longer has a request in flight.

---

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

}

static void FinishTokenFetch(TokenFetchContext* ctx, grpc_error* error) {

Please use an anonymous namespace instead of declaring things static.

Or better yet, make these functions methods of the TokenFetchContext class, as I suggested elsewhere.

---

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

    return;
  }
  std::string response_body =

This needlessly allocates a new string. Instead, you can do this:

absl::string_view response_body(
    ctx->service_account_impersonate_response.body,
    ctx->service_account_impersonate_response.body_length);

---

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

      std::string(ctx->service_account_impersonate_response.body,
                  ctx->service_account_impersonate_response.body_length);
  Json::Object::const_iterator it;

Please define this where it is first used instead of doing it here. And you can use auto for it.

---

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

  Json::Object::const_iterator it;
  Json json = Json::Parse(response_body.c_str(), &error);
  std::string access_token =

There is never any need to say std::string foo = std::string(...). Instead, you can just say std::string foo(...).

Same thing throughout.

---

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

      "{\"access_token\":\"%s\", \"expires_in\":%d, \"token_type\":\"%s\"}",
      access_token, expire_in, "Bearer");

Please remove blank lines within functions.

---

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

static void ServiceAccountImpersenate(TokenFetchContext* ctx) {
  Json::Object::const_iterator it;

Please declare this where it's first used instead of doing it here.

---

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

  Json::Object::const_iterator it;
  grpc_error* error = GRPC_ERROR_NONE;
  std::string response_body =

This needlessly allocates a new string. Instead, you can do this:

absl::string_view response_body(
    ctx->token_exchange_response.body,
    ctx->token_exchange_response.body_length);

---

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

                  ctx->token_exchange_response.body_length);
  Json json = Json::Parse(response_body.c_str(), &error);
  if (error != GRPC_ERROR_NONE || json.type() != Json::Type::OBJECT) {

If there is an error here, you are holding a ref to the error, so you need to either pass it somewhere or call GRPC_ERROR_UNREF() on it. Otherwise, there will be a memory leak.

Also, you should include this error in the one you are passing to FinishTokenFetch().

---

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

  grpc_httpcli_post(ctx->httpcli_context, ctx->pollent, resource_quota,
                    &request, body.c_str(), body.size(), ctx->deadline,
                    GRPC_CLOSURE_CREATE(OnServiceAccountImpersenate, ctx,

This allocates memory, which we should avoid doing unnecessarily. Instead, let's put a grpc_closure data member in TokenFetchContext for this.

---

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

  TokenFetchContext* ctx = static_cast<TokenFetchContext*>(arg);
  if (error != GRPC_ERROR_NONE) {
    FinishTokenFetch(ctx, error);

Need GRPC_ERROR_REF() here, since callback functions do not take ownership of a ref to the error they are passed, but other functions do.

https://github.com/grpc/grpc/blob/master/doc/core/grpc-error.md#ownership-rules

---

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

      ctx->httpcli_context, ctx->pollent, resource_quota, &request,
      body.c_str(), body.size(), ctx->deadline,
      GRPC_CLOSURE_CREATE(OnTokenExchange, ctx, grpc_schedule_on_exec_ctx),

This allocates memory, which we should avoid doing unnecessarily. Instead, let's put a grpc_closure data member in TokenFetchContext for this.

(We can use the same closure for both requests, since only one will be in flight at a time.)

---

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

// The token fetching flow:
// 1. Retrieve subject token - Call RetrieveSubjectToken() and get the subject

It looks like you're making 3 different HTTP requests here. We don't yet support cancellation for an individual HTTP request (see #11781), so grpc_oauth2_token_fetcher_credentials doesn't bother trying to cancel what it expects will be a single request. But if the channel gets shut down while the first request is in flight, we should probably have some way of seeing that cancelation so that we can avoid starting the remaining requests.

I won't insist that you fix this as part of this PR, but please do add a TODO about it.

---

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

    grpc_httpcli_context* httpcli_context, grpc_polling_entity* pollent,
    grpc_iomgr_cb_func response_cb, grpc_millis deadline) {
  ctx_->metadata_req = metadata_req;

These parameters can all be passed to the TokenFetchContext ctor.

[renkelvin]

Reviewable status: 18 of 19 files reviewed, 25 unresolved discussions (waiting on @markdroth, @renkelvin, and @ZhenLian)

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 49 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I think the design still needs work here. It still appears to be structured as a single flat struct with a whole slew of fields shoved into it and a bunch of standalone functions that operate on that struct. That approach would make sense for C code, but C++ code is expected to provide better encapsulation. In general, you should seek to avoid exposing details where they aren't needed, both in terms of namespace and in terms of access control.

For example, the API currently appears to be exposing this entire struct to subclasses. Is that actually necessary? What functionality is going to be the same for all subclasses and what functionality will differ between them? Can we provide an interface for subclasses to implement that gives the subclass only the information that it needs to do its subclass-specific work, and have the parent class or some other shared code take care of the rest of it? I'm guessing that the only things that subclasses really need to do are populating the initial HTTP request and specifying the URI to talk to. Do they actually need access to all of this information in order to do that? If not, let's provide an interface that lets them do what they need without exposing them to all of the state that they don't care about.

On a related note, this struct seems to actually represent an in-flight token-fetch request, and most of the standalone functions in the .cc file are effectively methods to execute the request. Given that, I think this should be a class, and I think those standalone functions should be methods of this class.

One option to consider would be to define this class as the thing that subclasses will define, with virtual methods to perform the parts that they need to do. Another option would be to have the subclass implement a separate interface that is used by this class, and keep this class private within the .cc file.

If this struct does stay as part of the subclass API, it should be a member of ExternalAccountCredentials, in the protected section.

Thanks for your suggestions and I understand that C++ code should provide better encapsulation.

I actually tried to encapsulate the whole logic into ExternalAccountCredentials, however it seems not very feasible. The reason is that grpc_httpcli_post method accepts a grpc_closure* as callback, which must be created with a grpc_iomgr_cb_func, so the callback functions have to be standalone to be a type of grpc_iomgr_cb_func.

It seems most of the issues are related to this one. So I'd like to know your thoughts before proceed.

[markdroth]

Reviewable status: 18 of 19 files reviewed, 25 unresolved discussions (waiting on @renkelvin and @ZhenLian)

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 49 at r2 (raw file):

Previously, renkelvin (Chuan Ren) wrote…

Thanks for your suggestions and I understand that C++ code should provide better encapsulation.

I actually tried to encapsulate the whole logic into ExternalAccountCredentials, however it seems not very feasible. The reason is that grpc_httpcli_post method accepts a grpc_closure* as callback, which must be created with a grpc_iomgr_cb_func, so the callback functions have to be standalone to be a type of grpc_iomgr_cb_func.

It seems most of the issues are related to this one. So I'd like to know your thoughts before proceed.

The pre-bound parameter can be this, and the callback function can be a static method of the class, like this:

class MyClass {
 public:
  void DoHttpRequest() {
    GRPC_CLOSURE_INIT(&closure_, OnHttpRequestComplete, this, nullptr);
    // ...set up HTTP request...
    grpc_httpcli_post(..., &closure_, ...);
  }

 private:
  static void OnHttpRequestComplete(void* arg, grpc_error* error) {
    MyClass* self = static_cast<MyClass*>(arg);
    self->OnHttpRequestCompleteInternal(GRPC_ERROR_REF(error));
  }

  void OnHttpRequestCompleteInternal(grpc_error* error) {
    // handle callback here
  }

  grpc_closure closure_;
};

[renkelvin]

The code is now refactored to provide better encapsulation.

I'll find someone with C++ readability to help me with this PR. Thanks for your review.

Reviewable status: 18 of 19 files reviewed, 25 unresolved discussions (waiting on @markdroth and @ZhenLian)

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 28 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This is not allowed by the style guide:

https://google.github.io/styleguide/cppguide.html#Namespaces (see last bullet in that section)

It's also not actually necessary here, because all of your code is inside the grpc_core namespace anyway.

Done.

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 34 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This struct can be in the public section of ExternalAccountCredentials.

Done.

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 49 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

The pre-bound parameter can be this, and the callback function can be a static method of the class, like this:

class MyClass {
 public:
  void DoHttpRequest() {
    GRPC_CLOSURE_INIT(&closure_, OnHttpRequestComplete, this, nullptr);
    // ...set up HTTP request...
    grpc_httpcli_post(..., &closure_, ...);
  }

 private:
  static void OnHttpRequestComplete(void* arg, grpc_error* error) {
    MyClass* self = static_cast<MyClass*>(arg);
    self->OnHttpRequestCompleteInternal(GRPC_ERROR_REF(error));
  }

  void OnHttpRequestCompleteInternal(grpc_error* error) {
    // handle callback here
  }

  grpc_closure closure_;
};

Done. The approach above looks cool.

Here is a summary of the changes to provide better encapsulation:

1. Eliminate the fields of TokenFetchContext that are not necessarily exposed to subclasses. It now only includes the required information to make http requests. Move the definition to protected section.
2. Make standalone functions the methods of ExternalAccountCredentials. This is a bit different with your suggestion (making them methods of TokenFetchContext), because ExternalAccountCredentials now holds the full information while TokenFetchContext includes the minimum shareable information.
3. Make data fields of ExternalAccountCredentials private (and no accessor introduced). Pass const pointers to TokenFetchContext and ExternalAccountCredentialsOptions to subclasses to retrieve subject token. These two should be the minimum information needed.

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 58 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

There is no need to copy these two fields. Instead, these can be const pointers to the fields in the ExternalAccountCredentials object, since we know that object will outlive this one.

Done. These two fields are removed from the context struct so they won't be always exposed. They will be passed to subclasses when needed.

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 83 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This needs to document exactly what subclasses are required to do. For example, if there are particular fields that need to be populated or callbacks that need to be invoked when the token is retrieved, please indicate what those are. The explanation should be clear enough that a reader can tell whether a given subclass implementation meets the requirements.

Done.

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 86 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Data members must be private, as per the style guide.

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

If you want to make these accessible to subclasses, you should provide accessor methods. Although from the implementation, it's not clear to me why these need to be exposed to begin with.

Done.

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 97 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This should be initialized to nullptr.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

Why make a copy if you don't need to? This seems like needless inefficiency.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

Why allocate this automatically if we aren't actually sending a request?

Instead, I suggest allocating it when you start a request in fetch_oauth2(), and then de-allocating it as soon as the request is finished. There's no reason to hold on to memory that we're not actually using.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

These two lines should be in the TokenFetchContex dtor.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

This should be done in FinishTokenFetch(), which should also reset the ctx_ field in ExternalAccountCredentials to null to indicate that it no longer has a request in flight.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

Please use an anonymous namespace instead of declaring things static.

Or better yet, make these functions methods of the TokenFetchContext class, as I suggested elsewhere.

Done. These functions are now methods of ExternalAccountCredentials class.

---

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

Previously, markdroth (Mark D. Roth) wrote…

This needlessly allocates a new string. Instead, you can do this:

absl::string_view response_body(
    ctx->service_account_impersonate_response.body,
    ctx->service_account_impersonate_response.body_length);

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

Please define this where it is first used instead of doing it here. And you can use auto for it.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

There is never any need to say std::string foo = std::string(...). Instead, you can just say std::string foo(...).

Same thing throughout.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

Please remove blank lines within functions.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

Please declare this where it's first used instead of doing it here.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

This needlessly allocates a new string. Instead, you can do this:

absl::string_view response_body(
    ctx->token_exchange_response.body,
    ctx->token_exchange_response.body_length);

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

If there is an error here, you are holding a ref to the error, so you need to either pass it somewhere or call GRPC_ERROR_UNREF() on it. Otherwise, there will be a memory leak.

Also, you should include this error in the one you are passing to FinishTokenFetch().

Got it. Now the ownership of the error is passed to FinishTokenFetch() where GRPC_ERROR_UNREF() is called.

---

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

Previously, markdroth (Mark D. Roth) wrote…

This allocates memory, which we should avoid doing unnecessarily. Instead, let's put a grpc_closure data member in TokenFetchContext for this.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

Need GRPC_ERROR_REF() here, since callback functions do not take ownership of a ref to the error they are passed, but other functions do.

https://github.com/grpc/grpc/blob/master/doc/core/grpc-error.md#ownership-rules

Got it. GRPC_ERROR_REF() is now called in the upper level callback method.
For example:

static void OnTokenExchange(void* arg, grpc_error* error) {
    ExternalAccountCredentials
        * self = static_cast<ExternalAccountCredentials*>(arg);
    self->OnTokenExchangeInternal(GRPC_ERROR_REF(error)); <- GRPC_ERROR_REF() called.
 }

We just pass the ownership to FinishTokenFetch().

---

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

Previously, markdroth (Mark D. Roth) wrote…

This allocates memory, which we should avoid doing unnecessarily. Instead, let's put a grpc_closure data member in TokenFetchContext for this.

(We can use the same closure for both requests, since only one will be in flight at a time.)

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

It looks like you're making 3 different HTTP requests here. We don't yet support cancellation for an individual HTTP request (see #11781), so grpc_oauth2_token_fetcher_credentials doesn't bother trying to cancel what it expects will be a single request. But if the channel gets shut down while the first request is in flight, we should probably have some way of seeing that cancelation so that we can avoid starting the remaining requests.

I won't insist that you fix this as part of this PR, but please do add a TODO about it.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

These parameters can all be passed to the TokenFetchContext ctor.

Done.

[renkelvin]

It seems the diff is a bit inaccurate due to bulk changes. Added a few comments to help locate the changes.

Reviewable status: 16 of 19 files reviewed, 25 unresolved discussions (waiting on @markdroth and @ZhenLian)

---

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

Previously, renkelvin (Chuan Ren) wrote…

Done.

It seems the diff is a bit off. Please search "~TokenFetchContext()" in page to see the new TokenFetchContext struct.

---

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

Previously, renkelvin (Chuan Ren) wrote…

Done.

It seems the diff is a bit off. Please search "~TokenFetchContext()" in page to see the new TokenFetchContext struct.

---

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

Previously, renkelvin (Chuan Ren) wrote…

Done.

It seems the diff is a bit off. You can search "TODO(chuanr)" in page to see the fix.

[duobianxing]

Reviewable status: 16 of 19 files reviewed, 40 unresolved discussions (waiting on @markdroth, @renkelvin, and @ZhenLian)

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 97 at r2 (raw file):

Previously, renkelvin (Chuan Ren) wrote…

Done.

Is this ctr_ used anywhere?

---

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

scopes

why this is not part of "options"?

---

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

TokenFetchContext

This looks like purely "HTTPRequestContext"? TokenFetchContext gives me the impression that there is something related the token.

---

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

Quoted 8 lines of code…

TokenFetchContext(grpc_httpcli_context* httpcli_context,
                      grpc_polling_entity* pollent, grpc_millis deadline)
        : httpcli_context(httpcli_context),
          pollent(pollent),
          deadline(deadline) {
      closure = {};
      response = {};
    }

nit: will this work?

TokenFetchContext(grpc_httpcli_context* httpcli_context,
grpc_polling_entity* pollent, grpc_millis deadline)
: httpcli_context(httpcli_context),
pollent(pollent),
deadline(deadline),
closure({}),
reponse({}), {
}

---

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

const TokenFetchContext* ctx,

Does caller need to pass in "ctx", isn't ctr_ already saved in the instance?

Also, can caller pass in nullptr "ctx"? If not, let's use "const TokenFetchContext& ctx" to forbidden that.

---

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

fetch_oauth2

Where this function was referenced or called?

---

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

static void OnTokenExchange

Why static here and couple functions below? Is this because of the grpc method we are using?

---

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

static

Question: why static here and above couple functions?

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 42 at r3 (raw file):

GOOGLE_CLOUD_PLATFORM_DEFAULT_SCOPE

I suggest that when customers provide an empty list, we directly push_back to scopes_ and return here to avoid a copy later on

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 57 at r3 (raw file):

Quoted 12 lines of code…

// The token fetching flow:
// 1. Retrieve subject token - Call RetrieveSubjectToken() and get the subject
// token in OnRetrieveSubjectToken().
// 2. Token exchange - Make a token exchange call in TokenExchange() with the
// subject token in #1. Get the response in OnTokenExchange().
// 3. (Optional) Service account impersonation - Make a http call in
// ServiceAccountImpersenate() with the access token in #2. Get an impersonated
// access token in OnServiceAccountImpersenate().
// 4. Return back the response that contains an access token in
// FinishTokenFetch().
// TODO(chuanr): Avoid starting the remaining requests if the channel gets shut
// down.

Is there a test that does this flow explicitly? Maybe a test will help to demonstrate to reviewer on how to use this class?

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 85 at r3 (raw file):

}

void ExternalAccountCredentials::TokenExchange(std::string subject_token) {

This function seems extremely long and hard for read later on.

I suggest grouping lines into smaller private functions and give meaningful names.

For example something like:

void ExternalAccountCredentials::TokenExchange(std::string subject_token) {

.....
request.http.hdrs = GenereateHeaderFromTokenExchangeOptions(options_);
std::string body = GenerateRequestBodyFromTokenExchangeOptions(options_);
....

}

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 170 at r3 (raw file):

}

void ExternalAccountCredentials::ServiceAccountImpersenate() {

Nit: break down this function into smaller function so it is easy to ready & maintain?

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 174 at r3 (raw file):

Parse
``

Does this "Parse" function have any configuration that allow caller to loose the json parse rule? Something like: "ignore whitespace"

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 211 at r3 (raw file):

"Authorization"

Nit: can we name this constant to a variable: GCP_AUTH_HEADER_KEY? This will clarify to reader what this constant meant.

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 230 at r3 (raw file):

OnServiceAccountImpersenate

Nit: function should be call onImpersonateServiceAccount, since it's WL Pool to impersonateServiceAccount, not ServiceAccount to impersonate.

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 297 at r3 (raw file):

delete ctx_;

I notice delete happens only here. What if caller call function like this: fetch_oauth2 -> fetch_oauth2 -> FinishTokenFetch? Will this create memory leak?

[duobianxing]

Hello, this is Will. I took a look at the class first and put some comments. Mostly related to C++ language and plus a bit understanding of the flow from my side.

Feel free to let me know if you want to clarify some of my comments.

Reviewable status: 16 of 19 files reviewed, 40 unresolved discussions (waiting on @markdroth, @renkelvin, and @ZhenLian)

[renkelvin]

Reviewable status: 16 of 19 files reviewed, 38 unresolved discussions (waiting on @duobianxing, @markdroth, and @ZhenLian)

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 97 at r2 (raw file):

Previously, duobianxing (duobianxing) wrote…

Is this ctr_ used anywhere?

Yeah, ctx_ is the http request context that will used in 1. retrieving subject token 2. sts token exchange 3. impersonate service account

---

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

Previously, duobianxing (duobianxing) wrote…

scopes

why this is not part of "options"?

It's based on the spec defined in go/guac-3pi. options is generated by gcloud given the 3rd party credentials. After that, developers can use it with different scopes.

---

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

Previously, duobianxing (duobianxing) wrote…

TokenFetchContext

This looks like purely "HTTPRequestContext"? TokenFetchContext gives me the impression that there is something related the token.

Done. Yeah, sounds better.

---

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

Previously, duobianxing (duobianxing) wrote…

TokenFetchContext(grpc_httpcli_context* httpcli_context,
                      grpc_polling_entity* pollent, grpc_millis deadline)
        : httpcli_context(httpcli_context),
          pollent(pollent),
          deadline(deadline) {
      closure = {};
      response = {};
    }

nit: will this work?

TokenFetchContext(grpc_httpcli_context* httpcli_context,
grpc_polling_entity* pollent, grpc_millis deadline)
: httpcli_context(httpcli_context),
pollent(pollent),
deadline(deadline),
closure({}),
reponse({}), {
}

Done. Yeah, seems better.

---

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

Previously, duobianxing (duobianxing) wrote…

const TokenFetchContext* ctx,

Does caller need to pass in "ctx", isn't ctr_ already saved in the instance?

Also, can caller pass in nullptr "ctx"? If not, let's use "const TokenFetchContext& ctx" to forbidden that.

Yeah, ctx_ is already in the instance as a private data member. To expose it to the subclasses, we can either make a protected accessor, or pass it as a parameter of the virtual method.
I prefer the later approach since the access seems more limited. What do you think?
It can't be nullptr.

---

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

Previously, duobianxing (duobianxing) wrote…

fetch_oauth2

Where this function was referenced or called?

It's called in grpc_oauth2_token_fetcher_credentials::get_request_metadata

grpc/src/core/lib/security/credentials/oauth2/oauth2_credentials.cc

Line 319 in cdb3b82

fetch_oauth2(grpc_credentials_metadata_request_create(this->Ref()),

---

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

Previously, duobianxing (duobianxing) wrote…

static void OnTokenExchange

Why static here and couple functions below? Is this because of the grpc method we are using?

Yeah, grpc_httpcli_post requires the callback function to be static. You can see Mark's comments (start with "The pre-bound parameter can be this") above.

---

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

Previously, duobianxing (duobianxing) wrote…

static

Question: why static here and above couple functions?

Because grpc_httpcli_post requires a static function pointer as the callback.

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 42 at r3 (raw file):

Previously, duobianxing (duobianxing) wrote…

GOOGLE_CLOUD_PLATFORM_DEFAULT_SCOPE

I suggest that when customers provide an empty list, we directly push_back to scopes_ and return here to avoid a copy later on

Added std::move() on I think so there shouldn't be a copy later on.

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 57 at r3 (raw file):

Previously, duobianxing (duobianxing) wrote…

// The token fetching flow:
// 1. Retrieve subject token - Call RetrieveSubjectToken() and get the subject
// token in OnRetrieveSubjectToken().
// 2. Token exchange - Make a token exchange call in TokenExchange() with the
// subject token in #1. Get the response in OnTokenExchange().
// 3. (Optional) Service account impersonation - Make a http call in
// ServiceAccountImpersenate() with the access token in #2. Get an impersonated
// access token in OnServiceAccountImpersenate().
// 4. Return back the response that contains an access token in
// FinishTokenFetch().
// TODO(chuanr): Avoid starting the remaining requests if the channel gets shut
// down.

Is there a test that does this flow explicitly? Maybe a test will help to demonstrate to reviewer on how to use this class?

Yeah, there are tests covering these flows. (with and without step 3).
The comments here seem a bit confusing. These steps are automatically triggered after fetch_oauth2() is called. Developers don't need to call them explicitly. Slightly changed the comments.

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 85 at r3 (raw file):

Previously, duobianxing (duobianxing) wrote…

This function seems extremely long and hard for read later on.

I suggest grouping lines into smaller private functions and give meaningful names.

For example something like:

void ExternalAccountCredentials::TokenExchange(std::string subject_token) {

.....
request.http.hdrs = GenereateHeaderFromTokenExchangeOptions(options_);
std::string body = GenerateRequestBodyFromTokenExchangeOptions(options_);
....

}

I agree that the function is long. However, grouping them into smaller functions may slightly decrease the efficiency? I'm not sure if the gRPC library owner prefer it.

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 170 at r3 (raw file):

I agree that the function is long. However, grouping them into smaller functions may slightly decrease the efficiency? I'm not sure if the gRPC library owner prefer it.
Same as above comments.

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 174 at r3 (raw file):

Previously, duobianxing (duobianxing) wrote…

Parse
``

Does this "Parse" function have any configuration that allow caller to loose the json parse rule? Something like: "ignore whitespace"

I don't think so.

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 211 at r3 (raw file):

Previously, duobianxing (duobianxing) wrote…

"Authorization"

Nit: can we name this constant to a variable: GCP_AUTH_HEADER_KEY? This will clarify to reader what this constant meant.

Not sure if it's more clear since Authorization is a commonly used header key, not only for GCP.

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 230 at r3 (raw file):

Previously, duobianxing (duobianxing) wrote…

OnServiceAccountImpersenate

Nit: function should be call onImpersonateServiceAccount, since it's WL Pool to impersonateServiceAccount, not ServiceAccount to impersonate.

Done. Good point.

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 297 at r3 (raw file):

Previously, duobianxing (duobianxing) wrote…

delete ctx_;

I notice delete happens only here. What if caller call function like this: fetch_oauth2 -> fetch_oauth2 -> FinishTokenFetch? Will this create memory leak?

FinishTokenFetch() is a private function and we make sure it's automatically called when fetch_oauth2() is about to finish. So the new and delete should always match.

[renkelvin]

Thanks Will! I made some improvements based on your comments and our discussions. Please let me know if there is any questions!

Reviewable status: 16 of 19 files reviewed, 38 unresolved discussions (waiting on @duobianxing, @markdroth, and @ZhenLian)

---

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

Previously, renkelvin (Chuan Ren) wrote…

Yeah, ctx_ is already in the instance as a private data member. To expose it to the subclasses, we can either make a protected accessor, or pass it as a parameter of the virtual method.
I prefer the later approach since the access seems more limited. What do you think?
It can't be nullptr.

Actually, I just realized that ctx can be null for local subject token fetch (like file-sourced credentials).

[duobianxing]

Reviewed 1 of 3 files at r4.
Reviewable status: 17 of 19 files reviewed, 25 unresolved discussions (waiting on @markdroth and @ZhenLian)

[duobianxing]

Reviewed 1 of 2 files at r2.
Reviewable status: 17 of 19 files reviewed, 25 unresolved discussions (waiting on @markdroth and @ZhenLian)

[duobianxing]

Reviewed 16 of 19 files at r1, 2 of 3 files at r4.
Reviewable status: all files reviewed, 25 unresolved discussions (waiting on @markdroth and @ZhenLian)

[duobianxing]

LGTM, thanks for taking my input. The callback implementation looks really clean now.

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

[duobianxing]

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

[markdroth]

This looks much cleaner now! Remaining comments are relatively minor.

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

Reviewed 2 of 3 files at r4.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @renkelvin and @ZhenLian)

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 49 at r2 (raw file):

Previously, renkelvin (Chuan Ren) wrote…

Done. The approach above looks cool.

Here is a summary of the changes to provide better encapsulation:

1. Eliminate the fields of TokenFetchContext that are not necessarily exposed to subclasses. It now only includes the required information to make http requests. Move the definition to protected section.
2. Make standalone functions the methods of ExternalAccountCredentials. This is a bit different with your suggestion (making them methods of TokenFetchContext), because ExternalAccountCredentials now holds the full information while TokenFetchContext includes the minimum shareable information.
3. Make data fields of ExternalAccountCredentials private (and no accessor introduced). Pass const pointers to TokenFetchContext and ExternalAccountCredentialsOptions to subclasses to retrieve subject token. These two should be the minimum information needed.

Thanks, this looks much better!

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 29 at r4 (raw file):

namespace grpc_core {
namespace experimental {

No need to use experimental namespace for code inside of C-core. That's only needed for public APIs.

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 46 at r4 (raw file):

    std::string token_url;
    std::string token_info_url;
    grpc_core::Json credential_source;

No need for grpc_core:: prefix here, since you're already inside of that namespace.

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 66 at r4 (raw file):

          pollent(pollent),
          deadline(deadline),
          closure({}),

There's no need to initialize either closure or response here.

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 68 at r4 (raw file):

          closure({}),
          response({}) {}
    ~HTTPRequestContext() { grpc_http_response_destroy(&response); }

I think you need to call grpc_http_response_destroy() after each individual HTTP request. Otherwise, each response will just append data the previous response (both headers and body), which I don't think is what you want.

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 89 at r4 (raw file):

      const HTTPRequestContext* ctx,
      const ExternalAccountCredentialsOptions& options,
      std::function<void(std::string, grpc_error*)> cb) = 0;

The subclass implementation of this method is going to do an HTTP request, right? In that case, it will need to use the grpc_closure as a callback when the request is complete, since that's the API of grpc_httpcli_post(). Is the subclass going to have to clean up anything when the HTTP request is done, before it invokes this callback to tell the base class that it's done? If not, then using std::function<> doesn't really make sense, because that will be requiring the subclass to create its own grpc_closure callback function just for the purpose of invoking this std::function<>.

Instead, I suggest just pre-populating ctx->closure to call OnRetrieveSubjectToken() and telling the subclass to use that closure as the callback for the HTTP request.

---

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

#include "absl/time/clock.h"
#include "absl/time/time.h"

Please re-add this blank line, as per the style guide.

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 71 at r4 (raw file):

    grpc_httpcli_context* httpcli_context, grpc_polling_entity* pollent,
    grpc_iomgr_cb_func response_cb, grpc_millis deadline) {
  ctx_ = new HTTPRequestContext(httpcli_context, pollent, deadline);

Please check that ctx_ was null before this. Otherwise, if we accidentally start a new fetch while the previous one was still in flight, we will wind up causing a memory leak.

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 74 at r4 (raw file):

  metadata_req_ = metadata_req;
  response_cb_ = response_cb;
  auto cb =

Suggest writing this as a lambda for clarity:

auto cb = [this](std::string token, grpc_error* error) {
  OnRetrieveSubjectTokenInternal(token, error);
};

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 93 at r4 (raw file):

  grpc_uri* uri = grpc_uri_parse(options_.token_url, false);
  if (uri == nullptr) {
    FinishTokenFetch(GRPC_ERROR_CREATE_FROM_STATIC_STRING(

Need to use GRPC_ERROR_CREATE_FROM_COPIED_STRING() if the string is not actually static.

Same thing throughout.

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 183 at r4 (raw file):

    FinishTokenFetch(GRPC_ERROR_CREATE_FROM_STATIC_STRING(
        absl::StrFormat("Invalid token exchange response. Error: %s.",
                        grpc_error_string(error))

Instead of just including the string from the original error, you should actually link in the original error object, like this:

GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING(
    "Invalid token exchange response",  &error, 1)

Same thing throughout.

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 298 at r4 (raw file):

void ExternalAccountCredentials::FinishTokenFetch(grpc_error* error) {
  GRPC_LOG_IF_ERROR("Fetch external account credentials access token",
                    GRPC_ERROR_REF(error));

No need to ref here. This will cause a memory leak.

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 299 at r4 (raw file):

  GRPC_LOG_IF_ERROR("Fetch external account credentials access token",
                    GRPC_ERROR_REF(error));
  response_cb_(metadata_req_, error);

Once you call the response callback, there could be another fetch started before the rest of the data here has been cleared out. To avoid that possibility, I suggest doing this:

// Move object state into local variables.
auto* cb = response_cb_;
response_cb_ = nullptr;
auto* metadata_req = metadata_req_;
metadata_req_ = nullptr;
auto* ctx = ctx_;
ctx_ = nullptr;
// Invoke the callback.
cb(metadata_req, error);
// Delete context.
delete ctx;

[renkelvin]

Thanks for your review!

btw, it seems I'm not able to trigger the CI flow because my limited access to grpc/grpc repo. Is there anything should be fixed?

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

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 29 at r4 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

No need to use experimental namespace for code inside of C-core. That's only needed for public APIs.

Done.

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 46 at r4 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

No need for grpc_core:: prefix here, since you're already inside of that namespace.

Done.

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 66 at r4 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

There's no need to initialize either closure or response here.

Done.

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 68 at r4 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

I think you need to call grpc_http_response_destroy() after each individual HTTP request. Otherwise, each response will just append data the previous response (both headers and body), which I don't think is what you want.

Done.

---

src/core/lib/security/credentials/external/external_account_credentials.h, line 89 at r4 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

The subclass implementation of this method is going to do an HTTP request, right? In that case, it will need to use the grpc_closure as a callback when the request is complete, since that's the API of grpc_httpcli_post(). Is the subclass going to have to clean up anything when the HTTP request is done, before it invokes this callback to tell the base class that it's done? If not, then using std::function<> doesn't really make sense, because that will be requiring the subclass to create its own grpc_closure callback function just for the purpose of invoking this std::function<>.

Instead, I suggest just pre-populating ctx->closure to call OnRetrieveSubjectToken() and telling the subclass to use that closure as the callback for the HTTP request.

The subclass implementation is a bit more complex than an HTTP request.

For url-sourced credentials, it's mostly making a single HTTP request, and retrieving subject token from the response. What you suggest should work.

For file-sourced credentials, the subject token will be retrieved from a local file so there won't be HTTP calls.

For aws-sourced credentials, there would be multiple HTTP requests. After that, we need to manually construct the subject token and pass it back. So passing the response back via grpc_closure may not work.

So I think providing a callback that directly accepts the subject token could be more generic for different types of credentials.

---

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

Previously, markdroth (Mark D. Roth) wrote…

Please re-add this blank line, as per the style guide.

Done.

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 71 at r4 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Please check that ctx_ was null before this. Otherwise, if we accidentally start a new fetch while the previous one was still in flight, we will wind up causing a memory leak.

Done.

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 74 at r4 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Suggest writing this as a lambda for clarity:

auto cb = [this](std::string token, grpc_error* error) {
  OnRetrieveSubjectTokenInternal(token, error);
};

Done.

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 93 at r4 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Need to use GRPC_ERROR_CREATE_FROM_COPIED_STRING() if the string is not actually static.

Same thing throughout.

Done.

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 183 at r4 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Instead of just including the string from the original error, you should actually link in the original error object, like this:

GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING(
    "Invalid token exchange response",  &error, 1)

Same thing throughout.

Done.

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 298 at r4 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

No need to ref here. This will cause a memory leak.

According to rule3 of https://github.com/grpc/grpc/blob/master/doc/core/grpc-error.md#ownership-rules, the ownership of error should be passed to GRPC_LOG_IF_ERROR(). Since we still need access to the error later, we need to increase the ref count by 1 here. Did I misunderstand something here?

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 299 at r4 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

Once you call the response callback, there could be another fetch started before the rest of the data here has been cleared out. To avoid that possibility, I suggest doing this:

// Move object state into local variables.
auto* cb = response_cb_;
response_cb_ = nullptr;
auto* metadata_req = metadata_req_;
metadata_req_ = nullptr;
auto* ctx = ctx_;
ctx_ = nullptr;
// Invoke the callback.
cb(metadata_req, error);
// Delete context.
delete ctx;

Done. Yeah, good point.

[markdroth]

I've triggered the tests to be run. It looks like credentials_test is crashing in several of the tests.

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

Reviewed 3 of 3 files at r5.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @renkelvin and @ZhenLian)

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 71 at r4 (raw file):

Previously, renkelvin (Chuan Ren) wrote…

Done.

If ctx_ is not null when we enter this function, it's a bug, and we should probably just fail immediately. The way you have this code written right now, it will not create a new context, but it will start a new request with the pre-existing context, which may be destroyed at any time. This is not safe.

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 298 at r4 (raw file):

Previously, renkelvin (Chuan Ren) wrote…

According to rule3 of https://github.com/grpc/grpc/blob/master/doc/core/grpc-error.md#ownership-rules, the ownership of error should be passed to GRPC_LOG_IF_ERROR(). Since we still need access to the error later, we need to increase the ref count by 1 here. Did I misunderstand something here?

Oh, looks like you're right. For some reason I didn't think GRPC_LOG_IF_ERROR() took a ref.

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 148 at r5 (raw file):

  grpc_resource_quota* resource_quota =
      grpc_resource_quota_create("external_account_credentials");
  grpc_http_response_destroy(&ctx_->response);

You also need to reset the fields in the response. If you leave them uninitialized, the next request will try to append to what's there, because it uses the counters there to know to append.

Same thing for every request.

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 280 at r5 (raw file):

  if (!absl::ParseTime(absl::RFC3339_full, expire_time, &t, nullptr)) {
    FinishTokenFetch(GRPC_ERROR_CREATE_FROM_COPIED_STRING(
        absl::StrFormat("Invalid expire time of service "

This is a static string. No need for absl::StrFormat(), and you can use GRPC_ERROR_CREATE_FROM_STATIC_STRING().

[renkelvin]

Made a fix the CI and verified the fix by running

./tools/run_tests/run_tests.py -l c++
bazel test //test/core/security:credentials_test --test_output=errors --config=asan
bazel test //test/core/security:credentials_test --test_output=errors --config=msan (there seems to be an existing issue on master)
bazel test //test/core/security:credentials_test --test_output=errors --config=tsan
bazel test //test/core/security:credentials_test --test_output=errors --config=ubsan

I wonder if there is a better way to make sure the CI is green before pushing the code? Thanks!

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

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 71 at r4 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

If ctx_ is not null when we enter this function, it's a bug, and we should probably just fail immediately. The way you have this code written right now, it will not create a new context, but it will start a new request with the pre-existing context, which may be destroyed at any time. This is not safe.

Good point.

While if ctx_ is not null, we may want to clean up the previous context and start the new token fetch instead of failing out. Otherwise, the token fetch will be always blocked.

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 148 at r5 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

You also need to reset the fields in the response. If you leave them uninitialized, the next request will try to append to what's there, because it uses the counters there to know to append.

Same thing for every request.

Done.

The requests are created locally and never reused, so it seems fine to not reset them.

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 280 at r5 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This is a static string. No need for absl::StrFormat(), and you can use GRPC_ERROR_CREATE_FROM_STATIC_STRING().

Done.

[markdroth]

We don't really use run_tests.py anymore. But in this case, you're only touching code that is used in a single test, so you can just manually run that test with bazel. Try the following commands:

bazel test test/core/security:credentials_test
bazel test -c dbg --config=asan test/core/security:credentials_test
bazel test -c dbg --config=tsan test/core/security:credentials_test
bazel test -c dbg --config=msan test/core/security:credentials_test
bazel test -c dbg --config=ubsan test/core/security:credentials_test

With regard to the PR, this is looking very good. Just one issue remaining.

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

Reviewed 2 of 2 files at r6.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @renkelvin and @ZhenLian)

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 71 at r4 (raw file):

Previously, renkelvin (Chuan Ren) wrote…

Good point.

While if ctx_ is not null, we may want to clean up the previous context and start the new token fetch instead of failing out. Otherwise, the token fetch will be always blocked.

It's not safe to simply delete the existing context, since there may still be callbacks pending that will use it.

I think this should just be an assertion. ctx_ must never be non-null when we get here, or we know that there's a serious bug, and we can just crash if that happens.

[renkelvin]

There is a failure("MemorySanitizer: use-of-uninitialized-value") with msan testing, while it seems to be an existing issue on master branch. The other tests passed locally.

Could you please trigger the CI again? If it passes then this pr should be ready! Thanks!

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

---

src/core/lib/security/credentials/external/external_account_credentials.cc, line 71 at r4 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

It's not safe to simply delete the existing context, since there may still be callbacks pending that will use it.

I think this should just be an assertion. ctx_ must never be non-null when we get here, or we know that there's a serious bug, and we can just crash if that happens.

Done. Yeah, make sense.

[markdroth]

The code looks great! I've triggered the tests again, so let's see what they show.

Reviewed 1 of 1 files at r7.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ZhenLian)

[renkelvin]

It seems there is an assertion failed on Windows specifically, and I think this is what's happening.

There are 4 main checks to make sure the happy flow works as intended:
In test_external_account_creds_success:
1.Start token fetch. A valid access token should be returned.
2.Start a second token fetch. The token cached in 1 should be returned and there should be no http calls.

In test_external_account_creds_success_with_service_account_impersonation:
3.Start token fetch. A valid impersonated access token should be returned.
4.Start a second token fetch. The token cached in 3 should be returned and there should be no http calls.

The step 4 fails because the step 3 takes longer so step 4 starts before a valid token is cached, so an http call will be made.

To resolve this, I think we can just remove step 4 because its purpose is to make sure the cache mechanism works, which is duplicate with step 2. Please let me know if you have any concerns. Thanks!

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ZhenLian)

[markdroth]

This looks good. Please squash your commits. Once you do that, I'll trigger one more run of the tests, and then we can get this merged.

Reviewed 1 of 1 files at r8.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ZhenLian)

[renkelvin]

Done, commits squashed.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ZhenLian)

[markdroth]

All tests are green! Merging now.

[renkelvin]

Awesome, thanks!