You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2020/07/17 09:20:39 UTC

[GitHub] [incubator-apisix] membphis opened a new pull request #1863: bugfix: read the request body from the temporary file if it was cached.

membphis opened a new pull request #1863:
URL: https://github.com/apache/incubator-apisix/pull/1863


   ### What this PR does / why we need it:
   
   as title, fixed #1784
   
   ### Pre-submission checklist:
   
   * [ ] Did you explain what problem does this PR solve? Or what new features have been added?
   * [ ] Have you added corresponding test cases?
   * [ ] Have you modified the corresponding document?
   * [ ] Is this PR backward compatible?
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-apisix] spacewander commented on a change in pull request #1863: bugfix: read the request body from the temporary file if it was cached.

Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #1863:
URL: https://github.com/apache/incubator-apisix/pull/1863#discussion_r457186013



##########
File path: apisix/admin/init.lua
##########
@@ -117,10 +117,13 @@ local function run()
         core.response.exit(404)
     end
 
-    ngx.req.read_body()
-    local req_body = ngx.req.get_body_data()
-
+    local req_body = core.request.get_body()
     if req_body then
+        if #req_body > 1024 * 1024 * 1.5 then
+            core.log.error("maximum request body is 1.5mb, but got ", #req_body)

Review comment:
       IMHO, we can use MiB to indicate the size is 1024 base.

##########
File path: apisix/core/request.lua
##########
@@ -94,7 +99,36 @@ function _M.get_remote_client_port(ctx)
         ctx = ngx.ctx.api_ctx
     end
     return tonumber(ctx.var.remote_port)
-  end
+end
+
+
+local function get_file(file_name)
+    local f = io_open(file_name, 'r')

Review comment:
       Better to check the second error returned value before using the `f`.

##########
File path: apisix/core/request.lua
##########
@@ -94,7 +99,36 @@ function _M.get_remote_client_port(ctx)
         ctx = ngx.ctx.api_ctx
     end
     return tonumber(ctx.var.remote_port)
-  end
+end
+
+
+local function get_file(file_name)
+    local f = io_open(file_name, 'r')
+    if f then
+        local req_body = f:read("*all")

Review comment:
       Ditto

##########
File path: apisix/admin/init.lua
##########
@@ -117,10 +117,13 @@ local function run()
         core.response.exit(404)
     end
 
-    ngx.req.read_body()
-    local req_body = ngx.req.get_body_data()
-
+    local req_body = core.request.get_body()
     if req_body then
+        if #req_body > 1024 * 1024 * 1.5 then

Review comment:
       We need to detect the size (use `stat`, for example) before reading it. It doesn't make sense to do the check if we have already read the very BIG file.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-apisix] membphis commented on pull request #1863: bugfix: read the request body from the temporary file if it was cached.

Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #1863:
URL: https://github.com/apache/incubator-apisix/pull/1863#issuecomment-660813356


   https://github.com/apache/incubator-apisix/pull/1863/files#diff-00625723b6e737f3cdb18af67165b70fR1899-R1901
   
   @moonming in this test case, we found two bugs.
   
   1. big request body for admin API
   2. slower to detect whether the objects in the test array are unique (jsonschema).
   
   ![image](https://user-images.githubusercontent.com/6814606/87903209-0f5ad800-ca8e-11ea-9c5a-72d5226bc88b.png)
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-apisix] membphis merged pull request #1863: bugfix: read the request body from the temporary file if it was cached.

Posted by GitBox <gi...@apache.org>.
membphis merged pull request #1863:
URL: https://github.com/apache/incubator-apisix/pull/1863


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-apisix] moonming commented on a change in pull request #1863: bugfix: read the request body from the temporary file if it was cached.

Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #1863:
URL: https://github.com/apache/incubator-apisix/pull/1863#discussion_r458640687



##########
File path: apisix/admin/init.lua
##########
@@ -117,10 +117,13 @@ local function run()
         core.response.exit(404)
     end
 
-    ngx.req.read_body()
-    local req_body = ngx.req.get_body_data()
-
+    local req_body = core.request.get_body()
     if req_body then
+        if #req_body > 1024 * 1024 * 1.5 then

Review comment:
       agreed.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-apisix] moonming commented on a change in pull request #1863: bugfix: read the request body from the temporary file if it was cached.

Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #1863:
URL: https://github.com/apache/incubator-apisix/pull/1863#discussion_r457062357



##########
File path: rockspec/apisix-master-0.rockspec
##########
@@ -47,7 +47,7 @@ dependencies = {
     "luafilesystem = 1.7.0-2",
     "lua-tinyyaml = 0.1",
     "lua-resty-prometheus = 1.1",
-    "jsonschema = 0.8",
+    "jsonschema = 0.9",

Review comment:
       should not modify this line

##########
File path: t/admin/health-check.t
##########
@@ -288,7 +288,7 @@ GET /t
 GET /t
 --- error_code: 400
 --- response_body
-{"error_msg":"invalid configuration: property \"upstream\" validation failed: property \"checks\" validation failed: property \"active\" validation failed: property \"healthy\" validation failed: property \"http_statuses\" validation failed: expected unique items but items 2 and 1 are equal"}
+{"error_msg":"invalid configuration: property \"upstream\" validation failed: property \"checks\" validation failed: property \"active\" validation failed: property \"healthy\" validation failed: property \"http_statuses\" validation failed: expected unique items but items 1 and 2 are equal"}

Review comment:
       Why did you change this test case?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [incubator-apisix] moonming commented on pull request #1863: bugfix: read the request body from the temporary file if it was cached.

Posted by GitBox <gi...@apache.org>.
moonming commented on pull request #1863:
URL: https://github.com/apache/incubator-apisix/pull/1863#issuecomment-660816853


   one PR only do one think.
   
   Thanks,
   Ming Wen
   Twitter: _WenMing
   
   
   YuanSheng Wang <no...@github.com> 于2020年7月20日周一 下午1:37写道：
   
   >
   > https://github.com/apache/incubator-apisix/pull/1863/files#diff-00625723b6e737f3cdb18af67165b70fR1899-R1901
   >
   > @moonming <https://github.com/moonming> in this test case, we found two
   > bugs.
   >
   >    1. big request body for admin API
   >    2. slower to detect whether the objects in the test array are unique
   >    (jsonschema).
   >
   > [image: image]
   > <https://user-images.githubusercontent.com/6814606/87903209-0f5ad800-ca8e-11ea-9c5a-72d5226bc88b.png>
   >
   > —
   > You are receiving this because you were mentioned.
   > Reply to this email directly, view it on GitHub
   > <https://github.com/apache/incubator-apisix/pull/1863#issuecomment-660813356>,
   > or unsubscribe
   > <https://github.com/notifications/unsubscribe-auth/AGJZBK6ZDCBMC7X6NUD4EK3R4PJZJANCNFSM4O54VNRQ>
   > .
   >
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org