Skip to content

feat: enable DirectPath bound token in InstantiatingGrpcChannelProvider #3572

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 20 commits into from
Feb 10, 2025
Merged

feat: enable DirectPath bound token in InstantiatingGrpcChannelProvider #3572

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
2f56310
wip
rockspore Jan 16, 2025
88dc795
wip++
rockspore Jan 17, 2025
25003a9
wip++
rockspore Jan 17, 2025
53d3206
wip
rockspore Jan 17, 2025
74a0166
Merge branch 'directpath-bound-token' into directpath-bound-token-wit…
rockspore Jan 17, 2025
d11c03f
wip++
rockspore Jan 17, 2025
0b734ae
one more test case
rockspore Jan 21, 2025
64a4399
tweak comment
rockspore Jan 21, 2025
a1d9691
Merge branch 'main' of https://github.com/googleapis/sdk-platform-jav…
rockspore Jan 31, 2025
7c0ec55
remove local snapshots in pom.xml
rockspore Jan 31, 2025
71f2e52
wip
rockspore Feb 5, 2025
e1a8dbf
merge with upstream
rockspore Feb 5, 2025
84c9be4
move the alts call creds creation into the builder
rockspore Feb 5, 2025
2287136
revert some out-of-scope format changes
rockspore Feb 5, 2025
391829a
format
rockspore Feb 5, 2025
1ed143c
Merge branch 'main' into directpath-bound-token-with-patch
rockspore Feb 6, 2025
80ee661
check emptiness
rockspore Feb 7, 2025
beefd82
comment
rockspore Feb 7, 2025
99d49b3
Merge branch 'main' into directpath-bound-token-with-patch
rockspore Feb 7, 2025
28547bb
Merge branch 'main' into directpath-bound-token-with-patch
rockspore Feb 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 30 additions & 1 deletion ...java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ public final class InstantiatingGrpcChannelProvider implements TransportChannelP
@Nullable private final Boolean keepAliveWithoutCalls;
private final ChannelPoolSettings channelPoolSettings;
@Nullable private final Credentials credentials;
@Nullable private final CallCredentials altsCallCredentials;
@Nullable private final CallCredentials mtlsS2ACallCredentials;
Comment on lines +144 to 145
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can altsCallCredentials and mtlsS2ACallCredentials share the same variable? Something like hardBoundCallCredentials?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, from reading the code below, I'm assuming this is a limitation from directpath.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I guess we can at this point, because it's always one or the other. But does that provide any benefit?

If in the future, we want gRPC to use mTLS when falling back from DirectPath to CloudPath, we will need these two different creds again.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it always going to be one or the other? I'd rather just have one variable to keep track of the hard bound callcredential state -> mtls vs alts if possible. Adding more variables makes it harder to keep track of all the combinations of possible states (mtls can be null or non-null + alts can be null or non-null).

If in the future, we want gRPC to use mTLS when falling back from DirectPath to CloudPath

Is this something that is in the works and you know that it will require multiple types of callcredentials? Or is this just a hypothetical example?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's consolidate the discussion around keeping two credentials to this thread.

Is it always going to be one or the other?

I believe so in normal use cases although at this point you can see that DirectPath enablement is also controlled by an environment variable. So in theory, I suppose the decision could change in the runtime so two credentials need to exist independently to ensure there is no interference.

Even when that env var switch gets removed in the future, IMO we should still keep two credentials separate because we use a list of allowed bound token types to control the enablement of them independently. From that perspective, it should be fairly straightforward to track the null-ness of these two variables - they are enabled by the enum in the list.

Is this something that is in the works and you know that it will require multiple types of callcredentials? Or is this just a hypothetical example?

Our team has talked about that a few times but there is no concrete plan as of now. So yes it's fair to be considered as a hypothetical example.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DirectPath enablement is also controlled by an environment variable. So in theory, I suppose the decision could change in the runtime so two credentials need to exist independently to ensure there is no interference.

+1, the answer to "can DirectPath be used" could change from after this transportChannelProvider is built (.build()) but before calling transportChannelProvider.createSingleChannel().

In general, I think it may be simpler to keep these two creds separate. I think that combining them would complicate our isMtlsS2AHardBoundTokensEnabled and isDirectPathBoundTokenEnabled logic below. Since both can be true on GCE and DirectPath takes precedence over using S2A, I think we would end up needing to combine the logic from those functions if we wanted a single hardBoundCallCredential. While DirectPath and S2A are doing similar things when it comes to enabling the usage hard bound tokens, I think at this time it might be difficult to try and reason about them together like this

Additionally, @lqiu96, from our discussion in #3591 (comment), It seems like creating 2 creds is ok, as they are created in the provider once, and are not exposed by the provider? That was a detailed discussion though, so I could be missing something.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure I'm following because I am still only see this as a limitation with how DirectPath is being resolved.

Correct me if I'm wrong. For hard bound tokens, even if MTLS_S2A and ALTS are both in the hard-bound list, only one of them is going to to be used, right? Even if both are currently resolved to be non-null. The intended logic is roughly as follows:

If hardbound tokens list is non-empty:
- If DirectPath + ATLS token included -> Create the altsCallCredentials
- If Non-DirectPath + S2A enabled + S2A_MTLS  token included -> Create the mtlsCallCredentials

Else:
- Use the normal flow

Changing how DirectPath is done is out of scope for this PR and two different hard-bound tokens can live here for now. I'm just not piecing together why two separate hard bound tokens need to exist (besides for the directpath limitation), since it's going to be either ALTS or MTLS, right? Or is there some fallback logic that I may be forgetting about.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So due to the environment variable check inside isDirectPathEnabled which is called by canUseDirectPath used by createSingleChannel, we have to keep these two credentials separated for now.

I guess the remaining discussion is about whether it makes sense otherwise to keep these two credentials inside the object. You are right that only one of the bound tokens will be used in practice. So I agree there will be no technical reason preventing us from consolidating them into one private class member.

I tend to believe it's just our opinions on which way makes the code easier to reason and minimizes confusion that differ. As @rmehta19 pointed out, isMtlsS2AHardBoundTokensEnabled and isDirectPathBoundTokenEnabled would both mutate the hardBoundCallCredentials. IMHO, it feels less intuitive and more error-prone, if the order of populating hardBoundCallCredentials is accidentally changed or so. But I personally don't feel strongly about this. At the point, the environment variable check is gone for DirectPath enablement, I'm fine if it's concluded that one hardBoundCallCredentials is the better way to go.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes agreed. Really either way is fine logic-wise, but I just want to be cautious of adding in so many additional credential variables (as it went from 1 to 3). Each may include multiple states and this increases the mental load when figuring out how channels are created, which channels are created, and which credentials are used for each channel used.

@Nullable private final ChannelPrimer channelPrimer;
@Nullable private final Boolean attemptDirectPath;
Expand Down Expand Up @@ -191,6 +192,7 @@ private InstantiatingGrpcChannelProvider(Builder builder) {
this.channelPoolSettings = builder.channelPoolSettings;
this.channelConfigurator = builder.channelConfigurator;
this.credentials = builder.credentials;
this.altsCallCredentials = builder.altsCallCredentials;
this.mtlsS2ACallCredentials = builder.mtlsS2ACallCredentials;
this.channelPrimer = builder.channelPrimer;
this.attemptDirectPath = builder.attemptDirectPath;
Expand Down Expand Up @@ -616,8 +618,14 @@ private ManagedChannel createSingleChannel() throws IOException {
boolean useDirectPathXds = false;
if (canUseDirectPath()) {
CallCredentials callCreds = MoreCallCredentials.from(credentials);
// altsCallCredentials may be null and GoogleDefaultChannelCredentials
// will solely use callCreds. Otherwise it uses altsCallCredentials
// for DirectPath connections and callCreds for CloudPath fallbacks.
ChannelCredentials channelCreds =
GoogleDefaultChannelCredentials.newBuilder().callCredentials(callCreds).build();
GoogleDefaultChannelCredentials.newBuilder()
.callCredentials(callCreds)
.altsCallCredentials(altsCallCredentials)
.build();
useDirectPathXds = isDirectPathXdsEnabled();
if (useDirectPathXds) {
// google-c2p: CloudToProd(C2P) Directpath. This scheme is defined in
Expand Down Expand Up @@ -822,6 +830,7 @@ public static final class Builder {
@Nullable private Boolean keepAliveWithoutCalls;
@Nullable private ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> channelConfigurator;
@Nullable private Credentials credentials;
@Nullable private CallCredentials altsCallCredentials;
@Nullable private CallCredentials mtlsS2ACallCredentials;
@Nullable private ChannelPrimer channelPrimer;
private ChannelPoolSettings channelPoolSettings;
Expand Down Expand Up @@ -853,6 +862,7 @@ private Builder(InstantiatingGrpcChannelProvider provider) {
this.keepAliveWithoutCalls = provider.keepAliveWithoutCalls;
this.channelConfigurator = provider.channelConfigurator;
this.credentials = provider.credentials;
this.altsCallCredentials = provider.altsCallCredentials;
this.mtlsS2ACallCredentials = provider.mtlsS2ACallCredentials;
this.channelPrimer = provider.channelPrimer;
this.channelPoolSettings = provider.channelPoolSettings;
Expand Down Expand Up @@ -919,6 +929,7 @@ Builder setUseS2A(boolean useS2A) {
this.useS2A = useS2A;
return this;
}

/*
* Sets the allowed hard bound token types for this TransportChannelProvider.
*
Expand Down Expand Up @@ -996,6 +1007,7 @@ public Integer getMaxInboundMetadataSize() {
public Builder setKeepAliveTime(org.threeten.bp.Duration duration) {
return setKeepAliveTimeDuration(toJavaTimeDuration(duration));
}

/** The time without read activity before sending a keepalive ping. */
public Builder setKeepAliveTimeDuration(java.time.Duration duration) {
this.keepAliveTime = duration;
Expand Down Expand Up @@ -1172,6 +1184,18 @@ boolean isMtlsS2AHardBoundTokensEnabled() {
.anyMatch(val -> val.equals(HardBoundTokenTypes.MTLS_S2A));
}

boolean isDirectPathBoundTokenEnabled() {
// If the list of allowed hard bound token types is empty or doesn't contain
// {@code HardBoundTokenTypes.ALTS}, the {@code credentials} are null or not of type
// {@code ComputeEngineCredentials} then DirectPath hard bound tokens should not be used.
// DirectPath hard bound tokens should only be used on ALTS channels.
if (allowedHardBoundTokenTypes.isEmpty()
|| this.credentials == null
|| !(credentials instanceof ComputeEngineCredentials)) return false;
return allowedHardBoundTokenTypes.stream()
.anyMatch(val -> val.equals(HardBoundTokenTypes.ALTS));
}

CallCredentials createHardBoundTokensCallCredentials(
ComputeEngineCredentials.GoogleAuthTransport googleAuthTransport,
ComputeEngineCredentials.BindingEnforcement bindingEnforcement) {
Expand All @@ -1194,6 +1218,11 @@ public InstantiatingGrpcChannelProvider build() {
ComputeEngineCredentials.GoogleAuthTransport.MTLS,
ComputeEngineCredentials.BindingEnforcement.ON);
}
if (isDirectPathBoundTokenEnabled()) {
this.altsCallCredentials =
createHardBoundTokensCallCredentials(
ComputeEngineCredentials.GoogleAuthTransport.ALTS, null);
}
InstantiatingGrpcChannelProvider instantiatingGrpcChannelProvider =
new InstantiatingGrpcChannelProvider(this);
instantiatingGrpcChannelProvider.removeApiKeyCredentialDuplicateHeaders();
Expand Down
54 changes: 54 additions & 0 deletions .../gax-grpc/src/test/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@

import com.google.api.core.ApiFunction;
import com.google.api.gax.grpc.InstantiatingGrpcChannelProvider.Builder;
import com.google.api.gax.grpc.InstantiatingGrpcChannelProvider.HardBoundTokenTypes;
import com.google.api.gax.rpc.FixedHeaderProvider;
import com.google.api.gax.rpc.HeaderProvider;
import com.google.api.gax.rpc.TransportChannel;
Expand Down Expand Up @@ -735,6 +736,59 @@ public void canUseDirectPath_happyPath() throws IOException {
.setEndpoint(DEFAULT_ENDPOINT)
.setEnvProvider(envProvider)
.setHeaderProvider(Mockito.mock(HeaderProvider.class));
Truth.assertThat(builder.isDirectPathBoundTokenEnabled()).isFalse();
InstantiatingGrpcChannelProvider provider =
new InstantiatingGrpcChannelProvider(builder, GCE_PRODUCTION_NAME_AFTER_2016);
Truth.assertThat(provider.canUseDirectPath()).isTrue();

// verify this info is passed correctly to transport channel
TransportChannel transportChannel = provider.getTransportChannel();
Truth.assertThat(((GrpcTransportChannel) transportChannel).isDirectPath()).isTrue();
transportChannel.shutdownNow();
}

@Test
public void canUseDirectPath_boundTokenNotEnabledWithNonComputeCredentials() {
System.setProperty("os.name", "Linux");
Credentials credentials = Mockito.mock(Credentials.class);
EnvironmentProvider envProvider = Mockito.mock(EnvironmentProvider.class);
Mockito.when(
envProvider.getenv(
InstantiatingGrpcChannelProvider.DIRECT_PATH_ENV_DISABLE_DIRECT_PATH))
.thenReturn("false");
InstantiatingGrpcChannelProvider.Builder builder =
InstantiatingGrpcChannelProvider.newBuilder()
.setAttemptDirectPath(true)
.setAllowHardBoundTokenTypes(Collections.singletonList(HardBoundTokenTypes.ALTS))
.setCredentials(credentials)
.setEndpoint(DEFAULT_ENDPOINT)
.setEnvProvider(envProvider);
Truth.assertThat(builder.isDirectPathBoundTokenEnabled()).isFalse();
InstantiatingGrpcChannelProvider provider =
new InstantiatingGrpcChannelProvider(builder, GCE_PRODUCTION_NAME_AFTER_2016);
Truth.assertThat(provider.canUseDirectPath()).isFalse();
}

@Test
public void canUseDirectPath_happyPathWithBoundToken() throws IOException {
System.setProperty("os.name", "Linux");
EnvironmentProvider envProvider = Mockito.mock(EnvironmentProvider.class);
Mockito.when(
envProvider.getenv(
InstantiatingGrpcChannelProvider.DIRECT_PATH_ENV_DISABLE_DIRECT_PATH))
.thenReturn("false");
// verify the credentials gets called and returns a non-null builder.
Mockito.when(computeEngineCredentials.toBuilder())
.thenReturn(ComputeEngineCredentials.newBuilder());
InstantiatingGrpcChannelProvider.Builder builder =
InstantiatingGrpcChannelProvider.newBuilder()
.setAttemptDirectPath(true)
.setCredentials(computeEngineCredentials)
.setAllowHardBoundTokenTypes(Collections.singletonList(HardBoundTokenTypes.ALTS))
.setEndpoint(DEFAULT_ENDPOINT)
.setEnvProvider(envProvider)
.setHeaderProvider(Mockito.mock(HeaderProvider.class));
Truth.assertThat(builder.isDirectPathBoundTokenEnabled()).isTrue();
InstantiatingGrpcChannelProvider provider =
new InstantiatingGrpcChannelProvider(builder, GCE_PRODUCTION_NAME_AFTER_2016);
Truth.assertThat(provider.canUseDirectPath()).isTrue();
Expand Down
Loading