-
-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat: Add support for OPA authorizer #474
Conversation
7cb366f
to
a685927
Compare
a685927
to
c2b60aa
Compare
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.
LGTM
In terms of CRD change we have two options: 1. Enable authorizer and group-mapper simultaneous # Enable authorizer and group-mapper at the same time
clusterConfig:
authorization: # optional
opa: # mandatory
configMapName: opa # mandatory
package: hdfs # mandatory
2. Enable authorizer and group-mapper separately clusterConfig:
authorization: # optional
opaAuthorization: # mandatory
configMapName: opa # mandatory
package: hdfs # mandatory
opaGroupMapping: # optional
configMapName: opa # mandatory
package: hdfs # mandatory
Originally I was thinking of 2., but now I am in favor of 1., as it's simpler and more consistent and 2. only enables stuff we should probably not support :) |
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.
Just reviewed the docs so far with a few comments. Nit: we use regorule, rego rule and rego-rule here: I don't mind which it is but we should be consistent. The opa docs seem to use two separate words i.e. rego rules.
Co-authored-by: Andrew Kenworthy <andrew.kenworthy@stackable.de>
Co-authored-by: Andrew Kenworthy <andrew.kenworthy@stackable.de>
@adwk67 feedback should be addressed |
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.
LGTM! Can merge when the CI tests are all done.
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.
LGTM! Can merge when the CI tests are all done.
And also a full testsuite run: https://ci.stackable.tech/view/02%20Operator%20Tests%20(custom)/job/hdfs-operator-it-custom/130/ |
Another full testsuite run, after I increased the resources in stackabletech/ci@40937a9: |
Full testsuite passed 🚀 |
Description
Closes #400
Definition of Done Checklist
Author
Reviewer
Acceptance