Skip to content

Commit 5a427a6

Browse files
bart-vmwarebart-vmware
committed
Review feedback: Use options pattern for DnsResolverOptions
1 parent 8625557 commit 5a427a6

File tree

9 files changed

+202
-55
lines changed

9 files changed

+202
-55
lines changed

‎src/Libraries/Microsoft.Extensions.ServiceDiscovery.Dns/DnsResolverOptionsValidator.cs‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using System;
5+
using Microsoft.Extensions.Options;
6+
7+
namespace Microsoft.Extensions.ServiceDiscovery.Dns;
8+
9+
internal sealed class DnsResolverOptionsValidator : IValidateOptions<DnsResolverOptions>
10+
{
11+
// CancellationTokenSource.CancelAfter has a maximum timeout of Int32.MaxValue milliseconds.
12+
private static readonly TimeSpan s_maxTimeout = TimeSpan.FromMilliseconds(int.MaxValue);
13+
14+
public ValidateOptionsResult Validate(string? name, DnsResolverOptions options)
15+
{
16+
if (options.Servers is null)
17+
{
18+
return ValidateOptionsResult.Fail($"{nameof(options.Servers)} must not be null.");
19+
}
20+
21+
if (options.MaxAttempts < 1)
22+
{
23+
return ValidateOptionsResult.Fail($"{nameof(options.MaxAttempts)} must be one or greater.");
24+
}
25+
26+
if (options.Timeout != Timeout.InfiniteTimeSpan)
27+
{
28+
if (options.Timeout <= TimeSpan.Zero)
29+
{
30+
return ValidateOptionsResult.Fail($"{nameof(options.Timeout)} must not be negative or zero.");
31+
}
32+
33+
if (options.Timeout > s_maxTimeout)
34+
{
35+
return ValidateOptionsResult.Fail($"{nameof(options.Timeout)} must not be greater than {s_maxTimeout.TotalMilliseconds} milliseconds.");
36+
}
37+
}
38+
39+
return ValidateOptionsResult.Success;
40+
}
41+
}

‎src/Libraries/Microsoft.Extensions.ServiceDiscovery.Dns/DnsSrvServiceEndpointProviderOptions.cs‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,6 @@ public class DnsSrvServiceEndpointProviderOptions
2828
/// </summary>
2929
public double RetryBackOffFactor { get; set; } = 2;
3030

31-
/// <summary>
32-
/// Gets or sets the options used to configure the underlying DNS resolution behavior.
33-
/// </summary>
34-
public DnsResolverOptions DnsResolverOptions { get; set; } = new();
35-
3631
/// <summary>
3732
/// Gets or sets the default DNS query suffix for services resolved via this provider.
3833
/// </summary>

‎src/Libraries/Microsoft.Extensions.ServiceDiscovery.Dns/Resolver/DnsResolver.cs‎

Lines changed: 3 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99
using System.Net.Sockets;
1010
using System.Runtime.InteropServices;
1111
using System.Security.Cryptography;
12-
using Microsoft.Extensions.DependencyInjection;
1312
using Microsoft.Extensions.Logging;
1413
using Microsoft.Extensions.Logging.Abstractions;
1514
using Microsoft.Extensions.Options;
@@ -21,20 +20,17 @@ internal sealed partial class DnsResolver : IDnsResolver, IDisposable
2120
private const int IPv4Length = 4;
2221
private const int IPv6Length = 16;
2322

24-
// CancellationTokenSource.CancelAfter has a maximum timeout of Int32.MaxValue milliseconds.
25-
private static readonly TimeSpan s_maxTimeout = TimeSpan.FromMilliseconds(int.MaxValue);
26-
2723
private bool _disposed;
2824
private readonly DnsResolverOptions _options;
2925
private readonly CancellationTokenSource _pendingRequestsCts = new();
3026
private readonly TimeProvider _timeProvider;
3127
private readonly ILogger<DnsResolver> _logger;
3228

33-
public DnsResolver(TimeProvider timeProvider, ILogger<DnsResolver> logger, DnsResolverOptions options)
29+
public DnsResolver(TimeProvider timeProvider, ILogger<DnsResolver> logger, IOptions<DnsResolverOptions> options)
3430
{
3531
_timeProvider = timeProvider;
3632
_logger = logger;
37-
_options = options;
33+
_options = options.Value;
3834

3935
if (_options.Servers.Count == 0)
4036
{
@@ -50,15 +46,9 @@ public DnsResolver(TimeProvider timeProvider, ILogger<DnsResolver> logger, DnsRe
5046
throw new ArgumentException("At least one DNS server is required.", nameof(options));
5147
}
5248
}
53-
54-
if (options.Timeout != Timeout.InfiniteTimeSpan)
55-
{
56-
ArgumentOutOfRangeException.ThrowIfLessThanOrEqual(options.Timeout, TimeSpan.Zero);
57-
ArgumentOutOfRangeException.ThrowIfGreaterThan(options.Timeout, s_maxTimeout);
58-
}
5949
}
6050

61-
internal DnsResolver(DnsResolverOptions options) : this(TimeProvider.System, NullLogger<DnsResolver>.Instance, options)
51+
internal DnsResolver(DnsResolverOptions options) : this(TimeProvider.System, NullLogger<DnsResolver>.Instance, new OptionsWrapper<DnsResolverOptions>(options))
6252
{
6353
}
6454

@@ -70,16 +60,6 @@ internal DnsResolver(IPEndPoint server) : this(new DnsResolverOptions { Servers
7060
{
7161
}
7262

73-
internal static DnsResolver Create(IServiceProvider serviceProvider)
74-
{
75-
ArgumentNullException.ThrowIfNull(serviceProvider);
76-
77-
var resolverOptions = serviceProvider.GetRequiredService<IOptions<DnsSrvServiceEndpointProviderOptions>>().Value.DnsResolverOptions;
78-
var timeProvider = serviceProvider.GetRequiredService<TimeProvider>();
79-
var logger = serviceProvider.GetRequiredService<ILoggerFactory>().CreateLogger<DnsResolver>();
80-
return new DnsResolver(timeProvider, logger, resolverOptions);
81-
}
82-
8363
public ValueTask<ServiceResult[]> ResolveServiceAsync(string name, CancellationToken cancellationToken = default)
8464
{
8565
ObjectDisposedException.ThrowIf(_disposed, this);

‎src/Libraries/Microsoft.Extensions.ServiceDiscovery.Dns/ServiceDiscoveryDnsServiceCollectionExtensions.cs‎

Lines changed: 50 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ public static IServiceCollection AddDnsSrvServiceEndpointProvider(this IServiceC
5050

5151
if (!GetDnsClientFallbackFlag())
5252
{
53-
services.TryAddSingleton<IDnsResolver>(DnsResolver.Create);
53+
services.TryAddSingleton<IDnsResolver, DnsResolver>();
5454
}
5555
else
5656
{
@@ -60,24 +60,10 @@ public static IServiceCollection AddDnsSrvServiceEndpointProvider(this IServiceC
6060

6161
services.AddSingleton<IServiceEndpointProviderFactory, DnsSrvServiceEndpointProviderFactory>();
6262
var options = services.AddOptions<DnsSrvServiceEndpointProviderOptions>();
63-
options.Configure(o => configureOptions?.Invoke(o));
63+
options.Configure(o => configureOptions.Invoke(o));
64+
6465
return services;
6566

66-
static bool GetDnsClientFallbackFlag()
67-
{
68-
if (AppContext.TryGetSwitch("Microsoft.Extensions.ServiceDiscovery.Dns.UseDnsClientFallback", out var value))
69-
{
70-
return value;
71-
}
72-
73-
var envVar = Environment.GetEnvironmentVariable("MICROSOFT_EXTENSIONS_SERVICE_DISCOVERY_DNS_USE_DNSCLIENT_FALLBACK");
74-
if (envVar is not null && (envVar.Equals("true", StringComparison.OrdinalIgnoreCase) || envVar.Equals("1")))
75-
{
76-
return true;
77-
}
78-
79-
return false;
80-
}
8167
}
8268

8369
/// <summary>
@@ -110,9 +96,55 @@ public static IServiceCollection AddDnsServiceEndpointProvider(this IServiceColl
11096
ArgumentNullException.ThrowIfNull(configureOptions);
11197

11298
services.AddServiceDiscoveryCore();
99+
100+
if (!GetDnsClientFallbackFlag())
101+
{
102+
services.TryAddSingleton<IDnsResolver, DnsResolver>();
103+
}
104+
else
105+
{
106+
services.TryAddSingleton<IDnsResolver, FallbackDnsResolver>();
107+
services.TryAddSingleton<DnsClient.LookupClient>();
108+
}
109+
113110
services.AddSingleton<IServiceEndpointProviderFactory, DnsServiceEndpointProviderFactory>();
114111
var options = services.AddOptions<DnsServiceEndpointProviderOptions>();
115-
options.Configure(o => configureOptions?.Invoke(o));
112+
options.Configure(o => configureOptions.Invoke(o));
113+
116114
return services;
117115
}
116+
117+
/// <summary>
118+
/// Configures the DNS resolver used for service discovery.
119+
/// </summary>
120+
/// <param name="services">The service collection.</param>
121+
/// <param name="configureOptions">The DNS resolver options.</param>
122+
/// <returns>The provided <see cref="IServiceCollection"/>.</returns>
123+
public static IServiceCollection ConfigureDnsResolver(this IServiceCollection services, Action<DnsResolverOptions> configureOptions)
124+
{
125+
ArgumentNullException.ThrowIfNull(services);
126+
ArgumentNullException.ThrowIfNull(configureOptions);
127+
128+
var options = services.AddOptions<DnsResolverOptions>();
129+
options.Configure(configureOptions);
130+
services.AddTransient<IValidateOptions<DnsResolverOptions>, DnsResolverOptionsValidator>();
131+
return services;
132+
}
133+
134+
private static bool GetDnsClientFallbackFlag()
135+
{
136+
if (AppContext.TryGetSwitch("Microsoft.Extensions.ServiceDiscovery.Dns.UseDnsClientFallback", out var value))
137+
{
138+
return value;
139+
}
140+
141+
var envVar = Environment.GetEnvironmentVariable("MICROSOFT_EXTENSIONS_SERVICE_DISCOVERY_DNS_USE_DNSCLIENT_FALLBACK");
142+
if (envVar is not null && (envVar.Equals("true", StringComparison.OrdinalIgnoreCase) || envVar.Equals("1")))
143+
{
144+
return true;
145+
}
146+
147+
return false;
148+
}
149+
118150
}

‎test/Libraries/Microsoft.Extensions.ServiceDiscovery.Dns.Tests/DnsServiceEndpointResolverTests.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ public async Task ResolveServiceEndpoint_Dns_MultiShot()
1717
var timeProvider = new FakeTimeProvider();
1818
var services = new ServiceCollection()
1919
.AddSingleton<TimeProvider>(timeProvider)
20-
.AddSingleton<IDnsResolver>(DnsResolver.Create)
20+
.AddSingleton<IDnsResolver, DnsResolver>()
2121
.AddServiceDiscoveryCore()
2222
.AddDnsServiceEndpointProvider(o => o.DefaultRefreshPeriod = TimeSpan.FromSeconds(30))
2323
.BuildServiceProvider();

‎test/Libraries/Microsoft.Extensions.ServiceDiscovery.Dns.Tests/DnsServicePublicApiTests.cs‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,12 +68,23 @@ public void AddDnsServiceEndpointProviderWithConfigureOptionsShouldThrowWhenServ
6868
}
6969

7070
[Fact]
71-
public void AddDnsServiceEndpointProviderWithConfigureOptionsShouldThrowWhenConfigureOptionsIsNull()
71+
public void ConfigureDnsResolverShouldThrowWhenServicesIsNull()
72+
{
73+
IServiceCollection services = null!;
74+
75+
var action = () => services.ConfigureDnsResolver(_ => { });
76+
77+
var exception = Assert.Throws<ArgumentNullException>(action);
78+
Assert.Equal(nameof(services), exception.ParamName);
79+
}
80+
81+
[Fact]
82+
public void ConfigureDnsResolverShouldThrowWhenConfigureOptionsIsNull()
7283
{
7384
IServiceCollection services = new ServiceCollection();
74-
Action<DnsServiceEndpointProviderOptions> configureOptions = null!;
85+
Action<DnsResolverOptions> configureOptions = null!;
7586

76-
var action = () => services.AddDnsServiceEndpointProvider(configureOptions);
87+
var action = () => services.ConfigureDnsResolver(configureOptions);
7788

7889
var exception = Assert.Throws<ArgumentNullException>(action);
7990
Assert.Equal(nameof(configureOptions), exception.ParamName);

‎test/Libraries/Microsoft.Extensions.ServiceDiscovery.Dns.Tests/Resolver/LoopbackDnsTestBase.cs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55
using System.Text;
66
using Microsoft.Extensions.DependencyInjection;
77
using Microsoft.Extensions.Logging;
8+
using Microsoft.Extensions.Logging.Abstractions;
9+
using Microsoft.Extensions.Options;
810
using Microsoft.Extensions.ServiceDiscovery.Dns.Tests;
911
using Microsoft.Extensions.Time.Testing;
1012
using Xunit.Abstractions;
@@ -40,8 +42,7 @@ DnsResolver InitializeResolver()
4042
ServiceCollection services = new();
4143
services.AddXunitLogging(Output);
4244

43-
// construct DnsResolver manually via internal constructor which accepts DnsResolverOptions
44-
var resolver = new DnsResolver(TimeProvider, services.BuildServiceProvider().GetRequiredService<ILogger<DnsResolver>>(), Options);
45+
var resolver = new DnsResolver(TimeProvider, NullLogger<DnsResolver>.Instance, new OptionsWrapper<DnsResolverOptions>(Options));
4546
return resolver;
4647
}
4748

‎test/Libraries/Microsoft.Extensions.ServiceDiscovery.Dns.Tests/ServiceDiscoveryDnsServiceCollectionExtensionsTests.cs‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using System;
5+
using System.Collections.Generic;
6+
using System.Linq;
7+
using System.Text;
8+
using System.Threading.Tasks;
9+
using Microsoft.Extensions.DependencyInjection;
10+
using Microsoft.Extensions.Hosting;
11+
using Microsoft.Extensions.Options;
12+
13+
namespace Microsoft.Extensions.ServiceDiscovery.Dns.Tests;
14+
15+
public class ServiceDiscoveryDnsServiceCollectionExtensionsTests
16+
{
17+
[Fact]
18+
public void AddDnsServiceEndpointProviderShouldRegisterDependentServices()
19+
{
20+
var services = new ServiceCollection();
21+
services.AddDnsServiceEndpointProvider();
22+
23+
using var serviceProvider = services.BuildServiceProvider(true);
24+
_ = serviceProvider.GetServices<IServiceEndpointProviderFactory>();
25+
}
26+
27+
[Fact]
28+
public void AddDnsSrvServiceEndpointProviderShouldRegisterDependentServices()
29+
{
30+
var services = new ServiceCollection();
31+
services.AddDnsSrvServiceEndpointProvider();
32+
33+
using var serviceProvider = services.BuildServiceProvider(true);
34+
_ = serviceProvider.GetServices<IServiceEndpointProviderFactory>();
35+
}
36+
37+
[Fact]
38+
public void ConfigureDnsResolverShouldThrowWhenServersIsNull()
39+
{
40+
var services = new ServiceCollection();
41+
services.ConfigureDnsResolver(options => options.Servers = null!);
42+
43+
using var serviceProvider = services.BuildServiceProvider();
44+
var options = serviceProvider.GetRequiredService<IOptions<DnsResolverOptions>>();
45+
46+
var exception = Assert.Throws<OptionsValidationException>(() => options.Value);
47+
Assert.Equal("Servers must not be null.", exception.Message);
48+
}
49+
50+
[Fact]
51+
public void ConfigureDnsResolverShouldThrowWhenMaxAttemptsIsZero()
52+
{
53+
var services = new ServiceCollection();
54+
services.ConfigureDnsResolver(options => options.MaxAttempts = 0);
55+
56+
using var serviceProvider = services.BuildServiceProvider();
57+
var options = serviceProvider.GetRequiredService<IOptions<DnsResolverOptions>>();
58+
59+
var exception = Assert.Throws<OptionsValidationException>(() => options.Value);
60+
Assert.Equal("MaxAttempts must be one or greater.", exception.Message);
61+
}
62+
63+
[Fact]
64+
public void ConfigureDnsResolverShouldThrowWhenTimeoutIsZero()
65+
{
66+
var services = new ServiceCollection();
67+
services.ConfigureDnsResolver(options => options.Timeout = TimeSpan.Zero);
68+
69+
using var serviceProvider = services.BuildServiceProvider();
70+
var options = serviceProvider.GetRequiredService<IOptions<DnsResolverOptions>>();
71+
72+
var exception = Assert.Throws<OptionsValidationException>(() => options.Value);
73+
Assert.Equal("Timeout must not be negative or zero.", exception.Message);
74+
}
75+
76+
[Fact]
77+
public void ConfigureDnsResolverShouldThrowWhenTimeoutExceedsMaximum()
78+
{
79+
var services = new ServiceCollection();
80+
services.ConfigureDnsResolver(options => options.Timeout = TimeSpan.FromMilliseconds(1L + int.MaxValue));
81+
82+
using var serviceProvider = services.BuildServiceProvider();
83+
var options = serviceProvider.GetRequiredService<IOptions<DnsResolverOptions>>();
84+
85+
var exception = Assert.Throws<OptionsValidationException>(() => options.Value);
86+
Assert.Equal("Timeout must not be greater than 2147483647 milliseconds.", exception.Message);
87+
}
88+
}

‎test/Libraries/Microsoft.Extensions.ServiceDiscovery.Yarp.Tests/YarpServiceDiscoveryTests.cs‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,13 @@
33

44
using Microsoft.Extensions.DependencyInjection;
55
using Microsoft.Extensions.Hosting;
6-
using Xunit;
76
using Yarp.ReverseProxy.Configuration;
87
using System.Net;
98
using System.Net.Sockets;
109
using Microsoft.Extensions.ServiceDiscovery.Dns.Resolver;
1110
using Microsoft.Extensions.Configuration;
1211
using Microsoft.Extensions.Options;
13-
using Microsoft.Extensions.Logging.Abstractions;
12+
using Microsoft.Extensions.ServiceDiscovery.Dns;
1413

1514
namespace Microsoft.Extensions.ServiceDiscovery.Yarp.Tests;
1615

@@ -232,7 +231,7 @@ public async Task ServiceDiscoveryDestinationResolverTests_Configuration_Disallo
232231
[Fact]
233232
public async Task ServiceDiscoveryDestinationResolverTests_Dns()
234233
{
235-
DnsResolver resolver = new DnsResolver(TimeProvider.System, NullLogger<DnsResolver>.Instance, new DnsResolverOptions());
234+
DnsResolver resolver = new DnsResolver(new DnsResolverOptions());
236235

237236
await using var services = new ServiceCollection()
238237
.AddSingleton<IDnsResolver>(resolver)

0 commit comments

Comments
 (0)