Skip to content

Conversation

dio
Copy link
Member

@dio dio commented Nov 4, 2019

Description:
This patch fixes the missing body in CheckRequest when the with_request_body is enabled and the downstream protocol is HTTP/2.

Risk Level: Low
Testing: integration-test
Docs Changes: N/A
Release Notes: N/A
Fixes #8666

Signed-off-by: Dhi Aurrahman dio@tetrate.io

dio added 3 commits November 4, 2019 22:43
This patch fixes missing body in CheckRequest when the with_request_body
is enabled and the downstream protocol is HTTP/2.

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio dio changed the title Fix 8666 ext_authz: Fix missing body in CheckRequest for HTTP/2 Nov 4, 2019
@dio dio marked this pull request as ready for review November 4, 2019 17:30
@dio dio requested a review from gsagula November 4, 2019 17:30
@dio
Copy link
Member Author

dio commented Nov 5, 2019

/azp run envoy-linux

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

dio added 2 commits November 5, 2019 14:45
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
gsagula
gsagula previously approved these changes Nov 8, 2019
Copy link
Member

@gsagula gsagula left a comment

Choose a reason for hiding this comment

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

It looks good to me. Thanks for adding the integration test!

uint64_t request_size) {
initializeWithDownstreamProtocol(downstream_protocol);
initiateClientConnection(request_size);
waitForExtAuthzRequest(fmt::format(R"EOF(
Copy link
Member

Choose a reason for hiding this comment

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

@dio The integration tests are great but is there a way that we can make this part a bit more readable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. Updated.

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
Copy link
Member Author

dio commented Nov 8, 2019

@zuercher could you help to review this? Thank you!

Copy link
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

Thanks for working out the integration tests. I have one question about how this works in cases where there are multiple decodeData calls.

dio added 6 commits November 12, 2019 12:04
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
@dio
Copy link
Member Author

dio commented Nov 13, 2019

@zuercher do we need more changes?

Copy link
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

Looks good to me!

mayflower-markus pushed a commit to mayflower-markus/envoy that referenced this pull request Dec 2, 2019
This patch fixes the missing body in `CheckRequest` when the `with_request_body` is enabled and the downstream protocol is HTTP/2.

Fixes envoyproxy#8666 

Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ext_authz does not send Body in CheckRequest with HTTP2
3 participants