Merge pull request #518 from dotnet-maestro-bot/merge/release/2.2-to-master

[automated] Merge branch 'release/2.2' => 'master'

src/Microsoft.AspNetCore.Diagnostics.HealthChecks/Builder/HealthCheckApplicationBuilderExtensions.cs

@@ -21,8 +21,10 @@ namespace Microsoft.AspNetCore.Builder
         /// <returns>A reference to the <paramref name="app"/> after the operation has completed.</returns>
         /// <remarks>
         /// <para>
-        /// This method will use <see cref="MapExtensions.Map(IApplicationBuilder, PathString, Action{IApplicationBuilder})"/> to
-        /// listen to health checks requests on the specified URL path.
+        /// If <paramref name="path"/> is set to <c>null</c> or the empty string then the health check middleware
+        /// will ignore the URL path and process all requests. If <paramref name="path"/> is set to a non-empty
+        /// value, the health check middleware will process requests with a URL that matches the provided value
+        /// of <paramref name="path"/> case-insensitively, allowing for an extra trailing slash ('/') character.
         /// </para>
         /// <para>
         /// The health check middleware will use default settings from <see cref="IOptions{HealthCheckOptions}"/>.
@@ -48,8 +50,10 @@ namespace Microsoft.AspNetCore.Builder
         /// <returns>A reference to the <paramref name="app"/> after the operation has completed.</returns>
         /// <remarks>
         /// <para>
-        /// This method will use <see cref="MapExtensions.Map(IApplicationBuilder, PathString, Action{IApplicationBuilder})"/> to
-        /// listen to health checks requests on the specified URL path.
+        /// If <paramref name="path"/> is set to <c>null</c> or the empty string then the health check middleware
+        /// will ignore the URL path and process all requests. If <paramref name="path"/> is set to a non-empty
+        /// value, the health check middleware will process requests with a URL that matches the provided value
+        /// of <paramref name="path"/> case-insensitively, allowing for an extra trailing slash ('/') character.
         /// </para>
         /// </remarks>
         public static IApplicationBuilder UseHealthChecks(this IApplicationBuilder app, PathString path, HealthCheckOptions options)
@@ -77,8 +81,11 @@ namespace Microsoft.AspNetCore.Builder
         /// <returns>A reference to the <paramref name="app"/> after the operation has completed.</returns>
         /// <remarks>
         /// <para>
-        /// This method will use <see cref="MapWhenExtensions.MapWhen(IApplicationBuilder, Func{HttpContext, bool}, Action{IApplicationBuilder})"/> to
-        /// listen to health checks requests on the specified URL path and port.
+        /// If <paramref name="path"/> is set to <c>null</c> or the empty string then the health check middleware
+        /// will ignore the URL path and process all requests on the specified port. If <paramref name="path"/> is
+        /// set to a non-empty value, the health check middleware will process requests with a URL that matches the 
+        /// provided value of <paramref name="path"/> case-insensitively, allowing for an extra trailing slash ('/') 
+        /// character.
         /// </para>
         /// <para>
         /// The health check middleware will use default settings from <see cref="IOptions{HealthCheckOptions}"/>.
@@ -104,8 +111,11 @@ namespace Microsoft.AspNetCore.Builder
         /// <returns>A reference to the <paramref name="app"/> after the operation has completed.</returns>
         /// <remarks>
         /// <para>
-        /// This method will use <see cref="MapWhenExtensions.MapWhen(IApplicationBuilder, Func{HttpContext, bool}, Action{IApplicationBuilder})"/> to
-        /// listen to health checks requests on the specified URL path and port.
+        /// If <paramref name="path"/> is set to <c>null</c> or the empty string then the health check middleware
+        /// will ignore the URL path and process all requests on the specified port. If <paramref name="path"/> is
+        /// set to a non-empty value, the health check middleware will process requests with a URL that matches the 
+        /// provided value of <paramref name="path"/> case-insensitively, allowing for an extra trailing slash ('/') 
+        /// character.
         /// </para>
         /// <para>
         /// The health check middleware will use default settings from <see cref="IOptions{HealthCheckOptions}"/>.
@@ -142,8 +152,11 @@ namespace Microsoft.AspNetCore.Builder
         /// <returns>A reference to the <paramref name="app"/> after the operation has completed.</returns>
         /// <remarks>
         /// <para>
-        /// This method will use <see cref="MapExtensions.Map(IApplicationBuilder, PathString, Action{IApplicationBuilder})"/> to
-        /// listen to health checks requests on the specified URL path.
+        /// If <paramref name="path"/> is set to <c>null</c> or the empty string then the health check middleware
+        /// will ignore the URL path and process all requests on the specified port. If <paramref name="path"/> is
+        /// set to a non-empty value, the health check middleware will process requests with a URL that matches the 
+        /// provided value of <paramref name="path"/> case-insensitively, allowing for an extra trailing slash ('/') 
+        /// character.
         /// </para>
         /// </remarks>
         public static IApplicationBuilder UseHealthChecks(this IApplicationBuilder app, PathString path, int port, HealthCheckOptions options)
@@ -172,8 +185,11 @@ namespace Microsoft.AspNetCore.Builder
         /// <returns>A reference to the <paramref name="app"/> after the operation has completed.</returns>
         /// <remarks>
         /// <para>
-        /// This method will use <see cref="MapExtensions.Map(IApplicationBuilder, PathString, Action{IApplicationBuilder})"/> to
-        /// listen to health checks requests on the specified URL path.
+        /// If <paramref name="path"/> is set to <c>null</c> or the empty string then the health check middleware
+        /// will ignore the URL path and process all requests on the specified port. If <paramref name="path"/> is
+        /// set to a non-empty value, the health check middleware will process requests with a URL that matches the 
+        /// provided value of <paramref name="path"/> case-insensitively, allowing for an extra trailing slash ('/') 
+        /// character.
         /// </para>
         /// </remarks>
         public static IApplicationBuilder UseHealthChecks(this IApplicationBuilder app, PathString path, string port, HealthCheckOptions options)
@@ -204,16 +220,35 @@ namespace Microsoft.AspNetCore.Builder
 
         private static void UseHealthChecksCore(IApplicationBuilder app, PathString path, int? port, object[] args)
         {
-            if (port == null)
+            // NOTE: we explicitly don't use Map here because it's really common for multiple health
+            // check middleware to overlap in paths. Ex: `/health`, `/health/detailed` - this is order
+            // sensititive with Map, and it's really surprising to people.
+            //
+            // See:
+            // https://github.com/aspnet/Diagnostics/issues/511
+            // https://github.com/aspnet/Diagnostics/issues/512
+            // https://github.com/aspnet/Diagnostics/issues/514
+
+            Func<HttpContext, bool> predicate = c =>
             {
-                app.Map(path, b => b.UseMiddleware<HealthCheckMiddleware>(args));
-            }
-            else
-            {
-                app.MapWhen(
-                    c => c.Connection.LocalPort == port,
-                    b0 => b0.Map(path, b1 => b1.UseMiddleware<HealthCheckMiddleware>(args)));
-            }
+                return
+
+                    // Process the port if we have one
+                    (port == null || c.Connection.LocalPort == port) &&
+
+                    // We allow you to listen on all URLs by providing the empty PathString.
+                    (!path.HasValue ||
+
+                        // If you do provide a PathString, want to handle all of the special cases that 
+                        // StartsWithSegments handles, but we also want it to have exact match semantics.
+                        //
+                        // Ex: /Foo/ == /Foo (true)
+                        // Ex: /Foo/Bar == /Foo (false)
+                        (c.Request.Path.StartsWithSegments(path, out var remaining) &&
+                        string.IsNullOrEmpty(remaining)));
+            };
+
+            app.MapWhen(predicate, b => b.UseMiddleware<HealthCheckMiddleware>(args));
         }
     }
 }

test/Microsoft.AspNetCore.Diagnostics.HealthChecks.Tests/HealthCheckMiddlewareTests.cs

@@ -395,6 +395,131 @@ namespace Microsoft.AspNetCore.Diagnostics.HealthChecks
             Assert.Equal("Healthy", await response.Content.ReadAsStringAsync());
         }
 
+        [Fact]
+        public async Task CanListenWithPath_AcceptsRequestWithExtraSlash()
+        {
+            var builder = new WebHostBuilder()
+                .Configure(app =>
+                {
+                    app.UseHealthChecks("/health");
+                })
+                .ConfigureServices(services =>
+                {
+                    services.AddHealthChecks();
+                });
+
+            var server = new TestServer(builder);
+            var client = server.CreateClient();
+
+            var response = await client.GetAsync("http://localhost:5001/health/");
+
+            Assert.Equal(HttpStatusCode.NotFound, response.StatusCode);
+        }
+
+        [Fact]
+        public async Task CanListenWithPath_AcceptsRequestWithCaseInsensitiveMatch()
+        {
+            var builder = new WebHostBuilder()
+                .Configure(app =>
+                {
+                    app.UseHealthChecks("/health");
+                })
+                .ConfigureServices(services =>
+                {
+                    services.AddHealthChecks();
+                });
+
+            var server = new TestServer(builder);
+            var client = server.CreateClient();
+
+            var response = await client.GetAsync("http://localhost:5001/HEALTH");
+
+            Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+            Assert.Equal("text/plain", response.Content.Headers.ContentType.ToString());
+            Assert.Equal("Healthy", await response.Content.ReadAsStringAsync());
+        }
+
+        [Fact]
+        public async Task CanListenWithPath_RejectsRequestWithExtraSegments()
+        {
+            var builder = new WebHostBuilder()
+                .Configure(app =>
+                {
+                    app.UseHealthChecks("/health");
+                })
+                .ConfigureServices(services =>
+                {
+                    services.AddHealthChecks();
+                });
+
+            var server = new TestServer(builder);
+            var client = server.CreateClient();
+
+            var response = await client.GetAsync("http://localhost:5001/health/detailed");
+
+            Assert.Equal(HttpStatusCode.NotFound, response.StatusCode);
+        }
+
+        // See: https://github.com/aspnet/Diagnostics/issues/511
+        [Fact]
+        public async Task CanListenWithPath_MultipleMiddleware_LeastSpecificFirst()
+        {
+            var builder = new WebHostBuilder()
+                .Configure(app =>
+                {
+                    // Throws if used
+                    app.UseHealthChecks("/health", new HealthCheckOptions()
+                    {
+                        ResponseWriter = (c, r) => throw null,
+                    });
+
+                    app.UseHealthChecks("/health/detailed");
+                })
+                .ConfigureServices(services =>
+                {
+                    services.AddHealthChecks();
+                });
+
+            var server = new TestServer(builder);
+            var client = server.CreateClient();
+
+            var response = await client.GetAsync("http://localhost:5001/health/detailed");
+
+            Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+            Assert.Equal("text/plain", response.Content.Headers.ContentType.ToString());
+            Assert.Equal("Healthy", await response.Content.ReadAsStringAsync());
+        }
+
+        // See: https://github.com/aspnet/Diagnostics/issues/511
+        [Fact]
+        public async Task CanListenWithPath_MultipleMiddleware_MostSpecificFirst()
+        {
+            var builder = new WebHostBuilder()
+                .Configure(app =>
+                {
+                    app.UseHealthChecks("/health/detailed");
+
+                    // Throws if used
+                    app.UseHealthChecks("/health", new HealthCheckOptions()
+                    {
+                        ResponseWriter = (c, r) => throw null,
+                    });
+                })
+                .ConfigureServices(services =>
+                {
+                    services.AddHealthChecks();
+                });
+
+            var server = new TestServer(builder);
+            var client = server.CreateClient();
+
+            var response = await client.GetAsync("http://localhost:5001/health/detailed");
+
+            Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+            Assert.Equal("text/plain", response.Content.Headers.ContentType.ToString());
+            Assert.Equal("Healthy", await response.Content.ReadAsStringAsync());
+        }
+
         [Fact]
         public async Task CanListenOnPort_AcceptsRequest_OnSpecifiedPort()
         {
@@ -486,6 +611,78 @@ namespace Microsoft.AspNetCore.Diagnostics.HealthChecks
             Assert.Equal(HttpStatusCode.NotFound, response.StatusCode);
         }
 
+        [Fact]
+        public async Task CanListenOnPort_MultipleMiddleware()
+        {
+            var builder = new WebHostBuilder()
+                .Configure(app =>
+                {
+                    app.Use(next => async (context) =>
+                    {
+                        // Need to fake setting the connection info. TestServer doesn't
+                        // do that, because it doesn't have a connection.
+                        context.Connection.LocalPort = context.Request.Host.Port.Value;
+                        await next(context);
+                    });
 
+                    // Throws if used
+                    app.UseHealthChecks("/health", port: 5001, new HealthCheckOptions()
+                    {
+                        ResponseWriter = (c, r) => throw null,
+                    });
+
+                    app.UseHealthChecks("/health/detailed", port: 5001);
+                })
+                .ConfigureServices(services =>
+                {
+                    services.AddHealthChecks();
+                });
+
+            var server = new TestServer(builder);
+            var client = server.CreateClient();
+
+            var response = await client.GetAsync("http://localhost:5001/health/detailed");
+
+            Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+            Assert.Equal("text/plain", response.Content.Headers.ContentType.ToString());
+            Assert.Equal("Healthy", await response.Content.ReadAsStringAsync());
+        }
+
+        [Fact]
+        public async Task CanListenOnPort_MultipleMiddleware_DifferentPorts()
+        {
+            var builder = new WebHostBuilder()
+                .Configure(app =>
+                {
+                    app.Use(next => async (context) =>
+                    {
+                        // Need to fake setting the connection info. TestServer doesn't
+                        // do that, because it doesn't have a connection.
+                        context.Connection.LocalPort = context.Request.Host.Port.Value;
+                        await next(context);
+                    });
+
+                    // Throws if used
+                    app.UseHealthChecks("/health", port: 5002, new HealthCheckOptions()
+                    {
+                        ResponseWriter = (c, r) => throw null,
+                    });
+
+                    app.UseHealthChecks("/health", port: 5001);
+                })
+                .ConfigureServices(services =>
+                {
+                    services.AddHealthChecks();
+                });
+
+            var server = new TestServer(builder);
+            var client = server.CreateClient();
+
+            var response = await client.GetAsync("http://localhost:5001/health");
+
+            Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+            Assert.Equal("text/plain", response.Content.Headers.ContentType.ToString());
+            Assert.Equal("Healthy", await response.Content.ReadAsStringAsync());
+        }
     }
 }