grpc/client: Implement Channel builder

[nathanielford]

Introduces a type-builder Channel builder for creating new Channels, using an idiomatic pattern for adding in configuration options.

Motivation

In order to flexibly and ergonomically provide for channel configuration on construction, the type builder is introduced. This allows users to construct channels in a manner similar to:

let channel = Channel::builder("https://api.darkcave.com:443")
    .credential(my_creds)
    .runtime(my_runtime)
    .override_authority("arrow.wumpus.api")
    // ... any other options.
    .build();  // No error is returned here

Solution

The solution uses a 'type builder' so that users can build up their channel configuration over various calls, and the compiler can catch type issues. This also allows us to encapsulate various ergonomics (such as default selection of runtime). This PR implements the critical values at this time and defers additional optional values not being used until a later point. It also leaves work around the internals of the PersistentChannel/ActiveChannel for a future PR.

Because the public API is being altered, this requires a version bump.

[arjan-bal]

As discussed offline, I've raised #2694 with only the changes to the trait bounds for the credentials. I'll wait for Doug to review it.

[arjan-bal]

Update: #2703 has been merged to make the ChannelCredentials trait object safe. You should be able to use Arc<dyn ChannelCredentials> everywhere in builder.

[arjan-bal]

I have left some new comments, and some of the previous feedback still needs to be addressed. Please take a look.

[arjan-bal]

We should not need the IntoCredentialConfig trait anymore, we can directly use Arc<dyn ChannelCredentials> everywhere we need to accept the credentials.

[nathanielford]

To be clear, this exists so that passing both arced and non-arced creds are possibe:

let my_creds: ChannelCredentials = ...
let my_other_creds: ChannelCredentials = ...
let my_arced_creds: Arc<dyn ChannelCredentials> = Arc::new(my_other_creds)

let my_channel = Channel::builder("https://api.darkcave.com:443")
    .credential(my_creds)
    .build()
let my_other_channel = Channel::builder("https://api.darkcave.com:443")
    .credential(my_arced_creds)
    .build()

In both cases they are, within the builder, coerced into a CredentialConfig, which is held for the lifetime of the builder but is not a thing the user needs even know about.

[arjan-bal]

Introducing this trait adds an unnecessary layer of indirection for users trying to identify the concrete types their structs must implement. I recommend avoiding this approach.

If we want to improve ergonomics later, we can have the credential constructors return Arc-wrapped credentials directly, or add a builder method that accepts unwrapped credentials. However, it feels a bit too early to introduce these abstractions.

[nathanielford]

There is no indirection that the users are required to know here. The blanket implementations act as a pass-through type:

• If the credentials are passed as an arc, the implementation for CredentialConfig for arcs will leave it as-is.
• If the credentials are not passed as an arc, that impl will arc it. The credentials are saved to the struct as a PresentOpt<Arc<dyn ChannelCredentials>>.
• If anything but an Arc<dyn ChannelCredentials> or a ChannelCredentials is passed, it throws a compiler error.

At this point, any type inspection will show that the builder is of type ChannelBuilder<PresentOpt<Arc<dyn ChannelCredentials>>, MissingOpt> (modulo that last MissingOpt being filled in already).

[arjan-bal]

Introducing this trait adds an unnecessary layer of indirection for users trying to identify the concrete types their structs must implement. I recommend avoiding this approach.

If we want to improve ergonomics later, we can have the credential constructors return Arc-wrapped credentials directly, or add a builder method that accepts unwrapped credentials. However, it feels a bit too early to introduce these abstractions.

[arjan-bal]

Since many of these features are not yet implemented, adding a TODO to configure them feels premature. I recommend omitting the comment, as it lacks a tracking issue and is likely to become stale.

[nathanielford]

Are we using tracking issues for any of this? I was told we weren't, really.

That said, I do intend to address all of these in near-future followups for optional values and for the ServiceConfig work (which is actively blocked on this). I'm not entirely sure the best way to track the state of work given the uncomfortable tension between a) avoiding TODOs, b) not using issues, c) avoiding huge CLs.

[arjan-bal]

Rustdoc for impl blocks is usually unnecessary. Instead we should document the public methods and struct.

[arjan-bal]

Please add rustdoc for public methods. I would also recommend using documentation tests to demonstrate the usage.

[nathanielford]

(Still working on this.)

[arjan-bal]

What is the benefit of using the typed builder pattern for credentials compared to passing them directly into the builder's constructor? From my understanding, typed builders help enforce parameter configuration order, but our credentials do not seem to have any interdependencies.

[nathanielford]

There are two primary reasons we discussed in the design doc:

• Type builders to accumulate options is pretty standard in the wider Rust ecosystem.
• This gives us flexibility w/o cruft around what the options are.

The credentials() call is a good example of a place where there is enough uncertainty on how we want to provide credentials in the future that being able to provide paths for satisfying that requirement is advantageous. Right now, .credentials(my_creds) is what we provide, but we are free to add any number of functions that yield a ChannelBuilder<PresentOpt<Arc<dyn ChannelCredentials>>, T>`. As with the runtime, if we wanted to provide a 'default' option in the future, or one configured via env variables or whatever other method.

(To be clear, we probably won't with credentials, but there are some questions around how this interacts with various concerns that giving ourselves a clear pathway to handle this cleanly is nice. Given this and the fact we are doing it with the runtime means it makes sense to keep it consistent.)

There are other nice to haves (e.g. free ordering of parameters) but the above are the two key reasons we went this route.

[arjan-bal]

Similar to my question above, what is the benefit of using a typed builder for the runtime? A simpler alternative would be to introduce a feature-flag-protected constructor that defaults to the Tokio runtime when enabled:

impl Channel {
    #[cfg(feature = "_runtime-tokio")]
    pub fn builder(target: impl Into<String>) -> ChannelBuilder {}

    #[cfg(not(feature = "_runtime-tokio"))]
    pub fn builder(target: impl Into<String>, rt: Arc<dyn Runtime>) -> ChannelBuilder {}
}

[nathanielford]

Idiomatically this isn't how most libraries do their builders. You land yourself in a world where a string of parameters creates craziness. Consider how the example above looks if you have two parameters with feature flag and feature flag-less pathways, and what happens to code that flips a flag. This also lets us handle optional parameters in the same fashion.

[arjan-bal]

This authority will get used by the xDS resolver, we should keep it.

[nathanielford]

I would like to leave this for a follow up; the persistent_channel.security_opts.authority is the wrong type for ResolverOptions, but I don't want to make a decision about how to manage this right now since it's not directly related to the ChannelBuilder.

[arjan-bal]

What's wrong with registering plugins here?

[nathanielford]

It might be fine, and we might just leave it. However, this sets up the default registries for the entire library for all channels. It's a bit odd for this to be the responsibility of the channel builder, though it may be the first and only reasonable place for it to happen in actual usage.