feat: Add VM delete protection #1199

jcanocan · 2025-01-08T11:26:14Z

What this PR does / why we need it:
VirtualMachine objects are often managed by automation, CLI commands, 3rd party tools, etc. These automations may result in deleting accidentally VMs that should have not been deleted. These deletions may lead to a service degradation or out of service. Moreover, the deleted VMs may lead to information loss if the underlining PVC is deleted as a result of a cascaded delete.

It adds the ability to protect VirtualMachine objects from being deleted. If the label kubevirt.io/vm-delete-protection is set to True, any attempt to delete the VM will be rejected by a VAP policy.

This protection enables a protection against non-intended VM deletions, providing security and confidence to users.

Which issue(s) this PR fixes:

Fixes # CNV-50741

Special notes for your reviewer:

Release note:

Enables delete protection to VirtualMachine objects

kubevirt-bot · 2025-01-08T11:26:18Z

[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 0xfelix 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

akrejcir

Why did you create a separate controllers instead of adding these new resources into an operand? We usually add new resources in operands. That way, they can be configured in the SSP resource.

Can you rename this PR and the commit? This is definitely not a chore.
Maybe something like:

feat: Add VM delete protection

akrejcir · 2025-01-08T13:35:38Z

internal/controllers/vap_controller.go

+			},
+			Validations: []admissionregistrationv1.Validation{
+				{
+					Expression:        `(!has(oldObject.metadata.labels) || !(variables.label in oldObject.metadata.labels) || !oldObject.metadata.labels[variables.label].matches('^(true|True)$'))`,


Would it be enough to test that the label exists, instead of checking its value?

I've considered clearer from the user point of view. My idea is: by forcing the user to add the "True" value, we make sure that user really wants the protection enabled, and I think it will avoid confusions. That being said, I'm not against it if we find it out better.

akrejcir · 2025-01-08T13:38:24Z

/hold

jcanocan · 2025-01-08T13:50:49Z

Why did you create a separate controllers instead of adding these new resources into an operand? We usually add new resources in operands. That way, they can be configured in the SSP resource.

It is my understanding that operands are better fit when you are using CRD than controllers. In this case, we are just using a build-in feature such as VAP and VAPB. Given that, I'm not particularly against adding it though an operand. In fact, I question this myself multiple times. As we consider best.

Can you rename this PR and the commit? This is definitely not a chore. Maybe something like:
feat: Add VM delete protection

You are absolutely right. Thanks.

akrejcir · 2025-01-08T14:07:04Z

Why did you create a separate controllers instead of adding these new resources into an operand? We usually add new resources in operands. That way, they can be configured in the SSP resource.

It is my understanding that operands are better fit when you are using CRD than controllers. In this case, we are just using a build-in feature such as VAP and VAPB. Given that, I'm not particularly against adding it though an operand. In fact, I question this myself multiple times. As we consider best.

I'm not sure what do you mean by "operands are better fit when you are using CRD than controllers". Can you explain it more?

The SSP controller deploys various resources based on what is configured in the SSP CR. VAP and VAPB are one of these resources. Currently we don't configure them, but maybe we can. Operands are an abstraction used to group related resources together in the code, so it is easier to understand. For example we deploy multiple ClusterRole objects, and it would be harder to understand if they were all in one package.

I would say that this is the exact use case to add a new operand.

jcanocan · 2025-01-08T14:27:58Z

Why did you create a separate controllers instead of adding these new resources into an operand? We usually add new resources in operands. That way, they can be configured in the SSP resource.

It is my understanding that operands are better fit when you are using CRD than controllers. In this case, we are just using a build-in feature such as VAP and VAPB. Given that, I'm not particularly against adding it though an operand. In fact, I question this myself multiple times. As we consider best.

I'm not sure what do you mean by "operands are better fit when you are using CRD than controllers". Can you explain it more?

The SSP controller deploys various resources based on what is configured in the SSP CR. VAP and VAPB are one of these resources. Currently we don't configure them, but maybe we can. Operands are an abstraction used to group related resources together in the code, so it is easier to understand. For example we deploy multiple ClusterRole objects, and it would be harder to understand if they were all in one package.

I would say that this is the exact use case to add a new operand.

All right! You convinced me! Thanks. Let's create an operand.

jcanocan · 2025-01-09T11:31:42Z

v2:

Dropped vap and vapb controllers.
Added a new operand to handle both vap and vapb resources.

internal/common/labels.go

internal/operands/vm-delete-protection/reconcile.go

tests/vm_deletion_protection_test.go

akrejcir · 2025-01-09T12:52:29Z

Please also add the new operand to the tests/cleanup_test.go, so it will check that cleanup works.

jcanocan · 2025-01-09T16:40:48Z

Please also add the new operand to the tests/cleanup_test.go, so it will check that cleanup works.

Added.

internal/operands/vm-delete-protection/resources.go

tests/vm_delete_protection_test.go

akrejcir · 2025-01-10T12:43:56Z

/unhold

akrejcir · 2025-01-10T15:17:03Z

internal/operands/vm-delete-protection/reconcile.go

+
+func New() operands.Operand {
+	if err := checkCelExpression(); err != nil {
+		panic("Invalid VM delete protection CEL expression")


Show also the error message in panic. It will be useful for debugging.

akrejcir · 2025-01-10T15:20:06Z

Can you also remove unneeded text from the PR's description?

It adds the ability to protect VirtualMachine objects from being deleted. If the label `kubevirt.io/vm-delete-protection` is set to `True`, any attempt to delete the VM will be rejected by a VAP policy. Signed-off-by: Javier Cano Cano <[email protected]>

sonarqubecloud · 2025-01-10T15:25:53Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

jcanocan · 2025-01-10T15:26:26Z

Can you also remove unneeded text from the PR's description?

Adjusted.
Is it better now?

openshift-ci · 2025-01-10T17:25:53Z

@jcanocan: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-upgrade-functests	`886d79d`	link	true	`/test e2e-upgrade-functests`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jan 8, 2025

kubevirt-bot requested review from akrejcir and codingben January 8, 2025 11:26

kubevirt-bot added the size/XL label Jan 8, 2025

akrejcir reviewed Jan 8, 2025

View reviewed changes

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 8, 2025

jcanocan force-pushed the vm-delete-protection branch from 57c3cc2 to e4f96d2 Compare January 8, 2025 13:51

jcanocan changed the title ~~chore(vm-delete-protection): Add VM delete protection controller~~ feat: Add VM delete protection Jan 8, 2025

jcanocan force-pushed the vm-delete-protection branch from e4f96d2 to 03ff41f Compare January 9, 2025 11:29

kubevirt-bot added size/L and removed size/XL labels Jan 9, 2025

akrejcir reviewed Jan 9, 2025

View reviewed changes

jcanocan force-pushed the vm-delete-protection branch from 03ff41f to e4d02a8 Compare January 9, 2025 16:37

akrejcir reviewed Jan 10, 2025

View reviewed changes

internal/operands/vm-delete-protection/resources.go Outdated Show resolved Hide resolved

tests/vm_delete_protection_test.go Outdated Show resolved Hide resolved

jcanocan force-pushed the vm-delete-protection branch from e4d02a8 to 2c8aa38 Compare January 10, 2025 11:54

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 10, 2025

jcanocan force-pushed the vm-delete-protection branch from 2c8aa38 to 4ff65b3 Compare January 10, 2025 13:48

akrejcir reviewed Jan 10, 2025

View reviewed changes

feat: Add VM delete protection

886d79d

It adds the ability to protect VirtualMachine objects from being deleted. If the label `kubevirt.io/vm-delete-protection` is set to `True`, any attempt to delete the VM will be rejected by a VAP policy. Signed-off-by: Javier Cano Cano <[email protected]>

jcanocan force-pushed the vm-delete-protection branch from 4ff65b3 to 886d79d Compare January 10, 2025 15:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add VM delete protection #1199

feat: Add VM delete protection #1199

jcanocan commented Jan 8, 2025 •

edited

Loading

kubevirt-bot commented Jan 8, 2025

akrejcir left a comment

akrejcir Jan 8, 2025

jcanocan Jan 8, 2025

akrejcir commented Jan 8, 2025

jcanocan commented Jan 8, 2025

akrejcir commented Jan 8, 2025 •

edited

Loading

jcanocan commented Jan 8, 2025

jcanocan commented Jan 9, 2025

akrejcir commented Jan 9, 2025

jcanocan commented Jan 9, 2025

akrejcir commented Jan 10, 2025

akrejcir Jan 10, 2025

jcanocan Jan 10, 2025

akrejcir commented Jan 10, 2025

sonarqubecloud bot commented Jan 10, 2025

jcanocan commented Jan 10, 2025

openshift-ci bot commented Jan 10, 2025

feat: Add VM delete protection #1199

Are you sure you want to change the base?

feat: Add VM delete protection #1199

Conversation

jcanocan commented Jan 8, 2025 • edited Loading

kubevirt-bot commented Jan 8, 2025

akrejcir left a comment

Choose a reason for hiding this comment

akrejcir Jan 8, 2025

Choose a reason for hiding this comment

jcanocan Jan 8, 2025

Choose a reason for hiding this comment

akrejcir commented Jan 8, 2025

jcanocan commented Jan 8, 2025

akrejcir commented Jan 8, 2025 • edited Loading

jcanocan commented Jan 8, 2025

jcanocan commented Jan 9, 2025

akrejcir commented Jan 9, 2025

jcanocan commented Jan 9, 2025

akrejcir commented Jan 10, 2025

akrejcir Jan 10, 2025

Choose a reason for hiding this comment

jcanocan Jan 10, 2025

Choose a reason for hiding this comment

akrejcir commented Jan 10, 2025

sonarqubecloud bot commented Jan 10, 2025

Quality Gate passed

jcanocan commented Jan 10, 2025

openshift-ci bot commented Jan 10, 2025

jcanocan commented Jan 8, 2025 •

edited

Loading

akrejcir commented Jan 8, 2025 •

edited

Loading