[3PI Support] Add url-sourced external account credentials

[renkelvin]

Structure of 3PI Support

The implementation of 3PI support is organized as follows:

1. Base external account credentials class [[3PI Support] Add base external account credentials class #24208]
2. Subclasses to base external account credentials
   a. url-sourced credentials [This PR]
   b. file-sourced credentials
   c. aws-sourced credentials
3. Internal and public apis of external account credentials

Description of this PR

This PR adds the url external account credentials.

• The url external account credentials makes a http get call to retrieve the subject token, then send the token back to the base class.

@markdroth Please take a look and let me know if you have any questions. Thanks!

---

This change is 

[markdroth]

This looks like a good start!

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

Reviewed 18 of 18 files at r1.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @jiangtaoli2016 and @renkelvin)

---

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

 protected:
  void RetrieveSubjectToken(

This can actually be private, since IIUC this class is not going to have any subclasses.

---

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

    std::string url;
    std::map<std::string, std::string> headers;
    struct {

Why put this fields in a nested struct? Why not just make them top-level fields?

---

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

UrlExternalAccountCredentials::UrlExternalAccountCredentials(
    ExternalAccountCredentialsOptions options, std::vector<std::string> scopes)
    : ExternalAccountCredentials(options, scopes) {}

Please use std::move() for both of these parameters.

---

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

    return;
  }
  ctx_ = const_cast<HTTPRequestContext*>(ctx);

Why are we removing the const-ness here? If we actually need to modify the context in subclasses, why is the API such that it's passed down as a const pointer to begin with?

---

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

  ctx_ = const_cast<HTTPRequestContext*>(ctx);
  cb_ = cb;
  grpc_error* error = ParseCredentialSource(options.credential_source);

This parsing should really be done at the time the credentials are created rather than doing it when a request is actually started, for two reasons. First, it's better for users for the error to be caught earlier. And second, there will be many requests over the lifetime of an application process, and it's inefficient to redo this parsing each time a request starts.

I suggest moving this parsing to the ctor and providing a way for the ctor to indicate failure back to the caller.

---

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

    return;
  }
  grpc_uri* uri = grpc_uri_parse(credential_source_.url.c_str(), false);

This can also be done once in the ctor. It does not need to be done for each request.

---

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

    headers[i].key = gpr_strdup(header.first.c_str());
    headers[i].value = gpr_strdup(header.second.c_str());
    i++;

Prefer preincrement to postincrement syntax, as per the style guide.

---

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

grpc_error* UrlExternalAccountCredentials::ParseCredentialSource(
    const Json& json) {
  CredentialSource credential_source = {};

No need for = {}.

---

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

    return error;
  } else {
    credential_source.url = std::string(url);

No need to explicitly convert to std::string here; that will happen automatically.

Same thing throughout.

---

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

  auto it = json.object_value().find("headers");
  if (it != json.object_value().end()) {
    if (it->second.type() != grpc_core::Json::Type::OBJECT) {

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

---

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

          "The JSON value of credential source format is not an object.");
    }
    const char* type = grpc_json_get_string_property(it->second, "type", &error);

Please do not use grpc_json_get_string_property(). This is legacy code that needlessly allocates a C-style string, which is inefficient here -- we would basically be converting a C++ string to a C string and then back to a C++ string. Also, this requires you to free the C string, which you're not doing here, so this causes a memory leak.

Instead, just interact with the Json object directly:

const Json& format_json = it->second;
auto format_it = format_json.object_value().find("type");
if (format_it == format_json.object_value().end()) {
  return GRPC_ERROR_CREATE_FROM_STATIC_STRING(
      "format.type field not present");
}
if (format_it->second.type() != Json::Type::STRING) {
  return GRPC_ERROR_CREATE_FROM_STATIC_STRING(
      "format.type field must be a string");
}
credential_source.format.type = format_it->second.string_value();

Same thing throughout.

---

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

    grpc_error* error) {
  if (error != GRPC_ERROR_NONE) {
    FinishRetrieveSubjectToken("", error);

Suggest returning after this, so that there's no need for the else on the next line.

---

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

    FinishRetrieveSubjectToken("", error);
  } else {
    std::string subject_token;

No need for this local variable. Instead, just directly call FinishRetrieveSubjectToken() whenever you are setting this.

---

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

  } else {
    std::string subject_token;
    std::string response_body =

Please don't needlessly instantiate two objects and then assign one to the other. Instead, just instantiate the object you want directly:

std::string response_body(ctx_->response.body, ctx_->response.body_length);

---

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

          response_json,
          credential_source_.format.subject_token_field_name.c_str(), &error);
      if (error != GRPC_ERROR_NONE) {

Why bother checking this here? Why not just unconditionally pass both the token and the error to FinishRetrieveSubjectToken(), and let it check whether there's an error?

---

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

        FinishRetrieveSubjectToken("", error);
        return;
      } else {

No need for else, since you're returning on the line above.

---

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

      subject_token = response_body;
    }
    FinishRetrieveSubjectToken(subject_token, nullptr);

Please use GRPC_ERROR_NONE instead of nullptr here.

---

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

  }
  // Reset context
  ctx_ = nullptr;

This needs to be done before invoking the callback, since a new request can be started any time after that point. It should be moved up to the top of the function.

[renkelvin]

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

---

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

Previously, markdroth (Mark D. Roth) wrote…

This can actually be private, since IIUC this class is not going to have any subclasses.

Done. Yeah, you're right. Added final to the class definition as well.

---

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

Previously, markdroth (Mark D. Roth) wrote…

Why put this fields in a nested struct? Why not just make them top-level fields?

It's defined this way to match the format of the credentials file that user provides.

---

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

Previously, markdroth (Mark D. Roth) wrote…

Please use std::move() for both of these parameters.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

Why are we removing the const-ness here? If we actually need to modify the context in subclasses, why is the API such that it's passed down as a const pointer to begin with?

Done. Yeah, we actually need to modify the context in subclasses. Modify the related definitions.

---

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

Previously, markdroth (Mark D. Roth) wrote…

This parsing should really be done at the time the credentials are created rather than doing it when a request is actually started, for two reasons. First, it's better for users for the error to be caught earlier. And second, there will be many requests over the lifetime of an application process, and it's inefficient to redo this parsing each time a request starts.

I suggest moving this parsing to the ctor and providing a way for the ctor to indicate failure back to the caller.

It seems a better way is to have a factory function so that a nullptr can be returned when constructing failed. Moved parsing method to the factory function.

---

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

Previously, markdroth (Mark D. Roth) wrote…

This can also be done once in the ctor. It does not need to be done for each request.

Done. Made url of CredentialSource a type of grpc_uri and pass it in the factory function.

---

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

Previously, markdroth (Mark D. Roth) wrote…

Prefer preincrement to postincrement syntax, as per the style guide.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

No need for = {}.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

No need to explicitly convert to std::string here; that will happen automatically.

Same thing throughout.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

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

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

Please do not use grpc_json_get_string_property(). This is legacy code that needlessly allocates a C-style string, which is inefficient here -- we would basically be converting a C++ string to a C string and then back to a C++ string. Also, this requires you to free the C string, which you're not doing here, so this causes a memory leak.

Instead, just interact with the Json object directly:

const Json& format_json = it->second;
auto format_it = format_json.object_value().find("type");
if (format_it == format_json.object_value().end()) {
  return GRPC_ERROR_CREATE_FROM_STATIC_STRING(
      "format.type field not present");
}
if (format_it->second.type() != Json::Type::STRING) {
  return GRPC_ERROR_CREATE_FROM_STATIC_STRING(
      "format.type field must be a string");
}
credential_source.format.type = format_it->second.string_value();

Same thing throughout.

Done. Do you think it's worth making a helper function?

---

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

Previously, markdroth (Mark D. Roth) wrote…

Suggest returning after this, so that there's no need for the else on the next line.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

No need for this local variable. Instead, just directly call FinishRetrieveSubjectToken() whenever you are setting this.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

Please don't needlessly instantiate two objects and then assign one to the other. Instead, just instantiate the object you want directly:

std::string response_body(ctx_->response.body, ctx_->response.body_length);

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

Why bother checking this here? Why not just unconditionally pass both the token and the error to FinishRetrieveSubjectToken(), and let it check whether there's an error?

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

No need for else, since you're returning on the line above.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

Please use GRPC_ERROR_NONE instead of nullptr here.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

This needs to be done before invoking the callback, since a new request can be started any time after that point. It should be moved up to the top of the function.

Done.

[markdroth]

Please make sure to run credential_test under ASAN to detect memory leaks. I think you've got quite a few of them.

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

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

---

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

Previously, renkelvin (Chuan Ren) wrote…

It's defined this way to match the format of the credentials file that user provides.

Why is that important? This is an internal implementation detail, not something that's exposed to the user. If we know that we are only going to have one instance of these two fields, I don't see the value in putting them in a nested struct. It seems like this just makes the data structure harder to understand.

---

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

class UrlExternalAccountCredentials final : public ExternalAccountCredentials {
 public:
  static UrlExternalAccountCredentials* Create(

The return type should be a RefCountedPtr<> instead of a raw pointer.

---

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

  // credential source. It will be parsed and stored when RetrieveSubjectToken()
  // starts.
  struct CredentialSource {

Since this struct now has exactly the same lifetime as the UrlExternalAccountCredentials class, there's not really any benefit in making this a separate struct. I suggest just moving the data members right into the surrounding class.

(If you do keep this struct for some reason, it should be moved up to the top of the private section, as per the style guide.)

---

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

Previously, renkelvin (Chuan Ren) wrote…

Done. Do you think it's worth making a helper function?

I wouldn't worry about it. It's not enough code to matter.

---

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

#include "absl/strings/str_format.h"

#include "src/core/lib/security/util/json_util.h"

I believe this is no longer needed.

---

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

    std::vector<std::string> scopes) {
  UrlExternalAccountCredentials* creds =
      new UrlExternalAccountCredentials(options, scopes);

This should use MakeRefCounted().

---

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

    std::vector<std::string> scopes) {
  UrlExternalAccountCredentials* creds =
      new UrlExternalAccountCredentials(options, scopes);

Please use std::move() for these parameters.

---

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

  UrlExternalAccountCredentials* creds =
      new UrlExternalAccountCredentials(options, scopes);
  if (creds->ParseCredentialSource(options.credential_source) ==

If the result is not GRPC_ERROR_NONE, we need to pass the error back to the caller. I suggest adding a grpc_error** parameter to the factory function and setting the error to the result of this function. (Currently, we're not passing it back but also not unreffing it, so it's a leak.)

I don't think ParseCredentialSource() should be a separate method. Its functionality should be in the ctor, and the ctor can take the same grpc_error** argument to indicate errors.

---

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

          "The JSON value of credential source format is not an object.");
    }
    const Json& format_json = it->second;

You can move this line up above the preceding if block, so that line 112 can use format_json instead of it->second.

---

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

    return;
  }
  std::string response_body(ctx_->response.body, ctx_->response.body_length);

This can be an absl::string_view so that we're not needlessly allocating a copy of the string.

---

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

  if (credential_source_.format.type == "json") {
    grpc_error* error = GRPC_ERROR_NONE;
    Json response_json = Json::Parse(response_body, &error);

If error is set after this, you should call FinishRetriveSubjectToken() immediately.

---

test/core/security/credentials_test.cc, line 2252 at r2 (raw file):

          "client_secret",                    // client_secret;
      };
  auto creds = grpc_core::UrlExternalAccountCredentials::Create(options, {});

You're allocating a new creds object but not freeing it anywhere. This is a leak.

Same thing throughout.

[renkelvin]

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

---

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

Previously, markdroth (Mark D. Roth) wrote…

Why is that important? This is an internal implementation detail, not something that's exposed to the user. If we know that we are only going to have one instance of these two fields, I don't see the value in putting them in a nested struct. It seems like this just makes the data structure harder to understand.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

The return type should be a RefCountedPtr<> instead of a raw pointer.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

Since this struct now has exactly the same lifetime as the UrlExternalAccountCredentials class, there's not really any benefit in making this a separate struct. I suggest just moving the data members right into the surrounding class.

(If you do keep this struct for some reason, it should be moved up to the top of the private section, as per the style guide.)

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

I believe this is no longer needed.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

Please use std::move() for these parameters.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

This should use MakeRefCounted().

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

If the result is not GRPC_ERROR_NONE, we need to pass the error back to the caller. I suggest adding a grpc_error** parameter to the factory function and setting the error to the result of this function. (Currently, we're not passing it back but also not unreffing it, so it's a leak.)

I don't think ParseCredentialSource() should be a separate method. Its functionality should be in the ctor, and the ctor can take the same grpc_error** argument to indicate errors.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

You can move this line up above the preceding if block, so that line 112 can use format_json instead of it->second.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

This can be an absl::string_view so that we're not needlessly allocating a copy of the string.

Done.

---

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

Previously, markdroth (Mark D. Roth) wrote…

If error is set after this, you should call FinishRetriveSubjectToken() immediately.

Done.

---

test/core/security/credentials_test.cc, line 2252 at r2 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

You're allocating a new creds object but not freeing it anywhere. This is a leak.

Same thing throughout.

The type of creds is now RefCountedPtr<UrlExternalAccountCredentials>.

I'm a bit surprised that there is no memory leak without calling creds.reset() in the end. Is the ref count of RefCountedPtr automatically managed or I misunderstand something? Would mind providing me some general information of how to manage the memory of a RefCountedPtr? Thanks!

[markdroth]

Only one minor comment remaining! Once you've resolved that, please squash your commits, and I'll trigger the tests.

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

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

---

test/core/security/credentials_test.cc, line 2252 at r2 (raw file):

Previously, renkelvin (Chuan Ren) wrote…

The type of creds is now RefCountedPtr<UrlExternalAccountCredentials>.

I'm a bit surprised that there is no memory leak without calling creds.reset() in the end. Is the ref count of RefCountedPtr automatically managed or I misunderstand something? Would mind providing me some general information of how to manage the memory of a RefCountedPtr? Thanks!

The whole point of RefCountedPtr<> is that it automatically releases the ref when it goes out of scope, so you don't need to explicitly reset it.

This looks fine now.

---

test/core/security/credentials_test.cc, line 2325 at r3 (raw file):

      grpc_core::UrlExternalAccountCredentials::Create(options, {}, &error);
  GPR_ASSERT(creds == nullptr);
  grpc_error* expected_error =

There's no need to create an expected error, since all you care about here is the description slice, which you can create directly:

grpc_slice expected_error_slice =
    grpc_slice_from_static_string("Invalid credential source url.");

[renkelvin]

Remaining issue fixed and commits squashed. Let's see what the tests show. Thanks!

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

---

test/core/security/credentials_test.cc, line 2325 at r3 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

There's no need to create an expected error, since all you care about here is the description slice, which you can create directly:

grpc_slice expected_error_slice =
    grpc_slice_from_static_string("Invalid credential source url.");

Done.

[markdroth]

You broke the test when you made that change. Please try again. Once that's done, I'll trigger the tests.

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

---

test/core/security/credentials_test.cc, line 2325 at r3 (raw file):

Previously, renkelvin (Chuan Ren) wrote…

Done.

You removed the wrong code here. Now you're not actually checking the expected error against the actual error.

The code here should be:

grpc_slice expected_error_slice =
    grpc_slice_from_static_string("Invalid credential source url.");
grpc_slice actual_error_slice;
GPR_ASSERT(
    grpc_error_get_str(error, GRPC_ERROR_STR_DESCRIPTION, &actual_error_slice));
GPR_ASSERT(grpc_slice_cmp(expected_error_slice, error_slice) == 0);
GRPC_ERROR_UNREF(error);

[renkelvin]

Fixed it. I misunderstood it somehow. Confirmed the all tests passed locally.

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

[markdroth]

The code looks good now!

It looks like you have a merge conflict. Please merge from master and re-squash commits.

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

[renkelvin]

Done.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jiangtaoli2016)

[markdroth]

I've triggered the tests. Let's see how they look.

[renkelvin]

The CI catches an issue that the RetrieveSubjectToken() method of TestExternalAccountCredentials lacks the override notation. I just fixed it and I think the CI would be green now.

Reviewable status: 3 of 19 files reviewed, all discussions resolved (waiting on @jiangtaoli2016 and @markdroth)

[markdroth]

I've triggered the tests again.

[renkelvin]

OK, there is another issue saying the macro format of the header file is wrong, which was not raised in the previous try. Just fixed it.

I wonder if there is a way to run the sanity checks locally? or any other recommended local checks before triggering the CI? Thanks a lot for your help!

[markdroth]

You can try running some of the individual sanity checks in tools/run_tests/sanity, but some of them may require additional dependencies to be installed locally.

I've triggered the tests again.

[renkelvin]

The failure of CI seems like tooling issues to me. Can wen rerun those tests specifically?

[markdroth]

The failures are all infrastructure timeouts or failures fetching external dependencies. I think it's fine to ignore those.

I'll merge this now.

[renkelvin]

Great, thanks!