-
Notifications
You must be signed in to change notification settings - Fork 171
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
[THREESCALE-9542] Part 1: buffering policy #1408
[THREESCALE-9542] Part 1: buffering policy #1408
Conversation
79fc3ac
to
8367dce
Compare
8367dce
to
79c1b87
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking very good.
Left some comments
79c1b87
to
be4c520
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks great.
Two updates missing:
- Entry in the changelog
- README.md of the policy with brief description and maybe configuration example like
{
"name": "request_unbuffered",
"version": "builtin",
"configuration": {}
},
--- more_headers | ||
Transfer-Encoding: chunked | ||
--- request eval | ||
$::data = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added README and updated CHANGELOG
Verification steps 👍 🆗
First add diff --git a/dev-environments/plain-http-upstream/apicast-config.json b/dev-environments/plain-http-upstream/apicast-config.json
index 30c2db39..4dbea5f8 100644
--- a/dev-environments/plain-http-upstream/apicast-config.json
+++ b/dev-environments/plain-http-upstream/apicast-config.json
@@ -38,6 +38,11 @@
"host": "backend"
},
"policy_chain": [
+ {
+ "name": "request_unbuffered",
+ "version": "builtin",
+ "configuration": {}
+ },
{
"name": "apicast.policy.apicast"
} The run the gateway with the built image
That request should return
Note that upstream echo API is reporting that the request included
❯ cat <<EOF >chunked-request.py
import http.client
import time
def gen():
yield bytes('hi', "utf-8")
time.sleep(2)
yield bytes('there', "utf-8")
time.sleep(2)
yield bytes('bye', "utf-8")
conn = http.client.HTTPConnection('127.0.0.1', 8080)
headers = {'Content-type': 'application/octet-stream', 'Host': 'post.example.com'}
conn.request('POST', '/?user_key=foo', gen(), headers)
response = conn.getresponse()
print(response.read().decode())
EOF
Note that the upstream service got transfer encoding chunked request. Traffic between the apicast gateway and upstream can be inspected looking at logs from
Note the chunked encoding of the request with the length bytes preceding each chunk. |
95d212e
to
53b248d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
It will be ready to be merged, once all test pass
Some (prove) tests are failing, even locally. Let me know if you need some help to fix them. |
53b248d
to
84e40fa
Compare
It should be good now. I don't have permission to rerun the test, can you please help re-trigger the prove test for me? |
@eguzki I looked at those failed tests Both failed because I used liquid to include the shared file, but they used the old test method so the configuration file could not be expanded. 2 ways to fix this:
we need to move those two to Blackbox anyway, but let me know what you think. |
Another good reason to move those to backbox. Ok, let's merge those two first, and then rebase this PR on top of them. |
This commit extract @upstream config to a sepearate file so we can re-use the generic config for different upstream block by including the upstream_shared.conf file
This policy allow user to disable request buffering. With this change, the upstream location is changed based on the value provided in the context.
84e40fa
to
5bda7fc
Compare
codecov failed 🙄 still good to merge? |
it is already approved. you requested a new review? |
Ah sorry, I just wanted to ask if I can merge the PR even if the codecov fails, or all the tests MUST pass before I can merge. |
No hard rule here. It is true that the coverage dropped more than 3%. That was the threshold we specified https://github.com/3scale/APIcast/blob/master/.codecov.yml#L10-L19 You implemented tests. So, we can agree on "bypassing" exceptionally this time. LGTM |
What
As part of https://issues.redhat.com/browse/THREESCALE-9542, this PR adds a new buffering policy which enable users to disable request buffering for use cases which call for it (namely, large payloads using HTTP 1.1 chunked encoding).
(Note: currently only support use case with no proxy between APIcast and upstream).
By default that the request buffer is true (which is the same as before). Users can now turn the request buffering off per service.
When request buffering is disabled, nginx will forward request body to upstream ASAP.
Verification steps
The upstream return
200
withContent-Length
headerThis time we can see that upstream return
200
with"transfer-encoding": "chunked"
header