From c502e4fe1d065d3a0abdd3fea23efdb42020ca09 Mon Sep 17 00:00:00 2001 From: Paul Roy Date: Sat, 14 Sep 2024 18:47:59 +0200 Subject: [PATCH] Downgrade the Warning to Information on missing `Content-Length` header in `MultiplexingMiddleware` (#2146) * fix: downgrade the warning to information on missing content-length header * chore: add route name to logs * test: fixing multiplexing middleware tests * Code review by @raman-m --------- Co-authored-by: Paul Roy Co-authored-by: Raman Maksimchuk --- .../PollyQoSResiliencePipelineProvider.cs | 13 ++++-------- src/Ocelot/Configuration/DownstreamRoute.cs | 8 ++++++- .../Multiplexer/MultiplexingMiddleware.cs | 21 ++++++++++--------- .../ServiceDiscoveryProviderFactory.cs | 3 +-- .../MultiplexingMiddlewareTests.cs | 5 ++++- 5 files changed, 27 insertions(+), 23 deletions(-) diff --git a/src/Ocelot.Provider.Polly/PollyQoSResiliencePipelineProvider.cs b/src/Ocelot.Provider.Polly/PollyQoSResiliencePipelineProvider.cs index a0000c62d..6ec9b5d53 100644 --- a/src/Ocelot.Provider.Polly/PollyQoSResiliencePipelineProvider.cs +++ b/src/Ocelot.Provider.Polly/PollyQoSResiliencePipelineProvider.cs @@ -38,11 +38,7 @@ public PollyQoSResiliencePipelineProvider( }; protected virtual HashSet ServerErrorCodes { get; } = DefaultServerErrorCodes; - - protected virtual string GetRouteName(DownstreamRoute route) - => string.IsNullOrWhiteSpace(route.ServiceName) - ? route.UpstreamPathTemplate?.Template ?? route.DownstreamPathTemplate?.Value ?? string.Empty - : route.ServiceName; + protected virtual string GetRouteName(DownstreamRoute route) => route.Name(); /// /// Gets Polly V8 resilience pipeline (applies QoS feature) for the route. @@ -57,9 +53,8 @@ public ResiliencePipeline GetResiliencePipeline(DownstreamR return ResiliencePipeline.Empty; // shortcut -> No QoS } - var currentRouteName = GetRouteName(route); return _registry.GetOrAddPipeline( - key: new OcelotResiliencePipelineKey(currentRouteName), + key: new OcelotResiliencePipelineKey(GetRouteName(route)), configure: (builder) => ConfigureStrategies(builder, route)); } @@ -78,7 +73,7 @@ protected virtual ResiliencePipelineBuilder ConfigureCircui } var options = route.QosOptions; - var info = $"Circuit Breaker for Route: {GetRouteName(route)}: "; + var info = $"Circuit Breaker for the route: {GetRouteName(route)}: "; var strategyOptions = new CircuitBreakerStrategyOptions { FailureRatio = 0.8, @@ -127,7 +122,7 @@ protected virtual ResiliencePipelineBuilder ConfigureTimeou Timeout = TimeSpan.FromMilliseconds(options.TimeoutValue), OnTimeout = _ => { - _logger.LogInformation($"Timeout for Route: {GetRouteName(route)}"); + _logger.LogInformation(() => $"Timeout for the route: {GetRouteName(route)}"); return ValueTask.CompletedTask; }, }; diff --git a/src/Ocelot/Configuration/DownstreamRoute.cs b/src/Ocelot/Configuration/DownstreamRoute.cs index 0241f9b61..818390a02 100644 --- a/src/Ocelot/Configuration/DownstreamRoute.cs +++ b/src/Ocelot/Configuration/DownstreamRoute.cs @@ -91,7 +91,6 @@ public DownstreamRoute( public string ServiceName { get; } public string ServiceNamespace { get; } public HttpHandlerOptions HttpHandlerOptions { get; } - public bool UseServiceDiscovery { get; } public bool EnableEndpointEndpointRateLimiting { get; } public QoSOptions QosOptions { get; } public string DownstreamScheme { get; } @@ -130,6 +129,13 @@ public DownstreamRoute( /// public HttpVersionPolicy DownstreamHttpVersionPolicy { get; } public Dictionary UpstreamHeaders { get; } + public bool UseServiceDiscovery { get; } public MetadataOptions MetadataOptions { get; } + + /// Gets the route name depending on whether the service discovery mode is enabled or disabled. + /// A object with the name. + public string Name() => string.IsNullOrEmpty(ServiceName) && !UseServiceDiscovery + ? UpstreamPathTemplate?.Template ?? DownstreamPathTemplate?.Value ?? "?" + : string.Join(':', ServiceNamespace, ServiceName, UpstreamPathTemplate?.Template); } } diff --git a/src/Ocelot/Multiplexer/MultiplexingMiddleware.cs b/src/Ocelot/Multiplexer/MultiplexingMiddleware.cs index 43a98fcd3..c148eb9b9 100644 --- a/src/Ocelot/Multiplexer/MultiplexingMiddleware.cs +++ b/src/Ocelot/Multiplexer/MultiplexingMiddleware.cs @@ -183,7 +183,7 @@ private IEnumerable> ProcessRouteWithComplexAggregation(Aggreg /// The cloned Http context. private async Task ProcessRouteAsync(HttpContext sourceContext, DownstreamRoute route, List placeholders = null) { - var newHttpContext = await CreateThreadContextAsync(sourceContext); + var newHttpContext = await CreateThreadContextAsync(sourceContext, route); CopyItemsToNewContext(newHttpContext, sourceContext, placeholders); newHttpContext.Items.UpsertDownstreamRoute(route); @@ -200,17 +200,18 @@ private static void CopyItemsToNewContext(HttpContext target, HttpContext source target.Items.SetIInternalConfiguration(source.Items.IInternalConfiguration()); target.Items.UpsertTemplatePlaceholderNameAndValues(placeholders ?? source.Items.TemplatePlaceholderNameAndValues()); - } - + } + /// /// Creates a new HttpContext based on the source. /// - /// The base http context. + /// The base http context. + /// Downstream route. /// The cloned context. - protected virtual async Task CreateThreadContextAsync(HttpContext source) + protected virtual async Task CreateThreadContextAsync(HttpContext source, DownstreamRoute route) { var from = source.Request; - var bodyStream = await CloneRequestBodyAsync(from, source.RequestAborted); + var bodyStream = await CloneRequestBodyAsync(from, route, source.RequestAborted); var target = new DefaultHttpContext { Request = @@ -245,7 +246,7 @@ protected virtual async Task CreateThreadContextAsync(HttpContext s // Once the downstream request is completed and the downstream response has been read, the downstream response object can dispose of the body's Stream object target.Response.RegisterForDisposeAsync(bodyStream); // manage Stream lifetime by HttpResponse object return target; - } + } protected virtual Task MapAsync(HttpContext httpContext, Route route, List contexts) { @@ -258,12 +259,12 @@ protected virtual Task MapAsync(HttpContext httpContext, Route route, List CloneRequestBodyAsync(HttpRequest request, CancellationToken aborted) + protected virtual async Task CloneRequestBodyAsync(HttpRequest request, DownstreamRoute route, CancellationToken aborted) { request.EnableBuffering(); if (request.Body.Position != 0) { - Logger.LogWarning("Ocelot does not support body copy without stream in initial position 0"); + Logger.LogWarning(() => $"Ocelot does not support body copy without stream in initial position 0 for the route {route.Name()}."); return request.Body; } @@ -276,7 +277,7 @@ protected virtual async Task CloneRequestBodyAsync(HttpRequest request, } else { - Logger.LogWarning("Aggregation does not support body copy without Content-Length header!"); + Logger.LogInformation(() => $"Aggregation does not support body copy without Content-Length header, skipping body copy for the route {route.Name()}."); } return targetBuffer; diff --git a/src/Ocelot/ServiceDiscovery/ServiceDiscoveryProviderFactory.cs b/src/Ocelot/ServiceDiscovery/ServiceDiscoveryProviderFactory.cs index b47e4d921..c42493a37 100644 --- a/src/Ocelot/ServiceDiscovery/ServiceDiscoveryProviderFactory.cs +++ b/src/Ocelot/ServiceDiscovery/ServiceDiscoveryProviderFactory.cs @@ -25,8 +25,7 @@ public Response Get(ServiceProviderConfiguration serv { if (route.UseServiceDiscovery) { - var routeName = route.UpstreamPathTemplate?.Template ?? route.ServiceName ?? string.Empty; - _logger.LogInformation(() => $"The {nameof(DownstreamRoute.UseServiceDiscovery)} mode of the route '{routeName}' is enabled."); + _logger.LogInformation(() => $"The {nameof(DownstreamRoute.UseServiceDiscovery)} mode of the route '{route.Name()}' is enabled."); return GetServiceDiscoveryProvider(serviceConfig, route); } diff --git a/test/Ocelot.UnitTests/Multiplexing/MultiplexingMiddlewareTests.cs b/test/Ocelot.UnitTests/Multiplexing/MultiplexingMiddlewareTests.cs index b94c247ff..acff4ae9a 100644 --- a/test/Ocelot.UnitTests/Multiplexing/MultiplexingMiddlewareTests.cs +++ b/test/Ocelot.UnitTests/Multiplexing/MultiplexingMiddlewareTests.cs @@ -63,12 +63,14 @@ public void should_not_multiplex() [Trait("Bug", "1396")] public async Task CreateThreadContextAsync_CopyUser_ToTarget() { + var route = new DownstreamRouteBuilder().Build(); + // Arrange GivenUser("test", "Copy", nameof(CreateThreadContextAsync_CopyUser_ToTarget)); // Act var method = _middleware.GetType().GetMethod("CreateThreadContextAsync", BindingFlags.NonPublic | BindingFlags.Instance); - var actual = await (Task)method.Invoke(_middleware, new object[] { _httpContext }); + var actual = await (Task)method.Invoke(_middleware, new object[] { _httpContext, route }); // Assert AssertUsers(actual); @@ -234,6 +236,7 @@ public async Task Should_Call_CloneRequestBodyAsync_Each_Time_Per_Requests(int n mock.Protected().Verify>("CloneRequestBodyAsync", numberOfRoutes > 1 ? Times.Exactly(numberOfRoutes) : Times.Never(), ItExpr.IsAny(), + ItExpr.IsAny(), ItExpr.IsAny()); }