xds bootstrap: enable using JWT Call Credentials (part 2 for A97) #8536

dimpavloff · 2025-08-22T11:47:30Z

Part two for grpc/proposal#492 (A97), following #8431 .

What this PR does is:

update internal/xds/bootstrap with support for loading multiple PerRPCCallCredentials specifed in a new call_creds field in the boostrap file as per A97
adjust xds/internal/xdsclient/clientimpl.goto use the call credentials when constructing the client
update xds/bootstrap to register the jwtcreds call credentials and make them available if GRPC_EXPERIMENTAL_XDS_BOOTSTRAP_CALL_CREDS is enabled

I have added DialOptionsWithCallCredsForTransport because, even though current and future call credentials are likely to all expect secure transport, I thought it would be safer to check of insecure transport just in case. If you prefer, I can just update DialOptions to use all call credentials regardless of the transport.

Relates to istio/istio#53532

RELEASE NOTES:

xds bootstrap: add support for loading a JWT from file and use it as Call Credentials (A97). This is guarded by GRPC_EXPERIMENTAL_XDS_BOOTSTRAP_CALL_CREDS

dimpavloff · 2025-09-26T17:47:20Z

@easwars I think this is ready for another review :)

internal/xds/bootstrap/bootstrap.go

easwars · 2025-10-01T23:03:48Z

Apologies for the delay in the review. I'm a bit swamped with other things. I will get to this asap.

easwars

I wasn't able to make a complete pass. But some comments to get you going. I will try to make another pass asap.

easwars · 2025-10-01T22:35:21Z

internal/envconfig/xds.go

 	// https://github.com/grpc/proposal/blob/master/A86-xds-http-connect.md
 	XDSHTTPConnectEnabled = boolFromEnv("GRPC_EXPERIMENTAL_XDS_HTTP_CONNECT", false)
+
+	// XDSBootstrapCallCredsEnabled controls if JWT call credentials can be used


Nit: This environment variable guards the newly added bootstrap field call_creds. I agree that the only call credentials that we currently support via this mechanism is the JWT call creds, but that could change any day (before we get rid of this env var). So, I would prefer we more explicitly call out what this env var guards.

internal/xds/bootstrap/bootstrap.go

easwars · 2025-10-01T23:14:07Z

internal/xds/bootstrap/jwtcreds/call_creds.go

+
+// NewCallCredentials returns a credentials.PerRPCCredentials The input config
+// should match the structure specified in gRFC A97 structure.
+// See gRFC A97: https://github.com/grpc/proposal/blob/master/A97-xds-jwt-call-creds.md


Some nits here.

There needs to be a period after the first sentence.

I think the next two lines can be shortened. (this is an internal API, so we can omit the link to the actual gRFC)

There is no mention about the second return value, func()

So, maybe something like

// NewCallCredentials returns a new JWT token based call credentials. The input // config must match the structure specified in gRFC A97. The caller is expected // to invoke the second return value when they are done using the returned call creds.

easwars · 2025-10-01T23:15:21Z

internal/xds/bootstrap/jwtcreds/call_creds_test.go

+	"google.golang.org/grpc/credentials"
+)
+
+func TestNewCallCredentials(t *testing.T) {


Please use the Tester defined in the grpctest package here:

grpc-go/internal/grpctest/grpctest.go

Line 52 in d0ebcdf

type Tester struct{}

It has some setup and teardown magic that we find useful for all of our tests.

easwars · 2025-10-01T23:17:08Z

internal/xds/bootstrap/jwtcreds/call_creds_test.go

+
+			if !tt.wantSuccess {
+				if err == nil {
+					t.Fatal("Expected error, got nil")


Please identify the function being tested in the test error message. See: https://google.github.io/styleguide/go/decisions#identify-the-function

easwars · 2025-10-01T23:24:19Z

internal/xds/bootstrap/bootstrap.go

+// CallCreds contains the call credentials configuration for individual RPCs.
+// It implements gRFC A97 call credentials structure.
+type CallCreds struct {
+	// Type contains a unique name identifying the call credentials type.


Hmm .. The gRFC does not state any uniqueness requirements for this.

Hm, you're right, it doesn't. I was influenced by the ChannelCreds.Type but there it makes more sense given that only one gets picked up. I've updated the comment

easwars · 2025-10-01T23:24:43Z

internal/xds/bootstrap/bootstrap.go

 }

+// CallCreds returns the call credentials configuration for this server.
+func (sc *ServerConfig) CallCreds() []CallCreds {


Should we call this CallCredsConfig and the next method CallCreds?

I was trying to keep symmetry with ChannelCreds and SelectedChannelCreds. However, SelectedCallCreds now returns []credentials.PerRPCCredentials rather than the config, still, which breaks this symmetry.

Realistically, I'm not sure even if the public methods are needed -- the PerRPCCredentials are added to the ServerConfig.extraDialOptions and used via ServerConfig.DialOptions()

WDYT, shall I remove the public methods? I've renamed them in the meantime. I've used CallCredsConfigs, plural, because I also renamed the Go type to CallCredsConfig and the method returns a collection of those. I've also renamed the relevant fields in ServerConfig (and ServerConfigTestingOptions) in similar way.

easwars · 2025-10-01T23:27:26Z

internal/xds/bootstrap/bootstrap.go

+	if envconfig.XDSBootstrapCallCredsEnabled {
+		// Process call credentials - unlike channel creds, we use ALL supported
+		// types. Also, call credentials are optional as per gRFC A97.
+		for _, callCredConfig := range server.CallCreds {


Nit: Can we use a shorter name for the loop variable. Maybe cfg?

easwars · 2025-10-01T23:28:54Z

internal/xds/bootstrap/bootstrap.go

+				// Skip unsupported call credential types (don't fail bootstrap).
+				continue
+			}
+			callCred, cancel, err := c.Build(callCredConfig.Config)


We refer to them as credentials and not credential even when there is a single one. This was a major source of confusion to me when I joined the team as well :)

So, s/callCred/callCreds/

Haha, yes, this did confuse me a bit when starting. Thanks for spotting this

easwars · 2025-10-01T23:29:57Z

internal/xds/xdsclient/clientimpl_test.go

 	}
 }
+
+func TestServerConfigCallCredsIntegration(t *testing.T) {


This needs to have the s receiver on it.

dimpavloff

Thanks for the review @easwars ; I think it's ready for another pass

dimpavloff · 2025-10-03T09:20:46Z

internal/xds/bootstrap/bootstrap.go

+// CallCreds contains the call credentials configuration for individual RPCs.
+// It implements gRFC A97 call credentials structure.
+type CallCreds struct {
+	// Type contains a unique name identifying the call credentials type.


Hm, you're right, it doesn't. I was influenced by the ChannelCreds.Type but there it makes more sense given that only one gets picked up. I've updated the comment

dimpavloff · 2025-10-03T09:33:05Z

internal/xds/bootstrap/bootstrap.go

 }

+// CallCreds returns the call credentials configuration for this server.
+func (sc *ServerConfig) CallCreds() []CallCreds {


I was trying to keep symmetry with ChannelCreds and SelectedChannelCreds. However, SelectedCallCreds now returns []credentials.PerRPCCredentials rather than the config, still, which breaks this symmetry.

Realistically, I'm not sure even if the public methods are needed -- the PerRPCCredentials are added to the ServerConfig.extraDialOptions and used via ServerConfig.DialOptions()

WDYT, shall I remove the public methods? I've renamed them in the meantime. I've used CallCredsConfigs, plural, because I also renamed the Go type to CallCredsConfig and the method returns a collection of those. I've also renamed the relevant fields in ServerConfig (and ServerConfigTestingOptions) in similar way.

dimpavloff · 2025-10-03T09:44:28Z

internal/xds/bootstrap/bootstrap.go

+				// Skip unsupported call credential types (don't fail bootstrap).
+				continue
+			}
+			callCred, cancel, err := c.Build(callCredConfig.Config)


Haha, yes, this did confuse me a bit when starting. Thanks for spotting this

dimpavloff · 2025-10-03T10:40:57Z

internal/xds/bootstrap/jwtcreds/call_creds_test.go

+			if err != nil {
+				t.Fatalf("NewCallCredentials failed: %v", err)
+			}
+			if callCreds == nil {
+				t.Fatal("Expected non-nil bundle")
+			}


Yep, much better, thanks!

internal/xds/bootstrap/bootstrap.go

easwars · 2025-10-08T00:44:55Z

internal/xds/xdsclient/clientimpl_test.go

+	originalJWTEnabled := envconfig.XDSBootstrapCallCredsEnabled
+	envconfig.XDSBootstrapCallCredsEnabled = true
+	defer func() {
+		envconfig.XDSBootstrapCallCredsEnabled = originalJWTEnabled
+	}()


We have a helper function for this:

grpc-go/internal/testutils/envconfig.go

Line 26 in 9ff80a7

func SetEnvConfig[T any](t *testing.T, variable *T, value T) {

easwars · 2025-10-10T07:01:39Z

internal/xds/bootstrap/jwtcreds/call_creds.go

+// NewCallCredentials returns a new JWT token based call credentials. The input
+// config must match the structure specified in gRFC A97. The caller is expected
+// to invoke the second return value when they are done using the returned call creds.
+// The cancel function is idempotent.
+func NewCallCredentials(configJSON json.RawMessage) (credentials.PerRPCCredentials, func(), error) {


How about adding named return values to help with the documentation.

// NewCallCredentials creates and returns JWT token-based call credentials // using the input config (which must match gRFC A97). // // The caller must invoke the returned cancel function (which is idempotent), // when they are done using the returned call credentials. func NewCallCredentials(configJSON json.RawMessage) (c credentials.PerRPCCredentials, cancel func(), err error) {

easwars · 2025-10-10T07:02:04Z

internal/xds/bootstrap/jwtcreds/call_creds_test.go

+	"google.golang.org/grpc/internal/grpctest"
+)
+
+const defaultCtxTimeout = 5 * time.Second


Nit: s/defaultCtxTimeout/defaultTestTimeout/

easwars · 2025-10-10T07:04:15Z

internal/xds/bootstrap/jwtcreds/call_creds_test.go

+			callCreds, cleanup, err := NewCallCredentials(json.RawMessage(tt.config))
+
+			if err == nil {
+				t.Fatal("NewCallCredentials: expected error, got nil")


Please consider adding the config to the error message:
t.Fatal("NewCallCredentials(%s) succeeded when expected to fail", tt.config)

easwars · 2025-10-10T07:05:46Z

internal/xds/bootstrap/jwtcreds/call_creds_test.go

+				t.Fatal("NewCallCredentials: expected error, got nil")
+			}
+			if callCreds != nil {
+				t.Error("NewCallCredentials: Expected nil bundle to be returned")


Nit: there is no bundle anymore.
t.Error("NewCallCredentials(%s) returned non-nil credentials", tt.config)

easwars · 2025-10-10T07:07:55Z

internal/xds/bootstrap/jwtcreds/call_creds_test.go

+	if cleanup == nil {
+		t.Error("NewCallCredentials: Expected non-nil cleanup function to be returned")
+	} else {
+		defer cleanup()
+	}


It would be nicer to always return a non-nil cancel func (even in error cases), to avoid such checks at callsites.

easwars · 2025-10-10T07:08:53Z

internal/xds/bootstrap/jwtcreds/call_creds_test.go

+	}
+
+	// Test that call credentials get used
+	ctx, cancel := context.WithTimeout(context.Background(), defaultCtxTimeout)


Do we really need a 5s timeout here? And this const is only used here, you could remove the global and just inline the value here. Maybe a second at best should be good enough?

easwars · 2025-10-10T07:11:28Z

internal/xds/bootstrap/jwtcreds/call_creds_test.go

+func writeTempFile(t *testing.T, content string) string {
+	t.Helper()
+	tempDir := t.TempDir()
+	filePath := filepath.Join(tempDir, "tempfile")


Probably doesn't matter. But could be rename the file to something more appropriate. Maybe "jwt_token"?

easwars · 2025-10-10T07:12:29Z

internal/xds/bootstrap/bootstrap.go

+	if cc.Config == nil {
+		return cc.Type
+	}
+	// We do not expect the Marshal call to fail since we wrote to cc.Config


Nit: Here and in a few other places, please terminate comment lines with periods. https://google.github.io/styleguide/go/decisions#comment-sentences

easwars · 2025-10-10T07:13:14Z

internal/xds/bootstrap/bootstrap.go

-	channelCreds   []ChannelCreds
-	serverFeatures []string
+	serverURI        string
+	channelCreds     []ChannelCreds


While you are here, could you please add a TODO to rename ChannelCreds to ChannelCredsConfig to match what we have with the call creds? Thanks.

easwars · 2025-10-10T18:03:13Z

internal/xds/bootstrap/bootstrap.go

 	return cc.Type + "-" + string(b)
 }

+// CallCredsConfig contains the call credentials configuration for individual RPCs.


I think it might be better to clarify in this docstring that these call creds are used when talking to the xDS management server. The docstring on the ChannelCreds type does that. So, what do you think about something like:

CallCredsConfig contains the call credentials configuration to be used on RPCs to the management server.

I think the part about A97 is not that important and could be skipped. Thanks.

easwars · 2025-10-10T18:09:32Z

internal/xds/bootstrap/bootstrap.go

 	case !slices.Equal(sc.serverFeatures, other.serverFeatures):
 		return false
-	case !sc.selectedCreds.Equal(other.selectedCreds):
+	case !sc.selectedChannelCreds.Equal(other.selectedChannelCreds):


I know this is existing code, but I'm wondering if we can remove this field from the equality check for a couple of reasons:

we are already checking the channel credentials configuration (from which we selected this credentials) for equality

this type is supposed to semantically represent configuration corresponding to an xDS server and not any runtime state. We are already abusing it a little (or optimizing) by storing the selected credentials in it, so that we don't have to parse the configuration again when we actually want to use the credentials.

easwars · 2025-10-10T18:12:17Z

internal/xds/bootstrap/bootstrap.go

 	}
 	features := strings.Join(sc.serverFeatures, "-")
-	return strings.Join([]string{sc.serverURI, sc.selectedCreds.String(), features}, "-")
+	return strings.Join([]string{sc.serverURI, sc.selectedChannelCreds.String(), features, sc.CallCredsConfigs().String()}, "-")


I remember you asking me if we want to include the channel creds here and I said yes. But thinking about this now, I'm wondering if the returned string is going to be too verbose, especially since the String() method of the call credentials config returns a JSON (as does the channel credentials). Have you see what this string actually looks like in any of your testing?

easwars · 2025-10-10T18:15:37Z

internal/xds/bootstrap/bootstrap.go

+// CallCreds returns the built call credentials that are ready to use.
+// These are the credentials that were successfully built from the call_creds
+// configuration.
+func (sc *ServerConfig) CallCreds() []credentials.PerRPCCredentials {
+	return sc.selectedCallCreds
+}


Do we need this method at all (and the selectedCallCreds field)? We are currently storing the selected call credentials directly as dial options in the extraDialOptions field.

easwars · 2025-10-10T18:21:56Z

internal/xds/bootstrap/bootstrap_test.go

+	original := envconfig.XDSBootstrapCallCredsEnabled
+	envconfig.XDSBootstrapCallCredsEnabled = true
+	defer func() {
+		envconfig.XDSBootstrapCallCredsEnabled = original
+	}()


You could use the helper function here as well testutils.SetEnvConfig.

easwars · 2025-10-10T18:34:11Z

xds/bootstrap/bootstrap.go

+	// Build returns a PerRPCCredentials created from the provided
+	// configuration, and a function to clean up any additional resources
+	// associated with them when they are no longer needed.
+	Build(config json.RawMessage) (credentials.PerRPCCredentials, func(), error)


I realize that I added a comment about possibly using named return values for this method on the JWT call creds implementation. But it looks like we haven't done that for the existing channel credentials interface which has a similar set of return values. So, I'm fine with leaving this as is. Thanks.

easwars · 2025-10-10T18:43:29Z

xds/bootstrap/bootstrap_test.go

-		builder  Credentials
+		typename              string
+		builder               ChannelCredentials
+		minimumRequiredConfig json.RawMessage


Do we really need this new field when it is always set to nil?

easwars · 2025-10-10T18:44:56Z

xds/bootstrap/bootstrap_test.go

+			if bundle == nil {
+				t.Errorf("%T.Build returned nil bundle, expected non-nil", test.builder)
+			}
+			stop()


Can we defer this call to stop right after the line where we call Build?

easwars · 2025-10-10T18:46:56Z

xds/bootstrap/bootstrap_test.go

+			if bundle == nil {
+				t.Errorf("%T.Build returned nil bundle, expected non-nil", test.builder)
+			}
 			stop()


easwars · 2025-10-10T18:49:04Z

xds/bootstrap/bootstrap_test.go

 }
+
+func TestJwtCallCredentials_DisabledIfFeatureNotEnabled(t *testing.T) {
+	builder := GetCallCredentials("jwt_call_creds")


Can we explicitly set the value of the env var to false instead of running this test under the assumption that it is false (which it currently is). But if we explicitly set it here, then we wont have to change the test when we change the default value of the env var to true.

easwars · 2025-10-10T18:51:06Z

@dimpavloff This PR is mostly looking good. Most of the comments in this pass are nits. We should be able to resolve them quickly and move on. Thank you very much!

dimpavloff added 30 commits July 6, 2025 18:47

xds: read JWT credentials from file as per A97

6e63daa

remove example

3268ea5

refactor test creation

b18a1f5

refactor token string padding

eb391af

remove example; mark as experimental

d43893a

reorganise struct attributes

167b86e

rename methods with Locked suffix

439d28c

remove context param from refreshTokenSync

b36d4b6

reformat comments; remove redundant cachedErrorTime field

26e0451

add defaultTestTimeout const

da2de8c

refactor test to use wantErr string only

51ce34c

fix punctuation

f87f1f2

less prosaic subtest names

15dd057

remove unit test

54cbbcb

rename preemptiveRefresh to forceRefresh

9c5035d

remove unused context param

ec915dc

rename files

1d95fa2

use cond variable

a797ed9

refactor to no longer need cond

fd388d1

fix docstring comment

790a2d9

cache authorization header instead of token

6713190

remove internal/ and xds/ changes

3f563eb

remove xds/bootstrap

a38573b

fix comment docstrings

12fedd5

xds: read JWT credentials from file as per A97

2c80eab

comment punctuation

b344fdb

remove leftover

517c6ad

refactor test to use wantErr string only

34689c8

tidy comments

0d5d0a5

Merge remote-tracking branch 'origin/master' into a97-stacked-3

e465e7a

easwars added Type: Feature New features or improvements in behavior Area: xDS Includes everything xDS related, including LB policies used with xDS. labels Sep 26, 2025

dimpavloff added 2 commits September 26, 2025 18:40

add comment to map var

3dbb546

xds -> xDS consistency

651cba1

dimpavloff requested a review from easwars September 26, 2025 17:46

dimpavloff removed their assignment Sep 26, 2025

easwars removed the Status: Requires Reporter Clarification label Sep 26, 2025

easwars self-assigned this Sep 26, 2025

dimpavloff commented Sep 26, 2025

View reviewed changes

internal/xds/bootstrap/bootstrap.go Outdated Show resolved Hide resolved

easwars reviewed Oct 1, 2025

View reviewed changes

easwars assigned dimpavloff and unassigned easwars Oct 1, 2025

dimpavloff added 7 commits October 3, 2025 10:17

change xds env var comment

c09c763

rename CallCreds to CallCredsConfig and SelectedCallCreds to CallCreds

deaa239

split success and failure test cases for TestNewCallCredentials

6a9dc14

method receiver; document idempotency

5cbe1a1

include CallCredsConfigs in ServerConfig.String method

a1944c6

use grpctest.Tester

92c8a49

add docstring

0dc3856

dimpavloff commented Oct 3, 2025

View reviewed changes

dimpavloff requested a review from easwars October 3, 2025 13:39

dimpavloff removed their assignment Oct 3, 2025

easwars self-assigned this Oct 8, 2025

easwars reviewed Oct 10, 2025

View reviewed changes

easwars assigned dimpavloff and unassigned easwars Oct 10, 2025

xds bootstrap: enable using JWT Call Credentials (part 2 for A97) #8536

Are you sure you want to change the base?

xds bootstrap: enable using JWT Call Credentials (part 2 for A97) #8536

Uh oh!

Conversation

dimpavloff commented Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dimpavloff commented Sep 26, 2025

Uh oh!

Uh oh!

easwars commented Oct 1, 2025

Uh oh!

easwars left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimpavloff 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!

Choose a reason for hiding this comment

Uh oh!

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!

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

dimpavloff commented Aug 22, 2025 •

edited

Loading