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

Conversation

alexreinert
Copy link
Contributor

@alexreinert alexreinert commented Feb 25, 2024

Follow-up to #1971

With the changes in Ocelot 23.0 the RequestMapper sets the Property Content using the new StreamHttpContent. Because the Content-Length is not computed in StreamHttpContent the header Transfer-Encoding: chunked will be set on all upstream requests even if there is no content in the downstream request (e.g. on GET requests). This causes issues when the upstream is an IIS working as reverse proxy (using ARR), in that case all GET requests are failing with status code 502.7 inside ARR.

Additionally in some cases the Content-Length is required by the upstream server, it will send status code 411 if it is missing, behind Ocelot all request will fail with status code 411, as the Content-Length was not transferred to the upstream server and instead Transfer-Encoding: chunked is sent to the upstream server.

Both issue are addressed with this PR.

Related issues

@raman-m
Copy link
Member

raman-m commented Feb 25, 2024

Thanks for PR recreation following our development process!

@alexreinert
Copy link
Contributor Author

alexreinert commented Feb 25, 2024

@alexreinert Ok, fine for the first part, we shouldn't have the headers if no content. And we should ensure that.

No, you have not and I have a wireshark dump, that proves that. If you set the Request.Content to a HttpContent, the framework will add headers when sending the request. Depending on TryComputeLength it will be Content-Length or Transfer-Encoding.
To avoid these headers you have to set Request.Content to null.

About the 411 status, I need to investigate that a bit more. We need to support streaming. I'm a bit concerned about the changes you made, possibly breaking this feature.

Please go forward, I tested it, and it worked. But I have no good idea how to test that automatically.

As of GET and DELETE without body, it's a large debate, as I worked with some APIs expecting a body with the GET method (eg. Elasticsearch).

Sure, but I in my opinion it is not up to Ocelot to change the request and add content. If the client request with empty content, Ocelot should send the request with empty content and if the client requests with no content, Ocelot should send the request without content. And if you look in my acceptance tests you will see, that my code does exactly that.

@ggnaegi
Copy link
Member

ggnaegi commented Feb 25, 2024

@alexreinert Ok, fine for the first part, we shouldn't have the headers if no content. And we should ensure that.

No, you have not and I have a wireshark dump, that proves that. If you set the Request.Content to a HttpContent, the framework will add headers when sending the request. Depending on TryComputeLength it will be Content-Length or Transfer-Encoding. To avoid these headers you have to set Request.Content to null.

Mmh, I was just writing that you were right on that part: We should ensure that, "should" meaning it's not the case yet.

About the 411 status, I need to investigate that a bit more. We need to support streaming. I'm a bit concerned about the changes you made, possibly breaking this feature.

Please go forward, I tested it, and it worked. But I have no good idea how to test that automatically.

Yes, it worked for you. You need to understand that we tested it too and we were very careful with the release since we refactored some important parts of the application. So now, I would like to make sure we are not introducing bugs with your PR.

Copy link
Member

@ggnaegi ggnaegi left a comment

Choose a reason for hiding this comment

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

Ok, you are right. We might need to add Content null for methods like HEAD or TRACE. But, I think you are going too far with a custom Http Content class for empty objects. I have written a lighter version:

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;

When creating the HttpContent object, we are setting the headers, this include the Content-Length header. So I would prefer using Headers.ContentLength instead of introducing a variable in HttpContent.

@@ -0,0 +1,15 @@
namespace Ocelot.Request.Mapper;

public class EmptyHttpContent : HttpContent
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 recommend using an existing content type instead. eg. ByteArrayContent

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?

@@ -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...


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?

@ggnaegi
Copy link
Member

ggnaegi commented Feb 26, 2024

@raman-m After a nightly review, I think we should push these changes in the January 24 Release. I have overseen scenarios that were covered by the ByteArrayContent implementation.

@alexreinert
Copy link
Contributor Author

alexreinert commented Feb 26, 2024

I used the EmptyHttpContent for two reasons: First is performance as both methods needs not to check the length of the content. Second, and important, the ByteArrayContent will add the Content-Length Header inside the constructor and later you add the header a second time.

@alexreinert
Copy link
Contributor Author

@alexreinert Ok, fine for the first part, we shouldn't have the headers if no content. And we should ensure that.

No, you have not and I have a wireshark dump, that proves that. If you set the Request.Content to a HttpContent, the framework will add headers when sending the request. Depending on TryComputeLength it will be Content-Length or Transfer-Encoding. To avoid these headers you have to set Request.Content to null.

Mmh, I was just writing that you were right on that part: We should ensure that, "should" meaning it's not the case yet.

Ok, sorry, was a misunderstanding by me.

About the 411 status, I need to investigate that a bit more. We need to support streaming. I'm a bit concerned about the changes you made, possibly breaking this feature.

Please go forward, I tested it, and it worked. But I have no good idea how to test that automatically.

Yes, it worked for you. You need to understand that we tested it too and we were very careful with the release since we refactored some important parts of the application. So now, I would like to make sure we are not introducing bugs with your PR.

Sure, I understand that. Maybe I have an idea for an automatic test, I will try that later.

@ggnaegi
Copy link
Member

ggnaegi commented Feb 26, 2024

I used the EmptyHttpContent for two reasons: First is performance as both methods needs not to check the length of the content. Second, and important, the ByteArrayContent will add ghe Content-Length Header inside the constructor and later you add the header a second time.

No, the ByteArrayContent shouldn't add the Content-Length Header. You need to set the value explicitely.

Don't understand me wrong, your Empty Content class is probably better, but it's a custom class, whereas ByteArrayContent is maintained by Microsoft.

@ggnaegi
Copy link
Member

ggnaegi commented Feb 26, 2024

@alexreinert Could you review the code and maybe push the changes later today, thanks!

@alexreinert
Copy link
Contributor Author

I added tests for streaming data. The idea of the tests is just to use that much data (100GB), that it would fail because of missing memory if it is not streamed.
I also did all changes requested by the review, except the EmptyContent and the use of a custom _content_length property, where I added comments above where needs to be decided, how to proceed.

@alexreinert
Copy link
Contributor Author

I used the EmptyHttpContent for two reasons: First is performance as both methods needs not to check the length of the content. Second, and important, the ByteArrayContent will add ghe Content-Length Header inside the constructor and later you add the header a second time.

No, the ByteArrayContent shouldn't add the Content-Length Header. You need to set the value explicitely.

Don't understand me wrong, your Empty Content class is probably better, but it's a custom class, whereas ByteArrayContent is maintained by Microsoft.

Ok, I changed it.

@raman-m raman-m added bug Identified as a potential bug Routing Ocelot feature: Routing Jan'24 January 2024 release labels Feb 26, 2024
@raman-m raman-m added this to the January'24 milestone Feb 26, 2024
@raman-m
Copy link
Member

raman-m commented Feb 26, 2024

@ggnaegi commented:

After a nightly review, I think we should push these changes in the January 24 Release. I have overseen scenarios that were covered by the ByteArrayContent implementation.

🆗 Added!

@raman-m
Copy link
Member

raman-m commented Feb 26, 2024

@alexreinert commented:

It is not just the iis. A GET request should not have content, but the Transfer-Encoding header indicates, that there is content (even if it is just zero bytes). For a DELETE request there must be no content, but the issue will send the Transfer-Encoding header there, too. So this could be also an issue with some WAF, which enforce the RFC rules.


GET request should not have content

We had the same debates 2 months ago. Our decision and compromise: don't change anything in request body. Ocelot should route the body as-is!


So this could be also an issue with some WAF, which enforce the RFC rules.

Let us know how does Ocelot changes these rules, or doesn't follow?
I believe you will see the same picture when connecting to your downstream service in the same IIS environment but without Ocelot. Or, am I wrong?

@raman-m
Copy link
Member

raman-m commented Feb 26, 2024

@alexreinert Do you have deployed Ocelot in any Production environments?

@alexreinert
Copy link
Contributor Author

@alexreinert Do you have deployed Ocelot in any Production environments?

Yes

@alexreinert
Copy link
Contributor Author

@alexreinert commented:

It is not just the iis. A GET request should not have content, but the Transfer-Encoding header indicates, that there is content (even if it is just zero bytes). For a DELETE request there must be no content, but the issue will send the Transfer-Encoding header there, too. So this could be also an issue with some WAF, which enforce the RFC rules.

GET request should not have content

We had the same debates 2 months ago. Our decision and compromise: don't change anything in request body. Ocelot should route the body as-is!

Same opinion on my side, it should be same. But with version 23 it is not the same. Neither if you send a request without content, nor when you send content with Content-Length Header, which will be "converted" to chunked encoding.

So this could be also an issue with some WAF, which enforce the RFC rules.

Let us know how does Ocelot changes these rules, or doesn't follow? I believe you will see the same picture when connecting to your downstream service in the same IIS environment but without Ocelot. Or, am I wrong?

Without Ocelot it works, with Ocelot 22 it works (but the request is changed and gets a Content-Lenght header), with Ocelot 23 it breaks because of the Transfer-Encoding Header Ocelot (or better say the non-null HttpContent in the HttpWebRequest) appends.

@ggnaegi
Copy link
Member

ggnaegi commented Feb 26, 2024

@alexreinert I will review the code in an hour or two. From your side could you deploy this version on an environment and give us a feedback? Thanks.

@alexreinert
Copy link
Contributor Author

@alexreinert I will review the code in an hour or two. From your side could you deploy this version on an environment and give us a feedback? Thanks.

I deployed it in our staging environment and I see no issues.

Copy link
Member

@ggnaegi ggnaegi left a comment

Choose a reason for hiding this comment

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

@alexreinert Maybe reduce the size to 1GB, I'm fine with the rest. We'll probably put the changes on some environments for a few days to validate them. Thanks, and you're welcome to contribute, we've got quite a few open issues. We're looking for someone who would like to propose a PR for SSE support. The new Release will be rolled out next week probably @raman-m yes?

@ggnaegi
Copy link
Member

ggnaegi commented Feb 26, 2024

@RaynaldM Maybe you could put the changes from this PR on a staging environment? Thanks!

@raman-m raman-m changed the title Avoid content if original request has no content and avoid Transfer-Encoding: chunked if Content-Length is known #1971 Avoid content if original request has no content and avoid Transfer-Encoding: chunked if Content-Length is known Feb 28, 2024
@raman-m
Copy link
Member

raman-m commented Feb 28, 2024

ggnaegi: The new Release will be rolled out next week probably @raman-m yes?
ggnaegi: @RaynaldM Maybe you could put the changes from this PR on a staging environment? Thanks!

Well... It depends on the progress which is ~50% today.
Guys, I'm not sure we are ready to deliver this fix right in the current release...
Time is marching! Jan'24 is small monthly release, and I cannot wait for a months! I want to release till Friday, March 1.

Please, keep me updated about the testing in your Production env please

@ggnaegi Can I start final review? I guess PR is ready for final code review...

@ggnaegi
Copy link
Member

ggnaegi commented Feb 28, 2024

@raman-m commented:

@ggnaegi Can I start final review? I guess PR is ready for final code review...

@raman-m yes, code is ready for final review.

Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

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

It is hard to review the design here, as for me.
But my suggestions are below! 👇

Question: Do we support Chunked Encoding now? If Yes, we have to update docs in the "Not Supported" chapter.
I see acceptance tests in MapRequestTests class which seems check exactly the case of Chunked Encoding...

src/Ocelot/Request/Mapper/RequestMapper.cs Show resolved Hide resolved
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.

@ggnaegi Gui, is it resolved?


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.

@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?

test/Ocelot.AcceptanceTests/MapRequestTests.cs Outdated Show resolved Hide resolved
test/Ocelot.AcceptanceTests/MapRequestTests.cs Outdated Show resolved Hide resolved
@raman-m raman-m requested review from RaynaldM and ggnaegi and removed request for ggnaegi February 28, 2024 08:42
@alexreinert
Copy link
Contributor Author

Question: Do we support Chunked Encoding now? If Yes, we have to update docs in the "Not Supported" chapter. I see acceptance tests in MapRequestTests class which seems check exactly the case of Chunked Encoding...

Fixed length content and chunked encoding is supported, depending how the request by the client looks like. And for both ways streaming the content data works.

@alexreinert
Copy link
Contributor Author

Feel free to change the PR, I will quit here. I have not the time to push it any further.

@raman-m
Copy link
Member

raman-m commented Feb 28, 2024

@alexreinert: Feel free to change the PR, I will quit here.

Got it! Thanks for this PR! We arrange PR delivery ourselves.
We expect other interesting PRs from you! 😉

@raman-m raman-m self-assigned this Feb 29, 2024
@raman-m raman-m force-pushed the avoid_transfer_encoding_if_possible branch from 4e1a10b to e30ca4d Compare February 29, 2024 11:03
@ggnaegi
Copy link
Member

ggnaegi commented Feb 29, 2024

@raman-m I'm going to work on this, this afternoon.

Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

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

Code review Round 2


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"

{
HttpContent content;

// no content if we have no body or if the request has no content according to RFC 2616 section 4.3
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 2
A few months ago I proposed to the team to develop the feature which will enforce RFC policies regarding request bodies for GET, HEAD, DELETE verbs... But you and @RaynaldM declined it...
This is the second notification issue from our community!
I believe it is time to develop this feature (introduce empty bodies for special types of HTTP verbs). We can open discussion thread or issue. At least I would like to add TODO...

Comment on lines +33 to +34
if (request.Body == null
|| (!request.ContentLength.HasValue && StringValues.IsNullOrEmpty(request.Headers.TransferEncoding)))
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

Comment on lines +39 to +41
content = request.ContentLength is 0
? new ByteArrayContent(Array.Empty<byte>())
: new StreamHttpContent(request.HttpContext);
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

@@ -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

Choose a reason for hiding this comment

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

And...

test/Ocelot.AcceptanceTests/MapRequestTests.cs Outdated Show resolved Hide resolved
test/Ocelot.AcceptanceTests/ServiceHandler.cs Outdated Show resolved Hide resolved
test/Ocelot.AcceptanceTests/StreamContentTest.cs Outdated Show resolved Hide resolved
test/Ocelot.AcceptanceTests/StreamContentTest.cs Outdated Show resolved Hide resolved
test/Ocelot.UnitTests/Request/Mapper/RequestMapperTests.cs Outdated Show resolved Hide resolved
Copy link
Member

@raman-m raman-m left a comment

Choose a reason for hiding this comment

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

Ready for delivery! ✔️

@ggnaegi But the 2nd follow-up PR is required with your design review, please!
Need your approval!

@raman-m raman-m changed the title #1971 Avoid content if original request has no content and avoid Transfer-Encoding: chunked if Content-Length is known #1971 #928 Avoid content if original request has no content and avoid Transfer-Encoding: chunked if Content-Length is known Feb 29, 2024
@raman-m
Copy link
Member

raman-m commented Feb 29, 2024

@ggnaegi I didn't link #928 in this PR because we have to check everything in the next your PR... So, we will link #928 to your PR only, to close officially.

Copy link
Member

@ggnaegi ggnaegi left a comment

Choose a reason for hiding this comment

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

@raman-m Ok for me, as discussed, I will create a new PR after that.

@raman-m raman-m merged commit 319e397 into ThreeMammals:develop Feb 29, 2024
2 checks passed
@raman-m raman-m mentioned this pull request Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Identified as a potential bug Jan'24 January 2024 release Routing Ocelot feature: Routing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants