Stabilize auth flow using CallCredentials #3289

ejona86 · 2017-07-27T22:01:52Z

As discussed in #1914, we need CallCredentials and MoreCallCredentials
to be stable, but there's less of a strong need for the contents of
CallCredentials to be stable. We're willing to commit to the name,
without needing to commit to the plumbing.

CC @garrettjonesgoogle

As discussed in grpc#1914, we need CallCredentials and MoreCallCredentials to be stable, but there's less of a strong need for the contents of CallCredentials to be stable. We're willing to commit to the name, without needing to commit to the plumbing.

garrettjonesgoogle · 2017-07-27T22:15:02Z

auth/src/main/java/io/grpc/auth/MoreCallCredentials.java

 public final class MoreCallCredentials {
  /**
   * Converts a Google Auth Library {@link Credentials} to {@link CallCredentials}.
+   *
+   * <p>Although this is a stable API, note that the returned instance's API is not stable. See
+   * {@link CallCredentials}.


Technically the type CallCredentials itself is now stable, but just none of the methods on it are.

@garrettjonesgoogle, I've reworded the Javadoc. See if it strikes you as more clear now.

That helps, I think it is more clear now.

carl-mastrangelo · 2017-07-27T22:46:51Z

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

 public interface CallCredentials {
  /**
   * The security level of the transport. It is guaranteed to be present in the {@code attrs} passed
   * to {@link #applyRequestMetadata}. It is by default {@link SecurityLevel#NONE} but can be
   * overridden by the transport.
   */
+  @ExperimentalApi("https//github.com/grpc/grpc-java/issues/1914")
  public static final Key<SecurityLevel> ATTR_SECURITY_LEVEL =


I think this should be moved off. It's weird to inherit ATTR_SECURITY_LEVEL if you implement CallCredentials.

Discussion of that sort should be part of the rest of the stabilization effort. I'm not trying to change the API here, just make a bare-minimum of it stable so that users can authenticate themselves safely.

carl-mastrangelo · 2017-07-27T22:47:41Z

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

@@ -65,6 +72,7 @@
   * @param applier The outlet of the produced headers. It can be called either before or after this
   *        method returns.
   */
+  @ExperimentalApi("https//github.com/grpc/grpc-java/issues/1914")
  void applyRequestMetadata(


This is not a valid. You can't make part of an interface experimental. All implementers have to implement it, which will break if it is ever removed.

Did you not read the text added for the interface?

No, and neither will anyone else. Please do not make our API support load based on the reading comprehension of our users.

If you want to make this stable and keep experimental methods, please make a base class that extends this. That is the correct place to put methods that may change.

They don't have to read the documentation, but it does explain what is going on. How can someone implement this API without touching Experimental API? How can someone call this API without touching Experimental API? They can't. But I do agree it is strange, thus the documentation.

base class that extends this

I don't understand that. I'd have understood "base class than this extends" and "make this a base class that is extended".

If you want to make this stable and keep experimental methods, please make a base class that extends this. That is the correct place to put methods that may change.

I don't know what you are talking about? That's not a thing we do.

And it doesn't help, since applyRequestMetadata could be completely changed.

They can implement it, but it needs to be a base class. Experimental API and interface are all or nothing.

I don't understand that. I'd have understood "base class than this extends" and "make this a base class that is extended".

Something like:

interface CallCredentials { void doNotImplementThisOrYouWillBreak(); } public abstract class BaseCallCredentials implements CallCredentials { public final void doNotImplementThisOrYouWillBreak(); }

That way CallCredentials can mostly live on as is, Experimental methods can be moved down to the base class, where they can change. If methods change, callers of experimental methods will change, but implementors of CallCredentials will not.

As the code stands with this PR, someone extending CallCredentials, a stable class, will suffer breakage when experimental methods come and go. That is not stable.

If methods change, ... implementors of CallCredentials will not [change].

I don't want to provide that guarantee. And I don't see how that is true, unless you are saying there would be no implementors so it is vacuously true.

As the code stands with this PR, someone extending CallCredentials, a stable class, will suffer breakage when experimental methods come and go. That is not stable.

It is impossible to extend CallCredentials without touching ExperimentalApi, so I feel like your sentence is a word-game that is skewing reality. Extending CallCredentials is not stable, because it is impossible with the current ExperimentalApi annotations and even explicitly mentioned in the documentation.

I'd be willing to make CallCredentials an abstract class or add a doNotImplementThisOrYouWillBreak(), but in either case I want to reserve the right to completely change the contents, like adding a new abstract method and removing the existing method.

I could maybe be okay with leaving ExperimentalApi on the interface, but say that we guarantee its name as stable.

but in either case I want to reserve the right to completely change the contents

Okay, but that isn't stable. Leave the @ExperimentalAPI up top if thats what you want.

I don't want to provide that guarantee.
If you remove the annotation, you provide that guarantee.

It is impossible to extend CallCredentials without touching ExperimentalApi, so I feel like your sentence is a word-game that is skewing reality.

This isn't a word game; you just didn't understand. Extendees of CallCredentials, after it has been unmarked ExperimentalApi, will expect their classes to not break. Leaving ExperimentalApi methods on it implies they will break, because said methods may change or be removed. That's contradicting signal, and certain to confuse some users. That causes us future pain. Do not cause us future pain.

Talked offline. It seems we're both okay with adding a useless method that implementors have to implement to make things clear. Adding a thisUsesUnstableApi() method.

(iAmUnstableApi() isn't accepted by checkstyle, and while I could change checkstyle... meh.)

implementors

carl-mastrangelo

LGTM

carl-mastrangelo · 2017-07-28T00:13:18Z

@ejona86 Please use the "package: thing that changed" style commit message when you squash merge.

zhangkun83 · 2017-07-28T00:56:42Z

LGTM

ejona86 · 2017-07-28T00:59:52Z

retest this please

ejona86 requested a review from zhangkun83 July 27, 2017 22:01

garrettjonesgoogle reviewed Jul 27, 2017

View reviewed changes

ejona86 added 2 commits July 27, 2017 15:45

Try to clarify javadoc

d777639

Remove ExperimentalApi on the places CallCredentials is passed

a8fab15

carl-mastrangelo reviewed Jul 27, 2017

View reviewed changes

Make the URLs URLs

479c58f

carl-mastrangelo reviewed Jul 27, 2017

View reviewed changes

Add trash method thisUsesUnstableApi() to make guarantees clear to

29d97a9

implementors

carl-mastrangelo approved these changes Jul 28, 2017

View reviewed changes

zhangkun83 approved these changes Jul 28, 2017

View reviewed changes

ejona86 merged commit 9be41ba into grpc:master Jul 28, 2017

ejona86 deleted the call-creds-pseudostable branch July 28, 2017 01:07

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stabilize auth flow using CallCredentials #3289

Stabilize auth flow using CallCredentials #3289

ejona86 commented Jul 27, 2017

garrettjonesgoogle Jul 27, 2017

ejona86 Jul 27, 2017

garrettjonesgoogle Jul 27, 2017

carl-mastrangelo Jul 27, 2017

ejona86 Jul 27, 2017

carl-mastrangelo Jul 27, 2017

ejona86 Jul 27, 2017

carl-mastrangelo Jul 27, 2017

ejona86 Jul 27, 2017

carl-mastrangelo Jul 27, 2017 •

edited

Loading

ejona86 Jul 27, 2017

carl-mastrangelo Jul 27, 2017

ejona86 Jul 28, 2017 •

edited

Loading

garrettjonesgoogle Jul 28, 2017

carl-mastrangelo left a comment

carl-mastrangelo commented Jul 28, 2017

zhangkun83 commented Jul 28, 2017

ejona86 commented Jul 28, 2017

Stabilize auth flow using CallCredentials #3289

Stabilize auth flow using CallCredentials #3289

Conversation

ejona86 commented Jul 27, 2017

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

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 Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejona86 Jul 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carl-mastrangelo left a comment

Choose a reason for hiding this comment

carl-mastrangelo commented Jul 28, 2017

zhangkun83 commented Jul 28, 2017

ejona86 commented Jul 28, 2017

carl-mastrangelo Jul 27, 2017 •

edited

Loading

ejona86 Jul 28, 2017 •

edited

Loading