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

[alexreinert]

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.

[raman-m]

@ggnaegi Please, review!

[raman-m]

@alexreinert Hi Alexander!
Welcome to Ocelot world! 🐯

And thank you for the bug reporting!

In my opinion, this user scenario is quite complex, but it definitely needs to be tested.
And we need to be careful with updating the system kernel. I expect and want to cover this user scenario with tests for thorough testing.

[ggnaegi]

@ggnaegi Please, review!

@raman-m didn't expect that, but yeah our friend iis.

[alexreinert]

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.

[ggnaegi]

@alexreinert Yes, unfortunately the waf we are using didn't complain about this and a colleague of us has put the feature on production for a good month without any issues. But it's probably a case for a hot fix, you're right. Could you maybe add a few test cases or give me the permissions on your branch? Thanks.

[alexreinert]

I try to add some test cases tomorrow.

[alexreinert]

@TomPallister You changed my fork, was this intended?

[alexreinert]

I added some tests. The unit tests can not cover the whole story as the Transfer-Encoding is not applied until real sending the content, therefor I added some acceptence tests to cover it.

[ggnaegi]

I added some tests. The unit tests can not cover the whole story as the Transfer-Encoding is not applied until real sending the content, therefor I added some acceptence tests to cover it.

@alexreinert Ok, I will review the code. Are you sure that returning null when no Content isn't sufficent? It would do the trick, and .net wouldn't add the content headers.

You have made some changes that enforce the content size in the new HttpContent class where we are using the Headers property. We could think that this value is never set, but that's not the case.

I can understand that, then you will not have TryComputeLength returning false when size is known and the content-encoding: "chunked" header will not be set.

And finally you're adding the content-encoding to the the unsupported headers.

Could you explain why? Thanks

[alexreinert]

Are you sure that returning null when no Content isn't sufficent? It would do the trick, and .net wouldn't add the content headers.

This is exactly what I do: I return null, if there is no content in the incoming request. But I changed the detection of content. Request.Body will be not null, even if there is no content. So I used as detection what is stated in RFC 2616 (HTTP 1.1) as detection mechanism.

You have made some changes that enforce the content size in the new HttpContent class where we are using the Headers property. We could think that this value is never set, but that's not the case.

I can understand that, then you will not have TryComputeLength returning false when size is known and the content-encoding: "chunked" header will not be set.

In my tests, the content will be chunked and the Transfer-Encoding header is added, if TryComputeLength return false. In that case, the Content-Length header will be removed, too. (Having both Content-Length and Transfer-Encoding headers is strictly forbidden and .net ensures that)

Of course,it would be possible to just send the content chunked without Content-Length and this would work in most cases, but some backends could respond with status "411 content length required".

And finally you're adding the content-encoding to the the unsupported headers.

No, I added Transfer-Encoding to the unsupported headers, as this header should be used between two systems and not end to end like Content-Encoding (that is stated in RFC2616). And if you think of Transfer-Encoding: gzip or deflate: This Header needs to be removed as reading the Body stream of the request will implicitly unzip the content and send uncompressed to the backend server (but chunked).

[raman-m]

@alexreinert commented:

You changed my fork, was this intended?

Yes, it was! I've asked him to do that.
The problem: you use develop branch in your fork as feature branch. That's wrong, because develop branch is default protected branch in your fork which purpose to be synced to head repo.
You need to create feature branch from develop one, and commit to feature branch only!
And your develop branch must remain for sync operations only!

According to GitHub Forking policy head repo owner has a right to apply limited set of operations in forked repo. One of them is notification to forked repo admin about changes in the head repo protected branch that top commits are changed.
But head repo admin can't administer forked repo for sure.
Head repo Admin and Maintainers have a right to make force push to any feature branches of the forked repo, but not to protected branches.
Do you understand GitHub forking process better now?

[ggnaegi]

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

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.

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

[raman-m]

@alexreinert
We have incidentally wrong diff now in your develop. To fix that please do the following:

• Create new feature branch from develop especially from this commit alexreinert@108bede !
• Cherry pick all your feature commits from develop
• Perform Sync Fork operation to make your develop identical to head repo one!!!
• Close current PR
• Create new PR based on new feature branch

Thanks!

@ggnaegi This PR will be closed!
Seems we should not accept PRs based on protected branches of forked repos...

[alexreinert]

Follow-up in #1972