Skip to content
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

Add delegate for HttpClient override #64

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AnPucel
Copy link
Contributor

@AnPucel AnPucel commented Dec 5, 2022

Description

This allows users to bring their own HttpClient instead of HttpClientHandler. This gives them more control over managing the lifecycle of the HttpClient if they choose, which is notoriously difficult in .Net. It also aligns with the recommended factory pattern of creating HttpClients recommended here

        public static HttpClient HttpClientProvider(HttpClientHandler handler)
        {
            return new HttpClient(handler);
        }
        VaultConfiguration config = new VaultConfiguration("http://127.0.0.1:8200", httpClientProvider: HttpClientProvider);

If you set the timeout when configuring VaultClient that will take precedent over any timeout configured in your provided HttpClient

Resolves # VAULT-9887

How has this been tested?

Tested locally with a generic HttpClient and preconfigured with some random settings.

@AnPucel AnPucel marked this pull request as ready for review December 5, 2022 23:35
@AnPucel AnPucel requested a review from a team December 5, 2022 23:35
public {{packageName}}Configuration(string basePath,
HttpClientHandler httpClientHandler = null,
public VaultConfiguration(string basePath,
Func<HttpClientHandler, HttpClient> httpClientProvider = null,
Copy link
Contributor

Choose a reason for hiding this comment

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

what is Func here? shouldn't this be HttpClient?
(sorry if dumb question, looks weird from a non-c# perspective)

I saw IHttpClientFactory above, shouldn't it take that as the param type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, this is kind of weird. But because the VaultClient needs to configure the HttpClientHandler (for TLS among other things), which the HttpClient is dependent on we have to allow users to pass a delegate (i.e. Func) to new up their own HttpClient with our pre-configured client handler.

Copy link
Contributor Author

@AnPucel AnPucel Dec 6, 2022

Choose a reason for hiding this comment

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

I was debating whether to allow just to pass their own HttpClient object, like you're suggesting, but if they wanted to utilize the TLS, then they'd have to preconfigure it, which may be annoying.

It could be that we offer both options.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the Go version, we allow you to provide HTTPClient directly but then override the TLS options (if configured). Not sure if this is something doable here.

Copy link

Choose a reason for hiding this comment

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

Does this make sense in the context of IHttpClientFactory, as the factory manages the HttpClientHandler instances?

We also allow you to bring your own `HttpClient` through a delegate. This can work
with any current factory pattern that you're using to create an `HttpClient`
object, which is one of the recommended patterns for managing your `HttpClient`
lifecycle. More on the `IHttpClientFactory` can be found [here][[http-client-factory]]
Copy link
Contributor

Choose a reason for hiding this comment

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

bad link?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh whooops

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.

4 participants