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

feat(redis) add support for username/password auth #121

Merged
merged 6 commits into from
Aug 13, 2024
Merged

feat(redis) add support for username/password auth #121

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9b4cb7a
feat(redis) add support for username/password auth
gruceo Aug 13, 2024
34bddd5
tests
gruceo Aug 13, 2024
3766433
add new username/password fields
gruceo Aug 13, 2024
8a7498c
fix tests
gruceo Aug 13, 2024
ad28d12
Apply suggestions from code review
fffonion Aug 13, 2024
68e451d
fix tests 2
gruceo Aug 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions lib/resty/acme/storage/redis.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,14 @@ local function op(self, op, ...)
if not ok then
return nil, err
end

if self.auth then
local _, err = client:auth(self.auth)
local _, err
if type(self.auth) == "table" then
_, err = client:auth(self.auth.username, self.auth.password)
Copy link
Owner

Choose a reason for hiding this comment

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

I would actually suggest new fields being added (username, password) in addition to auth, otherwise we may need to write complex compat code on kong side.

Copy link

Choose a reason for hiding this comment

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

I think from style perspective I'd rather prefer to have separate fields as well. However I'm wondering - will it really require compat code on kong side? Even the way it's implemented here? 🤔 CP/DP will run different versions of this lib so I think we're good, right?

Copy link
Owner

@fffonion fffonion Aug 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

we do, unfortunately, because we need to define the schema at kong side (https://github.com/Kong/kong/blob/master/kong/plugins/acme/schema.lua#L85).
actually, i'm thinking to implement a new redis storage, using the kong provided redis library + schema in the kong acme plugin, that skipped the redis storage provided by this library.
basically we only need to add a kong/plugins/acme/storage/redis.lua then do something similar as this line https://github.com/Kong/kong/blob/master/kong/plugins/acme/client.lua#L91.

then we can from now on have aligned redis schema in the acme plugin with other plugins too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done here 3766433

Copy link

Choose a reason for hiding this comment

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

@fffonion Please correct me if I'm wrong but I think with the introduction of config adapter: https://github.com/Kong/kong/blob/master/kong/plugins/acme/storage/config_adapters/redis.lua
There's no need to write any compat code since plugin's schema is separated from this library's config.

else
_, err = client:auth(self.auth)
end
if err then
return nil, "authentication failed " .. err
end
Expand Down