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

Mechanism to define provider for generating defaultCallOptions at request time. #1567

Closed
majelbstoat opened this issue Feb 9, 2023 · 12 comments

Comments

@majelbstoat
Copy link

Is your feature request related to a problem? Please describe it.

I have a service where responses vary based on whether the API request is authenticated or not. Authentication is done by means of passing a bearerToken in metadata. Basically:

func authenticatedCallOptions() {
  guard let bearerToken = TokenManager.bearerToken else {
    return CallOptions()
  }
  let headers: HPACKHeaders = ["authorization": "Bearer \(bearerToken)"]
  return CallOptions(customMetadata: headers)
}

// then ...
func getViewer() {
   let viewer = try await client.getCurrentViewer(Google_Protobuf_Empty(), callOptions: authenticatedCallOptions())
}

A separate sign-in/sign-out mechanism controls whether the TokenManager's optional bearerToken is populated or not.

It is tedious and easy to forget having to put authenticatedCallOptions() for every rpc method call - there are many dozens in this system.

I tried using defaultCallOptions:

 client = UserServiceAsyncClient(channel: GRPCManager.connection, defaultCallOptions: authenticatedCallOptions())

However, this is evaluated once, at client creation time, fixing the authorization header as empty or populated. When sign-in/sign-out occurs, this is not re-evaluated.

Describe the solution you'd like

I'd like to specify a common call options function, which is executed every time a call is made using the client. In my case, that would be:

client = UserServiceAsyncClient(channel: GRPCManager.connection, commonCallOptionProvider: authenticatedCallOptions)

(Note, no invocation of the function.)

That provider would be executed each time a request is made to get the CallOptions for the request.

Ideally, that would then be merged with any particular request-specific call options, for additional control, without removing any already specified, though I understand that might be a bit too magical. Would definitely settle for defaultCallOptionProvider, and then having to remember to generate the entirety of my call options if the default wasn't correct.

Describe alternatives you've considered

Building the call options at every single request call site.

@glbrntt
Copy link
Collaborator

glbrntt commented Feb 9, 2023

You should be able to achieve this with a client interceptor; you'd just need to intercept the request metadata, insert the latest bearer token and then forward it.

See also: https://github.com/grpc/grpc-swift/blob/main/docs/interceptors-tutorial.md

@majelbstoat
Copy link
Author

Ah, fantastic, thank you.

@majelbstoat
Copy link
Author

Having looked at this fully, I was surprised to find that client interceptor approach is on a per-rpc basis that still requires a lot of boilerplate if you want the same behaviour to affect all requests.

If you compare to what grpc-go does, for example, they are generic and not tightly coupled to a specific rpc method. They provide the context, and sufficient info to introspect the request/response if necessary. Here's a go server-side log interceptor, for example, that logs every request that comes in:

func RequestLogInterceptor(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp interface{}, err error) {
	ctx = WithLogger(ctx)
	Track(ctx, "method", info.FullMethod)

	start := time.Now()
	resp, err = handler(ctx, req)

	size := 0
	if p, ok := resp.(proto.Message); ok {
		size = proto.Size(p)
	}
	logGRPCRequest(ctx, start, info.FullMethod, err, size)

	return
}

I can kind of understand the desire to support common behaviour on a per-rpc basis, but in my experience, we tend to write a wrapper around the lower level grpc client calls anyway, which already provide a handy place to do "before" and "after" work for a specific RPC. For example, I always tend to have a single local getUser() method which wraps the client.getUser() and does conversion to local types. Because of this, per-rpc interceptors don't actually offer any value to me, and just shifts the place that I have to remember to add token/authentication injection.

The value of interceptors in my opinion is centralising common behaviour, like logging, tracing, authentication etc. To do those, we typically don't need the concrete types, just protocols, and we don't need it to apply to selective requests.

Forgive me if ClientInterceptor can actually be used generically in this way, but I don't see it either in docs or looking at the generated client implementations.

I'll keep this closed, because I can see that a runtime-processed default call options provider isn't the right way to go. But I wonder if you'd consider an alternative/additional implementation of client interceptors around request and response protocols instead of concrete types. If you're amenable, I'll open a new issue.

@Lukasa
Copy link
Collaborator

Lukasa commented Feb 10, 2023

Client interceptors can be made generic easily. Using the LoggingEchoClientInterceptor, the example proposes this:

class LoggingEchoClientInterceptor: ClientInterceptor<Echo_EchoRequest, Echo_EchoResponse>

To make this generic, you can change the definition to:

class LoggingClientInterceptor<Request, Response>: ClientInterceptor<Request, Response>

You do need to instantiate these separately for each type in your application, but if you needed to centralise behaviour you can delegate to another object that doesn't hold the request/response types at all, or that erases them to Any.

@majelbstoat
Copy link
Author

If I do that, do I still need a factory conforming to UserServiceClientInterceptorFactoryProtocol that provides functions like makeGetUserInterceptors, makeGetUserSimilarityInterceptors, makeGetUsersInterceptors, makeSignInInterceptors, etc etc?

@Lukasa
Copy link
Collaborator

Lukasa commented Feb 10, 2023

Yes, you do. That definitely adds some noise to the code, though the functions are straightforward and small, and should be able to be satisfied by a single factory with generic functions. Up to @glbrntt but we could probably envision a simpler model for interceptors that don't need to intimately know about the message types.

@glbrntt
Copy link
Collaborator

glbrntt commented Feb 10, 2023

It's certainly clunky having to do this and it comes up frequently enough that it probably does deserve some time spent on it.

@talksik
Copy link

talksik commented Dec 11, 2023

Why am I unable to do this with an interceptor? Can someone please show me an example of setting headers before sending a request? I'm surprised that this is not simple, or maybe I lack knowledge of swift.

image

@majelbstoat
Copy link
Author

Here’s what works for me:

//
//  IdentifiedViewerInterceptor.swift
//  Pinian
//
//  Created by Jamie Talbot on 2/21/23.
//

import Foundation
import GRPC
import NIOCore

class IdentifiedViewerInterceptor<Request, Response>: ClientInterceptor<Request, Response> {
    override func send(
        _ part: GRPCClientRequestPart<Request>,
        promise: EventLoopPromise<Void>?,
        context: ClientInterceptorContext<Request, Response>
    ) {
        var newPart = part
        switch part {
        case var .metadata(headers):
            if let bearerToken = ViewerManager.shared.identifiedViewer?.bearerToken {
                headers.add(name: "authorization", value: "Bearer \(bearerToken)")
                newPart = .metadata(headers)
            }
        default:
            break
        }

        context.send(newPart, promise: promise)
    }

    override func receive(
        _ part: GRPCClientResponsePart<Response>,
        context: ClientInterceptorContext<Request, Response>
    ) {
        switch part {
        case let .metadata(headers):
            if let bearerToken = headers["pinian-auth-token"].first {
                let expiry = Int(headers["pinian-auth-expiry"].first ?? "0")
                if bearerToken.isEmpty || expiry == 0 {
                    // The server cleared our bearer token or it expired, so stop sending it.
                    ViewerManager.shared.clearIdentifiedViewer()
                }
            }
        default:
            break
//            print("< Received headers:", headers)
//
//        case let .message(response):
//            print("< Received response '\(response)'")
//
//        case let .end(status, trailers):
//            print("< Response stream closed with status: '\(status)' and trailers:", trailers)
        }

        context.receive(part)
    }
}

@talksik
Copy link

talksik commented Dec 11, 2023

Thank you for the prompt response @majelbstoat !!

The workaround of creating a copy of the part makes sense. I am still running into the same error because in your code, you are still mutating the headers of the original part.

Sending image to show error
image

Perhaps something may have changed between versions of grpc-swift? I am on 1.20.0

I believe maybe this should do it? I haven't tested it though.

  override func send(
    _ part: GRPCClientRequestPart<Request>,
    promise: EventLoopPromise<Void>?,
    context: ClientInterceptorContext<Request, Response>
  ) {
      var newPart = part
    switch part {
    // The (user-provided) request headers, we send these at the start of each RPC. They will be
    // augmented with transport specific headers once the request part reaches the transport.
    case let .metadata(headers):
        // TODO: FIX
        var newHeaders: HPACKHeaders = ["human-session": self.sessionToken ?? ""]
        newHeaders.add(contentsOf: headers)
        newPart = .metadata(newHeaders)

    // The request message and metadata (ignored here). For unary and server-streaming RPCs we
    // expect exactly one message, for client-streaming and bidirectional streaming RPCs any number
    // of messages is permitted.
    case let .message(request, _):
        print("> Sending request with text '\(request)'")

    // The end of the request stream: must be sent exactly once, after which no more messages may
    // be sent.
    case .end:
      print("> Closing request stream")
    }

    // Forward the request part to the next interceptor.
    context.send(newPart, promise: promise)
  }

@majelbstoat
Copy link
Author

I think you need to use var instead of let, otherwise you can’t mutate headers

@talksik
Copy link

talksik commented Dec 11, 2023

Ahhhh so dumb of me. Thank you @majelbstoat. That worked for me as well!

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

No branches or pull requests

4 participants