Add permissions on hnc CRDs to default clusterRoles: cluster-reader, view, edit, admin

[zfrhv]

Changing permissions to default clusterRoles.
Each user that has admin/edit can manage sub-namespaces and HRQs
Each user that has view/cluster-reader can see all hnc CRs

Related to: #317

[k8s-ci-robot]

Hi @zfrhv. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[zfrhv]

Also does hnc supposed to have cluster-admin?:
https://github.com/kubernetes-sigs/hierarchical-namespaces/blob/master/config/rbac/role.yaml#L19C1-L24C8

[adrianludwin]

If I'm understanding this correctly, nothing should change for "admin" users, but cluster-edit users will get the same permissions that "admins" used to have, and cluster-viewers will get "view" permissions. Is that correct? If so, this lgtm after some nits

[adrianludwin]

Also does hnc supposed to have cluster-admin?: https://github.com/kubernetes-sigs/hierarchical-namespaces/blob/master/config/rbac/role.yaml#L19C1-L24C8

Basically, yes, since HNC can be used to copy any namespaced object. If there were a way to only give permissions for namespaced objects we'd do that, but AFAIK this doesn't exist.

Individual HNC users can easily restrict HNC to only have access to an allowlisted set of resources, but this is the only default that will reliably work in all cases.

[zfrhv]

If there were a way to only give permissions for namespaced objects we'd do that, but AFAIK this doesn't exist.

Isn't thats what the operatorGroups are made for?
When installing an operator with OLM operatorGroup must exist, and there you can specify the list of the namespaces on which the operator will have permissions.
You can even use a label selector, just put the label on the namespace and the operator should get control of it.
I hope I'm not mistaken.

[adrianludwin]

OperatorGroups appear to only be an OpenShift concept, not a K8s concept. So I can't comment on that... plus from your description it's a way to select the namespaces for the operator, but not the types of objects.

[zfrhv]

OperatorGroups appear to only be an OpenShift concept

OLM can be installed in kubernetes, but I understand if you dont want to use it because probably most of kubernetes people dont use it.

plus from your description it's a way to select the namespaces for the operator, but not the types of objects.

Im pretty sure you can configure the objects that you need the permissions for, and OLM will create permissions for those objects in the listed namespaces (maybe you can specify *)

I'm not too confident because the only operator I made was from helm chart, and it was bad quality.

[jouve]

current admin-role has * so hncconfigurations too

[zfrhv]

why hncconfigurations too?
isnt its clusterWide object?
you mean hierarchyconfigurations?

[jouve]

indeed :)

[zfrhv]

I dont have practice with hierarchyconfigurations so I dont know the consequences, a bit afraid to add something I dont know

[zfrhv]

Ok from what I see this CR allows to add annotations/labels to current namespace and his children.
This roleBinding is aggregated to edit/admin permission, user that has edit/admin shouldn't be able to modify the namespace including annotations/labels.

If hierarchyconfigurations would influence only the children namespaces then yes I would add it, but because it effects the parent namespace as well, I consider it as a security breach because edit/admin on the namespace shouldnt have permissions to modify the parent namespace.

Sorry I wont add it to the list, if there will be a way to configure hierarchyconfigurations only to effect the children (maybe from hncConfiguration or something) then yes we can add it as default edit permission.

But thank you for the feedback!

[adrianludwin]

It might make sense to distinguish between hnc-edit, which gives permission to the namespaced objects (SubnamespaceAnchor, HierarchyConfiguration, HierarchicalResourceQuota) and hnc-admin, which would add permission to administer HNC itself (HNCConfiguration). That is, hnc-edit would let users modify any part of the namespace hierarchy, while hnc-admin would let users change the way HNC works.

Being able to edit HierarchyConfiguration looks like it lets you change the parent of a namespace, but in reality HNC itself imposes limitations on this:

However, being the admin of a namespace does not give you free reign over that namespace’s config. For example, if you are the admin of a child namespace but not its parent, you may not change the parent of your namespace, as this would remove privileges from the admin of the parent (otherwise known as a superadmin, which in HNC's context means an admin of any ancestor namespace). Similarly, even if your namespace is a root, you may not set its parent to any namespace of which you are not currently an admin, since the namespace could then inherit sensitive information such as Secrets.

In general, to change the parent of your namespace N from A to B, you must have the following privileges:

• You must be the admin of the highest namespace that will no longer be an ancestor of namespace N after this change, in order to confirm that you are happy to lose your privileges in namespace N.
• You must be the admin of namespace B, in order to acknowledge that sensitive objects from B may be copied into namespace N.

This is implemented here.

[jouve]

for clarification, admin is supposed to be used with a RoleBinding ref, so a user would not be able to use HNCConfiguration which is ClusterScoped anyway:

• view: view resources in namespace
• edit: view+edit resources (except rbac Role/Rolebinding) in namespace
• admin: view+edit resource (incl. Role/RoleBinding) in namespace
• cluster-admin: view+edit anything (resources * verbs: *) ref , with clusterrolebinding

as a matter of preference, I prefer explicit declarations instead of *

[zfrhv]

Sounds great, but my point was that without HNC, when giving someone admin on namespace A, he cannot change the labels nor annotations of that namespace.
But with the help of HNC, the admin user can configure hiearchyConfiguration to add annotations including on self namespace A.

So its kind of a workaround to be able to manage the namesoace labels+annotations without actually having the permissions.

But now as I think, why not to actually give the ability for the user to label or annotate his namespace even if he cant modify the object? (as long as he is admin)
I bet its not allowed as default only because labels+annotations are part of the object, and its impossible to give permissions only on a specific fields in the object.

[adrianludwin]

Ah, thanks @jouve , yes, if these are all supposed to be used by RoleBindings and not ClusterRoleBindings, then there's no point to add HNCConfiguration.

Sounds great, but my point was that without HNC, when giving someone admin on namespace A, he cannot change the labels nor annotations of that namespace.

There's no general concept of "admin" on a namespace without HNC AFAIK - either you can edit the namespace object, in which case you can modify the labels/annotations, or you can't. As you say, there's no way to grant permissions to only specific fields.

With HNC, if you are allowed to modify the HierarchyConfiguration in namespace A, then you can also edit the specific labels and annotations managed by HNC. It's not a general-purpose way to modify any label/annotation. But yes, granting someone hnc-admin at the cluster level would allow them to modify any managed label/annotation on any namespace; that's by design.

[zfrhv]

But yes, granting someone hnc-admin at the cluster level would allow them to modify any managed label/annotation on any namespace; that's by design.

Thats the point, if someone is granted at not cluster level (only namespaced) he can still modify the annotations of his namespace.

Simple user has only roleBinding and can create hierarchyConfiguration, HNC will create the annotation for him even if he doesn't has permissions.

Anyways I think if someone has admin on the namespace it makes sense that he could add labels to the namespace.

Give me 30 min I will update the PR

[zfrhv]

Also I didn't knew i had to update incubator/hnc/config/rbac/kustomization.yaml
so imma do that

[rjbez17]

/ok-to-test

/assign @adrianludwin

[adrianludwin]

Failure looks unrelated, probably just a flake?

/lgtm
/retest

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[rjbez17]

/remove-lifecycle stale

[rjbez17]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rjbez17, zfrhv

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [rjbez17]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment