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/09/03 02:11:13 UTC
[GitHub] [apisix] gy09535 opened a new pull request #2121: bugfix: updated the value of remaining when `count` changes in limit-count plugin.
gy09535 opened a new pull request #2121:
URL: https://github.com/apache/apisix/pull/2121
### What this PR does / why we need it:
fix https://github.com/apache/apisix/issues/2098
### Pre-submission checklist:
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [x] Have you added corresponding test cases?
* [ ] Have you modified the corresponding document?
* [x] 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] [apisix] membphis commented on a change in pull request #2121: fix: limit count config not effective
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2121:
URL: https://github.com/apache/apisix/pull/2121#discussion_r479862432
##########
File path: apisix/plugin.lua
##########
@@ -297,6 +297,7 @@ local function merge_service_route(service_conf, route_conf)
local new_conf = core.table.deepcopy(service_conf)
new_conf.value.service_id = new_conf.value.id
new_conf.value.id = route_conf.value.id
+ new_conf.modifiedIndex=route_conf.modifiedIndex
Review comment:
`new_conf.modifiedIndex=route_conf.modifiedIndex`
->
`new_conf.modifiedIndex = route_conf.modifiedIndex`
##########
File path: apisix/plugin.lua
##########
@@ -332,7 +333,8 @@ function _M.merge_service_route(service_conf, route_conf)
core.log.info("service conf: ", core.json.delay_encode(service_conf))
core.log.info(" route conf: ", core.json.delay_encode(route_conf))
- return merged_route(route_conf, service_conf,
+ local rout_service_key = route_conf.modifiedIndex .. service_conf.modifiedIndex
Review comment:
add a separator: `modifiedIndex .. "#" .. modifiedIndex`
##########
File path: t/plugin/limit-count.t
##########
@@ -637,3 +637,54 @@ GET /t
passed
--- no_error_log
[error]
+
Review comment:
three blank lines between different test cases
##########
File path: apisix/init.lua
##########
@@ -361,21 +361,12 @@ function _M.http_access_phase()
return core.response.exit(404)
end
- local changed
- route, changed = plugin.merge_service_route(service, route)
+ route= plugin.merge_service_route(service, route)
api_ctx.matched_route = route
-
- if changed then
- api_ctx.conf_type = "route&service"
- api_ctx.conf_version = route.modifiedIndex .. "&"
- .. service.modifiedIndex
- api_ctx.conf_id = route.value.id .. "&"
- .. service.value.id
- else
- api_ctx.conf_type = "service"
- api_ctx.conf_version = service.modifiedIndex
- api_ctx.conf_id = service.value.id
- end
+ api_ctx.conf_type = "route&service"
+ api_ctx.conf_version = route.modifiedIndex .. "&" .. service.modifiedIndex
+ api_ctx.conf_id = route.value.id .. "&"
+ .. service.value.id
Review comment:
bad indentation
##########
File path: t/plugin/limit-count.t
##########
@@ -637,3 +637,54 @@ GET /t
passed
--- no_error_log
[error]
+
+=== TEST 21: when the count is changed, check the limit is correct
+--- config
+ location /t1 {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "methods": ["GET"],
+ "plugins": {
+ "limit-count": {
+ "count": 1,
+ "time_window": 60,
+ "key": "remote_addr"
+ }
+ },
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ },
+ "uri": "/hello"
+ }]],
+ [[{
+ "node": {
+ "value": {
+ "plugins": {
+ "limit-count": {
+ "rejected_code": 503
+ }
+ }
+ }
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+
Review comment:
remove this line
----------------------------------------------------------------
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] [apisix] gy09535 commented on pull request #2121: bugfix: updated the value of remaining when `count` changes in limit-count plugin.
Posted by GitBox <gi...@apache.org>.
gy09535 commented on pull request #2121:
URL: https://github.com/apache/apisix/pull/2121#issuecomment-686189817
Is their anything I can do for this PR?
----------------------------------------------------------------
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] [apisix] moonming commented on a change in pull request #2121: fix: limit count config not effective
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2121:
URL: https://github.com/apache/apisix/pull/2121#discussion_r479962776
##########
File path: apisix/plugin.lua
##########
@@ -332,7 +333,8 @@ function _M.merge_service_route(service_conf, route_conf)
core.log.info("service conf: ", core.json.delay_encode(service_conf))
core.log.info(" route conf: ", core.json.delay_encode(route_conf))
- return merged_route(route_conf, service_conf,
+ local rout_service_key = route_conf.modifiedIndex .. "#" .. service_conf.modifiedIndex
Review comment:
`rout_service_key` -> `route_service_key`
##########
File path: apisix/plugin.lua
##########
@@ -297,6 +297,7 @@ local function merge_service_route(service_conf, route_conf)
local new_conf = core.table.deepcopy(service_conf)
new_conf.value.service_id = new_conf.value.id
new_conf.value.id = route_conf.value.id
+ new_conf.modifiedIndex = route_conf.modifiedIndex
Review comment:
`new_conf.modifiedIndex` -> new_conf.modified_index
----------------------------------------------------------------
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] [apisix] moonming commented on a change in pull request #2121: fix: limit count config not effective
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2121:
URL: https://github.com/apache/apisix/pull/2121#discussion_r479962013
##########
File path: apisix/init.lua
##########
@@ -361,21 +361,11 @@ function _M.http_access_phase()
return core.response.exit(404)
end
- local changed
- route, changed = plugin.merge_service_route(service, route)
+ route= plugin.merge_service_route(service, route)
Review comment:
need a space before `=`
----------------------------------------------------------------
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] [apisix] moonming commented on pull request #2121: bugfix: updated the value of remaining when `count` changes in limit-count plugin.
Posted by GitBox <gi...@apache.org>.
moonming commented on pull request #2121:
URL: https://github.com/apache/apisix/pull/2121#issuecomment-683760175
```
# Failed test 'TEST 1: log rotate - response_body_like - response is expected (2020-08-31_10-25-32__access.log 2020-08-31_10-25-31__access.log 2020-08-31_10-25-33__access.log . 2020-08-31_10-25-32__error.log error.log 2020-08-31_10-25-33__error.log access.log 2020-08-31_10-25-31__error.log .. nginx.pid)'
# at /home/runner/work/apisix/apisix/test-nginx/lib/Test/Nginx/Socket.pm line 1635.
# '2020-08-31_10-25-32__access.log
# 2020-08-31_10-25-31__access.log
# 2020-08-31_10-25-33__access.log
# .
# 2020-08-31_10-25-32__error.log
# error.log
# 2020-08-31_10-25-33__error.log
# access.log
# 2020-08-31_10-25-31__error.log
# ..
# nginx.pid
# '
# doesn't match '(?^:\_error\.log[\s\S]*\_access\.log)'
# Looks like you failed 1 test of 7.
t/plugin/log-rotate.t ....................
```
@membphis please take a look this failed 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] [apisix] moonming commented on a change in pull request #2121: fix: limit count config not effective
Posted by GitBox <gi...@apache.org>.
moonming commented on a change in pull request #2121:
URL: https://github.com/apache/apisix/pull/2121#discussion_r479986030
##########
File path: apisix/plugin.lua
##########
@@ -297,6 +297,7 @@ local function merge_service_route(service_conf, route_conf)
local new_conf = core.table.deepcopy(service_conf)
new_conf.value.service_id = new_conf.value.id
new_conf.value.id = route_conf.value.id
+ new_conf.modifiedIndex = route_conf.modifiedIndex
Review comment:
got it
##########
File path: t/plugin/limit-count.t
##########
@@ -637,3 +637,55 @@ GET /t
passed
--- no_error_log
[error]
+
+
+
+=== TEST 21: when the count is changed, check the limit is correct
Review comment:
need add test case to cover: `count` from 1 to 2.
##########
File path: t/plugin/limit-count.t
##########
@@ -637,3 +637,55 @@ GET /t
passed
--- no_error_log
[error]
+
+
+
+=== TEST 21: when the count is changed, check the limit is correct
Review comment:
and `count` from 2 to 2.
----------------------------------------------------------------
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] [apisix] gy09535 commented on pull request #2121: fix: limit count config not effective
Posted by GitBox <gi...@apache.org>.
gy09535 commented on pull request #2121:
URL: https://github.com/apache/apisix/pull/2121#issuecomment-683585415
thanks for reviewing , all required format has fixed.
----------------------------------------------------------------
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] [apisix] gy09535 edited a comment on pull request #2121: fix: limit count config not effective
Posted by GitBox <gi...@apache.org>.
gy09535 edited a comment on pull request #2121:
URL: https://github.com/apache/apisix/pull/2121#issuecomment-683585415
thanks for reviewing , all required format has resolved.
----------------------------------------------------------------
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] [apisix] gy09535 removed a comment on pull request #2121: bugfix: updated the value of remaining when `count` changes in limit-count plugin.
Posted by GitBox <gi...@apache.org>.
gy09535 removed a comment on pull request #2121:
URL: https://github.com/apache/apisix/pull/2121#issuecomment-686189817
Is their anything I can do for this PR?
----------------------------------------------------------------
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] [apisix] moonming commented on pull request #2121: fix: limit count config not effective
Posted by GitBox <gi...@apache.org>.
moonming commented on pull request #2121:
URL: https://github.com/apache/apisix/pull/2121#issuecomment-681198918
@gy09535 please add test cases, thx
----------------------------------------------------------------
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] [apisix] gy09535 commented on pull request #2121: fix: limit count config not effective
Posted by GitBox <gi...@apache.org>.
gy09535 commented on pull request #2121:
URL: https://github.com/apache/apisix/pull/2121#issuecomment-681575628
> @gy09535 please add test cases, thx
added
----------------------------------------------------------------
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] [apisix] moonming merged pull request #2121: bugfix: updated the value of remaining when `count` changes in limit-count plugin.
Posted by GitBox <gi...@apache.org>.
moonming merged pull request #2121:
URL: https://github.com/apache/apisix/pull/2121
----------------------------------------------------------------
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] [apisix] gy09535 commented on a change in pull request #2121: fix: limit count config not effective
Posted by GitBox <gi...@apache.org>.
gy09535 commented on a change in pull request #2121:
URL: https://github.com/apache/apisix/pull/2121#discussion_r480024641
##########
File path: t/plugin/limit-count.t
##########
@@ -637,3 +637,55 @@ GET /t
passed
--- no_error_log
[error]
+
+
+
+=== TEST 21: when the count is changed, check the limit is correct
Review comment:
added
----------------------------------------------------------------
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] [apisix] membphis commented on pull request #2121: bugfix: updated the value of remaining when `count` changes in limit-count plugin.
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2121:
URL: https://github.com/apache/apisix/pull/2121#issuecomment-683882604
> # at /home/runner/work/apisix/apisix/test-nginx/lib/Test/Nginx/Socket.pm line 1635.
> # '2020-08-31_10-25-32__access.log
> # 2020-08-31_10-25-31__access.log
> # 2020-08-31_10-25-33__access.log
> # .
> # 2020-08-31_10-25-32__error.log
> # error.log
> # 2020-08-31_10-25-33__error.log
> # access.log
> # 2020-08-31_10-25-31__error.log
> # ..
> # nginx.pid
> # '
> # doesn't match '(?^:\_error\.log[\s\S]*\_access\.log)'
> # Looks like you failed 1 test of 7.
> t/plugin/log-rotate.t ....................
> ```
we need to update this test case. will create a new issue later
----------------------------------------------------------------
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] [apisix] gy09535 commented on a change in pull request #2121: fix: limit count config not effective
Posted by GitBox <gi...@apache.org>.
gy09535 commented on a change in pull request #2121:
URL: https://github.com/apache/apisix/pull/2121#discussion_r479969372
##########
File path: apisix/plugin.lua
##########
@@ -297,6 +297,7 @@ local function merge_service_route(service_conf, route_conf)
local new_conf = core.table.deepcopy(service_conf)
new_conf.value.service_id = new_conf.value.id
new_conf.value.id = route_conf.value.id
+ new_conf.modifiedIndex = route_conf.modifiedIndex
Review comment:
the "modifiedIndex" while be used in other space as route modified Index, if I change this key ,should change in all place.
----------------------------------------------------------------
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] [apisix] membphis commented on pull request #2121: bugfix: updated the value of remaining when `count` changes in limit-count plugin.
Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #2121:
URL: https://github.com/apache/apisix/pull/2121#issuecomment-687167584
it the test case is unstable, we need to merge https://github.com/apache/apisix/pull/2164 first
----------------------------------------------------------------
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] [apisix] membphis edited a comment on pull request #2121: bugfix: updated the value of remaining when `count` changes in limit-count plugin.
Posted by GitBox <gi...@apache.org>.
membphis edited a comment on pull request #2121:
URL: https://github.com/apache/apisix/pull/2121#issuecomment-683882604
```
> # at /home/runner/work/apisix/apisix/test-nginx/lib/Test/Nginx/Socket.pm line 1635.
> # '2020-08-31_10-25-32__access.log
> # 2020-08-31_10-25-31__access.log
> # 2020-08-31_10-25-33__access.log
> # .
> # 2020-08-31_10-25-32__error.log
> # error.log
> # 2020-08-31_10-25-33__error.log
> # access.log
> # 2020-08-31_10-25-31__error.log
> # ..
> # nginx.pid
> # '
> # doesn't match '(?^:\_error\.log[\s\S]*\_access\.log)'
> # Looks like you failed 1 test of 7.
> t/plugin/log-rotate.t ....................
> ```
```
we need to update this test case. will create a new issue later
----------------------------------------------------------------
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] [apisix] gy09535 closed pull request #2121: bugfix: updated the value of remaining when `count` changes in limit-count plugin.
Posted by GitBox <gi...@apache.org>.
gy09535 closed pull request #2121:
URL: https://github.com/apache/apisix/pull/2121
----------------------------------------------------------------
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