Skip to content
This repository has been archived by the owner on Dec 18, 2018. It is now read-only.

Honor PreferHostingUrls #1639

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/Microsoft.AspNetCore.Server.Kestrel.Core/KestrelServer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ public void Start<TContext>(IHttpApplication<TContext> application)
var hasListenOptions = listenOptions.Any();
var hasServerAddresses = _serverAddresses.Addresses.Any();

if (hasListenOptions && hasServerAddresses)
if (hasListenOptions && hasServerAddresses && !_serverAddresses.PreferHostingUrls)
{
var joined = string.Join(", ", _serverAddresses.Addresses);
_logger.LogWarning($"Overriding address(es) '{joined}'. Binding to endpoints defined in UseKestrel() instead.");
Expand All @@ -131,8 +131,15 @@ public void Start<TContext>(IHttpApplication<TContext> application)

return;
}
else if (!hasListenOptions)
else if (!hasListenOptions || (_serverAddresses.PreferHostingUrls && hasServerAddresses))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big fan of how these if else are structured since it's somewhat convoluted. As far as I can tell, it's to reduce code duplication for translating server addresses to listen options and binding to the listen options.

I have an alternative approach added to the branch at https://github.com/aspnet/KestrelHttpServer/compare/johluo/use-hosting-urls?diff=split&expand=1&name=johluo%2Fuse-hosting-urls but there's a bit of moving things around so I'm not sure if it's worth the effort.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the alternative approach; it's easier to follow the logic in that version.

{
if (hasListenOptions)
{
var joined = string.Join(", ", _serverAddresses.Addresses);
_logger.LogWarning($"Overriding endpoints defined in UseKestrel() since {nameof(IServerAddressesFeature.PreferHostingUrls)} is set to true. Binding to address(es) '{joined}' instead.");
listenOptions.Clear();
}

// If no endpoints are configured directly using KestrelServerOptions, use those configured via the IServerAddressesFeature.
var copiedAddresses = _serverAddresses.Addresses.ToArray();
_serverAddresses.Addresses.Clear();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,10 +153,7 @@ private async Task RegisterDefaultServerAddresses_Success(IEnumerable<string> ad

var hostBuilder = new WebHostBuilder()
.UseKestrel()
.ConfigureServices(services =>
{
services.AddSingleton<ILoggerFactory>(new KestrelTestLoggerFactory(testLogger));
})
.UseLoggerFactory(_ => new KestrelTestLoggerFactory(testLogger))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use the overload that takes an instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were thinking of obsoleting that overload. aspnet/Hosting#1007. We want to use the overloads that also take in the WebHostBuilderContext.

.Configure(ConfigureEchoAddress);

using (var host = hostBuilder.Build())
Expand Down Expand Up @@ -218,6 +215,71 @@ public void ThrowsWhenBindingToIPv6AddressInUse()
}
}

[ConditionalFact]
[PortSupportedCondition(5002)]
public async Task OverrideDirectConfigurationWithIServerAddressesFeature_Succeeds()
{
var overrideAddress = "http://localhost:5002";
var testLogger = new TestApplicationErrorLogger();
var hostBuilder = new WebHostBuilder()
.UseKestrel(options => options.Listen(IPAddress.Loopback, 5001))
.UseUrls(overrideAddress)
.PreferHostingUrls(true)
.UseLoggerFactory(_ => new KestrelTestLoggerFactory(testLogger))
.Configure(ConfigureEchoAddress);

using (var host = hostBuilder.Build())
{
host.Start();

Assert.Equal(5002, host.GetPort());
Assert.Single(testLogger.Messages, log => log.LogLevel == LogLevel.Warning &&
string.Equals($"Overriding endpoints defined in UseKestrel() since {nameof(IServerAddressesFeature.PreferHostingUrls)} is set to true. Binding to address(es) '{overrideAddress}' instead.",
log.Message, StringComparison.Ordinal));

Assert.Equal(new Uri(overrideAddress).ToString(), await HttpClientSlim.GetStringAsync(overrideAddress));
}
}

[ConditionalFact]
[PortSupportedCondition(5001)]
public async Task DoesNotOverrideDirectConfigurationWithIServerAddressesFeature_IfPreferHostingUrlsFalse()
{
var endPointAddress = "http://localhost:5001";
var hostBuilder = new WebHostBuilder()
.UseKestrel(options => options.Listen(IPAddress.Loopback, 5001))
.UseUrls("http://localhost:5002")
.PreferHostingUrls(false)
.Configure(ConfigureEchoAddress);

using (var host = hostBuilder.Build())
{
host.Start();

Assert.Equal(5001, host.GetPort());
Assert.Equal(new Uri(endPointAddress).ToString(), await HttpClientSlim.GetStringAsync(endPointAddress));
}
}

[ConditionalFact]
[PortSupportedCondition(5000)]
public async Task DoesNotOverrideDirectConfigurationWithIServerAddressesFeature_IfAddressesEmpty()
{
var endPointAddress = "http://localhost:5001";
var hostBuilder = new WebHostBuilder()
.UseKestrel(options => options.Listen(IPAddress.Loopback, 5001))
.PreferHostingUrls(true)
.Configure(ConfigureEchoAddress);

using (var host = hostBuilder.Build())
{
host.Start();

Assert.Equal(5001, host.GetPort());
Assert.Equal(new Uri(endPointAddress).ToString(), await HttpClientSlim.GetStringAsync(endPointAddress));
}
}

[Fact]
public void ThrowsWhenBindingLocalhostToIPv4AddressInUse()
{
Expand Down