Add a Configuration package and tests. #1878

Tratcher · 2017-06-01T16:04:52Z

#1875, aspnet/Hosting#1095
This moves config code from Microsoft.AspNetCore.dll down to Kestrel's own config package. A reaction PR will follow for MetaPackages.
The cross targeting is due to the src package.

davidfowl · 2017-06-01T16:06:30Z

src/Microsoft.AspNetCore.Server.Kestrel.Configuration/KestrelConfigurationExtensions.cs

+        /// <returns>
+        /// The Microsoft.AspNetCore.Hosting.IWebHostBuilder.
+        /// </returns>
+        public static IWebHostBuilder UseKestrelWithConfiguration(this IWebHostBuilder hostBuilder)


What is this and why do we need it?

Is there no way to takeover the default UseKestrel() which would make it match all of the other AddXyz()'s that also add default AddConfigureOptions?

Can we just add a reference to Microsoft.AspNetCore.Certificates.Configuration.Sources and ConfigurationBinder in Microsoft.AspNetCore.Server.Kestrel and do this inside of UseKestrel?

Not without pulling in the .Https dependency.

Lets just make the .Kestrel package the uber one and nuke this package.

davidfowl · 2017-06-01T16:07:23Z

src/Microsoft.AspNetCore.Server.Kestrel.Configuration/KestrelServerOptionsSetup.cs

+
+namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal
+{
+    internal class KestrelServerConfigureOptions : ConfigureDefaultOptions<KestrelServerOptions>


Make this public.

Do we want all of these ConfigureDefaultOptions public? Or just Kestrel's since its doing real work unlike the rest which are usually one liners that only call GetSection().Bind which are mostly private.

It should be pubternal yea.

Ok I'll file an issue to make them all pubternal post preview2 if I don't end up touching them again this week. We need to standardize the naming then.

Xyz.Infrastructure/Internal.ConfigureDefaultXyzOptions?

davidfowl · 2017-06-01T17:35:51Z

src/Microsoft.AspNetCore.Server.Kestrel/Microsoft.AspNetCore.Server.Kestrel.csproj

    <GenerateDocumentationFile>true</GenerateDocumentationFile>
    <PackageTags>aspnetcore;kestrel</PackageTags>
    <NoWarn>CS1591;$(NoWarn)</NoWarn>
    <EnableApiCheck>false</EnableApiCheck>
  </PropertyGroup>

  <ItemGroup>
+    <PackageReference Include="Microsoft.AspNetCore.Certificates.Configuration.Sources" Version="$(AspNetCoreVersion)" />


PrivateAssets=All

davidfowl · 2017-06-01T17:36:07Z

src/Microsoft.AspNetCore.Server.Kestrel/WebHostBuilderKestrelExtensions.cs

@@ -28,6 +29,7 @@ public static IWebHostBuilder UseKestrel(this IWebHostBuilder hostBuilder)
            return hostBuilder.ConfigureServices(services =>
            {
                services.AddTransient<IConfigureOptions<KestrelServerOptions>, KestrelServerOptionsSetup>();
+                services.AddTransient<IConfigureOptions<KestrelServerOptions>, KestrelServerConfigureOptions>();


This doesn't look right.

Tratcher · 2017-06-01T17:37:24Z

src/Microsoft.AspNetCore.Server.Kestrel/WebHostBuilderKestrelExtensions.cs

@@ -28,6 +29,7 @@ public static IWebHostBuilder UseKestrel(this IWebHostBuilder hostBuilder)
            return hostBuilder.ConfigureServices(services =>
            {
                services.AddTransient<IConfigureOptions<KestrelServerOptions>, KestrelServerOptionsSetup>();
+                services.AddTransient<IConfigureOptions<KestrelServerOptions>, KestrelServerConfigureOptions>();


Doing it this way temporarily to work around aspnet/DependencyInjection#500

@HaoK are all of the other sub systems doing this for preview2?

That works, the other alternative would be to register

services.AddTransient<IConfigureOptions<KestrelServerOptions>, ConfigureDefaults<KestrelServerConfigureOptions>()

Which would let us just remove this line once the DI is fixed.

For preview 2 I don't think it really matters which one we do. Its slightly easier to cleanup if we go with lining up all the defaults for now.

chatted a bit offline with @davidfowl lets use services.AddTransient<IConfigureOptions<KestrelServerOptions>, ConfigureDefaults<ConfigureDefaultKestrelServerOptions>() so when the DI issues is fixed, we only have to remove this line, and won't have to update the ConfigureDefaultOptions class

It's getting fixed: aspnet/DependencyInjection#530
That said, @halter73 says we can't rely on that because it doesn't guarantee the same ordering and KestrelServerConfigureOptions requires KestrelServerOptionsSetup to have run first.

cesarblum · 2017-06-01T18:58:16Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/KestrelServerConfigureOptions.cs

+
+            if (!IPAddress.TryParse(configAddress, out var address))
+            {
+                throw new InvalidOperationException($"Invalid IP address in configuration: {configAddress}");


User facing strings should go in the resx file.

cesarblum · 2017-06-01T18:59:51Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/KestrelServerConfigureOptions.cs

+                        }
+                        catch (KeyNotFoundException) when (certificateConfig.Value.Equals(DevelopmentSSLCertificateName, StringComparison.Ordinal) && _hostingEnvironment.IsDevelopment())
+                        {
+                            var storeLoader = new CertificateStoreLoader();


I know you're just moving the code, but it still bothers me that we're newing this up here. I think loading the default cert should be a method on CertificateLoader. Thoughts?

This really isn't the time to refactor it.

Yeah just file an issue for post preview 2 :)

cesarblum · 2017-06-01T19:00:15Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/KestrelServerConfigureOptions.cs

+                            if (certificate == null)
+                            {
+                                var logger = _loggerFactory.CreateLogger("Microsoft.AspNetCore.Server.Kestrel.KestrelServerConfigureOptions");
+                                logger.LogError("No HTTPS certificate was found for development. For information on configuring HTTPS see https://go.microsoft.com/fwlink/?linkid=848054.");


cesarblum · 2017-06-01T19:00:17Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/KestrelServerConfigureOptions.cs

+
+                        if (certificate == null)
+                        {
+                            throw new InvalidOperationException($"No certificate found for endpoint '{endPoint.Key}'.");


cesarblum · 2017-06-01T19:00:20Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/KestrelServerConfigureOptions.cs

+                    }
+                    catch (Exception ex)
+                    {
+                        throw new InvalidOperationException("Unable to configure HTTPS endpoint. For information on configuring HTTPS see https://go.microsoft.com/fwlink/?linkid=848054.", ex);


cesarblum · 2017-06-01T19:00:33Z

src/Microsoft.AspNetCore.Server.Kestrel/Microsoft.AspNetCore.Server.Kestrel.csproj

@@ -4,19 +4,22 @@

  <PropertyGroup>
    <Description>ASP.NET Core Kestrel cross-platform web server.</Description>
-    <TargetFramework>netstandard2.0</TargetFramework>
+    <TargetFrameworks>netstandard2.0;netcoreapp2.0</TargetFrameworks>


The cert loader sources package requires it

Where is the ifdef?

Here: https://github.com/aspnet/Hosting/pull/1095/files#diff-b877901ee14c476e81bad19896b94775R182

I see, what happens on .NET Framework?

It uses the netstandard version that doesn't have this new feature

Which does what?

Just move this to the Core project. Currently only the "meta" project only contains WebHostBuilderKestrelExtensions.

nvm. I just realized this depends on both the Core and Https projects.

halter73 · 2017-06-01T18:59:22Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/KestrelServerConfigureOptions.cs

+
+namespace Microsoft.AspNetCore.Server.Kestrel.Internal
+{
+    public class KestrelServerConfigureOptions : IConfigureOptions<KestrelServerOptions>


This is now the only class in the meta project other than WebHostBuilderKestrelExtensions. Why not just put this in Core/Internal alongside KestrelServerOptionsSetup? Or better yet, just combine this with KestrelServerOptionsSetup.

No, lets keep core clean. I don't want configuration leaking into the core APIs. The "meta package" is where all of the dependencies sit.

You can't combine them as one is a true default that you always want to apply, while the config binding one is a 'squishy' default, that we only want to apply by default via WebHostBuilder

It requires .Https

It wouldn't work anyway since this calls UseHttps() 😢

halter73 · 2017-06-01T19:06:02Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/ConfigurationTests.cs

+        private static int GetWebHostPort(IWebHost webHost)
+            => webHost.ServerFeatures.Get<IServerAddressesFeature>().Addresses
+                .Select(serverAddress => new Uri(serverAddress).Port)
+                .FirstOrDefault();


nit: Single()

halter73 · 2017-06-01T19:13:33Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/ConfigurationTests.cs

+using Microsoft.Extensions.DependencyInjection;
+using Xunit;
+
+namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests


Any tests verifying the behavior when something goes wrong like being given an invalid address value or a bad/missing cert? Would the host fail to start even if there are other addresses that are successfully configured?

It throws. I added a test.

davidfowl · 2017-06-02T06:24:57Z

src/Microsoft.AspNetCore.Server.Kestrel/Microsoft.AspNetCore.Server.Kestrel.csproj

    <ProjectReference Include="..\Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv\Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv.csproj" />
    <ProjectReference Include="..\Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets\Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets.csproj" />
  </ItemGroup>

+  <ItemGroup>


Tratcher · 2017-06-02T17:35:45Z

That should be everything, Please review.

Tratcher · 2017-06-02T19:40:48Z

rebased and merged.

#1875 Add a Configuration package and tests.

a98aa80

Tratcher added this to the 2.0.0-preview2 milestone Jun 1, 2017

Tratcher self-assigned this Jun 1, 2017

Tratcher requested review from davidfowl, cesarblum and HaoK June 1, 2017 16:04

dnfclas added the cla-already-signed label Jun 1, 2017

davidfowl reviewed Jun 1, 2017

View reviewed changes

Tratcher mentioned this pull request Jun 1, 2017

Kestrel config binder should understand * and localhost aspnet/MetaPackages#73

Closed

danroth27 mentioned this pull request Jun 1, 2017

Kestrel config binder should understand * and localhost #1879

Closed

Tratcher added 2 commits June 1, 2017 09:38

Work around ConfigureDefaultOptions bug. Fix file name.

885d3f1

Squash .Configuration back into .Kestrel

7008933

Tratcher mentioned this pull request Jun 1, 2017

Change Certificates.Configuration to a Sources package aspnet/MetaPackages#133

Merged

davidfowl reviewed Jun 1, 2017

View reviewed changes

Tratcher commented Jun 1, 2017

View reviewed changes

PrivateAssets

eda5453

cesarblum suggested changes Jun 1, 2017

View reviewed changes

halter73 suggested changes Jun 1, 2017

View reviewed changes

Tratcher added 4 commits June 1, 2017 13:23

Resx

0335de8

Tests

96327e3

More resx

969cc41

Change options registration

e274ce1

davidfowl reviewed Jun 2, 2017

View reviewed changes

Tratcher added 2 commits June 2, 2017 10:25

Fix DI

79da5e7

Remove extra resx build task

ee9a357

HaoK approved these changes Jun 2, 2017

View reviewed changes

halter73 approved these changes Jun 2, 2017

View reviewed changes

Tratcher closed this Jun 2, 2017

Tratcher deleted the tratcher/config branch June 2, 2017 19:40

halter73 mentioned this pull request Jul 31, 2017

Https configuration issues with Kestrel in aspnetcore 2.0 #1851

Closed

muratg mentioned this pull request Aug 18, 2017

Kestrel config binder should understand * and localhost aspnet/AspNetCoreModule#136

Closed

Add a Configuration package and tests. #1878

Add a Configuration package and tests. #1878

Uh oh!

Conversation

Tratcher commented Jun 1, 2017

Uh oh!

davidfowl Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HaoK Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HaoK Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

davidfowl Jun 1, 2017 •

edited

Loading

HaoK Jun 1, 2017 •

edited

Loading

HaoK Jun 1, 2017 •

edited

Loading

davidfowl Jun 1, 2017 •

edited

Loading