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.

Sign up for GitHub

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

Jump to bottom

Content-Length header addition #2209

Open
const-t opened this issue Aug 12, 2024 · 1 comment · May be fixed by #2243
Open

Content-Length header addition #2209

const-t opened this issue Aug 12, 2024 · 1 comment · May be fixed by #2243
Assignees
@EvgeniiMekhanik
Labels
enhancement good to start Start form this tasks if you're new in Tempesta FW
Milestone
0.8 - Beta

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.
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
@ai-tmpst
Do not add Content-Length header for HTTP2 responses (#2209)
4a790f8 
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 <[email protected]>
@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
@krizhanovsky krizhanovsky linked a pull request Oct 29, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@EvgeniiMekhanik EvgeniiMekhanik

Labels
enhancement good to start Start form this tasks if you're new in Tempesta FW
Projects
None yet
Milestone
0.8 - Beta
Development

Successfully merging a pull request may close this issue.

Do not add Content-Length header for HTTP2 responses (#2209) tempesta-tech/tempesta
4 participants
@krizhanovsky @const-t @EvgeniiMekhanik @ai-tmpst