Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#1971 #928 Avoid content if original request has no content and avoid Transfer-Encoding: chunked if Content-Length is known #1972

Merged
Merged
176 changes: 90 additions & 86 deletions src/Ocelot/Request/Mapper/RequestMapper.cs
Original file line number Diff line number Diff line change
@@ -1,87 +1,91 @@
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Http.Extensions;
using Microsoft.Extensions.Primitives;
using Ocelot.Configuration;

namespace Ocelot.Request.Mapper;

public class RequestMapper : IRequestMapper
{
private static readonly HashSet<string> UnsupportedHeaders = new(StringComparer.OrdinalIgnoreCase) { "host" };
private static readonly string[] ContentHeaders = { "Content-Length", "Content-Language", "Content-Location", "Content-Range", "Content-MD5", "Content-Disposition", "Content-Encoding" };

public HttpRequestMessage Map(HttpRequest request, DownstreamRoute downstreamRoute)
{
var requestMessage = new HttpRequestMessage
{
Content = MapContent(request),
Method = MapMethod(request, downstreamRoute),
RequestUri = MapUri(request),
Version = downstreamRoute.DownstreamHttpVersion,
};

MapHeaders(request, requestMessage);

return requestMessage;
}

private static HttpContent MapContent(HttpRequest request)
{
// TODO We should check if we really need to call HttpRequest.Body.Length
// But we assume that if CanSeek is true, the length is calculated without an important overhead
if (request.Body is null or { CanSeek: true, Length: <= 0 })
{
return null;
}

var content = new StreamHttpContent(request.HttpContext);

AddContentHeaders(request, content);

return content;
}

private static void AddContentHeaders(HttpRequest request, HttpContent content)
{
if (!string.IsNullOrEmpty(request.ContentType))
{
content.Headers
.TryAddWithoutValidation("Content-Type", new[] { request.ContentType });
}

// The performance might be improved by retrieving the matching headers from the request
// instead of calling request.Headers.TryGetValue for each used content header
var matchingHeaders = ContentHeaders.Where(header => request.Headers.ContainsKey(header));

foreach (var key in matchingHeaders)
{
if (!request.Headers.TryGetValue(key, out var value))
{
continue;
}

content.Headers.TryAddWithoutValidation(key, value.ToArray());
}
}

private static HttpMethod MapMethod(HttpRequest request, DownstreamRoute downstreamRoute) =>
!string.IsNullOrEmpty(downstreamRoute?.DownstreamHttpMethod) ?
new HttpMethod(downstreamRoute.DownstreamHttpMethod) : new HttpMethod(request.Method);

// TODO Review this method, request.GetEncodedUrl() could throw a NullReferenceException
private static Uri MapUri(HttpRequest request) => new(request.GetEncodedUrl());

private static void MapHeaders(HttpRequest request, HttpRequestMessage requestMessage)
{
foreach (var header in request.Headers)
{
if (IsSupportedHeader(header))
{
requestMessage.Headers.TryAddWithoutValidation(header.Key, header.Value.ToArray());
}
}
}

private static bool IsSupportedHeader(KeyValuePair<string, StringValues> header) =>
!UnsupportedHeaders.Contains(header.Key);
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Http.Extensions;
using Microsoft.Extensions.Primitives;
using Ocelot.Configuration;

namespace Ocelot.Request.Mapper;

public class RequestMapper : IRequestMapper
{
private static readonly HashSet<string> UnsupportedHeaders = new(StringComparer.OrdinalIgnoreCase) { "host", "transfer-encoding" };
Copy link
Member

Choose a reason for hiding this comment

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

@ggnaegi Note 1
"transfer-encoding"

private static readonly string[] ContentHeaders = { "Content-Length", "Content-Language", "Content-Location", "Content-Range", "Content-MD5", "Content-Disposition", "Content-Encoding" };

public HttpRequestMessage Map(HttpRequest request, DownstreamRoute downstreamRoute)
{
var requestMessage = new HttpRequestMessage
{
Content = MapContent(request),
Method = MapMethod(request, downstreamRoute),
RequestUri = MapUri(request),
Version = downstreamRoute.DownstreamHttpVersion,
};

MapHeaders(request, requestMessage);

return requestMessage;
}
raman-m marked this conversation as resolved.
Show resolved Hide resolved

private static HttpContent MapContent(HttpRequest request)
{
HttpContent content;

// No content if we have no body or if the request has no content according to RFC 2616 section 4.3
if (request.Body == null
Copy link
Member

Choose a reason for hiding this comment

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

Why not use this instead, it's imo more compact.

if (request.Body == null || (!request.ContentLength.HasValue && StringValues.IsNullOrEmpty(request.Headers.TransferEncoding)))
{
    return null;
}

HttpContent content = request.ContentLength is 0
    ? new ByteArrayContent([])
    : new StreamHttpContent(request.HttpContext);

AddContentHeaders(request, content);

return content;

Copy link
Member

Choose a reason for hiding this comment

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

@ggnaegi Gui, is it resolved?

|| (!request.ContentLength.HasValue && StringValues.IsNullOrEmpty(request.Headers.TransferEncoding)))
Comment on lines +33 to +34
Copy link
Member

Choose a reason for hiding this comment

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

Old version

-        // TODO We should check if we really need to call HttpRequest.Body.Length
-        // But we assume that if CanSeek is true, the length is calculated without an important overhead
-        if (request.Body is null or { CanSeek: true, Length: <= 0 })
+        // no content if we have no body or if the request has no content according to RFC 2616 section 4.3
+        if (request.Body == null
+            || (!request.ContentLength.HasValue && StringValues.IsNullOrEmpty(request.Headers.TransferEncoding)))

New version

{
return null;
}

content = request.ContentLength is 0
? new ByteArrayContent(Array.Empty<byte>())
: new StreamHttpContent(request.HttpContext);
Comment on lines +39 to +41
Copy link
Member

Choose a reason for hiding this comment

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

Old version

-        var content = new StreamHttpContent(request.HttpContext);
+        content = request.ContentLength is 0
+            ? new ByteArrayContent(Array.Empty<byte>()) 
+            : new StreamHttpContent(request.HttpContext);

New version


AddContentHeaders(request, content);

return content;
}

private static void AddContentHeaders(HttpRequest request, HttpContent content)
{
if (!string.IsNullOrEmpty(request.ContentType))
{
content.Headers
.TryAddWithoutValidation("Content-Type", new[] { request.ContentType });
}

// The performance might be improved by retrieving the matching headers from the request
// instead of calling request.Headers.TryGetValue for each used content header
var matchingHeaders = ContentHeaders.Where(header => request.Headers.ContainsKey(header));

foreach (var key in matchingHeaders)
{
if (!request.Headers.TryGetValue(key, out var value))
{
continue;
}

content.Headers.TryAddWithoutValidation(key, value.ToArray());
}
}

private static HttpMethod MapMethod(HttpRequest request, DownstreamRoute downstreamRoute) =>
!string.IsNullOrEmpty(downstreamRoute?.DownstreamHttpMethod) ?
new HttpMethod(downstreamRoute.DownstreamHttpMethod) : new HttpMethod(request.Method);

// TODO Review this method, request.GetEncodedUrl() could throw a NullReferenceException
private static Uri MapUri(HttpRequest request) => new(request.GetEncodedUrl());

private static void MapHeaders(HttpRequest request, HttpRequestMessage requestMessage)
{
foreach (var header in request.Headers)
{
if (IsSupportedHeader(header))
{
requestMessage.Headers.TryAddWithoutValidation(header.Key, header.Value.ToArray());
}
}
}

private static bool IsSupportedHeader(KeyValuePair<string, StringValues> header) =>
!UnsupportedHeaders.Contains(header.Key);
}
17 changes: 8 additions & 9 deletions src/Ocelot/Request/Mapper/StreamHttpContent.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,25 +8,24 @@ public class StreamHttpContent : HttpContent
private const int DefaultBufferSize = 65536;
public const long UnknownLength = -1;
private readonly HttpContext _context;
private readonly long _contentLength;
Copy link
Member

@ggnaegi ggnaegi Feb 26, 2024

Choose a reason for hiding this comment

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

Ok, it's fine like this, but I would prefer using Headers.ContentLength. This value is set when calling AddContentHeaders method and setting the content headers.

Copy link
Member

Choose a reason for hiding this comment

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

And...


public StreamHttpContent(HttpContext context)
{
_context = context ?? throw new ArgumentNullException(nameof(context));
_contentLength = context.Request.ContentLength ?? UnknownLength;
}

protected override async Task SerializeToStreamAsync(Stream stream, TransportContext context,
CancellationToken cancellationToken)
=> await CopyAsync(_context.Request.Body, stream, Headers.ContentLength ?? UnknownLength, false,
cancellationToken);
protected override Task SerializeToStreamAsync(Stream stream, TransportContext context, CancellationToken cancellationToken)
=> CopyAsync(_context.Request.Body, stream, _contentLength, false, cancellationToken);

protected override async Task SerializeToStreamAsync(Stream stream, TransportContext context)
=> await CopyAsync(_context.Request.Body, stream, Headers.ContentLength ?? UnknownLength, false,
CancellationToken.None);
protected override Task SerializeToStreamAsync(Stream stream, TransportContext context)
=> CopyAsync(_context.Request.Body, stream, _contentLength, false, CancellationToken.None);

protected override bool TryComputeLength(out long length)
{
length = -1;
return false;
length = _contentLength;
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer Headers.ContentLength ?? UnknownLength so the content length reflects the headers' value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the headers do not contains the Content-Length header, when it will try to Calculate the content length using TryComputeLength. And on chunked content, there is no Content-Length header set, so it will result in a stack overflow.

Copy link
Member

Choose a reason for hiding this comment

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

No, just use length = Headers.ContentLength ?? UnknownLength Headers.ContentLength is a long?, so it is per default null, even if the header is not set.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer this approach, so we are relying again on Microsoft implementations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the implementation of Headers.ContentLength: https://github.com/microsoft/referencesource/blob/51cf7850defa8a17d815b4700b67116e3fa283c2/System/net/System/Net/Http/Headers/HttpContentHeaders.cs#L76
It will not just return null, it tries to compute the content length if the header is not explicitly set, which results in a stack overflow when you use it in TryComputeLength.

Copy link
Member

Choose a reason for hiding this comment

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

I would say it is pretty uncommon, that the content has to "ask" the Header how long it is, the content should know that without the header. So even if MS will change that, it will be not in the near future.

No, that's your point of view, at the end, it's a content header, so it's per se part of the content

Copy link
Member

Choose a reason for hiding this comment

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

@ggnaegi: Yeah well, Microsoft is using this approach with Headers.ContentLength in YARP, so that's why I'm a bit surprised.

Alexander's concern: the code Headers.ContentLength leads to stack overflow. First of all, did someone caught this exception?
My recommendation: Moving Headers.ContentLength out of the method to the very beginning of the Invoke. So, can we move headers reading to upper contexts, 1 or even 2 levels up?
Will it help to avoid stack overflow error?

Copy link
Member

Choose a reason for hiding this comment

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

@alexreinert 2 days ago

We are very sorry, but you've provided the link to the repo with the code 5 years old!!! 🤣
image
Why we should take it into account? 😉
We need fresh .NET Core source!

Copy link
Member

@raman-m raman-m Feb 28, 2024

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Did we finish debates here?

return length >= 0;
}

// This is used internally by HttpContent.ReadAsStreamAsync(...)
Expand Down
32 changes: 27 additions & 5 deletions test/Ocelot.AcceptanceTests/HttpTests.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Server.Kestrel.Core;
using Ocelot.Configuration.File;
using System.Security.Authentication;

namespace Ocelot.AcceptanceTests
{
Expand Down Expand Up @@ -216,25 +218,45 @@ public void should_return_response_200_when_using_http_two_to_talk_to_server_run
}

private void GivenThereIsAServiceRunningOn(string baseUrl, string basePath, int port, HttpProtocols protocols)
{
_serviceHandler.GivenThereIsAServiceRunningOn(baseUrl, basePath, async context =>
{
void options(KestrelServerOptions serverOptions)
{
serverOptions.Listen(IPAddress.Loopback, port, listenOptions =>
{
listenOptions.Protocols = protocols;
});
}

_serviceHandler.GivenThereIsAServiceRunningOnWithKestrelOptions(baseUrl, basePath, options, async context =>
{
context.Response.StatusCode = 200;
var reader = new StreamReader(context.Request.Body);
var body = await reader.ReadToEndAsync();
await context.Response.WriteAsync(body);
}, port, protocols);
});
}

private void GivenThereIsAServiceUsingHttpsRunningOn(string baseUrl, string basePath, int port, HttpProtocols protocols)
{
_serviceHandler.GivenThereIsAServiceRunningOnUsingHttps(baseUrl, basePath, async context =>
void options(KestrelServerOptions serverOptions)
{
serverOptions.Listen(IPAddress.Loopback, port, listenOptions =>
{
listenOptions.UseHttps("mycert.pfx", "password", options =>
{
options.SslProtocols = SslProtocols.Tls12;
});
listenOptions.Protocols = protocols;
});
}

_serviceHandler.GivenThereIsAServiceRunningOnWithKestrelOptions(baseUrl, basePath, options, async context =>
{
context.Response.StatusCode = 200;
var reader = new StreamReader(context.Request.Body);
var body = await reader.ReadToEndAsync();
await context.Response.WriteAsync(body);
}, port, protocols);
});
}

public void Dispose()
Expand Down
Loading