Skip to content
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

Server-side power API #179

Closed
raboof opened this issue Apr 19, 2018 · 25 comments
Closed

Server-side power API #179

raboof opened this issue Apr 19, 2018 · 25 comments
Labels
Milestone

Comments

@raboof
Copy link
Member

raboof commented Apr 19, 2018

For users who need more low-level access:

For cross-cutting concerns where the service implementation does not need access to the details, the Handler could accept a Directive that is wrapped around the generated Route. Directives may mutate the request, but we discourage passing information from this Directive to the method body (and will not provide infrastructure for it). If this is required, implement the relevant processing in the method body instead of using the cross-cutting Directive (which might require refactoring).

When the service implementation needs access to details, we can enable this through protoc generator parameters. This could both enable the power API for the entire service, or per individual method.

The generated 'powerful' signature will gain an HttpRequest or another class that wraps the request headers. We do not expect to need to change the response type, since the response can be manipulated by throwing an exception or failing the Future/Source and taking care of further details in the exception handler.

@raboof raboof added the server label Apr 19, 2018
@johanandren
Copy link
Member

Not sure I understand this right @raboof, you think you could clarify a bit how it would look?

@raboof
Copy link
Member Author

raboof commented May 4, 2018

'Cross-cutting' checks by accepting a Directive for the Handler could looks something like (very pseudo):

val securityHeaderCheck: Directive0 = extractHeader[SecurityHeader] { h =>
  if (headerValid(h)) pass()
  else reject(SecurityRejection)
}
val service: HttpRequest => Future[HttpResponse] = GreeterServiceHandler(new GreeterServiceImpl(mat), securityHeaderCheck)

So the user implements the GreeterServiceImpl like it is currently, but passes the extra Directive to the generated GreeterServiceHandler to apply the security check to each request.

@johanandren
Copy link
Member

Hmm, ok, and for method-specific we'd have def myMethod(rawRequest: HttpRequest, grpcRequest: MyRequestType): Future[MyresponseType] or something like that?

@raboof
Copy link
Member Author

raboof commented May 4, 2018

This allows for an easy hook if you can get away with it, but won't give the GreeterServiceImpl implementation access to the request details. If you need that, you'd add an extra parameter to the protoc generator (alongside flat_package) that requests the 'power API' and causes the serverside trait to be generated something like:

trait GreeterService { 
  def sayHello(in: HelloRequest, details: SomethingHoldingDetailsLikeHeaders): Future[HelloReply]
}

One thing I didn't realize yet before writing this is that this does mean the trait will be different between the server side and the client side. Perhaps we should generate an 'in-between' abstract class to implement and expect in the generated Handler:

abstract class PowerServerGreeterService extends GreeterService {
  final override def sayHello(in: HelloRequest): Future[HelloReply] = // some exception
  abstract def sayHello(in: HelloRequest, details: SomethingHoldingDetailsLikeHeaders): Future[HelloReply]
}

Notably absent here is a way to further customize the response. Our intuition so far is that this might not be necessary: we couldn't really come up with a scenario where you'd want detailed access to the response except to signal error conditions, and error conditions can be handled by returning a failed future/source and using a custom error handler.

All of this is not set in stone of course, but the above is what we came up with so far in Lisbon.

@johanandren
Copy link
Member

Ah, so you'd fail the call with a custom exception and then transform that to actual response with a per-service error handler?

Isn't the introduction of the routes-api a bit surprising, given that for the simple-api it is completely invisible, if you think about it in a Play context for example?

@raboof
Copy link
Member Author

raboof commented May 4, 2018

@johanandren I think introducing akka-http Directives to the API could be in line with our goal to break down the barriers between the akka-http/play/lagom "silos": we want to grow towards exposing the fact that these technologies build upon each other and make combining them easy - so I'd say it's OK from that perspective.

However, perhaps indeed we should indeed not add the Directive as a parameter to the generated Handler, but instead show how to take that Directive and the Handler and turn that into the Route to pass to bindAndHandle. That way we could perhaps allow Play users to still use Action composition (which as far as I understand is the 'native' way to do such cross-cutting concerns in Play). Eventually I wonder if we could provide API's to adapt between Actions and Directives - but I'd say that's rather outside of the scope of this issue (and perhaps not trivial since Play currently only uses the low-level API).

@ctoomey
Copy link
Contributor

ctoomey commented Sep 12, 2018

We're heavy Play users and are looking to start using grpc in Play, so this set of features (extra request details to service methods as well as some kind of action composition / filtering mechanism) is one we will definitely need. With our REST APIs we currently we use an ActionBuilder to handle Oauth 2 token validation on requests and have several filters for logging, request UUID tracking, etc.

What Play 2.7 milestone are you guys thinking of for introducing this? And will you update this ticket with further design drafts as you work on them? We may be able to pitch in a bit on the work too though we're vetting our ability to build production grpc APIs using Play (or otherwise).

@ctoomey
Copy link
Contributor

ctoomey commented Sep 25, 2018

Here's what we'd like to see for a power method. It’s comprised of 3 orthogonal proposals which can be discussed individually:

  1. Make the "details" parameter the grpc metadata, represented by a richer Metadata trait as proposed below
  2. Provide access to the outgoing response metadata — this is equivalent to being able to modify response headers in handlers which is important for us
  3. Make the API functional and not dependent on throwing exceptions for returning errors

We want to start using this in the very near future, so will likely be forking the project to add it, but definitely would like to work with you to come up with something that we could contribute back.

Using the above example, we’d have

abstract class PowerServerGreeterService {
  final override def sayHello(in: HelloRequest): Future[HelloReply] = // some exception
  abstract def sayHello(in: HelloRequest, metadata: Metadata): Future[GrpcResult[HelloReply]]
}

Here are draft definitions of the new types. For Metadata we don’t care that much about the details but need something much richer than what’s currently in akka-grpc.

sealed trait GrpcResult[T] {
  def metadata: Option[Metadata]
}

object GrpcResult {
  // T would be either a protobuf msg or a Source[P, NotUsed] for streams of some protobuf type P
  case class Success[T](res: T, override val metadata: Option[Metadata] = None) extends GrpcResult[T]

  // Error results can also include metadata
  case class Error[T](status: Status, override val metadata: Option[Metadata] = None) extends GrpcResult[T]
}

trait Metadata {
  // From akka.grpc.scaladsl.Metadata

  /**
    * @return The text header value for `key` if one exists, if the same key has multiple values the last occurrence
    *         that is a text key is used.
    */
  def getText(key: String): Option[String]
  /**
    * @return The binary header value for `key` if one exists, if the same key has multiple values the last occurrence
    *         that is a text key is used.
    */
  def getBinary(key: String): Option[ByteString]

  // Motivated by io.grpc.Metadata

  def getAllText(key: String): Seq[String]
  def getAllBinary(key: String): Seq[ByteString]

  def textKeys(): Set[String] // keys of text entries
  def binaryKeys(): Set[String] // keys of binary entries

  // Motivated by io.grpc.Metadata, akka.grpc.internal.MetadataImpl

  def add(key: String, value: String): Metadata
  def add(key: String, value: ByteString): Metadata

  def remove(key: String): Metadata

  // Extras / alternatives to some of above

  def getAll: Seq[MetadataEntry]
  def add(entry: MetadataEntry*)
}

sealed trait MetadataEntry {
  def key: String
}

object MetadataEntry {
  case class TextMetadataEntry(key: String, value: String) extends MetadataEntry
  case class BinaryMetadataEntry(key: String, value: ByteString) extends MetadataEntry
}

object Metadata {
  def empty: Metadata = ???
  def apply(entries: MetadataEntry*): Metadata = ???
}

Thoughts?

@johanandren
Copy link
Member

I think that if we are going the method(request, metadata) path, then the response should also not be one object but a tuple (and Pair for Java) to be consistent, rather than a result type.

With a single request-response representing this is easy, but it is harder to say what would be a good API for streaming methods, do you already have the Metadata when you return a Source or do you have it first when the source is materialized for example. Probably useful to try out some scenarios to see that it works.

One thought is that we probably have different needs for request side vs response side, on the incoming side there should never be a need to change anything, so could be worth having separate APIs, to optimize - for example if all calls are going to go through the metadata variant we may not want to incur overhead unless the metadata is actually read. (We do stuff like this in the request builders on the client side)

We may also need to support trailing metadata for the response from the server, for the streaming cases, not sure when or what that is useful for.

@johanandren
Copy link
Member

Ah, missed out on the third part with responding with an error. This one I'm not sure about. I think the "more functional" way to encode errors would be to do so explicitly in the protocol (just like you do with the return value in functional code with Try for example), and reserve gRPC errors for failures - exceptional unexpected/problems - see it as the exception of gRPC.

@ctoomey
Copy link
Contributor

ctoomey commented Sep 27, 2018

@johanandren thanks for reviewing this.

I reread the list of gRPC status codes and see now that they really are lower-level gRPC-specific issues that should never be created directly by gRPC service implementations, so I agree they don't make sense in the service API.

Re: making the result be a tuple in Scala, that'd be fine but then we'd lose the ability to default the optional metadata to None, which would be a little less convenient for service implementers. But not a huge deal.

For a first pass on the streaming response I think returning the response metadata at the same time as the Source is the right way to go and matches that for unary responses so will proceed with that.

For the optimization, I'm not clear if you're suggesting a change to the proposed API signature or just an implementation optimization (which would almost definitely be beyond my pay grade)?

Any thoughts on the Metadata trait?

@johanandren
Copy link
Member

I think we have deal with the case where you don't care about metadata at all (probably most cases) in a way that it isn't any syntactical overhead. One idea we discussed previously is opting in on power-api vs simple-api through the build config. Bad aspect of that is that if you'd want metadata for one method in a server, you'd get different signatures for all the other methods as well.

Always generating the power-user API would allow us to call the simple signature from the one with metadata by default, but the problem with that is that you'd have to keep the concrete simple signature and let it be a no-op.

I think we were never quite happy with either of these two path previously when we discussed it.

Perhaps an API based on composition, more along the lines of the server DSL in Akka HTTP would be better. I'm afraid I/we don't have the cycles to really dig into exploring that right now though.

About the Metadata API specifics I think that is secondary to deciding the solution for how to get and provide it, that API is fine, but maybe on the read side you shouldn't be able to change it for example. I'm also wondering if we could limit API surface by rather using built in collections than providing something collection-like ourselves, not sure whats the best path there.

@raboof
Copy link
Member Author

raboof commented Sep 27, 2018

Thanks very much for your participation @ctoomey, looking forward to taking this further! This thread runs the risk of becoming unwieldy, I guess at some point we'd want to try and split it up, but for now this is OK. I'll try to keep topics together ;).

Notably absent here is a way to further customize the response. Our intuition so far is that this might not be necessary: we couldn't really come up with a scenario where you'd want detailed access to the response except to signal error conditions, and error conditions can be handled by returning a failed future/source and using a custom error handler.

Provide access to the outgoing response metadata — this is equivalent to being able to modify response headers in handlers which is important for us

Can you elaborate what kind of use cases require access to the response headers from the service implementation (rather than 'middleware')?

@raboof
Copy link
Member Author

raboof commented Sep 27, 2018

One idea we discussed previously is opting in on power-api vs simple-api through the build config. Bad aspect of that is that if you'd want metadata for one method in a server, you'd get different signatures for all the other methods as well.

I think we could configure the code generation via a configuration file, where you could control the code generation either for the whole service or on a per-method basis.

the problem with that is that you'd have to keep the concrete simple signature and let it be a no-op

Right: a disadvantage of the PowerServerGreeterService proposal above is that it keeps the 'simple' API around, but implements it with a no-op/exception. That is mostly invisible to the implementer though, so might be acceptable?

Perhaps an API based on composition, more along the lines of the server DSL in Akka HTTP would be better. I'm afraid I/we don't have the cycles to really dig into exploring that right now though.

Right - I'm not sure I see how that would be possible without introducing other complexity (or giving up the safeguard that all specified methods are indeed implemented), but if someone can come up with a proposal that would be interesting.

@raboof
Copy link
Member Author

raboof commented Sep 27, 2018

  1. Make the API functional and not dependent on throwing exceptions for returning errors

Our current API allows signaling errors by throwing exceptions, but I agree the 'more functional' approach of failing the returned Future/Source is nicer. This should be supported right now.

@ctoomey
Copy link
Contributor

ctoomey commented Sep 28, 2018

  1. Make the API functional and not dependent on throwing exceptions for returning errors

I reread the list of gRPC status codes and see now that they really are lower-level gRPC-specific issues that should never be created directly by gRPC service implementations, so I agree they don't make sense in the service API.

I revisited this statement today and found that there are definitely service implementation use cases where we return equivalent HTTP status codes today and so would need the ability to return grpc status codes, e.g.

  • 401 / PERMISSION_DENIED: The caller does not have permission to execute the specified operation.
  • 404 / NOT_FOUND: Some requested entity was not found — not that the service api not found, but the resource/entity requested wasn’t.
  • 409 / ALREADY_EXISTS: The entity that a client attempted to create already exists.

So we’d still like to have our third proposal for Scala, but it’s the least important of the 3 (most of us came from Java / exception-throwing vs. functional backgrounds).

I think we have deal with the case where you don't care about metadata at all (probably most cases) in a way that it isn't any syntactical overhead. One idea we discussed previously is opting in on power-api vs simple-api through the build config. Bad aspect of that is that if you'd want metadata for one method in a server, you'd get different signatures for all the other methods as well.

Speaking as a user vs. developer of akka-grpc and leading a dev team that’s about to move from REST to grpc for new apis, this is fine and what we’d like to have (toggle to generate all-simple vs. all-power apis) as we want a consistent api that provides metadata access.

Always generating the power-user API would allow us to call the simple signature from the one with metadata by default, but the problem with that is that you'd have to keep the concrete simple signature and let it be a no-op.

Not a problem for us. Let’s leave this “optimization” for later when/if there’s demand for it.

I think we were never quite happy with either of these two path previously when we discussed it.

We want to move forward with the power-only apis and would like to help develop and contribute them to akka-grpc.

Perhaps an API based on composition, more along the lines of the server DSL in Akka HTTP would be better. I'm afraid I/we don't have the cycles to really dig into exploring that right now though.

Nor do we, and we want/need to standardize on power apis.

About the Metadata API specifics I think that is secondary to deciding the solution for how to get and provide it, that API is fine, but maybe on the read side you shouldn't be able to change it for example. I'm also wondering if we could limit API surface by rather using built in collections than providing something collection-like ourselves, not sure whats the best path there.

What I proposed is immutable, but in the end we don’t care that much as long as we have access to the metadata. We could always build helpers if we go with a collection-like representation, but given the important distinction between binary and ascii header values, it needs to be something more than plain (String, Any) header tuples in the collection. I’d be fine with Seq[MetadataEntry] using something like the MetadataEntry I proposed above, or something else that’s reasonable.

Can you elaborate what kind of use cases require access to the response headers from the service implementation (rather than 'middleware')?

I looked at where we currently set response headers for our REST apis and found that it’s mostly for stuff that’s moot for grpc: Content-Type, Content-Length, etc. We also use OAuth 2 and there’s a WWW-Authenticate response header that’s part of the protocol, but not sure if we’ll need that for grpc.

So now I’m not sure we’ll need to be able to set response metadata from service implementations, and so for now we’d be OK with deferring this til it’s needed for sure. I will point out for the record though that this ability is available for Go servers.

In summary, we need and would be willing to work on and contribute back a power api solution that

  1. Is configurable in the build as a single toggle (simple vs. power) for all apis
  2. Provides service api access to request metadata but not response metadata
  3. Requires non-functional exception throwing to return grpc error statuses

if that’s agreeable as an initial solution at least for akka-grpc power api support, with more advanced solutions being deferred if/until warranted. We need this for our own purposes to move forward with Play/akka-grpc.

What do you say?

@raboof
Copy link
Member Author

raboof commented Sep 28, 2018

we need and would be willing to work on and contribute back a power api solution that (...). if that’s agreeable as an initial solution at least for akka-grpc power api support, with more advanced solutions being deferred if/until warranted. We need this for our own purposes to move forward with Play/akka-grpc. What do you say?

That sounds great! I've much enjoyed your contributions to this discussion so far, and would love to collaborate further!

@johanandren
Copy link
Member

I also think that sounds excellent. Please go ahead!

@nrktkt
Copy link

nrktkt commented Nov 7, 2018

Hey all, good discussion going on here on some tricky patterns.

One thought I'd add is the idea of using magnets for service responses. This should allow even less overhead than the existing API for simple use cases like

def sayHello(in: HelloRequest): ReplyMagnet[HelloReply] = {
  println(s"sayHello to ${in.name}")
  HelloReply(s"Hello, ${in.name}")
}

While also allowing more complicated use cases like

def sayHello(in: HelloRequest): ReplyMagnet[HelloReply] = {
  println(s"sayHello to ${in.name}")
  404 -> HelloReply(s"Who is ${in.name}?")
}

or

def sayHello(in: HelloRequest): ReplyMagnet[HelloReply] = {
  MyCustomHeader -> HelloReply(s"Hello, ${in.name}")
}

or whatever

A simple magnet might look like

trait ReplyMagnet[A] {
  def reply: Future[A]
  def mapResponse: HttpResponse => HttpResponse
}

@johanandren
Copy link
Member

I think we want to avoid magnets, because of the high threshold for new comers and that it makes us design the Java API in an either awkward way or fundamentally different from the Scala one.

@nrktkt
Copy link

nrktkt commented Nov 8, 2018

Those seem like solid motivations. I wonder how cumbersome users would find a magnet-minus-the-implicit-conversion pattern. It might be explicit for new users and compatible with a Java approach.
Something like

def sayHello(in: HelloRequest) = {
  println(s"sayHello to ${in.name}")
  ReplyMagnet.fromReply(HelloReply(s"Hello, ${in.name}"))
}
def sayHello(in: HelloRequest) = {
  println(s"sayHello to ${in.name}")
  ReplyMagnet.fromStatusCodeAndReply(404, HelloReply(s"Who is ${in.name}?"))
}

@zhengcan
Copy link

I hope I can get client ip address via the power api.

raboof pushed a commit that referenced this issue Feb 25, 2019
@ctoomey
Copy link
Contributor

ctoomey commented Apr 1, 2019

@raboof Will you go ahead and close this now that PR #429 is merged?

@ctoomey
Copy link
Contributor

ctoomey commented Apr 2, 2019

@zhengcan the power API just provides access to the gRPC request metadata. Normally this is just whatever the client provides, although you could theoretically augment it on the server side by injecting additional metadata like client IP from an upstream proxy server.

@raboof
Copy link
Member Author

raboof commented Apr 2, 2019

@zhengcan in this case you can use the power API and the synthetic Remote-Address header mentioned at https://doc.akka.io/docs/akka-http/current/routing-dsl/directives/misc-directives/extractClientIP.html . Let me know if that indeed works!

@ctoomey yes I think we can indeed close this one, and perhaps add more detailed tickets when we find further needs for extensions to the APIs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants