-
Notifications
You must be signed in to change notification settings - Fork 68
fix: plumb mtls endpoint to TransportChannelProvider #3673
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.
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
Changes from all commits
f0c638d
70153f7
59f3bf7
716c9ee
ac95145
812799b
7e36d1a
2afdbbc
1d1e8ef
50c04b0
92e07fe
c53c20e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -129,6 +129,7 @@ public final class InstantiatingGrpcChannelProvider implements TransportChannelP | |
private final HeaderProvider headerProvider; | ||
private final boolean useS2A; | ||
private final String endpoint; | ||
private final String mtlsEndpoint; | ||
// TODO: remove. envProvider currently provides DirectPath environment variable, and is only used | ||
// during initial rollout for DirectPath. This provider will be removed once the DirectPath | ||
// environment is not used. | ||
|
@@ -179,6 +180,7 @@ private InstantiatingGrpcChannelProvider(Builder builder) { | |
this.headerProvider = builder.headerProvider; | ||
this.useS2A = builder.useS2A; | ||
this.endpoint = builder.endpoint; | ||
this.mtlsEndpoint = builder.mtlsEndpoint; | ||
this.allowedHardBoundTokenTypes = builder.allowedHardBoundTokenTypes; | ||
this.mtlsProvider = builder.mtlsProvider; | ||
this.s2aConfigProvider = builder.s2aConfigProvider; | ||
|
@@ -258,6 +260,12 @@ public boolean needsEndpoint() { | |
return endpoint == null; | ||
} | ||
|
||
@InternalApi("This public method is used by Gax to help configure the MTLS endpoint for S2A") | ||
@Override | ||
public boolean needsMtlsEndpoint() { | ||
return mtlsEndpoint == null; | ||
} | ||
|
||
/** | ||
* Specify the endpoint the channel should connect to. | ||
* | ||
|
@@ -272,6 +280,22 @@ public TransportChannelProvider withEndpoint(String endpoint) { | |
return toBuilder().setEndpoint(endpoint).build(); | ||
} | ||
|
||
/** | ||
* Specify the mTLS endpoint the channel should connect to when using S2A. | ||
* | ||
* <p>The value of {@code mtlsEndpoint} must be of the form {@code host:port}. | ||
* | ||
* @param mtlsEndpoint The mtTLS endpoint to connect to | ||
* @return A new {@link InstantiatingGrpcChannelProvider} with the specified mTLS endpoint | ||
* configured | ||
*/ | ||
@InternalApi("This public method is used by Gax to help configure the MTLS endpoint for S2A") | ||
@Override | ||
public TransportChannelProvider withMtlsEndpoint(String mtlsEndpoint) { | ||
validateEndpoint(mtlsEndpoint); | ||
return toBuilder().setMtlsEndpoint(mtlsEndpoint).build(); | ||
} | ||
Comment on lines
+293
to
+297
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would you be able to add Given that we can't make this package-private scope, we should let users know that they really shouldn't be tinkering with these values since they're really used to help S2A (one flow for now). I think a small note in the comment (for this method and all the other public ones) as well warning users There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 7e36d1a. Would you prefer making the change in EndpointContext in this PR, or a follow up? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My thoughts are that we just leave EndpointContext as-is since we have publicly noted that these public methods have been marked with The existing way of setting the mtlsEndpoint in StubSettings is supported |
||
|
||
/** | ||
* Specify whether or not to use S2A. | ||
* | ||
|
@@ -666,7 +690,9 @@ private ManagedChannel createSingleChannel() throws IOException { | |
channelCredentials = | ||
CompositeChannelCredentials.create(channelCredentials, mtlsS2ACallCredentials); | ||
} | ||
builder = Grpc.newChannelBuilder(endpoint, channelCredentials); | ||
// Connect to the MTLS endpoint when using S2A because S2A is used to perform an MTLS | ||
// handshake. | ||
builder = Grpc.newChannelBuilder(mtlsEndpoint, channelCredentials); | ||
lqiu96 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} else { | ||
// Use default if we cannot initialize channel credentials via DCA or S2A. | ||
builder = ManagedChannelBuilder.forAddress(serviceAddress, port); | ||
|
@@ -819,6 +845,7 @@ public static final class Builder { | |
private Executor executor; | ||
private HeaderProvider headerProvider; | ||
private String endpoint; | ||
private String mtlsEndpoint; | ||
private boolean useS2A; | ||
private EnvironmentProvider envProvider; | ||
private SecureSessionAgent s2aConfigProvider = SecureSessionAgent.create(); | ||
|
@@ -926,6 +953,14 @@ public Builder setEndpoint(String endpoint) { | |
return this; | ||
} | ||
|
||
/** Sets the mTLS Endpoint used to reach the service, eg "localhost:8080". */ | ||
@InternalApi("This public method is used by Gax to help configure the MTLS endpoint for S2A") | ||
public Builder setMtlsEndpoint(String mtlsEndpoint) { | ||
validateEndpoint(mtlsEndpoint); | ||
this.mtlsEndpoint = mtlsEndpoint; | ||
return this; | ||
} | ||
|
||
Builder setUseS2A(boolean useS2A) { | ||
this.useS2A = useS2A; | ||
return this; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -97,6 +97,20 @@ public interface TransportChannelProvider { | |
*/ | ||
TransportChannelProvider withEndpoint(String endpoint); | ||
|
||
/** True for gRPC transport provider which has no mtlsEndpoint set. */ | ||
default boolean needsMtlsEndpoint() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know we tried to avoid introducing these new public interfaces by encapsulating the logic to determine if we should use S2A in EndpointContext. Now that we need to have access to mtlsEndpoint in TransportChannelProvider anyway, maybe we can move shouldUseS2A to InstantiatingGrpcChannelProvider.java? And remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed that by plumbing the MTLS endpoint separately, we no longer need the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @blakeli0 I started working on this and I realized that we should keep Given this, we probably want to keep things as is. WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I see, yeah I think this info only exists in EndpointContext. cc: @lqiu96 in case there are more ideas. |
||
return false; | ||
} | ||
|
||
/** | ||
* Sets the mTLS endpoint to use when constructing a new {@link TransportChannel} using S2A. | ||
* | ||
* <p>This method should only be called if {@link #needsMtlsEndpoint()} returns true. | ||
*/ | ||
default TransportChannelProvider withMtlsEndpoint(String mtlsEndpoint) { | ||
return this; | ||
} | ||
|
||
/** Sets whether to use S2A when constructing a new {@link TransportChannel}. */ | ||
@BetaApi( | ||
"The S2A feature is not stable yet and may change in the future. https://github.com/grpc/grpc-java/issues/11533.") | ||
|
Uh oh!
There was an error while loading. Please reload this page.