Don't throw away the inner exception

[haf]

Description

The RestClient for SchemaRegistry throws away the inner exception:

confluent-kafka-dotnet/src/Confluent.SchemaRegistry/Rest/RestService.cs

Line 248 in 9c5dfff

throw new HttpRequestException(aggregatedErrorMessage);

So that error messages become:

without an inner exception. Simply pass the caught exception into the c'tor to fix this issue.

How to reproduce

Make the schema registry TLS endpoint be something that this client doesn't like.

Similarly, it's impossible to override the logic for validating TLS certificates due to how the HttpClient is created:

confluent-kafka-dotnet/src/Confluent.SchemaRegistry/Rest/RestService.cs

Lines 68 to 75 in 9c5dfff

	if (certificates.Count > 0)
	{
	client = new HttpClient(CreateHandler(certificates, enableSslCertificateVerification)) { BaseAddress = new Uri(uri, UriKind.Absolute), Timeout = TimeSpan.FromMilliseconds(timeoutMs) };
	}
	else
	{
	client = new HttpClient() { BaseAddress = new Uri(uri, UriKind.Absolute), Timeout = TimeSpan.FromMilliseconds(timeoutMs) };
	}

This is doubly annoying because it would seem that, by explicitly creating a new HttpRequestMessage in CreateRequest, the global ServicePointManager.ServerCertificateValidationCallback that I tried setting, is overridden and defaulted.

Finally it's annoying because it would seem that on macOS, .Net Core doesn't default to using the system-configured certificates, so even if this certificate is valid for me outside .net, it's not inside .net.

Checklist

I haven't followed it because this issue is a review of the source code.

Please provide the following information:

• A complete (i.e. we can run it), minimal program demonstrating the problem. No need to supply a project file.
• Confluent.Kafka nuget version.
• Apache Kafka version.
• Client configuration.
• Operating system.
• Provide logs (with "debug" : "..." as necessary in configuration).
• Provide broker log excerpts.
• Critical issue.

[mhowlett]

thanks @haf - I think you have made me realize we should allow the user to pass in their own HttpClient instance, or collection of them for HA.

from an API perspective, I think that means it's time to introduce a SchemaRegistryClientBuilder, and this should create a ISchemaRegistryClient. I think CachedSchemaRegistryClient can remain user createable though as is, for now at least.

i probably won't get to this immediately, but it shouldn't be too hard. i'd probably be able to review a smallish high quality PR though to get it in quickly.

[dutchie032]

Hi there @mhowlett ,

Just quickly checking up on this here.
I get the statement the custom HttpClient would help with and solve this, but is there a reason for throwing away the InnerExceptions?

I'm currently running into a "HttpRequestException" with the message: "An error occurred while sending the request." which normally has useful innerExceptions.

Is there a reason for throwing away the exceptions at all?
I guess, with a big amount of brokers and registries it'll do a lot of retries and all the exception amount to a huge logfile.
I mean, as a developer I'd rather see it all and get a bloated log (when it goes wrong) and handle that than debugging with a message that states "something went wrong".

I guess the HttpClient would solve it all, but the question stated, about the innerException being thrown away, doesn't seem to be answered.

Thanks!
Tim

[mhowlett]

is there a reason for throwing away the InnerExceptions?

no, we should keep them

[dutchie032]

Ah okay!

Thanks for confirming that :)

Will there be a fix on its way or are there already branches open with that fix (couldn't find any)?
Could be worth for me picking up a prerelease for testing/debugging and reverting to a stable one afterwards.
Don't know how big of a change it will be and how other issues relate to this.

Sorry if I'm oblivious here

[mhowlett]

this hasn't been addressed yet. will happily review PR for exposing inner exceptions and get it merged if it's good.

[torbjornsk]

+1 on this one!

I'm currently getting this error message:

HttpRequestException: The SSL connection could not be established, see inner exception.

But as the inner exception is just swallowed, it provides very limited value. I need to pull out a debugger and spend 20x as much time on this as I would if I could just see the inner exception in the first place.