-
-
Notifications
You must be signed in to change notification settings - Fork 258
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
fix: oathkeeper - allow self managed rules when maester is disabled #685
base: master
Are you sure you want to change the base?
Conversation
Since PR ory#669, self-managed access rules are impossible to deploy. This commit fixes that by allowing the deployment controller to load the rules config map when maester is disabled.
Could you please update the |
Sure thing. I'll do it today. |
Just to further comment on the issue. We use oathkeeper as a subchart of our own chart, and therefore we build the access rules config map before deploying oathkeeper. We do this because we don't want to use maester at this point. Disabling |
My end goal would be to refactor the current setup into |
kind: ConfigMap | ||
metadata: | ||
# The name comes from oathkeeper's subchart. | ||
name: '{{ include "oathkeeper.fullname" . }}-rules' |
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.
this is a static file, not a template, functions are not supported 😉
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.
Yes! I know, haha. I just wanted to give you the feel for it since you were thinking about a larger refactor.
In principle, you would need, at least, the name of the release ahead of time, use that helper, or use fullnameOverride
with oathkeeper.
I should have written a comment after pushing this, I'm sorry about that. I'm going to put the PR in draft now.
Maybe I should just try doing the changes in the CI that would lead to this tests working? Maybe you can give me some guidance on how you would like to do this:
This is moderately bigger refactor that needs to be done in a separate issue/pr. In this case I think we can emulate the use-case by adding a oathkeeper-rules manifest here, so it will act as a pre-existing cm which has to be consumed by the chart
Thanks again for your help.
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.
No worries :) My idea for the CI refactor would be rather straightforward.
Current structure
hacks/
values/
oathkeeper.yaml
keto.yaml
...
New structure
hacks/
values/
oathkeeper/
case1.yaml
case2.yaml
keto/
foo.yaml
This would allow us to hold multiple config options in the CI and tests all of them automatically :)
In the quote you send i meant to deploy the cm as a static object, with a predefined foo
name, and then feed that name as a variable in the values
Since PR #669, self-managed access rules are impossible to deploy. This commit fixes that by allowing the deployment controller to load the rules config map when maester is disabled.
Related Issue or Design Document
In PR #669, logic was added to the deployment controller such that the access rules configmap would not be loaded when
.Values.managedAccessRules
was false. This breaks down self-templated access rules when maester is not desired.Checklist
If this pull request addresses a security vulnerability,
I confirm that I got approval (please contact [email protected]) from the maintainers to push the changes.
Further comments
There might be other ways to address this. Let me know if you have other suggestions. In PR #669, a suggestion was made restrict the logic only when maester's sideloader mode was enabled @cbrendanprice . This might be a solution as well.