-
Notifications
You must be signed in to change notification settings - Fork 3
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
UserInfoFetcher: ActiveDirectory backend #622
base: main
Are you sure you want to change the base?
Conversation
I saw this in the decision RfC column, anything particular you are looking for comments on? |
Primarily the CRD changes configuring the new backend. |
Moving CRD review to voting, please vote on this comment. |
|
||
[#backend-keycloak] |
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 know it was this way before, but IMHO it makes sense to move the Keycloak example above below the Keycloak section :)
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.
The API docs are fairly independent from the backend in use, mentioned that the example shows some keycloakisms in db44296
rust/crd/src/user_info_fetcher.rs
Outdated
#[serde(rename_all = "camelCase")] | ||
pub struct ActiveDirectoryBackend { | ||
/// Hostname of the domain controller, e.g. `ad-ds-1.contoso.com`. | ||
pub ldap_hostname: String, |
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.
In the AuthenticationClass we also have an ldapPort
field: https://docs.stackable.tech/home/stable/concepts/authentication#ldap. It might make sense to have that for consistency reasons as well.
If not please test a non-default port and update the CRD docs, e.g.
Hostname of the domain controller, e.g. `ad-ds-1.contoso.com` or `ad-ds-1.contoso.com:1234`.
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.
That's fair, the inspiration here was secret-op's LDAP support, which also doesn't support custom LDAP ports at the moment. As far as I know, there's no way to customize AD's LDAP port.
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.
In fairness, there are a few things we could port over from secret-op here:
- rename to
ldapServer
- explicitly forbid port numbers (though it should break the SASL bind anyway)
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.
As far as I know, there's no way to customize AD's LDAP port.
Oh that's a bit unexpected :D Happy to rename it to ldapServer
and close this discussion
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.
Co-authored-by: Sebastian Bernauer <[email protected]>
Closing CRD vote as accepted |
Proper testing is blocked on stackabletech/issues#629 |
Description
Fixes #524
Definition of Done Checklist
Author
Reviewer
Acceptance