Add OwnerReferencesPermissionEnforcement

[oshoval]

What this PR does / why we need it:
It will allow to detect RBAC errors that are enabled by default on OpenShift
but not by default on vanilla k8s clusters.
https://kubernetes.io/docs/reference/access-authn-authz/admission-controllers/#ownerreferencespermissionenforcement

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

• Design: A design document was considered and is present (link) or not required
• PR: The PR description is expressive enough and will help future contributors
• Code: Write code that humans can understand and Keep it simple
• Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
• Upgrade: Impact of this change on upgrade flows was considered and addressed if required
• Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
• Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
• Community: Announcement to kubevirt-dev was considered

Release note:

[kubevirt-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign awels for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[oshoval]

/hold
need to do the same for all the other providers

[oshoval]

/cc @brianmcarey @EdDev

Hi, are you interesting in this ?
it can find issues on k8s based provider before we test them on OpenShift
if so we need to apply it for all k8s versions on kubevirtci
Example of a bug kubevirt/ipam-extensions#54 (well this is on other provider, but just the example, that also can happen on kubevirtci vm based providers before this PR, had it on CNAO one time)

[EdDev]

/cc @brianmcarey @EdDev

Hi, are you interesting in this ? it can find issues on k8s based provider before we test them on OpenShift if so we need to apply it for all k8s versions on kubevirtci Example of a bug kubevirt/ipam-extensions#54 (well this is on other provider, but just the example, that also can happen on kubevirtci vm based providers before this PR, had it on CNAO one time)

Kubevirt is an U/S project and the reasoning to add something like this should be focused on the usefulness it has for it (and not for a D/S project). D/S projects have their own tests and rules.

If this validation improves Kubevirt security and stability, please reason for the exact benefits this validation has and how it helps the project, regardless of any D/S potential usage of it.

[oshoval]

/cc @brianmcarey @EdDev
Hi, are you interesting in this ? it can find issues on k8s based provider before we test them on OpenShift if so we need to apply it for all k8s versions on kubevirtci Example of a bug kubevirt/ipam-extensions#54 (well this is on other provider, but just the example, that also can happen on kubevirtci vm based providers before this PR, had it on CNAO one time)

Kubevirt is an U/S project and the reasoning to add something like this should be focused on the usefulness it has for it (and not for a D/S project). D/S projects have their own tests and rules.

If this validation improves Kubevirt security and stability, please reason for the exact benefits this validation has and how it helps the project, regardless of any D/S potential usage of it.

Well it helps us on upstream to stricly validate RBACs
for example it will enforce that setting BlockOwnerDeletion on an object, requires having update verb on the Owner /finalizers
This kind of requirement is set by default on OpenShift but not on k8s vanilla clusters, so it will be detectable only if one run Openshift (the tests at the end do run on OpenShift), or if someone enable this feature that this PR suggests.

All info in this short paragraph
https://kubernetes.io/docs/reference/access-authn-authz/admission-controllers/#ownerreferencespermissionenforcement

"This admission controller protects the access to the metadata.ownerReferences of an object so that only users with delete permission to the object can change it. This admission controller also protects the access to metadata.ownerReferences[x].blockOwnerDeletion of an object, so that only users with update permission to the finalizers subresource of the referenced owner can change it."

For me it could save time at the past when i developed CNAO rbacs optimization, and today a chance like this but oin kind saved me time to simulate an error that was found only on OpenShift, locally on kind.

[oshoval]

/test check-provision-k8s-1.28

[EdDev]

Well it helps us on upstream to stricly validate RBACs

For me it could save time at the past when i developed CNAO rbacs optimization, and today a chance like this but oin kind saved me time to simulate an error that was found only on OpenShift, locally on kind.

I tried to explain that you cannot reason such a change by naming a D/S project.

Please add to the PR and a short summary in the commit message why this change is beneficial to Kubevirt, how it helps it and what is gained.
You also need to explain and mention that it does not cause any problem with users that run the cluster without it.

Please add all the needed reasoning and convincing in the PR description and drop the mentioning of the D/S project. After you provide the convincing details, you could mention that some K8S distributions are using it by default (but to be clear, that is not the main reason that should be provided).

[oshoval]

Well it helps us on upstream to stricly validate RBACs
For me it could save time at the past when i developed CNAO rbacs optimization, and today a chance like this but oin kind saved me time to simulate an error that was found only on OpenShift, locally on kind.

I tried to explain that you cannot reason such a change by naming a D/S project.

Please add to the PR and a short summary in the commit message why this change is beneficial to Kubevirt, how it helps it and what is gained. You also need to explain and mention that it does not cause any problem with users that run the cluster without it.

Please add all the needed reasoning and convincing in the PR description and drop the mentioning of the D/S project. After you provide the convincing details, you could mention that some K8S distributions are using it by default (but to be clear, that is not the main reason that should be provided).

Ok thanks, closing now, will get to it in the future (need to finish few things)
(just waiting for check-provision-k8s-1.28 to finish first)

[oshoval]

before more changes, @brianmcarey wdyt about this change ?

[brianmcarey]

before more changes, @brianmcarey wdyt about this change ?

I would need a reason why this would be needed or useful to kubevirt as @EdDev mentioned.

[oshoval]

before more changes, @brianmcarey wdyt about this change ?

I would need a reason why this would be needed or useful to kubevirt as @EdDev mentioned.

Reasoning, and taking CNAO as example (but can also happen on kubevirt)
is that this way we are stricter on RBACs, which are already stricter on other K8S distributions)
so it makes us more robust

Given this info, would you / Edy want this going forward ?
I can just close it otherwise
Thanks

[oshoval]

Closing this now, i think it is totally fine to have a robust provider which is bit stricter as some k8s distributions are,
Will go back to this once there is more time and will address the comments (just prefer to close PRs instead having them dangling atm)
Thanks for the review

[oshoval]

Btw a generic solution can be to allow enabling additional admission-plugins via env vars
such as
KUBEVIRT_ADMISSION_PLUGINS=OwnerReferencesPermissionEnforcement (comma separated values list)
and / or even something more generic, allow overriding the kubeadm.conf file itself which can help while developing new providers or new features
anyhow we can start with the current approach of course,
wdyt?

[oshoval]

/hold cancel

drafing

[kubevirt-bot]

PR needs rebase.

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-sigs/prow repository.

[oshoval]

Since kubevirtci code bash was changed bigly this PR is not relevant anymore in the current state of it
in case i will have time in the future and see this is important i will be back for it