tomasfabian / ksqlDB.RestApi.Client-DotNet

ksqlDb.RestApi.Client is a C# LINQ-enabled client API for issuing and consuming ksqlDB push and pull queries and executing statements.
MIT License
93 stars 24 forks source link

Add support for custom IHttpClientFactory when using KSqlDBContext #23

Closed tomheijmans closed 2 years ago

tomheijmans commented 2 years ago

Motivation. Is your feature request related to a problem? Please describe. We're integrating with a KSql instance which requires client-certificate authentication. This is not supported out of the box by you library which is fine. In case we use KSqlDbRestApiClient it allows us to put in a custom IHttpClientFactory which contains our logic to dynamicly create an HttpClient with the client-certificate. Because client-certificates are short lived (hours) we don't use HttpClientFactory provided by your library as it caches the HttpClient and doesn't allow to refresh when the certificate needs to.

Now we got a case where we want to use KSqlDbContext as are dynamicly creating streams and that is were we got stuck. There is no point where we can construct it with a custom IHttpClientFactory, because it only contains an URL as constructor parameter (or KSqlDBContextOptions which also doesn't fit).

Describe the solution you'd like We like to have a constructor with argument IHttpClientFactory for KSqlDbContext like KSqlDbRestApiClient does have. Or maybe (or also), because we are using your IServiceCollection extensions for DI registration that should allow is to define overwrite the IHttpClientFactory in the internal ServiceCollection so it is used everywhere.

Describe alternatives you've considered A clear and concise description of any alternative solutions or features you've considered. Ideally we would integrate with the IServiceCollection extensions in the package and I tought we found an extension point over there but also couldn't get that to work. See the example below. With ReplaceHttpClient we can assign properties of the passed in HttpClient but can't provide one custom created. We need to provide a custom one because client-certifcates are set using the HttpClientHandler which are passed in as a constructor parameter of HttpClient.

services.AddDbContext<IKSqlDBContext, KSqlDBContext>(contextOptionsBuilder =>
{
    contextOptionsBuilder.UseKSqlDb("http://myurl")
        .SetJsonSerializerOptions(serializerOptions => { serializerOptions.Converters.Add(new CustomJsonConverter()); });

    contextOptionsBuilder.ReplaceHttpClient<IHttpClientFactory, HttpClientFactory>(httpClient =>
    {
        httpClient = new CustomKSqlHttpClientFactory().CreateClient();
        // Just changes the local reference, but doesn't change the actual httpClient used
    });
});

Although this extension point could work, it doesn't allow us to have the custom caching code for HttpClient for the duration of there client-certifcate lifetime.

Detailed design Points above should be in detail enough I think.

Additional context Just for context the basic code to add a client-certifcate to an HttpClient:

HttpClientHandler handler = new();
X509Certificate2 clientCertificate = ResolveClientCertificate();
handler.ClientCertificateOptions = ClientCertificateOption.Manual;
handler.ClientCertificates.Add(clientCertificate);

_httpClient = new HttpClient(handler)
{
    BaseAddress = new Uri(_kSqlSettings.Url),
};

Another point on my mind, while trying to find some extension point (or loophole) to fit in our custom IHttpClientFactory I was just trying to add the custom implementation to our ServiceCollection before calling services.AddDbContext<IKSqlDBContext, KSqlDBContext>, but AddDbContext doesn't take dependencies from the general DI container. It just contains an internal ServiceCollection with its own depencencies. The internal ServiceCollection is synced back to the global DI container but because of that internal ServiceCollection it doesn't allow us to overwrite some of the services. What's the reason of using an internal ServiceCollection besides of using the global one, which I think should have made it much more extensible?

Like to hear your thoughts about this. Maybe I could provide a pull request.

tomasfabian commented 2 years ago

Hi @tomheijmans, thank you very much for your detailed description!

Maybe you are very close to a solution for your problem. I don't know where does your HttpClientFactory comes from (which namespace), but I think you can change it as you already tried it:

public class CustomHttpClientFactory : ksqlDB.RestApi.Client.KSql.RestApi.Http.IHttpClientFactory {
      public HttpClient CreateClient()
      {
        //...
        return ...;
      }
}

Simply replace the ksqlDB.RestApi.Client.KSql.RestApi.Http.HttpClientFactory with your own as a generic parameter in ReplaceHttpClient:

services.AddDbContext<IKSqlDBContext, KSqlDBContext>(contextOptionsBuilder =>
{
  contextOptionsBuilder.UseKSqlDb(ksqlDbUrl);

  contextOptionsBuilder.ReplaceHttpClient<IHttpClientFactory, CustomHttpClientFactory>(_ => { })
          .ConfigurePrimaryHttpMessageHandler(c =>
          {
            HttpClientHandler handler = new();
            X509Certificate2 clientCertificate = ResolveClientCertificate();
            handler.ClientCertificateOptions = ClientCertificateOption.Manual;
            handler.ClientCertificates.Add(clientCertificate);

            return handler;
          });
});

Probably you can use the original HttpClientFactory see above how you can provide the certificate with ConfigurePrimaryHttpMessageHandler.

Hopefully it will help you somehow. I don't have time right now to try it out, but it should work.

In case that it didn't help you let me know and I will try to investigate it further later.

Thank you, Regards Tomas.

tomasfabian commented 2 years ago

Something came to my mind: ReplaceHttpClient expects that the provided IHttpClientFactory implementation has an HttpClient constructor argument. So please try to use the original HttpClientFactory and configure the certificate with ConfigurePrimaryHttpMessageHandler if possible. Thx

tomheijmans commented 2 years ago

Hi @tomasfabian,

Thanks for your quick reply and ideas for a solution. I already tried the ReplaceHttpClient with my custom implementation, but got to the same conclusion as you did: it needs an HttpClient as a parameter. Even when I added a dummy HttpClient parameter for testing this solution didn't fit my situation, because my CustomHttpClientFactory also needs a CustomSettings object. This CustomSettings contains configuration about the certifcate to use. The CustomSettings object is in our own ServiceCollection, but the internal used ServiceCollection doesn't resolve dependencies from the main container. Because of that it will result in an exception resolving dependency CustomSettings for CustomHttpClientFactory.

Example constructor signature:

public CustomHttpClientFactory(HttpClient dummyHttpClientCanBeIgnoreOnlyForDI, CustomSettings kSqlSettings)
{
   // dummyHttpClientCanBeIgnoreOnlyForDI is only for method signature so it can be used with AddHttpClient
   // constructor logic
}

The solution with ConfigurePrimaryHttpMessageHandler does allow us to put a client-certificate on the HttpClient (I got a connection)! Although I don't know if it's executed once in its lifetime or multiple times. Is the default IHttpClientFactory created singleton, scoped or transient during the lifetime of a single IKSqlDBContext instance? I thought IHttpClientFactory.CreateClient is executed for every method call on IKSqlDBContext(for example CreateQueryStream). The code in CustomHttpClientFactory.CreateClient reloads the certificate every time the lifetime exceeded, but if ConfigurePrimaryHttpMessageHandler is executed every time we call a method on IKSqlDBContext we could move that certificate lifetime check also to that place.

Thanks for your support, this helps us a lot and proves it does work with a client-certificate.

Regards Tom

tomasfabian commented 2 years ago

Hi @tomheijmans, great to hear that you were able to progress with the help of ConfigurePrimaryHttpMessageHandler.

If I remember it correctly the message handlers are pooled so their lifetime is probably the most important in this case.

The HttpMessageHandler objects in the pool have a lifetime that’s the length of time that an HttpMessageHandler instance in the pool can be reused. The default value is two minutes, but it can be overridden

.ConfigurePrimaryHttpMessageHandler(c =>
{
  //....
}
.SetHandlerLifetime(TimeSpan.FromMinutes(2))

ConfigurePrimaryHttpMessageHandler should be called each time the message handler is (re)created.

I probably don't understand how are you determining that the lifetime exceeded:

CustomHttpClientFactory.CreateClient reloads the certificate every time the lifetime exceeded

The ConfigurePrimaryHttpMessageHandler is not executed every time you call a method on IKSqlDBContext. Is it ok for you if it is called during the HttpMessageHandler (re)creations? For more info please check the .NET documentation.

Regards Tomas

tomheijmans commented 2 years ago

Hi @tomasfabian,

I think ConfigurePrimaryHttpMessageHandler will do the trick for us. It does create new handlers and so it will reload our certificate. Tested this in our test environment.

I probably don't understand how are you determining that the lifetime exceeded:

CustomHttpClientFactory.CreateClient reloads the certificate every time the lifetime exceeded

Our custom implementation will retrieve the certificate and take the expiry date. If DateTime.Now exceeds the expiry date a new HttpClient is created. See code below to illustrate.

public HttpClient CreateClient()
{
    RefreshHttpClientIfNeeded();
    return _httpClient;
}

private void RefreshHttpClientIfNeeded()
{
    if (_clientCertNotAfter <= DateTime.Now())
    {
        HttpClientHandler handler = new();
        X509Certificate2 clientCertificate = ResolveClientCertificate();
        _clientCertNotAfter = clientCertificate.NotAfter;

        handler.ClientCertificateOptions = ClientCertificateOption.Manual;
        handler.ClientCertificates.Add(clientCertificate);

        _httpClient = new HttpClient(handler)
        {
            BaseAddress = new Uri(_kSqlSettings.Url),
        };
    }
}

private X509Certificate2 ResolveClientCertificate() { ..... }

For now we can get all things to work, thanks for your help!

Tom