HNC: Add webhook rules to deny editing namespace tree labels

[yiqigao217]

Issue by yiqigao217
Tuesday Apr 27, 2021 at 20:10 GMT
Originally opened as kubernetes-retired/multi-tenancy#1494

---

Currently, the tree labels on namespaces can be edited without webhook blocking it. HNC will re-reconcile the namespace to update the tree labels.

We need to have our webhooks to block edits on namespace tree labels.

[RealHarshThakur]

@yiqigao217 Can I take this up?

Just had a question:

• Do we have a field manager/owner to identify only HNC is allowed to make changes to labels? If so, is that what we can use to reject requests from other field manager/owner?

[adrianludwin]

@RealHarshThakur sorry I didn't see this! Yiqi's not working on HNC anymore. Yes, this would be a nice and fairly straightforward change for you to make if you're still interested.

The namespace validators are here. As you can see on this line, the first thing we check is whether the change is being made by the HNC service account, and if so, we allow all changes. So the rest of the validator can safely deny anything that anyone who's not HNC shouldn't be allowed to do.

[RealHarshThakur]

@adrianludwin Thanks for the reply. Just to be sure: I just need to add a check here and deny all requests trying to modify the tree labels on a namespace, right? Wondering if it would be just the Update operation we need to handle

[adrianludwin]

Yes, that's roughly the right place. We also need to check Create operations as well since we don't want someone creating a namespace with the (wrong) labels on them.

We also try not to reject changes that don't contain new problems. E.g. if the namespace already has the wrong label, we shouldn't reject the new change that might be unrelated. Check out this line for an example of this logic. In fact, your function might look quite similar to this - but also make sure nothing's removed either.

[k8s-triage-robot]

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

This bot triages issues and 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 issue is closed

You can:

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

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

/lifecycle stale

[adrianludwin]

/remove-lifecycle stale

[rahulii]

@adrianludwin If its still needed, I can take this up! Could you point me to the updated place where we need to make a change, the above comments seem outdated

[rahulii]

/assign

[RealHarshThakur]

@rahulii I've already made a PR for this : #51

[rahulii]

oh damn! you should 've linked your PR to the issue ! I was looking into it since yesterday
:/

[RealHarshThakur]

Apologies. Hope you find another issue to work on

[erikgb]

Was this issue fully resolved by #51, and can be closed, or this there anything remaining?

[RealHarshThakur]

Yes, I think we can close this issue.

[erikgb]

/close

[k8s-ci-robot]

@erikgb: You can't close an active issue/PR unless you authored it or you are a collaborator.

In response to this:

/close

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.

[adrianludwin]

/close

[k8s-ci-robot]

@adrianludwin: Closing this issue.

In response to this:

/close

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.