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(response-ratelimiting): add support for secret rotation with redis connection #10570

Merged
merged 1 commit into from
Sep 21, 2023

Conversation

bungle
Copy link
Member

@bungle bungle commented Mar 28, 2023

Summary

Adds support for secret rotation for redis credentials in response-ratelimiting plugin.

@bungle bungle force-pushed the feat/response-rate-limiting-redis-secret-rotation branch 2 times, most recently from 0dc63b4 to dc8703b Compare March 31, 2023 12:45
ok, err = red:auth(conf.redis_username, conf.redis_password)
ok, err = kong.vault.try(function(cfg)
return red:auth(cfg.redis_username, cfg.redis_password)
end, conf)
Copy link
Member

Choose a reason for hiding this comment

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

Aapo, was the plan not to keep this transparent to the plugin authors and resolve the referenced username/password before calling any plugin handler code?

I'm surprised to see this code here.

Copy link
Member Author

@bungle bungle Apr 19, 2023

Choose a reason for hiding this comment

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

@hbagdi, this is the resolve-secret-on-failure approach. Most of the cases we have with secrets are resolve-on-secret-failure. But we decided to focus on more generic ttl, but that has limitations. E.g. with try, you can just go to vault and change secret and then change db password, and Kong should be able to pick it up. The TTL approach requires a more coordinated approach. That is when rotating, you should keep the old one at least for TTL amount of time, and after that you may drop the old credentials. The TTL approach can be seen in action here (the update function): 83bd52b

But the TTL we currently only re-resolve secrets only every minute as most:
20c2da8#diff-212b3488c5eb070820f7bc5178fbaa56e9435fe49cf050feb29091ca9aa1540bR49

There is no way to do fail-and-resolve anywhere else than where the failure happens.

@hbagdi hbagdi added this to the 3.3.0 milestone Apr 18, 2023
@oowl
Copy link
Member

oowl commented Apr 25, 2023

@bungle I have seen this PR was added to 3.3 milestones, And the code looks Okay. Why was this PR labeled draft?

@bungle bungle modified the milestones: 3.3.0, 3.4.0 May 2, 2023
@bungle
Copy link
Member Author

bungle commented May 2, 2023

Moving this to 3.4, and perhaps abstract it a bit more.

@bungle bungle force-pushed the feat/response-rate-limiting-redis-secret-rotation branch from dc8703b to b340c9f Compare May 10, 2023 08:46
@bungle bungle marked this pull request as ready for review May 10, 2023 08:46
@bungle bungle force-pushed the feat/response-rate-limiting-redis-secret-rotation branch from b340c9f to fd973ec Compare June 5, 2023 14:41
@kikito
Copy link
Member

kikito commented Jul 17, 2023

I'm going to remove this from the 3.4 milestone. We are too busy fixing the bugs we found on the "regular" (non-try-based) TTL code to expand the scope even more. Also, this PR has no tests.

@kikito kikito removed this from the 3.4.0 milestone Jul 17, 2023
@vm-001 vm-001 force-pushed the feat/response-rate-limiting-redis-secret-rotation branch from fd973ec to 605b7ae Compare August 29, 2023 18:20
@bungle bungle force-pushed the feat/response-rate-limiting-redis-secret-rotation branch from 605b7ae to 6b866ab Compare September 12, 2023 08:41
@kikito
Copy link
Member

kikito commented Sep 12, 2023

I'm approving this because (request-)ratelimiting has the same change and otherwise this would make both plugins iconsistent.

@bungle bungle force-pushed the feat/response-rate-limiting-redis-secret-rotation branch from 6b866ab to ddffc7a Compare September 12, 2023 10:31
@kikito kikito force-pushed the feat/response-rate-limiting-redis-secret-rotation branch from ddffc7a to 139635b Compare September 19, 2023 08:38
@bungle bungle force-pushed the feat/response-rate-limiting-redis-secret-rotation branch from 139635b to 31caef8 Compare September 19, 2023 13:40
…is connection

Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>
@bungle bungle force-pushed the feat/response-rate-limiting-redis-secret-rotation branch from 31caef8 to 3c513f4 Compare September 20, 2023 08:23
@bungle bungle merged commit 83721e6 into master Sep 21, 2023
32 checks passed
@bungle bungle deleted the feat/response-rate-limiting-redis-secret-rotation branch September 21, 2023 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants