Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
1 change: 1 addition & 0 deletions src/Ocelot/Errors/OcelotErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -44,4 +44,5 @@ public enum OcelotErrorCode
CouldNotFindLoadBalancerCreator = 39,
ErrorInvokingLoadBalancerCreator = 40,
PayloadTooLargeError = 41,
BadRequestError = 42,
}
29 changes: 19 additions & 10 deletions src/Ocelot/Middleware/OcelotPipelineExtensions.cs
Comment thread
raman-m marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,10 @@ namespace Ocelot.Middleware;

public static class OcelotPipelineExtensions
{
public static RequestDelegate BuildOcelotPipeline(this IApplicationBuilder app, OcelotPipelineConfiguration configuration)
public static RequestDelegate BuildOcelotPipeline(
this IApplicationBuilder app,
OcelotPipelineConfiguration configuration
)
{
// this sets up the downstream context and gets the config
app.UseMiddleware<ConfigurationMiddleware>();
Expand All @@ -34,7 +37,8 @@ public static RequestDelegate BuildOcelotPipeline(this IApplicationBuilder app,
app.UseMiddleware<ExceptionHandlerMiddleware>();

// If the request is for websockets upgrade we fork into a different pipeline
app.MapWhen(httpContext => httpContext.WebSockets.IsWebSocketRequest,
app.MapWhen(
httpContext => httpContext.WebSockets.IsWebSocketRequest,
ws =>
{
ws.UseMiddleware<DownstreamRouteFinderMiddleware>();
Expand All @@ -43,7 +47,8 @@ public static RequestDelegate BuildOcelotPipeline(this IApplicationBuilder app,
ws.UseMiddleware<LoadBalancingMiddleware>();
ws.UseMiddleware<DownstreamUrlCreatorMiddleware>();
ws.UseMiddleware<WebSocketsProxyMiddleware>();
});
}
);

// Allow the user to respond with absolutely anything they want.
app.UseIfNotNull(configuration.PreErrorResponderMiddleware);
Expand Down Expand Up @@ -128,11 +133,15 @@ public static RequestDelegate BuildOcelotPipeline(this IApplicationBuilder app,
return app.Build();
}

private static IApplicationBuilder UseIfNotNull(this IApplicationBuilder builder, Func<HttpContext, Func<Task>, Task> middleware)
=> middleware != null ? builder.Use(middleware) : builder;

private static IApplicationBuilder UseIfNotNull<TMiddleware>(this IApplicationBuilder builder, Func<HttpContext, Func<Task>, Task> middleware)
where TMiddleware : OcelotMiddleware => middleware != null
? builder.Use(middleware)
: builder.UseMiddleware<TMiddleware>();
private static IApplicationBuilder UseIfNotNull(
this IApplicationBuilder builder,
Func<HttpContext, Func<Task>, Task> middleware
) => middleware != null ? builder.Use(middleware) : builder;

private static IApplicationBuilder UseIfNotNull<TMiddleware>(
this IApplicationBuilder builder,
Func<HttpContext, Func<Task>, Task> middleware
)
where TMiddleware : OcelotMiddleware =>
middleware != null ? builder.Use(middleware) : builder.UseMiddleware<TMiddleware>();
}
12 changes: 12 additions & 0 deletions src/Ocelot/Requester/BadRequestError.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
using System;
using Ocelot.Errors;

namespace Ocelot.Requester;

public class BadRequestError : Error
{
public BadRequestError(string message)
: base(message, OcelotErrorCode.BadRequestError, 400)
Comment thread
raman-m marked this conversation as resolved.
Outdated
{
}
}
Comment thread
raman-m marked this conversation as resolved.
Outdated
8 changes: 8 additions & 0 deletions src/Ocelot/Requester/HttpExceptionToErrorMapper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,14 @@ public Error Map(Exception exception)
return new PayloadTooLargeError(exception);
}

// Late Catch: Map the HttpRequestException to a 400 Bad Request.
// If the header format is invalid, HttpClient throws this exception locally.
// By catching it here, we ensure Ocelot returns a clean 4xx client error instead of a generic 502 Bad Gateway.
if (exception is HttpRequestException && exception.Message.Contains("only ASCII characters"))
Comment thread
raman-m marked this conversation as resolved.
Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It’s hard to judge this solution until a valid acceptance test has been written.

I believe we can improve this area, since the Message.Contains method handles a specific user scenario. But what about codes, internal exceptions, and so on? We should inspect the generated exception and reuse its state rather than relying only on messages.

We might also consider covering more scenarios that return a BadRequestError response.
Given the purpose of BadRequestError, it should cover all exceptions that result in a 400 Bad Request status. So, I'd expect more if-else block(s).
You (we) might be inspired by this #2375 (reply in thread).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Appreciate the guidance

{
return new BadRequestError(exception.Message);
}

return new ConnectionToDownstreamServiceError(exception);
}

Expand Down
5 changes: 5 additions & 0 deletions src/Ocelot/Responder/ErrorsToHttpStatusCodeMapper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,11 @@ public int Map(List<Error> errors)
return 413;
}

if (errors.Any(e => e.Code == OcelotErrorCode.BadRequestError))
{
return 400;
Comment thread
raman-m marked this conversation as resolved.
Outdated
}

return 404;
}
}
Loading