support dual credentials in google_default_credentials by anniefrchz · Pull Request #39770 · grpc/grpc

anniefrchz · 2025-06-05T22:37:23Z

No description provided.

rockspore

Thanks a lot for the PR! Mostly LGTM. Left a few minor comments.

src/core/credentials/call/dual/dual_call_credentials.h

rockspore · 2025-06-06T16:17:43Z

src/core/credentials/transport/google_default/google_default_credentials.cc

 }

+grpc_channel_credentials*
+grpc_google_default_credentials_create_with_dual_call_credentials(


We don't need this New API. Instead we can modify the existing grpc_google_default_credentials_create by adding a second argument, which can default to nullptr.

rockspore · 2025-06-06T16:23:49Z

test/core/credentials/call/dual/dual_call_credentials_test.cc

+        GRPC_TRANSPORT_SECURITY_TYPE_PROPERTY_NAME,
+        GRPC_ALTS_TRANSPORT_SECURITY_TYPE);
+    tls_creds_ = MakeRefCounted<grpc_md_only_test_credentials>(
+        GRPC_TRANSPORT_SECURITY_TYPE_PROPERTY_NAME, "tls");


nit: You could use GRPC_TLS_TRANSPORT_SECURITY_TYPE.

rockspore · 2025-06-06T16:29:39Z

test/core/credentials/call/dual/dual_call_credentials_test.cc

+        GRPC_ALTS_TRANSPORT_SECURITY_TYPE);
+    tls_creds_ = MakeRefCounted<grpc_md_only_test_credentials>(
+        GRPC_TRANSPORT_SECURITY_TYPE_PROPERTY_NAME, "tls");
+    dual_creds_ = MakeRefCounted<DualCallCredentials>(tls_creds_->Ref(),


nit: Could we call the google_default_..._create API to make the coverage slightly more thorough?

rockspore · 2025-06-06T16:30:02Z

include/grpc/credentials.h

    grpc_call_credentials* call_credentials);

+GRPCAPI grpc_channel_credentials*
+grpc_google_default_credentials_create_with_dual_call_credentials(


We can just modify the API above.

Becase this is a backwards compatible C API, we cannot use default arguments so we need ot create the second API with 2 arguments regardless.

rockspore

Thanks! LGTM. Adding @markdroth to take a pass since we touched the grpc_google_default_credentials_create API.

rockspore · 2025-06-10T00:40:35Z

include/grpc/credentials.h

   mechanism.
+
+   The caller may specify a secondary alts credentials objects to attach to the
+   channel for ALTS connections. Whenever the channel connection require them,


Probably more accurate to say: if an alts credentials object is specified, it will be used if the underlying channel type is alts.

rockspore · 2025-06-10T00:48:13Z

Please fix the following failure:

/var/local/jenkins/grpc/src/cpp/client/secure_credentials.cc
  /var/local/jenkins/grpc/src/cpp/client/secure_credentials.cc:89:7: error: no matching function for call to 'grpc_google_default_credentials_create'
        grpc_google_default_credentials_create(nullptr));

from the failings tests, e.g. https://source.cloud.google.com/results/invocations/2a220610-2ab8-4a1e-a0e9-0574492772cf

Tests are failing.

src/core/credentials/transport/google_default/google_default_credentials.h

rockspore

Thanks for the refactoring effort! Some minor comments.

src/core/credentials/transport/google_default/google_default_credentials.cc

test/core/credentials/transport/google_default/google_default_call_credentials_test.cc

rockspore

Thanks! LGTM.

markdroth

This looks much better! Just a few remaining comments, all fairly minor.

include/grpc/credentials.h

src/core/credentials/transport/google_default/google_default_credentials.cc

markdroth

Looks great!

Closes grpc#39770 COPYBARA_INTEGRATE_REVIEW=grpc#39770 from anniefrchz:directpath2 35c0f4f PiperOrigin-RevId: 775501652

support dual credentials in google_default_credentials

bbedc10

anniefrchz self-assigned this Jun 5, 2025

github-actions bot added the lang/core label Jun 5, 2025

anniefrchz requested a review from rockspore June 5, 2025 22:39

anniefrchz added the release notes: no Indicates if PR should not be in release notes label Jun 5, 2025

grpc-checks bot added bloat/none per-call-memory/neutral per-channel-memory/neutral labels Jun 5, 2025

rockspore reviewed Jun 6, 2025

View reviewed changes

Fix comments

a0840de

rockspore previously approved these changes Jun 10, 2025

View reviewed changes

rockspore requested a review from markdroth June 10, 2025 00:46

rockspore self-requested a review June 10, 2025 00:48

anniefrchz added 4 commits June 10, 2025 15:02

Fix test

40b56d7

Add missing dependency

bf36df5

Removed duplicate dependency

a630391

Add missing dependency

ac1785c

yashykt added the area/security label Jun 10, 2025

Run generate_projects

b4c4270

github-actions bot added the lang/ruby label Jun 10, 2025

grpc-checks bot added bloat/low and removed bloat/none labels Jun 10, 2025

Create secondary API for C

4f76afd

anniefrchz marked this pull request as draft June 10, 2025 21:51

anniefrchz added 4 commits June 10, 2025 22:32

Fix DualCallCredentials class format

e04c3d0

Fix DualCallCredentials class format

6131b7e

Fix DualCallCredentials class format

2f8dcf0

Fix DualCallCredentials class format

7494bdf

anniefrchz added the kokoro:force-run label Jun 17, 2025

grpc-kokoro removed the kokoro:force-run label Jun 17, 2025

anniefrchz added the kokoro:force-run label Jun 18, 2025

grpc-kokoro removed the kokoro:force-run label Jun 18, 2025

grpc-checks bot added bloat/low and removed bloat/none labels Jun 18, 2025

anniefrchz added the kokoro:force-run label Jun 20, 2025

grpc-kokoro removed the kokoro:force-run label Jun 20, 2025

anniefrchz requested review from markdroth and rockspore June 20, 2025 18:28

grpc-checks bot added bloat/none and removed bloat/low labels Jun 20, 2025

rockspore reviewed Jun 20, 2025

View reviewed changes

src/core/credentials/transport/google_default/google_default_credentials.h Outdated Show resolved Hide resolved

Remove forward declaration

5fdfb5d

anniefrchz requested a review from rockspore June 23, 2025 17:50

anniefrchz marked this pull request as ready for review June 23, 2025 17:51

rockspore reviewed Jun 23, 2025

View reviewed changes

src/core/credentials/transport/google_default/google_default_credentials.cc Outdated Show resolved Hide resolved

test/core/credentials/transport/google_default/google_default_call_credentials_test.cc Show resolved Hide resolved

grpc-checks bot added bloat/low and removed bloat/none labels Jun 23, 2025

Add test and fix nits

c59fc3f

anniefrchz requested a review from rockspore June 23, 2025 21:44

rockspore approved these changes Jun 23, 2025

View reviewed changes

markdroth reviewed Jun 23, 2025

View reviewed changes

Fix nits

35c0f4f

markdroth approved these changes Jun 24, 2025

View reviewed changes

copybara-service bot closed this in ca2e8c9 Jun 25, 2025

anniefrchz added a commit to anniefrchz/grpc that referenced this pull request Jun 25, 2025

support dual credentials in google_default_credentials (grpc#39770)

04af08e

Closes grpc#39770 COPYBARA_INTEGRATE_REVIEW=grpc#39770 from anniefrchz:directpath2 35c0f4f PiperOrigin-RevId: 775501652

paulosjca pushed a commit to paulosjca/grpc that referenced this pull request Aug 23, 2025

support dual credentials in google_default_credentials (grpc#39770)

8b6f51c

Closes grpc#39770 COPYBARA_INTEGRATE_REVIEW=grpc#39770 from anniefrchz:directpath2 35c0f4f PiperOrigin-RevId: 775501652

anniefrchz deleted the directpath2 branch September 4, 2025 22:11

Conversation

anniefrchz commented Jun 5, 2025

Uh oh!

rockspore left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rockspore left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rockspore commented Jun 10, 2025

Uh oh!

Uh oh!

rockspore left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rockspore left a comment

Choose a reason for hiding this comment

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants