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

Release note:

Enables delete protection to VirtualMachine objects

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

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

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

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

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

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

Adjusted.
Is it better now?

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

tests/vm_delete_protection_test.go

0xFelix

/approve

Thanks

kubevirt-bot · 2025-01-14T15:01:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 0xFelix

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~~ [0xFelix]

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

akrejcir · 2025-01-14T15:15:36Z

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

+		}).
+		StatusFunc(func(resource client.Object) common.ResourceStatus {
+			vap := resource.(*admissionregistrationv1.ValidatingAdmissionPolicy)
+			if vap.Status.TypeChecking != nil && len(vap.Status.TypeChecking.ExpressionWarnings) != 0 {


If vap.Status.TypeChecking == nil, it means that it is still in progress, so we should not consider it a success.
This is written as a comment in the api:

ssp-operator/vendor/k8s.io/api/admissionregistration/v1/types.go

Lines 162 to 165 in d03ad50

// The results of type checking for each expression.

// Presence of this field indicates the completion of the type checking.

// +optional

TypeChecking *TypeChecking `json:"typeChecking,omitempty" protobuf:"bytes,2,opt,name=typeChecking"`

Really nice catch! Added. PTAL.

akrejcir · 2025-01-14T15:18:10Z

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

+	It("should create a valid CEL expression", func() {
+		celEnv, err := cel.NewEnv()
+		Expect(err).ToNot(HaveOccurred())
+
+		_, issues := celEnv.Parse(vmDeleteProtectionCELExpression)
+		Expect(issues.Err()).ToNot(HaveOccurred())
+	})


Please change this to a call to operand.Reconcile(&request), and then check the syntax on the VAP object that was created. That way we are sure it tests the correct thing even if the private constant changes.

akrejcir · 2025-01-14T15:22:31Z

tests/vm_delete_protection_test.go

+
+	AfterEach(func() {
+		err := apiClient.Get(ctx, client.ObjectKeyFromObject(vm), vm)
+		Expect(err).To(Or(Not(HaveOccurred()), MatchError(errors.IsNotFound, "VM not found")))


Please use MatchError(errors.IsNotFound, "errors.IsNotFound"), so that the error message will be:

Expected ... to match error function errors.IsNotFound

Instead of

Expected ... to match error function VM not found

This is where the second parameter is used:

ssp-operator/vendor/github.com/onsi/gomega/matchers/match_error_matcher.go

Line 76 in d693813

return format.Message(actual, fmt.Sprintf("to match error function %s", matcher.FuncErrDescription[0]))

Ah, I see. Thanks. Adjusted.

akrejcir · 2025-01-14T15:26:18Z

tests/vm_delete_protection_test.go

+		vm = createVMWithDeleteProtection(labelValue)
+
+		Expect(apiClient.Delete(ctx, vm)).To(Succeed())
+		waitForDeletion(client.ObjectKeyFromObject(vm), &kubevirtv1.VirtualMachine{})


No need to waste time waiting for deletion, because the above function would fail, if VAP blocked the deletion.

akrejcir · 2025-01-14T15:26:36Z

tests/vm_delete_protection_test.go

+		vm = createVMWithLabels(nil)
+
+		Expect(apiClient.Delete(ctx, vm)).To(Succeed())
+		waitForDeletion(client.ObjectKeyFromObject(vm), &kubevirtv1.VirtualMachine{})


same as above

akrejcir · 2025-01-14T15:27:38Z

tests/vm_delete_protection_test.go

+	})
+
+	AfterEach(func() {
+		err := apiClient.Get(ctx, client.ObjectKeyFromObject(vm), vm)


if vm == nil, this whole logic should be skipped

akrejcir · 2025-01-14T15:28:23Z

tests/vm_delete_protection_test.go

+			}, env.ShortTimeout(), time.Second).Should(Succeed())
+
+			Expect(apiClient.Delete(ctx, vm)).To(Succeed())
+			waitForDeletion(client.ObjectKeyFromObject(vm), &kubevirtv1.VirtualMachine{})


set the vm = nil after the VM is deleted, so that we don't potentially reuse it in a test, if the test forgets to create a new VM.

akrejcir · 2025-01-15T10:53:48Z

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

+			vap := resource.(*admissionregistrationv1.ValidatingAdmissionPolicy)
+			if vap.Status.TypeChecking == nil {
+				return common.ResourceStatus{
+					Progressing: ptr.To("Delete protection VAP type checking in progress"),


Also the other fields need to be set. If the object is progressing, it means that it is not yet available and it is degraded.

akrejcir · 2025-01-15T10:55:08Z

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

+				return common.ResourceStatus{
+					Progressing: ptr.To("Delete protection VAP type checking in progress"),
+				}
+			} else if vap.Status.TypeChecking != nil && len(vap.Status.TypeChecking.ExpressionWarnings) != 0 {


The above block returns, so else and vap.Status.TypeChecking != nil are not needed.

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-15T11:02:52Z

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

akrejcir · 2025-01-15T11:03:03Z

/lgtm

jcanocan · 2025-01-15T13:32:39Z

/retest-required

codingben

I'm late as a reviewer due to sick leave. I think it looks good and can be merged, just a few nits about naming.

/lgtm

codingben · 2025-01-15T20:18:17Z

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

+	}
+}
+
+type VMDeleteProtection struct{}


nit: I'd prefer to name it as VirtualMachineDeleteProtection.

codingben · 2025-01-15T20:20:11Z

internal/common/labels.go

+	AppComponentTemplating           AppComponent = "templating"
+	AppComponentTektonPipelines      AppComponent = "tektonPipelines"
+	AppComponentTektonTasks          AppComponent = "tektonTasks"
+	AppComponentVMDeletionProtection AppComponent = "vmDeleteProtection"


nit: I'd suggest to stick to AppComponentVmDeleteProtection instead of AppComponentVMDeletionProtection.

codingben · 2025-01-15T20:25:17Z

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

+		Reconcile()
+}
+
+func reconcileVAPB(request *common.Request) (common.ReconcileResult, error) {


nit: I'd name it as reconcileVAPBinding and also the variables below then should be foundVapBinding and expectedVapBinding.

codingben · 2025-01-15T20:32:08Z

/test e2e-upgrade-functests

kubevirt-bot · 2025-01-15T20:32:10Z

@codingben: No presubmit jobs available for kubevirt/ssp-operator@main

In response to this:

/test e2e-upgrade-functests

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.

jcanocan · 2025-01-16T08:43:54Z

@codingben Sorry, I didn't have the time to address your comments before it got merged. I will in a follow-up.

codingben · 2025-01-16T09:15:01Z

@codingben Sorry, I didn't have the time to address your comments before it got merged. I will in a follow-up.

No need to sorry because it was only nits and I came late to the review cycle.

iholder101 · 2025-02-13T14:56:06Z

Hey @jcanocan,

Too bad I wasn't aware of this PR before it merged.
The design proposal for this excact approach was rejected with many concerns from multiple people: kubevirt/community#363.

I find it concerning that it was merged without addressing the concerns that were raised up there and that no one of the reviewers were pinged to look at this PR.

Am I missing something? Were the concerns addressed in any way?
I think we need to consider reverting this before users can lay hand on it.

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

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

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

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2025

jcanocan force-pushed the vm-delete-protection branch from e7a1d76 to c2d3f89 Compare January 13, 2025 15:04

0xFelix reviewed Jan 13, 2025

View reviewed changes

jcanocan force-pushed the vm-delete-protection branch from c2d3f89 to 69bbfa5 Compare January 14, 2025 10:17

0xFelix reviewed Jan 14, 2025

View reviewed changes

tests/vm_delete_protection_test.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 69bbfa5 to 5c60ead Compare January 14, 2025 11:08

0xFelix reviewed Jan 14, 2025

View reviewed changes

tests/vm_delete_protection_test.go Outdated Show resolved Hide resolved

jcanocan force-pushed the vm-delete-protection branch 2 times, most recently from c9dcddc to 1c2b9e7 Compare January 14, 2025 14:50

0xFelix reviewed Jan 14, 2025

View reviewed changes

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 14, 2025

akrejcir reviewed Jan 14, 2025

View reviewed changes

jcanocan force-pushed the vm-delete-protection branch from 1c2b9e7 to 7271722 Compare January 15, 2025 10:47

akrejcir reviewed Jan 15, 2025

View reviewed changes

feat: Add VM delete protection

0ab684c

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 7271722 to 0ab684c Compare January 15, 2025 11:02

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2025

codingben approved these changes Jan 15, 2025

View reviewed changes

kubevirt-bot assigned codingben Jan 15, 2025

kubevirt-bot merged commit aa0a5cc into kubevirt:main Jan 15, 2025
12 checks passed

jcanocan mentioned this pull request Feb 11, 2025

design-proposal: VM delete protection kubevirt/community#363

Open

8 tasks

	// The results of type checking for each expression.
	// Presence of this field indicates the completion of the type checking.
	// +optional
	TypeChecking *TypeChecking `json:"typeChecking,omitempty" protobuf:"bytes,2,opt,name=typeChecking"`

feat: Add VM delete protection #1199

feat: Add VM delete protection #1199

Conversation

jcanocan commented Jan 8, 2025 • edited Loading

akrejcir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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 commented Jan 10, 2025

jcanocan commented Jan 10, 2025

0xFelix left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Jan 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 15, 2025

Quality Gate passed

akrejcir commented Jan 15, 2025

jcanocan commented Jan 15, 2025

codingben left a comment

Choose a reason for hiding this comment

codingben Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codingben commented Jan 15, 2025

kubevirt-bot commented Jan 15, 2025

jcanocan commented Jan 16, 2025

codingben commented Jan 16, 2025

iholder101 commented Feb 13, 2025

jcanocan commented Jan 8, 2025 •

edited

Loading

akrejcir commented Jan 8, 2025 •

edited

Loading

codingben Jan 15, 2025 •

edited

Loading