Skip to content

client: allow grpc dial option passthrough#3740

Merged
tonistiigi merged 1 commit intomoby:masterfrom
jedevc:grpc-client-options
Mar 27, 2023
Merged

client: allow grpc dial option passthrough#3740
tonistiigi merged 1 commit intomoby:masterfrom
jedevc:grpc-client-options

Conversation

@jedevc
Copy link
Copy Markdown
Member

@jedevc jedevc commented Mar 24, 2023

This allows consumers of buildkit's client API to manually specify grpc options, such as underlying transport details, or creating interceptors for requests.

Some example use cases:

  • Manual configuration of the grpc connection details, e.g. keepalive times, user agents, etc.
  • Configuration of grpc headers using interceptors, which could be useful for grpc-level proxies (e.g. as in Consistent hashing for proxy load balancers #3154) - this is the kind of case I'm specifically interested in.

Maybe this isn't the right approach? If we're interested in manually setting headers, maybe we would want a dedicated ClientOpt type?

@jedevc jedevc requested review from crazy-max and tonistiigi March 24, 2023 14:08
This allows consumers of buildkit's client API to manually specify grpc
options, such as underlying transport details, or creating interceptors
for requests. All custom options are appended *after* the internal
options, to allow for more specific overrides if desired.

Additionally, to prevent accidentally overwriding the internal
interceptors, we switch the client to using
`WithChain{Unary,Stream}Interceptor` instead of the singular form, which
will overwrite if multiple are specified.

Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc jedevc force-pushed the grpc-client-options branch from b999f4d to 17df884 Compare March 27, 2023 10:04
@jedevc jedevc requested review from crazy-max and tonistiigi March 27, 2023 10:04
@jedevc
Copy link
Copy Markdown
Member Author

jedevc commented Mar 27, 2023

Woops, I did some testing and figured out that this isn't quite right, we want to apply all the options at the end, instead of possibly in-between other options.

We also need to change the method used to create unary and stream interceptors, so that we don't accidentally override the previous ones.

I also added a test to make sure that the options are processed and that we can add new interceptors.

Comment thread client/client.go

contentapi "github.com/containerd/containerd/api/services/content/v1"
"github.com/containerd/containerd/defaults"
grpc_middleware "github.com/grpc-ecosystem/go-grpc-middleware"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up: can we now remove all other usage of grpc_middleware as well?

@tonistiigi tonistiigi merged commit e27a502 into moby:master Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants