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

Content-Length header addition #2209

Open
const-t opened this issue Aug 12, 2024 · 4 comments · Fixed by #2243
Open

Content-Length header addition #2209

const-t opened this issue Aug 12, 2024 · 4 comments · Fixed by #2243
Assignees
Labels
enhancement good to start Start form this tasks if you're new in Tempesta FW
Milestone

Comments

@const-t
Copy link
Contributor

const-t commented Aug 12, 2024

We have some not optimal behavior. In case when Content-Length not listed in response headers and upstream server indicates message finishing by closing connection in function tfw_http_resp_terminate() we add Content-Length header.
Upd: The first part of this task about not optimal behavior was done in PR2243.

But for http2 protocol we call tfw_h2_msg_cutoff_headers() and after HPack encode headers, it implies we must not add the header in tfw_http_resp_terminate() but add this header during http2 message adjusting.

@const-t const-t added enhancement good to start Start form this tasks if you're new in Tempesta FW labels Aug 12, 2024
@const-t const-t added this to the 0.9 - LA milestone Aug 12, 2024
ai-tmpst added a commit that referenced this issue Sep 17, 2024
The Content-Length header should not be added for HTTP2 responses in
tfw_http_resp_terminate(), as it is ultimately removed by the
tfw_h2_msg_cutoff_headers() function. Skip the Content-Length header
addition for HTTP2 clients to avoid unnecessary operations.

Signed-off-by: Alexander Ivanov <ai@tempesta-tech.com>
@ai-tmpst ai-tmpst self-assigned this Sep 17, 2024
@ai-tmpst
Copy link
Contributor

It looks we can just skip Content-Length addition for HTTP2 in tfw_http_resp_terminate().

As for addition this header field during http2 message adjusting, we add this field in tfw_h2_resp_encode_headers() only for chunked responses:

	if (test_bit(TFW_HTTP_B_CHUNKED, resp->flags)) {
		if (unlikely(tfw_h2_add_hdr_clen(resp)))
			goto clean;
	}

I'm not sure what behavior is desired. RFC 3119 says nothing about content-length usage. It's a completely optional field. This issue looks independent, and we could discuss it separately.

@krizhanovsky krizhanovsky modified the milestones: 0.9 - LA, 0.8 - Beta Oct 27, 2024
@krizhanovsky krizhanovsky reopened this Oct 29, 2024
ai-tmpst added a commit that referenced this issue Nov 15, 2024
The Content-Length header should not be added for HTTP2 responses in
tfw_http_resp_terminate(), as it is ultimately removed by the
tfw_h2_msg_cutoff_headers() function. Skip the Content-Length header
addition for HTTP2 clients to avoid unnecessary operations.

Signed-off-by: Alexander Ivanov <ai@tempesta-tech.com>
@ai-tmpst
Copy link
Contributor

Also there is PR726 in tempesta-test

@ai-tmpst
Copy link
Contributor

ai-tmpst commented Nov 15, 2024

In PR2243 we partly removed Content-length header for HTTP2. In the future we need to remove the rest of the places where this header is added. Also we need to correct tests (look for __prepare_chunked_expected_response function in framework/deproxy_auto_parser.py).

The first part of this task about not optimal behavior was done in PR2243.

@krizhanovsky krizhanovsky modified the milestones: 0.8 - Beta, 0.9 - LA Nov 18, 2024
ai-tmpst added a commit that referenced this issue Nov 18, 2024
The Content-Length header should not be added for HTTP2 responses in
tfw_http_resp_terminate(), as it is ultimately removed by the
tfw_h2_msg_cutoff_headers() function. Skip the Content-Length header
addition for HTTP2 clients to avoid unnecessary operations.

Signed-off-by: Alexander Ivanov <ai@tempesta-tech.com>
krizhanovsky added a commit that referenced this issue Nov 20, 2024
Do not add Content-Length header for HTTP2 responses (#2209)
@krizhanovsky
Copy link
Contributor

#2243 doesn't fully fix the problem

@krizhanovsky krizhanovsky reopened this Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement good to start Start form this tasks if you're new in Tempesta FW
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants