Add `IDestinationResolver` for resolving cluster destination addresses #2210

ReubenBond · 2023-08-01T16:56:32Z

This PR adds IDestinationResolver, an abstraction for expanding the ClusterConfig.Destinations property, resolving the destination addresses into one or more addresses. The intention is to better support service discovery in YARP.

/// <summary>
/// Resolves destination addresses.
/// </summary>
public interface IDestinationResolver
{
    /// <summary>
    /// Resolves the provided destinations and returns resolved destinations.
    /// </summary>
    /// <param name="destinations">The destinations to resolve.</param>
    /// <param name="cancellationToken">The cancellation token.</param>
    /// <returns>
    /// The resolved destinations and a change token used to indicate when resolution should be performed again.
    /// </returns>
    ValueTask<ResolvedDestinationCollection> ResolveDestinationsAsync(
        IReadOnlyDictionary<string, DestinationConfig> destinations,
        CancellationToken cancellationToken);
}

The PR also currently adds a Host property to the DestinationConfig, which is indented to act as the HTTP host name (for virtual host & SNI support).

ReubenBond · 2023-08-01T20:55:28Z

Is someone able to advise me on the best way to implement the abovementioned Host feature?

ReubenBond · 2023-08-01T22:04:50Z

I spoke with @Tratcher about how to flow the Host property through, and have implemented that in the current version of the PR.

ReubenBond · 2023-08-07T18:08:57Z

Alright, I consider this ready for review.

src/ReverseProxy/Management/ProxyConfigManager.cs

Co-authored-by: Arvin Kahbazi <akahbazi@gmail.com>

ReubenBond · 2023-08-08T18:12:26Z

@Tratcher @MihaZupan PTAL

MihaZupan

A few minor suggestions, otherwise this LGTM, thank you.

Might be worth adding a mention in the docs somewhere that this is an available extension point.

src/ReverseProxy/Configuration/DestinationConfig.cs

src/ReverseProxy/Health/DefaultProbingRequestFactory.cs

src/ReverseProxy/Management/ProxyConfigManager.cs

src/ReverseProxy/Transforms/RequestHeaderOriginalHostTransform.cs

test/ReverseProxy.Tests/Management/ProxyConfigManagerTests.cs

test/ReverseProxy.Tests/Transforms/Builder/TransformBuilderTests.cs

Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>

ReubenBond · 2023-08-09T18:21:15Z

Feedback addressed, thank you @MihaZupan

ReubenBond · 2023-08-09T18:55:01Z

Added a short doc on destination resolvers

src/ReverseProxy/Configuration/NoOpDestinationResolver.cs

ReubenBond · 2023-08-14T18:00:09Z

I'll add an implementation of IDestinationResolver using System.Net.Dns for completeness' sake

ReubenBond · 2023-08-14T19:00:15Z

Ok, I've added a DNS IDestinationResolver implementation with a test. It's opt-in and you configure it programmatically via reverseProxy.AddDnsDestinationResolver(...).

ReubenBond · 2023-08-14T19:08:10Z

DnsDestinationResolver defaults to adding a destination per IP, but that means we could get multiple destinations for the same physical host if DNS returns an IPv4 & IPv6 address for the same host. You can specify an AddressFamily to query in the options to choose to only get one or the other.

Alternatively, we could scan over the query results and choose to prefer only IPv4 addresses or only IPv6 addresses if there is a mix of IPv4 and IPv6 addresses. I'm open to feedback.

ReubenBond · 2023-08-14T21:58:20Z

BTW note that I placed IDestinationResolver and friends into a new namespace, Yarp.ReverseProxy.ServiceDiscovery.

docs/docfx/articles/destination-resolvers.md

Tratcher · 2023-08-14T20:55:18Z

src/ReverseProxy/ServiceDiscovery/DnsDestinationResolver.cs

+            _logger.LogError(ex, "Error resolving destinations");
+            throw;


Anti-pattern: Don't log and throw, this causes duplicate logs. The caller will log it right?

The caller will rethrow on first load, with "Unable to load or apply the proxy configuration."
On reload, it will log "Failed to reload config. Unable to register for change notifications, polling for changes until successful."

i.e, it will be logged the same as an IProxyConfig.GetConfig() exception. Is that fine?

Yes, that's fine. If you want a more specific log, log without the full exception, maybe just the message?

Tratcher · 2023-08-14T20:57:05Z

src/ReverseProxy/ServiceDiscovery/DnsDestinationResolver.cs

+        DestinationConfig originalConfig,
+        CancellationToken cancellationToken)
+    {
+        var originalUri = new Uri(originalConfig.Address);


Uri.TryParse? There's no telling what's in the config.

This is running before filters, right?

What should it do if the TryParse fails?

Log and return the original destination?

Is that valid behavior? Eg, a Warning level log saying Destination {DestinationId} has an unsupported Address. Addresses must be valid URIs.?

We're careful not to over-validate/restrict destination addresses because people can plug in alternative transports like named pipes and use custom address formats. I think we only validate that it's not null or empty. A warning log seems appropriate in this case.

docs/docfx/articles/destination-resolvers.md

src/ReverseProxy/ServiceDiscovery/DnsDestinationResolver.cs

Tratcher · 2023-08-14T21:09:14Z

src/ReverseProxy/ServiceDiscovery/DnsDestinationResolver.cs

+    /// <summary>
+    /// Creates a <see cref="UriBuilder"/> from the provided address, only setting the Port property if it is not the default value.
+    /// </summary>
+    private static UriBuilder CreateUriBuilder(Uri uri)


How is this different from new UriBuilder(uri)?

It's the Port. When I just use new UriBuilder(uri), it will include ":80" in the Host, which is ugly (it's probably not a showstopper). It should only include the port if it's a non-default port for the scheme. The doc comment tries to indicate this: "only setting the Port property if it is not the default value."

And there's no way to make it clear the default port later?

Not from what I could tell, and not from what Stack Overflow seemed to indicate 😁

Tratcher · 2023-08-14T21:13:33Z

src/ReverseProxy/ServiceDiscovery/DnsDestinationResolverOptions.cs

+    /// <remarks>
+    /// Defaults to 5 minutes.
+    /// </remarks>
+    public TimeSpan? RefreshPeriod { get; set; } = TimeSpan.FromMinutes(5);


Are you really expecting changes this often?

Ideally we'd use a dns library that could read the TTL data and refresh based on that.

Yea, but this is what gRPC ships with too (we should check their impl). Anything else requires external dependencies.

cc @JamesNK

gRPC keeps open connections with resolved endpoints. This allows it to track the health of endpoints in realtime. If one of those connections goes down, such as because a k8s pod instance is restarted, then gRPC immediately asks the resolver to refresh itself. k8s will then return the new set of IP addresses, with the old pod instance removed and the new instance added, and gRPC attempts to connect to the new address.

The gRPC's DnsResolver also has the option to configure an explicit refresh interval. This is mostly useful in situations where k8s scales up. Periodically checking DNS lets the client notice that there are new endpoints available.

Does DnsDestionationResolver need some kind of health check integration to trigger a refresh?

We could integrate YARP's health checks here, potentially to reload the IProxyConfig which the faulting destination belongs to. With DNS, we have no notification to tell us to perform a reload, but a Kubernetes-native solution could subscribe to those events by watching the relevant k8s Service resource for changes.

Tratcher · 2023-08-14T21:59:32Z

test/ReverseProxy.Tests/Transforms/Builder/TransformBuilderTests.cs

@@ -240,22 +242,31 @@ public void DefaultsCanBeDisabled()
        var results = transformBuilder.BuildInternal(route, new ClusterConfig());
        Assert.NotNull(results);
        Assert.False(results.ShouldCopyRequestHeaders);
-        Assert.Empty(results.RequestTransforms);
+        Assert.Single(results.RequestTransforms);


It's unfortunate that this can't be turned off completely like before. I don't want us to end up with default behavior that the user can't override. They can still replace or remove the host themselves, right?

I agree that this is somewhat unfortunate. The developer can still replace or remove the host. I had a version of this which moved the behavior to the front of the transform pipeline inside StructuredTransformer itself, but after speaking with Miha about it, I moved it to the existing transform at the end of the pipeline instead.

Co-authored-by: Chris Ross <Tratcher@Outlook.com>

docs/docfx/articles/destination-resolvers.md

adityamandaleeka · 2023-08-16T22:58:52Z

Woohoo, looks like this is green now. Ready to merge?

ReubenBond force-pushed the feature/destination-resolver branch from bed91e8 to 3b6e552 Compare August 1, 2023 18:27

Add IDestinationResolver for resolving cluster destination addresses

80648df

ReubenBond force-pushed the feature/destination-resolver branch from 19269d9 to 80648df Compare August 7, 2023 18:06

ReubenBond marked this pull request as ready for review August 7, 2023 18:08

ReubenBond requested review from Tratcher and MihaZupan as code owners August 7, 2023 18:08

Kahbazi reviewed Aug 7, 2023

View reviewed changes

src/ReverseProxy/Management/ProxyConfigManager.cs Outdated Show resolved Hide resolved

Update src/ReverseProxy/Management/ProxyConfigManager.cs

9a627a7

Co-authored-by: Arvin Kahbazi <akahbazi@gmail.com>

MihaZupan approved these changes Aug 9, 2023

View reviewed changes

ReubenBond and others added 5 commits August 9, 2023 11:00

Update src/ReverseProxy/Health/DefaultProbingRequestFactory.cs

9356fe5

Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>

Update src/ReverseProxy/Configuration/DestinationConfig.cs

979ad61

Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>

Update src/ReverseProxy/Management/ProxyConfigManager.cs

e78e7f5

Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>

Review feedback

f027472

Simplify destination host propagation condition

45cabf3

ReubenBond force-pushed the feature/destination-resolver branch from bdb88e8 to 45cabf3 Compare August 9, 2023 18:19

Add short doc on destination resolvers

b396935

davidfowl reviewed Aug 11, 2023

View reviewed changes

src/ReverseProxy/Configuration/NoOpDestinationResolver.cs Outdated Show resolved Hide resolved

ReubenBond added 3 commits August 14, 2023 11:16

Add DnsDestinationResolver and refactor

008639c

Add AddDnsDestinationResolver

d954184

Add DnsDestinationResolver test

8896bb1

Tratcher reviewed Aug 14, 2023

View reviewed changes

ReubenBond and others added 6 commits August 15, 2023 07:31

Update docs/docfx/articles/destination-resolvers.md

584e5b6

Co-authored-by: Chris Ross <Tratcher@Outlook.com>

Review feedback

2d983dd

Add doc for DnsDestinationResolver

6b11653

Tweak doc for DnsDestinationResolver

5026f86

Fix test

9c7f879

Workaround docfx bug with primary ctor

27a7be5

Tratcher reviewed Aug 16, 2023

View reviewed changes

docs/docfx/articles/destination-resolvers.md Outdated Show resolved Hide resolved

Tratcher approved these changes Aug 16, 2023

View reviewed changes

ReubenBond added 2 commits August 16, 2023 11:34

Review feedback

d6c0351

Remove CreateUriBuilder

3f1bc4a

ReubenBond merged commit 2fdb425 into microsoft:main Aug 17, 2023
6 checks passed

ReubenBond deleted the feature/destination-resolver branch August 17, 2023 16:21

Tratcher added this to the YARP 2.x milestone Aug 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `IDestinationResolver` for resolving cluster destination addresses #2210

Add `IDestinationResolver` for resolving cluster destination addresses #2210

ReubenBond commented Aug 1, 2023 •

edited

Loading

ReubenBond commented Aug 1, 2023

ReubenBond commented Aug 1, 2023

ReubenBond commented Aug 7, 2023

ReubenBond commented Aug 8, 2023

MihaZupan left a comment

ReubenBond commented Aug 9, 2023

ReubenBond commented Aug 9, 2023

ReubenBond commented Aug 14, 2023

ReubenBond commented Aug 14, 2023

ReubenBond commented Aug 14, 2023 •

edited

Loading

ReubenBond commented Aug 14, 2023

Tratcher Aug 14, 2023

ReubenBond Aug 15, 2023

Tratcher Aug 16, 2023

Tratcher Aug 14, 2023

ReubenBond Aug 15, 2023

Tratcher Aug 16, 2023

ReubenBond Aug 16, 2023

Tratcher Aug 16, 2023

Tratcher Aug 14, 2023

ReubenBond Aug 14, 2023

Tratcher Aug 16, 2023

ReubenBond Aug 16, 2023 •

edited

Loading

Tratcher Aug 14, 2023

davidfowl Aug 14, 2023

JamesNK Aug 16, 2023 •

edited

Loading

Tratcher Aug 16, 2023

ReubenBond Aug 16, 2023

Tratcher Aug 14, 2023

ReubenBond Aug 14, 2023 •

edited

Loading

adityamandaleeka commented Aug 16, 2023

Add IDestinationResolver for resolving cluster destination addresses #2210

Add IDestinationResolver for resolving cluster destination addresses #2210

Conversation

ReubenBond commented Aug 1, 2023 • edited Loading

ReubenBond commented Aug 1, 2023

ReubenBond commented Aug 1, 2023

ReubenBond commented Aug 7, 2023

ReubenBond commented Aug 8, 2023

MihaZupan left a comment

Choose a reason for hiding this comment

ReubenBond commented Aug 9, 2023

ReubenBond commented Aug 9, 2023

ReubenBond commented Aug 14, 2023

ReubenBond commented Aug 14, 2023

ReubenBond commented Aug 14, 2023 • edited Loading

ReubenBond commented Aug 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ReubenBond Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesNK Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ReubenBond Aug 14, 2023 • edited Loading

Choose a reason for hiding this comment

adityamandaleeka commented Aug 16, 2023

Add `IDestinationResolver` for resolving cluster destination addresses #2210

Add `IDestinationResolver` for resolving cluster destination addresses #2210

ReubenBond commented Aug 1, 2023 •

edited

Loading

ReubenBond commented Aug 14, 2023 •

edited

Loading

ReubenBond Aug 16, 2023 •

edited

Loading

JamesNK Aug 16, 2023 •

edited

Loading

ReubenBond Aug 14, 2023 •

edited

Loading