Add proxy mapper hook. #9372
Add proxy mapper hook. #9372
Conversation
There was a problem hiding this comment.
Thanks much for the PR.
When this is integrated with #9383, I think that we should update the comments to indicate which channel args to use in the mapper (if we have to live with the fact that the proxy mapper cannot return a handshaker object which would be conceptually cleaner IMO.
| bool grpc_proxy_mapper_map(grpc_exec_ctx* exec_ctx, grpc_proxy_mapper* mapper, | ||
| const grpc_resolved_address* address, | ||
| const grpc_channel_args* args, | ||
| grpc_resolved_address** new_address, |
There was a problem hiding this comment.
What do you think about returning a (http_connect) handshaker object here?
It is unclear to me how the CONNECT request is going to be built after this call (based on which channel_args for example)? Returning a handshaker object would make the implementer of this API choose the shape of the CONNECT request (host + other headers).
There was a problem hiding this comment.
However, maybe at the time where this function is called, it is too late (or impractical) to insert a handshaker which would call indeed for the use for channel args.
There was a problem hiding this comment.
The intent here is that the proxy mapper will set the channel args introduced in #9383 to trigger the use of HTTP CONNECT.
The main reason I didn't have the proxy mapper return a new handshaker is that it seemed redundant to have two different mechanisms to trigger the creation of handshakers. We've already got the handshaker factory mechanism to take care of that, and I think that mechanism is a more general solution, because there are cases where handshakers need to be added that having nothing to do with proxies (such as the security handshaker).
| /// Registers a new proxy mapper. Takes ownership. | ||
| /// If \a at_start is true, the new mapper will be at the beginning of | ||
| /// the list. Otherwise, it will be added to the end. | ||
| void grpc_proxy_mapper_register(bool at_start, grpc_proxy_mapper* mapper); |
There was a problem hiding this comment.
Is there a strong use case for multiple proxy mappers?
There was a problem hiding this comment.
I'm not aware of a concrete use-case, but it's conceivable that someone could have a client that wants to talk to two different environments similar to cases 2 or 3 in the gRFC. It that case, it would make sense to have a different proxy mapper for each environment.
There was a problem hiding this comment.
I see. OK that seems reasonable to me.
| grpc_arg new_arg = grpc_create_subchannel_address_arg(addr); | ||
| grpc_resolved_address *new_address = NULL; | ||
| grpc_channel_args *new_args = NULL; | ||
| if (grpc_proxy_mappers_map(exec_ctx, addr, args->args, &new_address, |
There was a problem hiding this comment.
Would it be possible to insert an http connect handshaker here? It would basically avoid the manual parsing of HTTP CONNECT headers from channel args (see #9383).
There was a problem hiding this comment.
See my response above.
|
Known failures: #9124 |
I believe that this is the last piece we need before getting rid of the initial_connect_string hack.