You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "ikatlinsky (via GitHub)" <gi...@apache.org> on 2023/12/06 10:49:01 UTC
[PR] Allow to use environment variables for limit-count plugin settings [apisix]
ikatlinsky opened a new pull request, #10607:
URL: https://github.com/apache/apisix/pull/10607
### Description
As per https://github.com/apache/apisix/issues/10597.
Fixes #10597
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [x] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) 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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: allow to use environment variables for limit-count plugin settings [apisix]
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #10607:
URL: https://github.com/apache/apisix/pull/10607#discussion_r1419872974
##########
t/plugin/limit-count-redis-cluster3.t:
##########
@@ -77,3 +81,55 @@ __DATA__
}
--- response_body
remaining: 1
+
+
+
+=== TEST 2: set route, with redis_cluster_nodes as environment variables and redis_cluster_name
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "uri": "/hello",
+ "plugins": {
+ "limit-count": {
+ "count": 2,
+ "time_window": 60,
+ "rejected_code": 503,
+ "key": "remote_addr",
+ "policy": "redis-cluster",
+ "redis_timeout": 1001,
+ "redis_cluster_nodes": [
+ "$ENV://REDIS_NODE_0",
+ "$ENV://REDIS_NODE_1"
Review Comment:
I misunderstood you, so you did two different thins in the pr: like `redis_password`
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: allow to use environment variables for limit-count plugin settings [apisix]
Posted by "ikatlinsky (via GitHub)" <gi...@apache.org>.
ikatlinsky commented on PR #10607:
URL: https://github.com/apache/apisix/pull/10607#issuecomment-1846603936
@monkeyDluffy6017 looking at failing tests (https://github.com/apache/apisix/actions/runs/7124716224/job/19435705333?pr=10607) and it seems like tests for multiple plugins are failed, like request id, openwisk and etc. It that something that is expected? Not sure if my changes can break other plugins.
Thanks
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: allow to use environment variables for limit-count plugin settings [apisix]
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #10607:
URL: https://github.com/apache/apisix/pull/10607#discussion_r1418271215
##########
t/plugin/limit-count-redis-cluster3.t:
##########
@@ -77,3 +81,55 @@ __DATA__
}
--- response_body
remaining: 1
+
+
+
+=== TEST 2: set route, with redis_cluster_nodes as environment variables and redis_cluster_name
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "uri": "/hello",
+ "plugins": {
+ "limit-count": {
+ "count": 2,
+ "time_window": 60,
+ "rejected_code": 503,
+ "key": "remote_addr",
+ "policy": "redis-cluster",
+ "redis_timeout": 1001,
+ "redis_cluster_nodes": [
+ "$ENV://REDIS_NODE_0",
+ "$ENV://REDIS_NODE_1"
Review Comment:
I think you are going to encrypt the `redis_password`, am i wrong?
##########
t/plugin/limit-count5.t:
##########
@@ -89,3 +91,49 @@ remaining: 2
remaining: 0
rejected
rejected
+
+
+
+=== TEST 2: set route(id: 1) using environment variable for key
+--- config
+ location /t {
+ 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": 2,
+ "time_window": 60,
+ "rejected_code": 503,
+ "key": "$ENV://LIMIT_COUNT_KEY"
Review Comment:
ditto
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: allow to use environment variables for limit-count plugin settings [apisix]
Posted by "ikatlinsky (via GitHub)" <gi...@apache.org>.
ikatlinsky commented on code in PR #10607:
URL: https://github.com/apache/apisix/pull/10607#discussion_r1419964275
##########
t/plugin/limit-count-redis-cluster3.t:
##########
@@ -77,3 +81,55 @@ __DATA__
}
--- response_body
remaining: 1
+
+
+
+=== TEST 2: set route, with redis_cluster_nodes as environment variables and redis_cluster_name
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "uri": "/hello",
+ "plugins": {
+ "limit-count": {
+ "count": 2,
+ "time_window": 60,
+ "rejected_code": 503,
+ "key": "remote_addr",
+ "policy": "redis-cluster",
+ "redis_timeout": 1001,
+ "redis_cluster_nodes": [
+ "$ENV://REDIS_NODE_0",
+ "$ENV://REDIS_NODE_1"
Review Comment:
yep, you are correct
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: allow to use environment variables for limit-count plugin settings [apisix]
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 merged PR #10607:
URL: https://github.com/apache/apisix/pull/10607
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: allow to use environment variables for limit-count plugin settings [apisix]
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #10607:
URL: https://github.com/apache/apisix/pull/10607#discussion_r1419875905
##########
apisix/plugins/limit-count/init.lua:
##########
@@ -68,6 +68,7 @@ local policy_to_additional_properties = {
type = "boolean", default = false,
},
},
+ encrypt_fields = {"redis_password"},
Review Comment:
I think this is not correct, please refer to https://github.com/apache/apisix/blob/master/apisix/plugins/kafka-proxy.lua#L36, and add a corresponding 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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: allow to use environment variables for limit-count plugin settings [apisix]
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #10607:
URL: https://github.com/apache/apisix/pull/10607#discussion_r1419872974
##########
t/plugin/limit-count-redis-cluster3.t:
##########
@@ -77,3 +81,55 @@ __DATA__
}
--- response_body
remaining: 1
+
+
+
+=== TEST 2: set route, with redis_cluster_nodes as environment variables and redis_cluster_name
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "uri": "/hello",
+ "plugins": {
+ "limit-count": {
+ "count": 2,
+ "time_window": 60,
+ "rejected_code": 503,
+ "key": "remote_addr",
+ "policy": "redis-cluster",
+ "redis_timeout": 1001,
+ "redis_cluster_nodes": [
+ "$ENV://REDIS_NODE_0",
+ "$ENV://REDIS_NODE_1"
Review Comment:
I misunderstood you, so you did two different thins in the pr:
1. encrypt private information, like `redis_password`
2. support environment variable
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: allow to use environment variables for limit-count plugin settings [apisix]
Posted by "moonming (via GitHub)" <gi...@apache.org>.
moonming commented on PR #10607:
URL: https://github.com/apache/apisix/pull/10607#issuecomment-1842960219
@ikatlinsky thank you for your contribution 👍
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: allow to use environment variables for limit-count plugin settings [apisix]
Posted by "ikatlinsky (via GitHub)" <gi...@apache.org>.
ikatlinsky commented on code in PR #10607:
URL: https://github.com/apache/apisix/pull/10607#discussion_r1419964826
##########
apisix/plugins/limit-count/init.lua:
##########
@@ -68,6 +68,7 @@ local policy_to_additional_properties = {
type = "boolean", default = false,
},
},
+ encrypt_fields = {"redis_password"},
Review Comment:
your are correct, that i am doing 2 different things in a single PR, let me ever this change
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: allow to use environment variables for limit-count plugin settings [apisix]
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on PR #10607:
URL: https://github.com/apache/apisix/pull/10607#issuecomment-1846608784
@ikatlinsky It has nothing to do with you changes
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: allow to use environment variables for limit-count plugin settings [apisix]
Posted by "ikatlinsky (via GitHub)" <gi...@apache.org>.
ikatlinsky commented on PR #10607:
URL: https://github.com/apache/apisix/pull/10607#issuecomment-1851297749
> please resolve the conflicts
done
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: allow to use environment variables for limit-count plugin settings [apisix]
Posted by "ikatlinsky (via GitHub)" <gi...@apache.org>.
ikatlinsky commented on code in PR #10607:
URL: https://github.com/apache/apisix/pull/10607#discussion_r1418457149
##########
t/plugin/limit-count5.t:
##########
@@ -89,3 +91,49 @@ remaining: 2
remaining: 0
rejected
rejected
+
+
+
+=== TEST 2: set route(id: 1) using environment variable for key
+--- config
+ location /t {
+ 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": 2,
+ "time_window": 60,
+ "rejected_code": 503,
+ "key": "$ENV://LIMIT_COUNT_KEY"
Review Comment:
same reasoning as above
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: allow to use environment variables for limit-count plugin settings [apisix]
Posted by "ikatlinsky (via GitHub)" <gi...@apache.org>.
ikatlinsky commented on code in PR #10607:
URL: https://github.com/apache/apisix/pull/10607#discussion_r1418454771
##########
apisix/plugins/limit-count.lua:
##########
@@ -14,11 +14,12 @@
-- See the License for the specific language governing permissions and
-- limitations under the License.
--
-local limit_count = require("apisix.plugins.limit-count.init")
+local fetch_secrets = require("apisix.secret").fetch_secrets
+local limit_count = require("apisix.plugins.limit-count.init")
-local plugin_name = "limit-count"
-local _M = {
- version = 0.4,
+local plugin_name = "limit-count"
+local _M = {
Review Comment:
my bad, seems like my auto-formatter configuration
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: allow to use environment variables for limit-count plugin settings [apisix]
Posted by "ikatlinsky (via GitHub)" <gi...@apache.org>.
ikatlinsky commented on code in PR #10607:
URL: https://github.com/apache/apisix/pull/10607#discussion_r1418454947
##########
apisix/plugins/limit-count/init.lua:
##########
@@ -68,6 +68,7 @@ local policy_to_additional_properties = {
type = "boolean", default = false,
},
},
+ encrypt_fields = {"redis_username", "redis_password"},
Review Comment:
actually, not really, will fix that
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: allow to use environment variables for limit-count plugin settings [apisix]
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #10607:
URL: https://github.com/apache/apisix/pull/10607#discussion_r1418264458
##########
apisix/plugins/limit-count.lua:
##########
@@ -14,11 +14,12 @@
-- See the License for the specific language governing permissions and
-- limitations under the License.
--
-local limit_count = require("apisix.plugins.limit-count.init")
+local fetch_secrets = require("apisix.secret").fetch_secrets
+local limit_count = require("apisix.plugins.limit-count.init")
-local plugin_name = "limit-count"
-local _M = {
- version = 0.4,
+local plugin_name = "limit-count"
+local _M = {
Review Comment:
Why do you add the indent? it's better to keep the original style
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: allow to use environment variables for limit-count plugin settings [apisix]
Posted by "monkeyDluffy6017 (via GitHub)" <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #10607:
URL: https://github.com/apache/apisix/pull/10607#discussion_r1418264880
##########
apisix/plugins/limit-count/init.lua:
##########
@@ -68,6 +68,7 @@ local policy_to_additional_properties = {
type = "boolean", default = false,
},
},
+ encrypt_fields = {"redis_username", "redis_password"},
Review Comment:
Does the `redis_username` need to be encrypted?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: allow to use environment variables for limit-count plugin settings [apisix]
Posted by "ikatlinsky (via GitHub)" <gi...@apache.org>.
ikatlinsky commented on code in PR #10607:
URL: https://github.com/apache/apisix/pull/10607#discussion_r1418456858
##########
t/plugin/limit-count-redis-cluster3.t:
##########
@@ -77,3 +81,55 @@ __DATA__
}
--- response_body
remaining: 1
+
+
+
+=== TEST 2: set route, with redis_cluster_nodes as environment variables and redis_cluster_name
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "uri": "/hello",
+ "plugins": {
+ "limit-count": {
+ "count": 2,
+ "time_window": 60,
+ "rejected_code": 503,
+ "key": "remote_addr",
+ "policy": "redis-cluster",
+ "redis_timeout": 1001,
+ "redis_cluster_nodes": [
+ "$ENV://REDIS_NODE_0",
+ "$ENV://REDIS_NODE_1"
Review Comment:
it is not about encryption mostly, but about using environment variables or secrets to configure the plugin. Based on my changes each attribute of the plugin which is:
- string
- array of strings
can be configured via env variable or secret.
So in this test i am checking if environment variables work on the example of array of strings field - redis_cluster_count.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org