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

fix(1902): correct trailer headers placement #650

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from
Draft

fix(1902): correct trailer headers placement #650

wants to merge 15 commits into from
+917 −73

Conversation

kingluo
Copy link
Contributor

@kingluo kingluo commented Jul 17, 2024 •
edited
Loading

@kingluo kingluo requested a review from RomanBelozerov July 17, 2024 07:35
@EvgeniiMekhanik EvgeniiMekhanik force-pushed the jinhua/fix-1902-trailers-placement branch from 456ef6e to 11ba65c Compare August 19, 2024 12:58
@EvgeniiMekhanik EvgeniiMekhanik force-pushed the jinhua/fix-1902-trailers-placement branch from bf820f2 to 743de58 Compare August 22, 2024 09:26
@EvgeniiMekhanik EvgeniiMekhanik force-pushed the jinhua/fix-1902-trailers-placement branch 8 times, most recently from 7b57b03 to 5116147 Compare September 13, 2024 10:33
RomanBelozerov
RomanBelozerov requested changes Sep 16, 2024
View reviewed changes
framework/deproxy_client.py Outdated
@@ -416,6 +420,12 @@ class ReqBodyBuffer:


class DeproxyClientH2(BaseDeproxyClient):
_ping_received = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable should add to _reinit_variables method and remove here. We must to reset the counter when we use start method

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes fixed

framework/deproxy_auto_parser.py Outdated Show resolved Hide resolved
framework/deproxy_auto_parser.py Show resolved Hide resolved
helpers/deproxy.py Outdated
@@ -439,9 +439,19 @@ def read_chunked_body(self, stream):
# Parsing trailer will eat last CRLF
self.parse_trailer(stream)

def convert_chunked_body(self):
def convert_chunked_body(self, http2, trailers, method_is_head):
Copy link
Contributor

Choose a reason for hiding this comment

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

you can get value for trailers using self:

trailers = len(self.trailer.headers)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes fixed, unnecessary variable

helpers/deproxy.py Outdated Show resolved Hide resolved
http2_general/test_h2_headers.py Outdated Show resolved Hide resolved
http2_general/test_h2_headers.py
@parameterize.expand(
[
param(
name="empty_body",
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we need these tests for http1?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes fixed

http2_general/test_h2_headers.py Outdated Show resolved Hide resolved
http2_general/test_h2_frame.py Outdated
Comment on lines 866 to 870
util.wait_until(
lambda: client.ping_received != ping_count,
5,
abort_cond=lambda: client.state != stateful.STATE_STARTED,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add the new wait_for_* method to DeproxyClientH2

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes fixed

cache/test_cache.py Outdated Show resolved Hide resolved
@EvgeniiMekhanik EvgeniiMekhanik marked this pull request as draft September 27, 2024 07:33
@EvgeniiMekhanik EvgeniiMekhanik force-pushed the jinhua/fix-1902-trailers-placement branch 4 times, most recently from 79ae44e to 2afef56 Compare October 1, 2024 10:40
@EvgeniiMekhanik EvgeniiMekhanik force-pushed the jinhua/fix-1902-trailers-placement branch 10 times, most recently from 01aa410 to 3f9274c Compare December 24, 2024 03:29
@EvgeniiMekhanik EvgeniiMekhanik force-pushed the jinhua/fix-1902-trailers-placement branch from 3f9274c to dba1ef7 Compare December 27, 2024 14:07
@EvgeniiMekhanik EvgeniiMekhanik force-pushed the jinhua/fix-1902-trailers-placement branch 5 times, most recently from e4c9814 to 846efba Compare December 28, 2024 02:07
kingluo and others added 12 commits January 3, 2025 01:29
@kingluo @EvgeniiMekhanik
fix(1902): correct trailer headers placement
3fa1581
@EvgeniiMekhanik
Implement new test to check flow control.
6fc6a37 
Check how Tempesta FW calculate http2
connection and stream window when response
contains trailers
@EvgeniiMekhanik
Add tests for long trailers
222e15b
@EvgeniiMekhanik
Add cache test for trailers
20bb693
@EvgeniiMekhanik
Add more tests to check trailers in responses
c2424cc
@EvgeniiMekhanik
Add tests for long body and trailers
45e1d56
@EvgeniiMekhanik
Fix tests according changes in Tempesta FW
89a74c9
@EvgeniiMekhanik
Fix according review
9fec36b
@EvgeniiMekhanik
Implement more tests for trailers
559ead1 
Implement tests to check how Tempesta FW satisfy
http2 requests from cache if response is for http1
request with trailers and vise versa.
@EvgeniiMekhanik
Implement tests to check hob by hope headers in trailers
b7f19e7 
If response or request contains trailers with hop by hop
headers, Tempesta FW should remove this headers. Also
Tempesta FW should remove hop by bop headers from
'Trailer' header.
@EvgeniiMekhanik
Several fixes in framework
8cef40b 
- Remove hop by hop headers from trailers
- Remove hop by jop headers from 'Trailer' header.
- Do not add last "\r\n" to the chunked body, if trailers
  doesn't exist, it break deproxy autoparser.
@EvgeniiMekhanik
Remove content-length header for http2 responses.
2d9c0e6
@EvgeniiMekhanik EvgeniiMekhanik force-pushed the jinhua/fix-1902-trailers-placement branch 2 times, most recently from 76c3471 to 8b02af5 Compare January 8, 2025 16:39
@EvgeniiMekhanik
Fix after discussion.
bb581e7 
- Remove 'Trailer' header for http1 and http2 requests/response.
- Drop 'http1' requests/responses if trailer headers  contain
  hop by hop headers.
@EvgeniiMekhanik EvgeniiMekhanik force-pushed the jinhua/fix-1902-trailers-placement branch from 8b02af5 to bb581e7 Compare January 8, 2025 16:51
@EvgeniiMekhanik
Add extra checks for hop-by-hop headers in trailers.
9acfd79 
Check that we drop requests (http1 and http2) if there is
a hop-by-hop headers in trailers (check all headers from
the list RFC 9110 7.6.1).
Also disable some tests on TCP segmentation.
@EvgeniiMekhanik EvgeniiMekhanik force-pushed the jinhua/fix-1902-trailers-placement branch 4 times, most recently from 47f52b0 to 630654e Compare January 14, 2025 17:41
@EvgeniiMekhanik EvgeniiMekhanik force-pushed the jinhua/fix-1902-trailers-placement branch from 630654e to ba2decf Compare January 14, 2025 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EvgeniiMekhanik EvgeniiMekhanik EvgeniiMekhanik left review comments

@RomanBelozerov RomanBelozerov Awaiting requested review from RomanBelozerov

@const-t const-t Awaiting requested review from const-t

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kingluo @RomanBelozerov @EvgeniiMekhanik