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

chore(rl): standarize redis configuration #12301

Merged
merged 2 commits into from
Jan 12, 2024
Merged

chore(rl): standarize redis configuration #12301

merged 2 commits into from
Jan 12, 2024

Conversation

nowNick
Copy link
Contributor

@nowNick nowNick commented Jan 8, 2024 •
edited
Loading

Summary

Rate Limiting right now has new config structure that reuses common redis connection configuration.

Many plugins differ in the way Redis is configured. The goal of this PR is to introduce a standard Redis config schema that could be shared across plugins.

Other PRs

This is the second in the chains of PRs that standardize Redis configuration.

  1. ACME: chore(acme): standardize redis configuration #12300
  2. RateLimiting: (this PR)
  3. Response-RateLimiting: chore(response-rl): standarize redis configuration #12302

Checklist

  • The Pull Request has tests
  • A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
  • N/A (docs automatically generated from plugin schema description) There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

KAG-3388

@nowNick nowNick force-pushed the chore/standarize-redis-configuration-rate-limiting branch from 89fc3b0 to d519f2a Compare January 8, 2024 08:19
@nowNick nowNick force-pushed the chore/standarize-redis-configuration-acme branch from dc4a0e8 to 0ad6ba1 Compare January 8, 2024 10:26
@nowNick nowNick force-pushed the chore/standarize-redis-configuration-rate-limiting branch from d519f2a to 0b01009 Compare January 8, 2024 10:28
This was referenced Jan 8, 2024
Merged
Merged
@nowNick nowNick marked this pull request as ready for review January 8, 2024 13:13
@nowNick nowNick requested a review from jschmid1 January 8, 2024 13:13
@nowNick nowNick marked this pull request as draft January 8, 2024 14:14
@nowNick nowNick force-pushed the chore/standarize-redis-configuration-acme branch 2 times, most recently from a8c7332 to aecbd9f Compare January 9, 2024 13:35
@nowNick nowNick force-pushed the chore/standarize-redis-configuration-rate-limiting branch 7 times, most recently from 6428b70 to 5073fd8 Compare January 10, 2024 13:21
@nowNick nowNick force-pushed the chore/standarize-redis-configuration-acme branch from aecbd9f to f55a468 Compare January 10, 2024 15:51
@nowNick nowNick force-pushed the chore/standarize-redis-configuration-rate-limiting branch from 5073fd8 to cec2f00 Compare January 10, 2024 15:55
@nowNick nowNick force-pushed the chore/standarize-redis-configuration-acme branch from f55a468 to 56165fb Compare January 10, 2024 16:26
@nowNick nowNick force-pushed the chore/standarize-redis-configuration-rate-limiting branch from cec2f00 to 6523fc1 Compare January 10, 2024 16:29
@nowNick nowNick force-pushed the chore/standarize-redis-configuration-acme branch 2 times, most recently from 5d5eb72 to bcdf751 Compare January 10, 2024 17:47
@nowNick nowNick force-pushed the chore/standarize-redis-configuration-rate-limiting branch from 6523fc1 to c36ec13 Compare January 10, 2024 18:06
@nowNick nowNick requested a review from locao January 10, 2024 19:08
@nowNick nowNick marked this pull request as ready for review January 10, 2024 19:08
@nowNick nowNick force-pushed the chore/standarize-redis-configuration-rate-limiting branch 2 times, most recently from a877569 to 1ef1dfc Compare January 11, 2024 08:25
@nowNick nowNick force-pushed the chore/standarize-redis-configuration-acme branch from bcdf751 to 3b8fbb8 Compare January 11, 2024 08:57
@nowNick nowNick force-pushed the chore/standarize-redis-configuration-rate-limiting branch from 1ef1dfc to e2938a7 Compare January 11, 2024 08:57
jschmid1
jschmid1 approved these changes Jan 11, 2024
View reviewed changes
Copy link
Contributor

@jschmid1 jschmid1 left a comment

Choose a reason for hiding this comment

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

looks great 🚀

locao
locao approved these changes Jan 11, 2024
View reviewed changes
Copy link
Contributor

@locao locao left a comment

Choose a reason for hiding this comment

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

Good work!

kong/clustering/compat/checkers.lua
@@ -27,7 +27,8 @@ local compatible_checkers = {
function(config_table, dp_version, log_suffix)
local has_update
local redis_plugins_update = {
acme = require("kong.plugins.acme.clustering.compat.redis_translation").adapter
acme = require("kong.plugins.acme.clustering.compat.redis_translation").adapter,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
acme = require("kong.plugins.acme.clustering.compat.redis_translation").adapter,
['acme'] = require("kong.plugins.acme.clustering.compat.redis_translation").adapter,

Nit: it looks prettier if all items follow the same pattern.

spec/02-integration/09-hybrid_mode/09-config-compat_spec.lua
timeout = 1100,
ssl = true,
ssl_verify = true,
server_name = "example.test"
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a simple man, I see a .test hostname, I love the code ❤️

Base automatically changed from chore/standarize-redis-configuration-acme to master January 11, 2024 21:36
@locao
Copy link
Contributor

locao commented Jan 11, 2024

@nowNick I merged the other two PRs, but now we have conflicts in this one. Can you check, please?

@nowNick nowNick force-pushed the chore/standarize-redis-configuration-rate-limiting branch from f020ab3 to 2c5f680 Compare January 12, 2024 07:47
@nowNick @locao
chore(rl): standarize redis configuration
835769d 
Rate-Limiting right now has new config structure that reuses
common redis connection configuration. The same as ACME plugin.

KAG-3388
@nowNick @locao
chore(response-rl): standarize redis configuration
c68f2bc 
Response-RateLimiting right now has new config structure
that reuses common redis connection configuration.
The same as ACME and RateLimiting plugin.

KAG-3388
@locao locao force-pushed the chore/standarize-redis-configuration-rate-limiting branch from 2c5f680 to c68f2bc Compare January 12, 2024 13:05
@locao locao merged commit 25da462 into master Jan 12, 2024
24 checks passed
@locao locao deleted the chore/standarize-redis-configuration-rate-limiting branch January 12, 2024 13:52
nowNick added a commit that referenced this pull request Feb 7, 2024
@nowNick
Revert "chore(rl): standarize redis configuration (#12301)"
1ac8c2f 
This reverts commit 25da462.
@nowNick nowNick mentioned this pull request Feb 7, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jschmid1 jschmid1 jschmid1 approved these changes

@locao locao locao approved these changes

Assignees

@nowNick nowNick

Labels
core/clustering plugins/acme plugins/rate-limiting plugins/response-ratelimiting schema-change-noteworthy size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nowNick @locao @jschmid1