core: add CallCredentials2 and deprecate CallCredentials' old interface #4902

zhangkun83 · 2018-10-03T00:39:30Z

This is the first step of introducing the 2nd edition of CallCredentials API (#4901)

Security level and authority are parameters required to be passed to
applyRequestMetadata(). This change wraps them, along with
MethodDescriptor and the transport attributes to RequestInfo, which is
more clear to the implementers.

ATTR_SECURITY_LEVEL is moved to the internal GrpcAttributes and
annotated as TransportAttr, because transports are required to set it,
but no user is actually reading them from
{Client,Server}Call.getAttributes().

ATTR_AUTHORITY is removed, because no transport is overriding it.

All involved interfaces are changed to abstract classes, as this will
make further API changes smoother.

This is the first step of smoothly changing the CallCredentials API. Security level and authority are parameters required to be passed to applyRequestMetadata(). This change wraps them, along with MethodDescriptor and the transport attributes to RequestInfo, which is more clear to the implementers. ATTR_SECURITY_LEVEL is moved to the internal GrpcAttributes and annotated as TransportAttr, because transports are required to set it, but no user is actually reading them from {Client,Server}Call.getAttributes(). ATTR_AUTHORITY is removed, because no transport is overriding it. All involved interfaces are changed to abstract classes, as this will make further API changes smoother. The CallCredentials name is stabilized, thus we first introduce CallCredentials2, ask CallCredentials implementations to migrate to it, while GRPC accepting both at the same time, then replace CallCredentials with CallCredentials2.

ejona86

I can't look at this in full at the moment, but the CallCredentials{,2} looks about right.

ejona86 · 2018-10-03T17:04:13Z

core/src/main/java/io/grpc/CallCredentials2.java

+ * <p>THIS CLASS IS MEANT TO BE REFERENCED BY IMPLEMENTIONS ONLY.  All users should reference {@link
+ * CallCredentials}.
+ */
+public abstract class CallCredentials2 implements CallCredentials {


This entire class should be marked experimental. It should deserve a note that it is part of a migration and is necessary to be used in the short-term but will be removed.

I wanted to use the deprecation warnings on the old interface to push implementations to switch to the new interface. If we deprecate it now, implementations would have to suppress the deprecation warnings and may accidentally mask off implementations that have not been migrated, and would also cause confusion when we want implementations to switch from CallCredentials2 back to CallCredentials. I think it's better to deprecate it after we replace CallCredentials with the new interface.

I have added the note, and an @Internal annotation to dissuade usage.

I wasn't suggesting marking it deprecated, just experimental. I guess internal works as well, although it seems experimental is more appropriate since we do expect some "normal" users to use it. I'd normally prefer to keep internal a "if you are using this, you are probably doing it wrong."

Sorry, read too fast :). Switched to experimental.

igorbernstein2 · 2018-10-05T20:32:01Z

I'm not sure if this is the right place to ask. But since you are creating a new interface for CallCredentials, would it be possible to make provisions to allow Credentials to take & respect the call deadlines?

carl-mastrangelo · 2018-10-05T20:59:35Z

@igorbernstein2 The io.grpc.Context is available at the time, so you can get it from there.

igorbernstein2 · 2018-10-05T21:16:31Z

@carl-mastrangelo I apologize for being dense, but can you clarify what you mean by “the grpc Context is available”? If I’m not mistaken, the deadline needs to be available inside applyRequestMetadata so that it can be passed to the credentials.

Thanks

carl-mastrangelo · 2018-10-05T21:25:51Z

@igorbernstein2 You can access it inside of your CallCredentials:

class MyCallCredentials extends CallCredentials {
  void applyRequestMetadata(/*...*/) {
    deadline = Context.current().getDeadline();
    // use deadline
  }
}

igorbernstein2 · 2018-10-05T21:33:41Z

Ah, I see, via a threadlocal. Would it not be cleaner to make it part of the api? All of the grpc seems to pass it around as parameter, it feels odd that CallCredentials have to rely on a threadlocal.

igorbernstein2 · 2018-10-05T21:35:59Z

Actually never mind, I’m getting confused with gax. Sorry for the noise

carl-mastrangelo · 2018-10-05T21:50:24Z

Technically Context does not have to be backed by a thread local. I have met users who use different backing storage. It is implemented by default as a thread local because that was the paradigm internally at Google. Legacy code has a strong influence, unfortunately.

igorbernstein2 · 2018-10-09T18:29:09Z

I took a closer look and it seems like Context.current().getDeadline() might not be enough. A deadline might be set via CallOptions.withDeadline which will not be stamped on the current Context and not be accessible by the CallCredentials

zhangkun83 · 2018-10-09T20:30:48Z

I can easily add CallOptions to the RequestInfo

igorbernstein2 · 2018-10-09T20:39:58Z

Should CallCredentials subclasses be responsible for figuring out the effective deadline between grpc.Context and CallOptions? I don't have a good answer, I was just requesting for this be a consideration in the api change

zhangkun83 · 2018-10-09T21:05:45Z

@igorbernstein2, can you file an issue for accessing deadline from CallCredentials?
It's not as trivial as I thought initially. Let's discuss it on its own issue.

igorbernstein2 · 2018-10-09T21:16:56Z

done: #4929

carl-mastrangelo · 2018-10-09T23:30:59Z

@zhangkun83 I don't feel comfortable expecting CallCredentials implementations having to recalculate the deadline as done in ClientCallImpl. Can the effective deadline be passed in instead?

Also, that's kind of weird. The CallOptions contains the credentials, but now the credentials will be passed the call options?

zhangkun83 · 2018-10-10T15:48:58Z

@carl-mastrangelo, let's discuss this in #4929

ejona86

One comment, but otherwise LGTM

ejona86 · 2018-10-10T22:42:00Z

core/src/main/java/io/grpc/CallCredentials2.java

+   * The request-related information passed to {@link #applyRequestMetadata}.
+   */
+  @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1914")
+  public abstract static class RequestInfo {


Should this be in CallCredentials? I'm thinking that moving the class to CallCredentials later may not be ABI-safe.

Makes sense. Moved to CallCredentials.

carl-mastrangelo · 2018-10-11T00:55:32Z

core/src/main/java/io/grpc/CallCredentials.java

@@ -92,16 +100,16 @@ void applyRequestMetadata(
   * <p>Exactly one of its methods must be called to make the RPC proceed.
   */
  @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1914")
-  public interface MetadataApplier {
+  public abstract class MetadataApplier {


An class under an interface is implicitly static. But it's no harm to add it (and it will be needed once CallCredentials becomes an abstract class).

zhangkun83 added 2 commits October 2, 2018 16:01

Fix warnings

b044cb0

zhangkun83 requested review from ejona86 and carl-mastrangelo October 3, 2018 00:39

ejona86 reviewed Oct 3, 2018

View reviewed changes

zhangkun83 changed the title ~~core: add CallCredentials2 and deprecate CallCredentials.~~ core: add CallCredentials2 and deprecate CallCredentials' old interface Oct 3, 2018

zhangkun83 added 2 commits October 3, 2018 10:45

Add more notes and annotate CallCredentials2 with @Internal

5bdeeed

ExperimentalApi

2ff6c07

ejona86 approved these changes Oct 10, 2018

View reviewed changes

Move RequestInfo to CallCredentials

9dc5f4c

carl-mastrangelo reviewed Oct 11, 2018

View reviewed changes

zhangkun83 added 2 commits October 10, 2018 18:04

Merge branch 'master' into call_credentials2

a5c8261

static MetadataApplier

8ae4bf2

zhangkun83 merged commit 861f914 into grpc:master Oct 11, 2018

lock bot locked as resolved and limited conversation to collaborators Jan 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: add CallCredentials2 and deprecate CallCredentials' old interface #4902

core: add CallCredentials2 and deprecate CallCredentials' old interface #4902

zhangkun83 commented Oct 3, 2018

ejona86 left a comment

ejona86 Oct 3, 2018

zhangkun83 Oct 3, 2018

ejona86 Oct 3, 2018

zhangkun83 Oct 3, 2018

igorbernstein2 commented Oct 5, 2018

carl-mastrangelo commented Oct 5, 2018

igorbernstein2 commented Oct 5, 2018

carl-mastrangelo commented Oct 5, 2018

igorbernstein2 commented Oct 5, 2018

igorbernstein2 commented Oct 5, 2018

carl-mastrangelo commented Oct 5, 2018

igorbernstein2 commented Oct 9, 2018

zhangkun83 commented Oct 9, 2018

igorbernstein2 commented Oct 9, 2018

zhangkun83 commented Oct 9, 2018

igorbernstein2 commented Oct 9, 2018

carl-mastrangelo commented Oct 9, 2018 •

edited

Loading

zhangkun83 commented Oct 10, 2018

ejona86 left a comment

ejona86 Oct 10, 2018

zhangkun83 Oct 11, 2018

carl-mastrangelo Oct 11, 2018

zhangkun83 Oct 11, 2018

core: add CallCredentials2 and deprecate CallCredentials' old interface #4902

core: add CallCredentials2 and deprecate CallCredentials' old interface #4902

Conversation

zhangkun83 commented Oct 3, 2018

ejona86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igorbernstein2 commented Oct 5, 2018

carl-mastrangelo commented Oct 5, 2018

igorbernstein2 commented Oct 5, 2018

carl-mastrangelo commented Oct 5, 2018

igorbernstein2 commented Oct 5, 2018

igorbernstein2 commented Oct 5, 2018

carl-mastrangelo commented Oct 5, 2018

igorbernstein2 commented Oct 9, 2018

zhangkun83 commented Oct 9, 2018

igorbernstein2 commented Oct 9, 2018

zhangkun83 commented Oct 9, 2018

igorbernstein2 commented Oct 9, 2018

carl-mastrangelo commented Oct 9, 2018 • edited Loading

zhangkun83 commented Oct 10, 2018

ejona86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carl-mastrangelo commented Oct 9, 2018 •

edited

Loading