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

[alexreinert]

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

• 928

[raman-m]

Thanks for PR recreation following our development process!

[alexreinert]

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

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

[ggnaegi]

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

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]

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

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]

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

[alexreinert]

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]

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]

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

:ok: Added!

[raman-m]

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

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

[alexreinert]

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

Yes

[alexreinert]

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

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

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

[ggnaegi]

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

[raman-m]

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]

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

[alexreinert]

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]

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

[raman-m]

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

[ggnaegi]

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

[raman-m]

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