Draft: Feature: Namespace-restricted option#6425
Draft: Feature: Namespace-restricted option#6425frigaut-orange wants to merge 3 commits intocrossplane:mainfrom
Conversation
882f1e5 to
cfddc6b
Compare
This option adds a --namespace-restricted option to the core crossplane controller, and has two consequences : - Restrict resource caching only to crossplane's own namespace (fixes crasshes in case crossplane does not have enough permissions) - Prevent events on cluster-wide resources It also adapts the helm chart accordingly, adding a namespaceRestricted variable in the values. This variable, when set to true: - Prevents the RBAC controller from being deployed - Deploys minimalistic RBAC resources instead - Prevents webhooks from being deployed
…r usages Signed-off-by: François Rigaut <francois.rigaut@orange.com>
f170ec5 to
4742ac1
Compare
There was a problem hiding this comment.
ClusterRoles would need to have unique names including the namespace itself, if you want to have multiple installations in parallel. Otherwise users would have to be careful and set a different name each time they install it.
| fieldRef: | ||
| fieldPath: spec.serviceAccountName | ||
| {{- if .Values.webhooks.enabled }} | ||
| {{- if and (.Values.webhooks.enabled) (not .Values.namespaceRestricted) }} |
There was a problem hiding this comment.
why disabling webhooks? they could just be configured to handle resources in a specific namespace
There was a problem hiding this comment.
I am still not quite sure about whether webhooks should be disabled or not.
The idea is that Crossplane cannot deploy cluster-wide resources excepts its own CRDs, and that includes WebhookConfigurations. They should then be treated separately (i.e. not deployed within the helm chart).
However, what you are saying is right. Not deploying them within the helm chart or not can be managed separately (with the webhooks.enabled variable).
--> I can remove this particular change, and make so that webhooks are deployed by default anyway, and let the users decide the webhooks behavior separately. However, this would not be consistent with what I did for the RBAC manager pod.
Let me know what you think about this, as I am not quite sure about what the right approach is.
Signed-off-by: François Rigaut <francois.rigaut@orange.com>
|
@frigaut-orange Is this still relevant, or is it superseded by your more focused/smaller PRs? |
|
@negz Again sorry for the delay This PR is still relevant, although out-of-date, especially with #6732 The idea is to have an argument in the helm chart, which brings all of the following features at once, according the the one-pager:
TL;DR the changes to the helm chart are relevant, the changes to the code are out of date I can clean up this PR but I would rather waiat for all the subsequent PRs to be merged beforehand. In the meantime, I set this PR to Draft. |
|
Crossplane does not currently have enough maintainers to address every issue and pull request. This pull request has been automatically marked as |
|
/fresh |
jbw976
left a comment
There was a problem hiding this comment.
Now that #6732 (review) is merged, can you please cleanly lay out the next steps for continuing with this broader namespace restriction effort @frigaut-orange? Thanks for your continued work here 🙇♂️
I think this is supposed to be draft but it's not, so I'm setting it to draft now 😇 |
|
Also, should #6348 be re-opened to track the continued effort of this PR? Having a clear list of tasks / remaining work in the body of that issue (if it should be re-opened) or the body of this PR will help keep track of what's left, that helps me at least get oriented when I come back here later 😁 |
Description of your changes
Add a
--namespace-restrictedoption to the core crossplane command. This option :defaultnamespace)Related docs PR : #6419
I have:
earthly +reviewableto ensure this PR is ready for review. (waiting for crossplane-runtime dependency : Add filter functions to event recorder crossplane-runtime#829)[ ] Addedbackport release-x.ylabels to auto-backport this PR.[ ] Followed the [API promotion workflow] if this PR introduces, removes, or promotes an API.