fix!: use maester rules by default

[kenankule]

BREAKING CHANGES: This patch changes the default value managedAccessRules to false. If the user would like to manage the rules with the helm chart, maester.enabled should be set to false and managedAccessRules should be set to true

Related Issue or Design Document

Fixes #512

Checklist

• [x] I have read the contributing guidelines and signed the CLA.
• [x] I have referenced an issue containing the design document if my change introduces a new feature.
• [x] I have read the security policy.
• [x] I confirm that this pull request does not address a security vulnerability. 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.
• [ ] I have added tests that prove my fix is effective or that my feature works.
• [ ] I have added the necessary documentation within the code base (if appropriate).

Further comments

Tested with the following configurations:

1- The user would like to manage rules manually. In this configuration the rules configmap and the initContainer to fix the permissions are not added to the deployment. rules volume is an emptydir:

oathkeeper:
  managedAccessRules: false
maester:
  enabled: false

2- The rules are managed by the helm chart oathkeeper.accessRules value. Rules configmap is created but the initContainer is not added to the deployment.

oathkeeper:
  managedAccessRules: true
maester:
  enabled: false

3- The rules are managed by the maester. Rules configmap is created by the maester chart and the volume name in the oathkeeper deployment is aligned with the maester chart. initContainer is not added to the deployment.

oathkeeper:
  managedAccessRules: false
maester:
  enabled: true

4- Invalid configuration so the helm chart rendering fails with an error message.

oathkeeper:
  managedAccessRules: true
maester:
  enabled: true

I believe the first configuration (where the chart creates an emptydir volume) should be replaced with a configuration where the chart gets an existing configmap name and uses that instead.

[CLAassistant]

All committers have signed the CLA.

[eroznik]

Any info on this one? Whats the hold up? :thinking: AFAIK, without this we're not able to use the chart as is - there is no way to make the Oathkeeper chart not create "empty rules" but still using the config map created by Maester ..

edit: relates to #669

[kenankule]

I've created this fix only for a PoC and we're not using the chart or the product anymore. I'd be happy if the maintainers take the ownership of this changeset.

[Demonsthere]

Hi there! The issue with this change is that it changes the default behaviour for people who do not use the maester controller, which we consider an addon. If we could provide the same function without changing the default behaviour (as still, most people use oathkeeper standalone, without the controller) i would be more than happy to merge this :)

[kenankule]

I think that (change in the default behavior) can be avoided if I set

managedAccessRules: true
maester:
  enabled: false

in the default values of the chart. Should I go ahead and change the defaults so that the change is somewhat backwards compatible? The only case it is not is when both of these values are set to true which I believe should not happen. There is even a comment in the values file about this.

[Demonsthere]

@kenankule yes please :) I think it even would make sense to add a check for this case (both are set to true) and fail the installation with an error message when that happens.

[kenankule]

I've changed the defaults, rebased the branch and pushed. Also edited the changeset description.

The validation you've requested is already implemented : https://github.com/ory/k8s/blob/6788f474786e7743a3867bdd769c99a425a56e68/helm/charts/oathkeeper/templates/configmap-rules.yaml#L3

[eroznik]

@Demonsthere any ETA for the next release? :))