alts: if ALTS is not running on GCP, fails call #4807
Conversation
| Metadata metadata, | ||
| ServerCallHandler<ReqT, RespT> nextHandler) { | ||
| if (!status.isOk()) { | ||
| serverCall.close(status, new Metadata()); |
There was a problem hiding this comment.
Not sure whether it is sufficient here.
There was a problem hiding this comment.
This would be appropriate, although, isn't it impossible to actually get here? Won't the server be unable to accept new connections? And without connections it won't get any RPCs.
Server-side does have start() that can throw an IOException...
|
@ejona86 Could you please take a look? |
jiangtaoli2016
force-pushed
the
alts_exception
branch
from
8650d07
to
e89e4c9
Compare
jiangtaoli2016
force-pushed
the
alts_exception
branch
from
e89e4c9
to
396c1ee
Compare
| + "ALTS handshaker service"); | ||
| } else { | ||
| status = | ||
| Status.FAILED_PRECONDITION.withDescription( |
There was a problem hiding this comment.
The gRPC library can't use FAILED_PRECONDITION: https://github.com/grpc/grpc/blob/master/doc/statuscodes.md
There was a problem hiding this comment.
How about INTERNAL? I feel FAILED_PRECONDITION is the right description though.
"Operation was rejected because the system is not in a state required for the operation's execution."
There was a problem hiding this comment.
INTERNAL is okay. We can change it later if need be.
| tcpfFactoryForTest = tcpfFactory; | ||
|
|
||
| return new AltsChannel(delegate().build()); | ||
| return delegate().intercept(new AltsClientInterceptor(status)).build(); |
There was a problem hiding this comment.
Only add the interceptor when it will be helpful (like in the else case above).
You should add a note that you can't call build() multiple times.
jiangtaoli2016
force-pushed
the
alts_exception
branch
from
2570af8
to
e2ebee2
Compare
|
@ejona86 code revised. PTAL |
| status = | ||
| Status.INTERNAL.withDescription("ALTS is only allowed to run on Google Cloud Platform"); | ||
| } | ||
| delegate().intercept(new AltsClientInterceptor(status)); |
| @@ -50,7 +44,7 @@ public void buildsNettyChannel() throws Exception { | |||
| assertThat(altsClientOptions).isNull(); | |||
|
|
|||
| ManagedChannel channel = builder.build(); | |||
| assertThat(channel).isInstanceOf(AltsChannel.class); | |||
| assertThat(channel).isInstanceOf(ManagedChannel.class); | |||
There was a problem hiding this comment.
This check dose nothing, since it's a ManagedChannel that is returned.
| Status.FAILED_PRECONDITION | ||
| .withDescription("Failed to get Google default credentials") | ||
| .withCause(e); | ||
| Status.INTERNAL.withDescription("Failed to get Google default credentials").withCause(e); |
| delegate.enterIdle(); | ||
| public <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall( | ||
| MethodDescriptor<ReqT, RespT> method, CallOptions callOptions, Channel next) { | ||
| if (!status.isOk()) { |
| Metadata metadata, | ||
| ServerCallHandler<ReqT, RespT> nextHandler) { | ||
| if (!status.isOk()) { | ||
| serverCall.close(status, new Metadata()); |
There was a problem hiding this comment.
This would be appropriate, although, isn't it impossible to actually get here? Won't the server be unable to accept new connections? And without connections it won't get any RPCs.
Server-side does have start() that can throw an IOException...
|
@ejona86 Revised. The reason we want to check GCP platform is to prevent network attack on handshaker service. If attacker re-directs metadata server, it may fool grpc client. In the normal case, if app is not running on GCP, it would fail on connecting to handshaker server. This GCP residency check is defense in depth. |
In ALTS, rather than throws RuntimeException if the application is not running on GCP, we fail at interceptCall.