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.

Sign up for GitHub

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

Jump to bottom

feat(response-ratelimiting) redis ssl #8595

Merged
merged 16 commits into from
Nov 16, 2022
Merged

feat(response-ratelimiting) redis ssl #8595

merged 16 commits into from
Nov 16, 2022

Conversation

dominikkukacka
Copy link
Contributor

Summary

Adds flags to the response-ratelimiter for redis_ssl along with server_name for sni support.
I basically copied over the changes from the rate-limiting plugin

@CLAassistant
Copy link

CLAassistant commented Mar 25, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@mayocream
Copy link
Contributor

@dominikkukacka could you add related tests?

@dominikkukacka
Copy link
Contributor Author

@mayocream I certainly would, but I am not sure where. I can't find any other test specifically for plugins. I also saw no tests added for the same change in the rate-limit plugging so I stopped looking :)

My other problem is I can't compile Kong at all (I am on macOS but I also tried on Ubuntu)
If the tests are running on CI I am still happy to add them but I would need you to point me to the location where I should implement them.

@dominikkukacka dominikkukacka force-pushed the feature/add-ssl-response-ratelimiter branch from 2a8e3b6 to 55aa0aa Compare March 29, 2022 06:19
ADD-SP
ADD-SP requested changes Apr 7, 2022
View reviewed changes
Copy link
Contributor

@ADD-SP ADD-SP left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@dominikkukacka I've updated the tests of the rate-limiting plugin by #8617 , and now the SSL connection between it and Redis can be tested. Would you mind referring to the test code of the rate-limiting plugin to add the relevant tests to your PR?

For now, you need to update the tests in the following two files.

  • spec/03-plugins/24-response-rate-limiting/04-access_spec.lua
  • spec/03-plugins/24-response-rate-limiting/05-integration_spec.lua

@ADD-SP ADD-SP added the pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. label Apr 7, 2022
@dominikkukacka dominikkukacka force-pushed the feature/add-ssl-response-ratelimiter branch from ca279f0 to 422f39c Compare April 7, 2022 07:15
@dominikkukacka dominikkukacka force-pushed the feature/add-ssl-response-ratelimiter branch from fd040ea to d580cf2 Compare April 7, 2022 07:31
@dominikkukacka
Copy link
Contributor Author

@ADD-SP Hey, I tried my best to "copy over" the changes needed. I can't run the test suit locally so I hope there was no mistake. Will the test run in CI now?

@mikefero mikefero added the version-compatibility Incompatibility with older data plane versions label Apr 7, 2022
@dominikkukacka
Copy link
Contributor Author

Is there anything I can help with?

ADD-SP
ADD-SP requested changes Apr 12, 2022
View reviewed changes
Copy link
Contributor

@ADD-SP ADD-SP left a comment

Choose a reason for hiding this comment

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

Some changes are needed.

spec/03-plugins/24-response-rate-limiting/05-integration_spec.lua Show resolved Hide resolved
@guanlan guanlan requested a review from a team as a August 25, 2022 01:38
@vm-001 vm-001 self-requested a review October 27, 2022 07:20
@vm-001 vm-001 mentioned this pull request Nov 1, 2022
Merged
vm-001
vm-001 reviewed Nov 1, 2022
View reviewed changes
spec/03-plugins/24-response-rate-limiting/04-access_spec.lua
limits = { video = { second = ITERATIONS } },
},
})
for redis_conf_name, redis_conf in pairs(redis_confs) do
Copy link
Contributor

Choose a reason for hiding this comment

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

I know these were copied from the rate-limiting. I think we can add some test cases only for the SSL connection feature instead of doing a loop for every existing test. This will reduce running time.

vm-001
vm-001 approved these changes Nov 1, 2022
View reviewed changes
Copy link
Contributor

@vm-001 vm-001 left a comment

Choose a reason for hiding this comment

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

LGTM.

The corresponding doc PR is here: Kong/docs.konghq.com#4681

@ADD-SP Could you please do another round of code review?

@hbagdi hbagdi requested a review from flrgh November 2, 2022 17:34
@vm-001 vm-001 added this to the 3.1 milestone Nov 11, 2022
@vm-001 vm-001 requested a review from ADD-SP November 11, 2022 06:24
ADD-SP
ADD-SP previously approved these changes Nov 11, 2022
View reviewed changes
Copy link
Contributor

@ADD-SP ADD-SP left a comment

Choose a reason for hiding this comment

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

LGTM

@ADD-SP ADD-SP removed the pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. label Nov 11, 2022
@ADD-SP ADD-SP dismissed their stale review November 14, 2022 10:03

This is a breaking change.

@ADD-SP ADD-SP dismissed their stale review November 16, 2022 08:53

It is not a breaking change.

ADD-SP
ADD-SP approved these changes Nov 16, 2022
View reviewed changes
Copy link
Contributor

@ADD-SP ADD-SP left a comment

Choose a reason for hiding this comment

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

LGTM

@kikito kikito merged commit a10d8b4 into Kong:master Nov 16, 2022
@dominikkukacka
Copy link
Contributor Author

🥳 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vm-001 vm-001 vm-001 approved these changes

@andreleoni andreleoni andreleoni left review comments

@mikefero mikefero mikefero left review comments

@ADD-SP ADD-SP ADD-SP approved these changes

@kikito kikito kikito approved these changes

@flrgh flrgh Awaiting requested review from flrgh

Assignees

@flrgh flrgh

Labels
changelog core/clustering plugins/response-ratelimiting size/XXL version-compatibility Incompatibility with older data plane versions
Projects
None yet
Milestone
3.1
Development

Successfully merging this pull request may close these issues.
9 participants
@dominikkukacka @CLAassistant @mayocream @kikito @mikefero @andreleoni @vm-001 @ADD-SP @flrgh