-
Notifications
You must be signed in to change notification settings - Fork 231
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
L90: Go: Support grpc.Dial interceptors #274
Open
anicr7
wants to merge
2
commits into
grpc:master
Choose a base branch
from
anicr7:dial_interceptor
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,104 @@ | ||
L90: Support global dial interceptors in Golang | ||
---- | ||
* Author(s): Anirudh Ramachandra | ||
* Approver: dfawley | ||
* Status: Draft | ||
* Implemented in: Go | ||
* Last updated: 2021-11-16 | ||
* Discussion at: <google group thread> (filled after thread exists) | ||
|
||
## Abstract | ||
|
||
Support dial interceptors in grpc-go which allow users to apply business logic globally to all clients including those created in the gRPC package. | ||
|
||
## Background | ||
|
||
grpc.Dial is usually the entry point for all client operations. It is possible to customize the behavior of grpc.Dial using dial options including interceptors. When dial options are not good enough, teams are free to create a wrapper Dial function and use it as the entry for their clients. Popular use cases include adding business logic to track statistics, making tracing easy and applying custom socket options. The wrapper Dial usually provides hooks to apply the necessary business logic before finally calling grpc.Dial. | ||
|
||
An issue arises when the gRPC package itself is responsible for creating clients. For example in the xDS layer, a gRPC client connection is established with the xDS server. As the gRPC package itself uses grpc.Dial and cannot use the wrapper Dial it is not possible to apply the business logic. | ||
|
||
This RFC proposes a new global dial interceptor in Golang to provide a cleaner way to add these hooks to all clients including those created within the gRPC library. | ||
|
||
### Related Proposals: | ||
N/A | ||
|
||
## Proposal | ||
|
||
gRPC will support setting a new global Dial interceptor which if set will be called as part of every grpc.Dial/grpc.DialContext. The application can set the dial interceptor as part of its initialization by calling "SetDialInterceptor" | ||
|
||
``` | ||
type dialerCallback func(ctx context.Context, target string, opts ...DialOption) (*ClientConn, error) | ||
|
||
type DialInterceptor func(ctx context.Context, target string, defaultDial dialerCallback, opts ...DialOption) (*ClientConn, error) | ||
|
||
var dialInterceptor DialInterceptor | ||
|
||
func SetDialInterceptor(interceptor DialInterceptor) { | ||
dialInterceptor = interceptor | ||
} | ||
``` | ||
|
||
Internally, the gRPC DialContext will be modified to call the dial interceptor if it's set. If the dial interceptor is not set, the current behavior is maintained. Global Dial interceptors will also receive the functor to grpc.dialContext which can be called after performing the required application logic. | ||
|
||
``` | ||
func DialContext(ctx context.Context, target string, opts ...DialOption) (conn *ClientConn, err error) { | ||
if dialInterceptor == nil { | ||
return dialContext(ctx, target, opts...) | ||
} | ||
return dialInterceptor(ctx, target, dialContext, opts...) | ||
} | ||
|
||
func dialContext(ctx context.Context, target string, opts ...DialOption) (conn *ClientConn, err error) { | ||
// Same logic as that existed before in DialContext. | ||
} | ||
``` | ||
|
||
## Rationale | ||
|
||
Supporting a dial interceptor allows users of gRPC go to inject their business logic while also handling clients that are implicitly created in the gRPC package. Currently, this is only an issue for gRPC clients created within the gRPC package i.e. xDS but longer term this will be a major pain point for users who have custom wrappers around grpc.Dial. | ||
|
||
The gRPC dial interceptor allows users to both inject custom logic as well as gRPC client specific dial options. | ||
|
||
An alternative approach to solve this problem is to expose a way of injecting Global Dial Options in clientconn.go. | ||
|
||
### Global Dial Options | ||
|
||
Similar to the dial interceptor, the global dial options can be set by the application as part of its initialization. | ||
``` | ||
// clientconn.go | ||
|
||
var globalDialOptions grpc.DialOption | ||
|
||
// AddGlobalDialOptions adds options that are applied globally to all | ||
// Dial calls. | ||
func AddGlobalDialOptions(options ...DialOption) { | ||
globalDialOptions = append(globalDialOptions, options...) | ||
} | ||
``` | ||
|
||
grpc.DialContext can then be augmented so that the global options are considered for each client that is created. This can be done by a simple prepend operation with DialContext opts params. | ||
``` | ||
func DialContext(ctx context.Context, target string, opts ...DialOption) (conn *ClientConn, err error) { | ||
// Apply globalDialOptions to Dial calls. | ||
opts = append(globalDialOptions, opts...) | ||
... | ||
} | ||
``` | ||
|
||
#### Pros | ||
|
||
* This approach is simpler as it restricts the possible configurations that can be configured as part of the application | ||
|
||
#### Cons | ||
|
||
* Does not allow any manipulation of the target or the context. | ||
* Manipulating the dial options is not enough to apply most application logic. | ||
|
||
|
||
While this use case might be able to handle *most* use cases where custom logic can be injected using interceptors, it is restrictive compared to the dial interceptor. The actual work needed for either of these options is similar, so it makes sense to converge on the Dial Interceptor. | ||
|
||
## Implementation | ||
|
||
* Support a new gRPC Dial interceptor which is similar to gRPC.DialContext, with gRPC.DialContext as a special parameter for callback. | ||
* Support a new function in clientconn.go to set this global interceptor. | ||
* Any teams that have custom wrappers that finally call grpc.Dial, can then shift their business logic to the interceptor. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think more discussion is needed here.
init
functions? I believe this impliesgrpc.Dial
may never be called in a goroutine forked in aninit
, which is a bit hard to explain.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is definitely possible to support this using another indirection layer(either in the application or in gRPC package)
This makes it possible to support multiple interceptors but also indicates that the ClientConn can only be established by the default dialer. The interceptors can only modify ctx, target, opts. Would this approach be preferable?
We can support setting the dial interceptor through atomic.LoadPointer and atomic.StorePointer to avoid locks. An example here:
Though even if we do support atomics, there is always going to be a race condition between an init() which calls both SetDialInterceptor and grpc.Dial in another go routine.
I don't think I completely understand the use case here. Calling grpc.Dial in init() should be fine with or without a go routine. But the race condition for the Set vs Dial might exist if they are executed in different go routines. The behavior of the grpc.Dial in the go routine will depend on whether Set was called earlier or not.
Is there a way we can prevent this race condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
init()
sounds good.grpc.Dial()
is called ininit()
, there doesn't seem to be any good solution to it. If the interceptor is not added by this or anotherinit()
, it's also too late for this dial.