Add extra Application Developer RBAC config

[anders-elastisys]

Warning

This is a public repository, ensure not to disclose:

• personal data beyond what is necessary for interacting with this pull request, nor
• business confidential information, such as customer names.

What kind of PR is this?

Required: Mark one of the following that is applicable:

• kind/feature
• kind/improvement
• kind/deprecation
• kind/documentation
• kind/clean-up
• kind/bug
• kind/other

Optional: Mark one or more of the following that are applicable:

Important

Breaking changes should be marked kind/admin-change or kind/dev-change depending on type
Critical security fixes should be marked with kind/security

• kind/admin-change
• kind/dev-change
• kind/security
• kind/adr

What does this PR do / why do we need this PR?

As an Platform Administrator, I want a declarative approach for providing additional RBAC to Application Developers without extending the current RBAC provided by Welkin. This can be used for special cases where an application developer might request a service that requires some additional RBAC that they themselves can't delegate.

This also fixes so that the user-crds ClusterRoleBinding is not created unless any userCRD resources have been specified, so now it will be the same check as for the user-crds ClusterRole.

• Fixes #

Information to reviewers

Checklist

• Proper commit message prefix on all commits
• Change checks:
  • The change is transparent
  • The change is disruptive
  • The change requires no migration steps
  • The change requires migration steps
  • The change upgrades CRDs
  • The change updates the config and the schema
• Documentation checks:
  • The public documentation required no updates
  • The public documentation required an update - [link to change](set-me)
• Metrics checks:
  • The metrics are still exposed and present in Grafana after the change
  • The metrics names didn't change (Grafana dashboards and Prometheus alerts are not affected)
  • The metrics names did change (Grafana dashboards and Prometheus alerts were fixed)
• Logs checks:
  • The logs do not show any errors after the change
• Pod Security Policy checks:
  • Any changed pod is covered by Pod Security Admission
  • Any changed pod is covered by Gatekeeper Pod Security Policies
  • The change does not cause any pods to be blocked by Pod Security Admission or Policies
• Network Policy checks:
  • Any changed pod is covered by Network Policies
  • The change does not cause any dropped packets in the NetworkPolicy Dashboard
• Audit checks:
  • The change does not cause any unnecessary Kubernetes audit events
  • The change requires changes to Kubernetes audit policy
• Falco checks:
  • The change does not cause any alerts to be generated by Falco
• Bug checks:
  • The bug fix is covered by regression tests

[viktor-f]

I think that this is a reasonable feature to add. 👍
But I'm not sure if userCRDs is the best chart to add it with. My initial expectation when thinking about it would be to add it via the userRBAC chart. I also think that it is a bit unexpected that these extra roles can only be added if gatekeeper.allowUserCRDs.enabled=true
But I understand that it was very convenient to add it via this chart, so I'm not sure if my concerns are worth acting on. What do you think of this?
Another alternative could be to add a new chart, called something like "userCustomRBAC".

[anders-elastisys]

Yes I do agree, but I feel like it is nice to use the RBAC templating of the userCRDs chart, personally I think it is the name for the user-crds chart that is a bit misleading as it is essentially just a chart for generating RBAC. I got a bit carried away in this PR to try to address this and change some of the values file and charts to make the configuration of user CRDs a bit more clear, but I have not had the time to go back to it. I guess this would be something to maybe discuss and bring up with @elastisys/goto-access-control

[viktor-f]

Yes, I agree that the name for the chart is a bit misleading and that we could probably restructure it significantly.
I think I missed that PR earlier, that kind of rework could be nice. But perhaps a bit out of scope and something that could be handed over /discussed with access control goto like you said.
Since you are trying to add this I assume that this is something you have need for. Is that correct? (You don't need to/should mention any details here).

I think I'd be ok with making these changes in the simplest way possible now and then reworking it later. The only thing that I might see as a true blocker is the oddity of locking this behind gatekeeper.allowUserCRDs.enabled=true.

[anders-elastisys]

How about having it as a separate chart? 450a7d1
This removes the dependency on gatekeeper.allowUserCRDs.enabled=true.
And then addressing the name of the chart can be a separate PR, and yes this templating is something we are using atm for cases where we need to add e.g. a single ClusterRoleBinding for an app developer without having to add it to our templating directly.

[viktor-f]

Yes, I like this version better.
I'm happy with this after you fix the comments from aarnq.

[anders-elastisys]

@aarnq PTAL 05cf393

[viktor-f]

Question: What is the reason for adding this in additionalProperties instead of properties? AFAIK the common thing would be to add all options in properties. But I'm no expert on the config schema yet, so I might just be missing something.
Same question for the other things you added further down.

[anders-elastisys]

Since what you configure are objects with "variable/custom" names I did this way as it is also how we do it for configuring constraints, see here. But maybe there is a better way to handle it? Maybe @aarnq has an opinion on this?

[aarnq]

Yes, this is the way to express map[string]object, as the key will become the name of the resource.

[viktor-f]

The items in this list is just the same rules format that is added to roles, right? Could we perhaps link to some kubernetes documentation to make it very clear what to put here?
Same for the other similar things further down.