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

Option to have more control when resolving endpoint IP address in SocketsHttpHandler #25401

Closed
AppBeat opened this issue Mar 11, 2018 · 17 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Net.Http
Milestone

Comments

@AppBeat
Copy link

AppBeat commented Mar 11, 2018

I think it may be useful in some cases to have more control over endpoint address resolving.
This would be new optional feature in SocketsHttpHandler implementation. Here is my initial idea.

Proposed API

public sealed class SocketsHttpHandler : HttpMessageHandler
{
   //existing members
   //...

   //Proposed API
   public Func<HttpRequestMessage, string, int, CancellationToken, ValueTask<IPEndPoint>> ResolveEndpointAsync {get;set;}
}

where delegate inputs are:

  • HttpRequestMessage -> request details
  • string -> request.RequestUri.IdnHost
  • int -> request.RequestUri.Port
  • CancellationToken

and delegate output is:

  • ValueTask<IPEndPoint> -> representing resolved IP address based on given input

Similar Func<> pattern is used in WinHttpHandler.ServerCertificateValidationCallback

Example usage

socketsHttpHandlerInstance.ResolveEndpointAsync = async delegate (HttpRequestMessage request, string host, int port, CancellationToken cancellationToken)
{
	var addresses = await Dns.GetHostAddressesAsync(host).ConfigureAwait(false);
	//resolve hostname to IPv6
	var address = addresses.Where(addr => addr.AddressFamily == AddressFamily.InterNetworkV6).FirstOrDefault();
	return address != null ? new IPEndPoint(address, port) : null;
};

Main goals

  • feature is optional and should not change current behaviour in any way
  • if feature is not used it should not affect existing performance

Possible use cases

  • one could implement ResolveEndpointAsync to use Google DNS or other DNS server for resolving addresses, without changing system settings
  • one could implement ResolveEndpointAsync to use round-robin for resolving IP Addresses
  • better control over IPv4/IPv6 resolving
  • could be used in functional testing to resolve test IP addresses

Prototype implementation: https://github.com/AppBeat/corefx
Very early PR: dotnet/corefx#27937

Kind regards,
Vladimir

@geoffkizer
Copy link
Contributor

This is essentially a hook for providing custom name resolution instead of calling Dns.GetHostEntry. Right?

As such, the callback signature should be similar to the Dns.GetHostEntry, which just takes a string and returns an IPHostEntry (which includes an array of IPAddresses). And it should probably be called something very similar, like GetHostEntryCallback.

Note that we don't resolve DNS names per-request; we only do it when we don't have an existing connection in the pool that we can use. So if you're expecting this callback to happen on every request, you're going to be disappointed. The naming/usage should hopefully make this clear.

Also note that SocketsHttpHandler does not itself call Dns.GetHostEntry; we just pass the DNS name to Socket.ConnectAsync and let it do its thing (which includes retrying on connection failure when there are multiple IPAddresses specified). So this may need to get plumbed all the way down to Socket in order to be implemented properly.

@AppBeat
Copy link
Author

AppBeat commented Mar 11, 2018

Hello.

As currently implemented on https://github.com/AppBeat/corefx, if this callback is set, this should be resolved before getting connection from the pool (at HttpConnectionPoolManager stage in new "GetConnectionKeyAsync" method).
Because it will be called at GetConnectionKey stage callback should be called for every request.

Later, when ConnectHelper.ConnectAsync is called, host parameter will always be resolved IP address at this point if SocketsHttpHandler.ResolveEndpointAsync callback is used:
https://github.com/dotnet/corefx/blob/master/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs

In theory my current experimental implementation should work without going too deep into Sockets or doing any drastic changes.

Please check my changes on https://github.com/AppBeat/corefx/commits/master

Kind regards,
Vladimir

@geoffkizer
Copy link
Contributor

So you're saying you want this callback to be per-request?

I don't think this is a good idea. It means we're potentially doing DNS lookup per-request, and DNS lookup is not necessarily cheap. That's why we only do it when we need to establish a new connection.

It's also a much bigger change to a critical code path, which scares me. A simple hook for DNS resolution is straightforward and self-contained.

@AppBeat
Copy link
Author

AppBeat commented Mar 11, 2018

It is up to implementer of ResolveEndpointAsync to take care of efficiency. We are giving him/her 100% freedom :)
In most cases implementation will probably use some kind of caching. This is why we are having ValueTask as callback result.

If SocketsHttpHandler.ResolveEndpointAsync is not set by user, we should have same experience / performance as today (same resolving logic will be used as today).


Will this feature be useful for some user, will it be ever used in real life? This I don't know :)
(I know I would use it in our monitoring solution :)

@geoffkizer
Copy link
Contributor

Let me put it this way. Is there some reason this needs to be per-request?

@AppBeat
Copy link
Author

AppBeat commented Mar 11, 2018

If connection pool is disposed as soon as SocketsHttpHandler is disposed (which I think it does), then IP address could be internally cached and callback wouldn't have to be called for each request (for the lifetime of SocketsHttpHandler instance). If this is not the case then we don't have full control.

In case of internal address caching it would make sense to change "ResolveEndpointAsync" signature and remove HttpRequestMessage to reflect this (as you suggested previously). We would then have something like this:

public sealed class SocketsHttpHandler : HttpMessageHandler
{
   //existing members
   //...

   //Proposed API
   public Func<string, int, CancellationToken, ValueTask<IPEndPoint>> ResolveEndpointAsync {get;set;}
}

@geoffkizer
Copy link
Contributor

If connection pool is disposed as soon as SocketsHttpHandler is disposed

It is.

it would make sense to change "ResolveEndpointAsync"

Which brings me back to my original feedback:

As such, the callback signature should be similar to the Dns.GetHostEntry, which just takes a string and returns an IPHostEntry (which includes an array of IPAddresses). And it should probably be called something very similar, like GetHostEntryCallback.

I'll modify this slightly: It should look like GetHostAddressesAsync. I don't think there's any reason we need an IPHostEntry here, just the IPAddress[] should be fine.

@AppBeat
Copy link
Author

AppBeat commented Mar 11, 2018

IPAddress without port should also be OK. But why IPAddress[] array would be returned instead of just one IPAddress? Which IPAddress would then be used by SocketsHttpHandler and underlying infrastructure?

@stephentoub
Copy link
Member

I do wonder whether we actually want to restrict this to IPAddress. What if someone wanted to, for example, target an UnixDomainSocketEndPoint?

@AppBeat
Copy link
Author

AppBeat commented Mar 12, 2018

@stephentoub I agree. Probably more general "EndPoint" should be in callback method signature.

@AppBeat
Copy link
Author

AppBeat commented Mar 12, 2018

Update: IPAddress was currently done on my fork because it was much easier to implement and plug-in into existing implementation (into "GetConnectionKey" logic).

@AppBeat
Copy link
Author

AppBeat commented Mar 12, 2018

Maybe users should vote for this feature if they find it relevant. I wouldn't change current API / do drastic changes if current solution fits "99.99%" of users.

@tmds
Copy link
Member

tmds commented Jun 20, 2018

Looking at the list of use-cases:

one could implement ResolveEndpointAsync to use Google DNS or other DNS server for resolving addresses, without changing system settings

This would be hard, since there is no API to do a DNS request against a specific DNS server.

one could implement ResolveEndpointAsync to use round-robin for resolving IP Addresses
better control over IPv4/IPv6 resolving

The system/corefx should do a good job here. It would be hard for the user to provide a better implementation.

could be used in functional testing to resolve test IP addresses

This seems a valid use-case. When setting up some services locally, it is not straight forward to make the system resolve hostnames to those addresses.
Maybe we could do something like:

// Adds a fixed host lookup.
Dns.AddHostEntry(string, IPHostEntry)

@AppBeat wdyt?

@AppBeat
Copy link
Author

AppBeat commented Jun 20, 2018

one could implement ResolveEndpointAsync to use Google DNS or other DNS server for resolving addresses, without changing system settings

I had something like this in mind:

socketsHttpHandlerInstance.ResolveEndpointAsync = async delegate (HttpRequestMessage request, string host, int port, CancellationToken cancellationToken)
{
	return ResolveIPv6FromGoogleDnsAsync(host, port); //method implemented by user
};

private static async Task<IPEndPoint> ResolveIPv6FromGoogleDnsAsync(string host, int port)
{
	//this is just quick example, user could also cache results...
	using (var client = new HttpClient())
	{
		var serializer = new JsonSerializer();

		using (var sr = new StreamReader(await client.GetStreamAsync(new Uri($"https://dns.google.com/resolve?type=AAAA&name={WebUtility.UrlEncode(host)}")).ConfigureAwait(false)))
		using (var jsonTextReader = new JsonTextReader(sr))
		{
			var json = serializer.Deserialize(jsonTextReader) as JObject;
			if (json == null)
			{
				return null;
			}

			var dnsAnswer = json.Property("Answer")?.Value as JArray;
			if (dnsAnswer == null)
			{
				return null;
			}

			foreach (var dnsAnswerEntry in dnsAnswer)
			{
				var data = dnsAnswerEntry.Value<string>("data");
				var type = Uri.CheckHostName(data);
				if (type == UriHostNameType.IPv6)
				{
					if (IPAddress.TryParse(data, out IPAddress ipv6Address))
					{
						return new IPEndPoint(ipv6Address, port);
					}
				}
			}

			return null; //or throw Exception if user requires IPv6 to be resolved - logic is 100% up to user
		}
	}
}

This seems a valid use-case. When setting up some services locally, it is not straight forward to make the system resolve hostnames to those addresses.
Maybe we could do something like:

// Adds a fixed host lookup.
Dns.AddHostEntry(string, IPHostEntry)

I think this could be generally useful for a lot of scenarios. This call would of course affect just running process so there would be no side effects. Now you would have to edit %SystemRoot%\System32\drivers\etc\hosts file for which you must have administrative privileges (and affects entire machine).

@karelz
Copy link
Member

karelz commented Oct 10, 2019

It will be solved by dotnet/corefx#35404

@karelz
Copy link
Member

karelz commented Oct 10, 2019

Duplicate of dotnet/corefx#35404

@karelz karelz closed this as completed Oct 10, 2019
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@scalablecory
Copy link
Contributor

This has been resolved via the API added here in .NET 5: #41949

@ghost ghost locked as resolved and limited conversation to collaborators Dec 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Net.Http
Projects
None yet
Development

No branches or pull requests

7 participants