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

fix: Adjust POST headers and review Transfer-Encoding handling in forward-auth #11023

Closed
wants to merge 0 commits into from
Closed

Conversation

motongxue
Copy link
Contributor

…ward-auth

Description

This is regarding the problem mentioned in Case 2 of issue #10927. I addressed the issue by retrieving the maximum body size using ngx.var.client_max_body_size and then comparing it with the Content-Length.

As a newcomer to APISIX, I have drafted the logic for the test case as I think it should be, but I would appreciate assistance in reviewing the correctness of the test case writing.

Fixes # (issue)

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@shreemaan-abhishek
Copy link
Contributor

@motongxue

  1. the CI is failing.
  2. I think it's okay to use client_body_reader by default as there are no negative implications. So just ensure to cache the req_body in a separate variable. Later, restore the request body from the variable.

@@ -488,3 +488,127 @@ Authorization: 333
}
}
--- error_code: 200

Copy link
Contributor

Choose a reason for hiding this comment

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

Three blanks are needed between test cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Three blanks are needed between test cases

Ok, I will fix that.

@motongxue
Copy link
Contributor Author

@motongxue

  1. the CI is failing.
  2. I think it's okay to use client_body_reader by default as there are no negative implications. So just ensure to cache the req_body in a separate variable. Later, restore the request body from the variable.

@motongxue

  1. the CI is failing.
  2. I think it's okay to use client_body_reader by default as there are no negative implications. So just ensure to cache the req_body in a separate variable. Later, restore the request body from the variable.

@shreemaan-abhishek
Hello abhishek, the CI has failed because the test cases have not been fixed yet.

Using client_body_reader by default is a more efficient method for handling large request bodies, as it prevents the entire large request body from being loaded into memory all at once. However, if the majority of the request bodies are small, frequently using streaming read can lead to unnecessary performance overheads, because each read operation requires time and resources, even if the data volume is small.

Therefore, a more balanced approach might be to select the appropriate reading strategy based on the anticipated size and usage of the request body. For example, a threshold could be set: use core.request.get_body() for request bodies smaller than this threshold, and use httpc:get_client_body_reader() for request bodies larger than the threshold.

@shreemaan-abhishek
Copy link
Contributor

@motongxue okay, it makes sense to me now.

@shreemaan-abhishek
Copy link
Contributor

@motongxue test cases are failing.

@motongxue
Copy link
Contributor Author

I will update soon.

@motongxue motongxue closed this Mar 22, 2024
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.

3 participants