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

feat: allow to use environment variables for limit-count plugin settings #10607

Conversation

ikatlinsky
Copy link
Contributor

@ikatlinsky ikatlinsky commented Dec 6, 2023

Description

As per #10597.

Fixes #10597

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)

@ikatlinsky ikatlinsky changed the title Allow to use environment variables for limit-count plugin settings feat: allow to use environment variables for limit-count plugin settings Dec 6, 2023
@ikatlinsky ikatlinsky marked this pull request as ready for review December 6, 2023 10:59
@moonming
Copy link
Member

moonming commented Dec 6, 2023

@ikatlinsky thank you for your contribution 👍

@@ -68,6 +68,7 @@ local policy_to_additional_properties = {
type = "boolean", default = false,
},
},
encrypt_fields = {"redis_username", "redis_password"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the redis_username need to be encrypted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, not really, will fix that

"count": 2,
"time_window": 60,
"rejected_code": 503,
"key": "$ENV://LIMIT_COUNT_KEY"
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same reasoning as above

@monkeyDluffy6017 monkeyDluffy6017 added the wait for update wait for the author's response in this issue/PR label Dec 7, 2023
@@ -68,6 +68,7 @@ local policy_to_additional_properties = {
type = "boolean", default = false,
},
},
encrypt_fields = {"redis_password"},
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

your are correct, that i am doing 2 different things in a single PR, let me ever this change

@ikatlinsky
Copy link
Contributor Author

@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

@monkeyDluffy6017
Copy link
Contributor

@ikatlinsky It has nothing to do with you changes

@monkeyDluffy6017 monkeyDluffy6017 added approved and removed wait for update wait for the author's response in this issue/PR user responded labels Dec 11, 2023
Copy link
Contributor

@monkeyDluffy6017 monkeyDluffy6017 left a comment

Choose a reason for hiding this comment

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

please resolve the conflicts

@ikatlinsky
Copy link
Contributor Author

please resolve the conflicts

done

@monkeyDluffy6017 monkeyDluffy6017 merged commit bf8940d into apache:master Dec 18, 2023
33 checks passed
@ikatlinsky ikatlinsky deleted the feat/add-env-vars-support-for-limit-count-plugin branch December 18, 2023 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: As a user i want to configure sensitive limit-count plugin properties using environment variables
5 participants