-
-
Notifications
You must be signed in to change notification settings - Fork 9
Rework authorization config #884
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
base: main
Are you sure you want to change the base?
Conversation
dervoeti
left a comment
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.
Nice, LGTM overall, just some nits.
We should provide migration instructions in the release notes for users that use LDAP.
| resources: Some(VolumeResourceRequirements { | ||
| requests: Some({ | ||
| let mut map = BTreeMap::new(); | ||
| map.insert("storage".to_string(), Quantity("16Mi".to_owned())); |
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.
Maybe make this a constant? Just an idea though, feel free to disregard. Like:
const FILE_BASED_STORAGE_SIZE: &str = "16Mi";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.
Yeah i can do that, however i was not quite sure how to size it. Feels fine for "normal" setups, but you can easily set permissions on a very granular level (or have just many many users) to blow up its size...
My preference would be to make it even configurable (without pod overrides) but i could not think of a clean solution.
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 mean we could do something like:
spec:
clusterConfig:
authorization:
standard:
accessPolicyProvider:
fileBased:
initialAdminUser: CN=admin,OU=admin-group,DC=example,DC=org
storage:
capacity: 1Gi # default to 16MiWe can also leave it at 16Mi hardcoded for now, but if it's not unlikely to cause problems in production at some point we could try to make it configurable. I can't really judge that.
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.
Yeah i like that but if we ever want to provide a UserGroupProvider where does it go :D ? It is a bit intertwined.
Co-authored-by: Lukas Krug <[email protected]>
Description
Spike for #792, https://github.com/stackabletech/decisions/issues/66
This consolidates the operators authorization part closer to the nifi internals:
The authorization part consisted of an
OPAandDefaultpart. TheDefaultpart was a mix out of the SingleUser authorizer (e.g. for SingleUser or OIDC authentication) as well as file-based for LDAP authentication.Filebased changes to users or authorizations were not persisted properly (ephermeral) which lead to problems.
This is now consolidated according to the diagram above:
This is breaking for LDAP users that now explicitly have to set the standard authorization method and an initial admin user.
The PVC provided size for the filebased authorization is currently fixed to 16MB and cannot be configured other than pod overrides.
Definition of Done Checklist
Author
Reviewer
Acceptance
type/deprecationlabel & add to the deprecation scheduletype/experimentallabel & add to the experimental features tracker