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

Commit c8c99a8

Browse files
rynowakrynowak
committed
Don't use Map
Fixes #511 and #514 It's really confusing to people that we use Map. Users expect that the URL they provide for the health check middleware will only process exact matches. The way it behaves when using map is not optimal for some of the common patterns.
1 parent b3db95e commit c8c99a8

File tree

2 files changed

+253
-21
lines changed

2 files changed

+253
-21
lines changed

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

Lines changed: 56 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,10 @@ public static class HealthCheckApplicationBuilderExtensions
2121
/// <returns>A reference to the <paramref name="app"/> after the operation has completed.</returns>
2222
/// <remarks>
2323
/// <para>
24-
/// This method will use <see cref="MapExtensions.Map(IApplicationBuilder, PathString, Action{IApplicationBuilder})"/> to
25-
/// listen to health checks requests on the specified URL path.
24+
/// If <paramref name="path"/> is set to <c>null</c> or the empty string then the health check middleware
25+
/// will ignore the URL path and process all requests. If <paramref name="path"/> is set to a non-empty
26+
/// value, the health check middleware will process requests with a URL that matches the provided value
27+
/// of <paramref name="path"/> case-insensitively, allowing for an extra trailing slash ('/') character.
2628
/// </para>
2729
/// <para>
2830
/// The health check middleware will use default settings from <see cref="IOptions{HealthCheckOptions}"/>.
@@ -48,8 +50,10 @@ public static IApplicationBuilder UseHealthChecks(this IApplicationBuilder app,
4850
/// <returns>A reference to the <paramref name="app"/> after the operation has completed.</returns>
4951
/// <remarks>
5052
/// <para>
51-
/// This method will use <see cref="MapExtensions.Map(IApplicationBuilder, PathString, Action{IApplicationBuilder})"/> to
52-
/// listen to health checks requests on the specified URL path.
53+
/// If <paramref name="path"/> is set to <c>null</c> or the empty string then the health check middleware
54+
/// will ignore the URL path and process all requests. If <paramref name="path"/> is set to a non-empty
55+
/// value, the health check middleware will process requests with a URL that matches the provided value
56+
/// of <paramref name="path"/> case-insensitively, allowing for an extra trailing slash ('/') character.
5357
/// </para>
5458
/// </remarks>
5559
public static IApplicationBuilder UseHealthChecks(this IApplicationBuilder app, PathString path, HealthCheckOptions options)
@@ -77,8 +81,11 @@ public static IApplicationBuilder UseHealthChecks(this IApplicationBuilder app,
7781
/// <returns>A reference to the <paramref name="app"/> after the operation has completed.</returns>
7882
/// <remarks>
7983
/// <para>
80-
/// This method will use <see cref="MapWhenExtensions.MapWhen(IApplicationBuilder, Func{HttpContext, bool}, Action{IApplicationBuilder})"/> to
81-
/// listen to health checks requests on the specified URL path and port.
84+
/// If <paramref name="path"/> is set to <c>null</c> or the empty string then the health check middleware
85+
/// will ignore the URL path and process all requests on the specified port. If <paramref name="path"/> is
86+
/// set to a non-empty value, the health check middleware will process requests with a URL that matches the
87+
/// provided value of <paramref name="path"/> case-insensitively, allowing for an extra trailing slash ('/')
88+
/// character.
8289
/// </para>
8390
/// <para>
8491
/// The health check middleware will use default settings from <see cref="IOptions{HealthCheckOptions}"/>.
@@ -104,8 +111,11 @@ public static IApplicationBuilder UseHealthChecks(this IApplicationBuilder app,
104111
/// <returns>A reference to the <paramref name="app"/> after the operation has completed.</returns>
105112
/// <remarks>
106113
/// <para>
107-
/// This method will use <see cref="MapWhenExtensions.MapWhen(IApplicationBuilder, Func{HttpContext, bool}, Action{IApplicationBuilder})"/> to
108-
/// listen to health checks requests on the specified URL path and port.
114+
/// If <paramref name="path"/> is set to <c>null</c> or the empty string then the health check middleware
115+
/// will ignore the URL path and process all requests on the specified port. If <paramref name="path"/> is
116+
/// set to a non-empty value, the health check middleware will process requests with a URL that matches the
117+
/// provided value of <paramref name="path"/> case-insensitively, allowing for an extra trailing slash ('/')
118+
/// character.
109119
/// </para>
110120
/// <para>
111121
/// The health check middleware will use default settings from <see cref="IOptions{HealthCheckOptions}"/>.
@@ -142,8 +152,11 @@ public static IApplicationBuilder UseHealthChecks(this IApplicationBuilder app,
142152
/// <returns>A reference to the <paramref name="app"/> after the operation has completed.</returns>
143153
/// <remarks>
144154
/// <para>
145-
/// This method will use <see cref="MapExtensions.Map(IApplicationBuilder, PathString, Action{IApplicationBuilder})"/> to
146-
/// listen to health checks requests on the specified URL path.
155+
/// If <paramref name="path"/> is set to <c>null</c> or the empty string then the health check middleware
156+
/// will ignore the URL path and process all requests on the specified port. If <paramref name="path"/> is
157+
/// set to a non-empty value, the health check middleware will process requests with a URL that matches the
158+
/// provided value of <paramref name="path"/> case-insensitively, allowing for an extra trailing slash ('/')
159+
/// character.
147160
/// </para>
148161
/// </remarks>
149162
public static IApplicationBuilder UseHealthChecks(this IApplicationBuilder app, PathString path, int port, HealthCheckOptions options)
@@ -172,8 +185,11 @@ public static IApplicationBuilder UseHealthChecks(this IApplicationBuilder app,
172185
/// <returns>A reference to the <paramref name="app"/> after the operation has completed.</returns>
173186
/// <remarks>
174187
/// <para>
175-
/// This method will use <see cref="MapExtensions.Map(IApplicationBuilder, PathString, Action{IApplicationBuilder})"/> to
176-
/// listen to health checks requests on the specified URL path.
188+
/// If <paramref name="path"/> is set to <c>null</c> or the empty string then the health check middleware
189+
/// will ignore the URL path and process all requests on the specified port. If <paramref name="path"/> is
190+
/// set to a non-empty value, the health check middleware will process requests with a URL that matches the
191+
/// provided value of <paramref name="path"/> case-insensitively, allowing for an extra trailing slash ('/')
192+
/// character.
177193
/// </para>
178194
/// </remarks>
179195
public static IApplicationBuilder UseHealthChecks(this IApplicationBuilder app, PathString path, string port, HealthCheckOptions options)
@@ -204,16 +220,35 @@ public static IApplicationBuilder UseHealthChecks(this IApplicationBuilder app,
204220

205221
private static void UseHealthChecksCore(IApplicationBuilder app, PathString path, int? port, object[] args)
206222
{
207-
if (port == null)
208-
{
209-
app.Map(path, b => b.UseMiddleware<HealthCheckMiddleware>(args));
210-
}
211-
else
223+
// NOTE: we explicitly don't use Map here because it's really common for multiple health
224+
// check middleware to overlap in paths. Ex: `/health`, `/health/detailed` - this is order
225+
// sensititive with Map, and it's really surprising to people.
226+
//
227+
// See:
228+
// https://github.com/aspnet/Diagnostics/issues/511
229+
// https://github.com/aspnet/Diagnostics/issues/512
230+
// https://github.com/aspnet/Diagnostics/issues/514
231+
232+
Func<HttpContext, bool> predicate = c =>
212233
{
213-
app.MapWhen(
214-
c => c.Connection.LocalPort == port,
215-
b0 => b0.Map(path, b1 => b1.UseMiddleware<HealthCheckMiddleware>(args)));
216-
}
234+
return
235+
236+
// Process the port if we have one
237+
(port == null || c.Connection.LocalPort == port) &&
238+
239+
// We allow you to listen on all URLs by providing the empty PathString.
240+
(!path.HasValue ||
241+
242+
// If you do provide a PathString, want to handle all of the special cases that
243+
// StartsWithSegments handles, but we also want it to have exact match semantics.
244+
//
245+
// Ex: /Foo/ == /Foo (true)
246+
// Ex: /Foo/Bar == /Foo (false)
247+
(c.Request.Path.StartsWithSegments(path, out var remaining) &&
248+
string.IsNullOrEmpty(remaining)));
249+
};
250+
251+
app.MapWhen(predicate, b => b.UseMiddleware<HealthCheckMiddleware>(args));
217252
}
218253
}
219254
}

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

Lines changed: 197 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -395,6 +395,131 @@ public async Task CanListenWithoutPath_AcceptsRequest()
395395
Assert.Equal("Healthy", await response.Content.ReadAsStringAsync());
396396
}
397397

398+
[Fact]
399+
public async Task CanListenWithPath_AcceptsRequestWithExtraSlash()
400+
{
401+
var builder = new WebHostBuilder()
402+
.Configure(app =>
403+
{
404+
app.UseHealthChecks("/health");
405+
})
406+
.ConfigureServices(services =>
407+
{
408+
services.AddHealthChecks();
409+
});
410+
411+
var server = new TestServer(builder);
412+
var client = server.CreateClient();
413+
414+
var response = await client.GetAsync("http://localhost:5001/health/");
415+
416+
Assert.Equal(HttpStatusCode.NotFound, response.StatusCode);
417+
}
418+
419+
[Fact]
420+
public async Task CanListenWithPath_AcceptsRequestWithCaseInsensitiveMatch()
421+
{
422+
var builder = new WebHostBuilder()
423+
.Configure(app =>
424+
{
425+
app.UseHealthChecks("/health");
426+
})
427+
.ConfigureServices(services =>
428+
{
429+
services.AddHealthChecks();
430+
});
431+
432+
var server = new TestServer(builder);
433+
var client = server.CreateClient();
434+
435+
var response = await client.GetAsync("http://localhost:5001/HEALTH");
436+
437+
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
438+
Assert.Equal("text/plain", response.Content.Headers.ContentType.ToString());
439+
Assert.Equal("Healthy", await response.Content.ReadAsStringAsync());
440+
}
441+
442+
[Fact]
443+
public async Task CanListenWithPath_RejectsRequestWithExtraSegments()
444+
{
445+
var builder = new WebHostBuilder()
446+
.Configure(app =>
447+
{
448+
app.UseHealthChecks("/health");
449+
})
450+
.ConfigureServices(services =>
451+
{
452+
services.AddHealthChecks();
453+
});
454+
455+
var server = new TestServer(builder);
456+
var client = server.CreateClient();
457+
458+
var response = await client.GetAsync("http://localhost:5001/health/detailed");
459+
460+
Assert.Equal(HttpStatusCode.NotFound, response.StatusCode);
461+
}
462+
463+
// See: https://github.com/aspnet/Diagnostics/issues/511
464+
[Fact]
465+
public async Task CanListenWithPath_MultipleMiddleware_LeastSpecificFirst()
466+
{
467+
var builder = new WebHostBuilder()
468+
.Configure(app =>
469+
{
470+
// Throws if used
471+
app.UseHealthChecks("/health", new HealthCheckOptions()
472+
{
473+
ResponseWriter = (c, r) => throw null,
474+
});
475+
476+
app.UseHealthChecks("/health/detailed");
477+
})
478+
.ConfigureServices(services =>
479+
{
480+
services.AddHealthChecks();
481+
});
482+
483+
var server = new TestServer(builder);
484+
var client = server.CreateClient();
485+
486+
var response = await client.GetAsync("http://localhost:5001/health/detailed");
487+
488+
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
489+
Assert.Equal("text/plain", response.Content.Headers.ContentType.ToString());
490+
Assert.Equal("Healthy", await response.Content.ReadAsStringAsync());
491+
}
492+
493+
// See: https://github.com/aspnet/Diagnostics/issues/511
494+
[Fact]
495+
public async Task CanListenWithPath_MultipleMiddleware_MostSpecificFirst()
496+
{
497+
var builder = new WebHostBuilder()
498+
.Configure(app =>
499+
{
500+
app.UseHealthChecks("/health/detailed");
501+
502+
// Throws if used
503+
app.UseHealthChecks("/health", new HealthCheckOptions()
504+
{
505+
ResponseWriter = (c, r) => throw null,
506+
});
507+
})
508+
.ConfigureServices(services =>
509+
{
510+
services.AddHealthChecks();
511+
});
512+
513+
var server = new TestServer(builder);
514+
var client = server.CreateClient();
515+
516+
var response = await client.GetAsync("http://localhost:5001/health/detailed");
517+
518+
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
519+
Assert.Equal("text/plain", response.Content.Headers.ContentType.ToString());
520+
Assert.Equal("Healthy", await response.Content.ReadAsStringAsync());
521+
}
522+
398523
[Fact]
399524
public async Task CanListenOnPort_AcceptsRequest_OnSpecifiedPort()
400525
{
@@ -486,6 +611,78 @@ public async Task CanListenOnPort_RejectsRequest_OnOtherPort()
486611
Assert.Equal(HttpStatusCode.NotFound, response.StatusCode);
487612
}
488613

614+
[Fact]
615+
public async Task CanListenOnPort_MultipleMiddleware()
616+
{
617+
var builder = new WebHostBuilder()
618+
.Configure(app =>
619+
{
620+
app.Use(next => async (context) =>
621+
{
622+
// Need to fake setting the connection info. TestServer doesn't
623+
// do that, because it doesn't have a connection.
624+
context.Connection.LocalPort = context.Request.Host.Port.Value;
625+
await next(context);
626+
});
627+
628+
// Throws if used
629+
app.UseHealthChecks("/health", port: 5001, new HealthCheckOptions()
630+
{
631+
ResponseWriter = (c, r) => throw null,
632+
});
633+
634+
app.UseHealthChecks("/health/detailed", port: 5001);
635+
})
636+
.ConfigureServices(services =>
637+
{
638+
services.AddHealthChecks();
639+
});
640+
641+
var server = new TestServer(builder);
642+
var client = server.CreateClient();
643+
644+
var response = await client.GetAsync("http://localhost:5001/health/detailed");
645+
646+
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
647+
Assert.Equal("text/plain", response.Content.Headers.ContentType.ToString());
648+
Assert.Equal("Healthy", await response.Content.ReadAsStringAsync());
649+
}
650+
651+
[Fact]
652+
public async Task CanListenOnPort_MultipleMiddleware_DifferentPorts()
653+
{
654+
var builder = new WebHostBuilder()
655+
.Configure(app =>
656+
{
657+
app.Use(next => async (context) =>
658+
{
659+
// Need to fake setting the connection info. TestServer doesn't
660+
// do that, because it doesn't have a connection.
661+
context.Connection.LocalPort = context.Request.Host.Port.Value;
662+
await next(context);
663+
});
664+
665+
// Throws if used
666+
app.UseHealthChecks("/health", port: 5002, new HealthCheckOptions()
667+
{
668+
ResponseWriter = (c, r) => throw null,
669+
});
489670

671+
app.UseHealthChecks("/health", port: 5001);
672+
})
673+
.ConfigureServices(services =>
674+
{
675+
services.AddHealthChecks();
676+
});
677+
678+
var server = new TestServer(builder);
679+
var client = server.CreateClient();
680+
681+
var response = await client.GetAsync("http://localhost:5001/health");
682+
683+
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
684+
Assert.Equal("text/plain", response.Content.Headers.ContentType.ToString());
685+
Assert.Equal("Healthy", await response.Content.ReadAsStringAsync());
686+
}
490687
}
491688
}

0 commit comments

Comments
 (0)