Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add a /config/rotate-root path to the ldap auth backend #24099
Add a /config/rotate-root path to the ldap auth backend #24099
Changes from 3 commits
0502420
2d66dfd
b052fc3
2d0e236
613bf5a
92e6d5f
6264368
a32e45e
958b5c5
ef2f8ec
02a85a1
5727653
88a791a
1223cab
9bdeb18
f94e892
6dd9bfe
7ba417c
21d7f1f
0439a51
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this live on the
*backend
struct so we can reuse it or does theclient
need to be created every time?Also, there's a newer cap/ldap client that we're trying to migrate to. Any reason not to use this one instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on potentially adding a
getClient
helper on thebackend
struct. That would also match up with other pluginsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i thought about this - this is the only place this particular is currently used in this (
credential/
)ldap, and i want to avoid confusion with the other similarly named one (cap/ldap/Client
).Possibly whoever adds the second use of this client can add the helper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not overly concerned with having a helper method, but more so wondering why we can't reuse the same instance of the ldap client. Probably not too concerning though, assuming that root rotation isn't a high-volume endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't notice the earlier mention of the cap client the first time around - the issue i encountered was that it wasn't interacting with LDAP at the level I needed to make Modify calls to the server, although maybe there was a way to do it that I missed.
I'm not too worried about multiple instantiations of the ldap client - whatever we're allocating is pretty thin each time, and rotate-root is going to be pretty low volume.