Add protocol helpers to infer procedure and type #756
Conversation
Two new methods are added to allow for inferring the procedure and protocol type of a request. These are provided to be used with http middleware to deduce information about the requests. For example, authentication middleware may wish to block on certain protocols or to conditionally allow routes. Signed-off-by: Edward McFarlane <emcfarlane@buf.build>
emcfarlane
force-pushed
the
ed/inferProtocolHelpers
branch
from
23627e1
to
81a58f8
Compare
protocol.go
Outdated
| // HTTP request. It inspects the request's method and headers to determine the | ||
| // protocol. If the request doesn't match any known protocol, an empty string | ||
| // is returned. | ||
| func InferProtocolFromRequest(request *http.Request) string { |
There was a problem hiding this comment.
- The rest of the API uses For and not From
- would this be better named "ProtocolForRequest"?
- would it be better to return string, bool?
There was a problem hiding this comment.
The For is used to distinguish streams that are to be consumed by the client rather than the handler. So I think the From is better here even tho its not currently in the API.
There was a problem hiding this comment.
In my mind they are not necessarily synonyms where you'd always use one over another for consistency.
In my mind, the distinction is that For in a function name is used when the function constructs a new value that corresponds to its input or even wraps its input; From is used when the function extracts a value out of the inputs. In that case, From seems appropriate to me.
As far as returning string vs. (string, bool): another option is to add a ProtocolUnknown constant, so that callers can use a switch and don't need to examine two values. If that approach sounds good, the constant value might need to be the empty string so that an uninitialized Peer object reports ProtocolUnknown. WDYT?
Signed-off-by: Edward McFarlane <emcfarlane@buf.build>
Signed-off-by: Edward McFarlane <emcfarlane@buf.build>
emcfarlane
force-pushed
the
ed/inferProtocolHelpers
branch
from
5c0f286
to
74290d8
Compare
| // HTTP request. It inspects the request's method and headers to determine the | ||
| // protocol. If the request doesn't match any known protocol, an empty string | ||
| // and false is returned. | ||
| func ProtocolFromRequest(request *http.Request) (string, bool) { |
There was a problem hiding this comment.
I've been thinking about if we'd want to ever return something more structured/richer -- like a Protocol type that can inform the caller (for example) if it's connect streaming vs. connect unary or even the version (in the event we ever add a v2 of the protocol), maybe the codec name, etc.
I suppose we can leave this as is for now. In Spec, we'd have to add another accessor -- like maybe ProtocolDetails, and maybe we could then deprecate this and add ProtocolDetailsFromRequest at that time. WDYT?
There was a problem hiding this comment.
The stream is needed by the error writer so it would be nice to expose. It doesn't translate nicely to the grpc or grpc web streaming where we need the descriptor to determine the stream type. These methods are useful, but not that useful so it's annoying for them to clutter the API, they should only be used if the Spec isn't available (maybe that needs to be in the description to avoid misuse?).
My preference would be to move this back to authn-go until we comeup with a better implementation. Maybe we could have a http library in the future that the connect package can utilize with more of these helper methods.
There was a problem hiding this comment.
My preference would be to move this back to authn-go until we comeup with a better implementation.
It would be nice to have some canonical implementation that we can also use from vanguard. Admittedly, in the past, we (with @akshayjshah) discussed a possible connectrpc/protocol-go repo where we could put lower-level details of the protocol like this, and connectrpc/connect-go (et al) could then import that. I'm a little concerned with putting it in authn-go if we expect to move it out later. But I guess it's okay as long as we have this figured out before we get to a v1.0 of authn-go 🤷.
protocol_test.go
Outdated
| name: "grpcGet", | ||
| contentType: "application/grpc+json", | ||
| method: http.MethodGet, | ||
| want: ProtocolConnect, | ||
| valid: true, |
There was a problem hiding this comment.
This is an odd result and maybe should be revisited to defaulting to unknown. We allow any application/ prefix to fall-through to be classified as a connect request.
There was a problem hiding this comment.
I don't think that's right. I've got an issue in gRPC with some spec issues/clarifications (grpc/grpc#36767), and the discussion there has brought up possible experimental support for other verbs, like GET, in gRPC.
If a content-type is set, we should likely use it to ascertain the protocol and not look at the HTTP method. If there is no content-type, then we'd see if it's GET and if so also check for the presence of required message and encoding query params. Otherwise, it's unknown. How does that sound?
There was a problem hiding this comment.
Sounds good! Now check for query parameters for GET requests. Removed Content-Type from all GET requests as that should not be set for any valid request. Still enforce http method for protocol checks as I do think the spec currently requires it, so these are now otherwise unknown.
Signed-off-by: Edward McFarlane <emcfarlane@buf.build>
emcfarlane
force-pushed
the
ed/inferProtocolHelpers
branch
from
cbeccdd
to
8810d37
Compare
|
Closing, talked offline. Can move these helpers to authn-go for now. |
Two new methods are added to allow for inferring the procedure and protocol type of a request. These are provided to be used with http middleware. For example, authentication middleware may wish to block on certain protocols or to conditionally allow routes. These new methods pair well with the authn-go middleware.