vms-admitter: remove validation of vm clone volume from webhook #12628

ShellyKa13 · 2024-08-19T14:48:24Z

When having this check in the webhook we are preventing the creation of VMs which their datasource might not exist yet. Furthermore there are cases like in restore of backup where the vms volumes are already populated and dont event need to create the datavolume hence for sure no need to prevent the VM from being created in absence of such source.
This fix aims to fix bug: https://issues.redhat.com/browse/CNV-43272 While also make our product more aligned with eventual consistency model.

Note: to clarify the removed authentication from the webhook is still being done in the vm reconcile loop before creating the datavolumes

Fixes # Jira-ticket: https://issues.redhat.com/browse/CNV-43272

Why we need it and why it was done in this way

The following tradeoffs were made:

The following alternatives were considered:

Links to places where the discussion took place:

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

VMs admitter: remove validation of vm clone volume from the webhook

ShellyKa13 · 2024-08-19T14:49:18Z

/sig storage
/sig code-quality

ShellyKa13 · 2024-08-19T14:49:39Z

/cc @mhenriks

mhenriks · 2024-08-21T20:37:17Z

/hold

Let's get #12547 in first so we have a PR that we can easily backport

Then this one will only hit main and release 1.4+

ShellyKa13 · 2024-08-28T14:08:31Z

/unhold
@mhenriks I wrote to you also in the PR you mentioned #12547
I think this PR fixes several issues related to creating a vm with dvtemplate with clone source which the other PR doesnt solve, hence I think we can and should backport this PR instead and drop PR #12547

mhenriks · 2024-08-28T14:12:15Z

@ShellyKa13 Not good form for you to unhold a PR you did not put a hold on

ShellyKa13 · 2024-08-28T14:28:25Z

/hold
You are right. Sorry.

mhenriks · 2024-10-08T18:46:50Z

tests/storage/datavolume.go

+				vm, err := virtClient.VirtualMachine(vm.Namespace).Create(context.Background(), vm, metav1.CreateOptions{})
+				Expect(err).ToNot(HaveOccurred())
+				defer func() {
+					err := virtClient.VirtualMachine(vm.Namespace).Delete(context.Background(), vm.Name, metav1.DeleteOptions{})


Can you remind me why this defer is necessary?

not sure why I added it, can't think of a reason why would it be needed since the vm should be cleaned up when the test ends anyways.. Will remove it..

mhenriks · 2024-10-10T13:54:27Z

/retest-required

mhenriks · 2024-10-12T00:05:32Z

/test pull-kubevirt-e2e-k8s-1.30-sig-storage

mhenriks · 2024-10-13T23:18:21Z

/approve

kubevirt-bot · 2024-10-13T23:18:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mhenriks

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~~ [mhenriks]

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

EdDev · 2024-10-14T07:14:47Z

@arnongilboa , can you please have a look at this change?

arnongilboa · 2024-10-14T10:22:45Z

@arnongilboa , can you please have a look at this change?

sure, it's in my queue

arnongilboa · 2024-10-15T10:17:46Z

tests/storage/datavolume.go

@@ -1025,30 +1025,6 @@ var _ = SIGDescribe("DataVolume Integration", func() {
 				Entry("with explicit role (all namespaces) snapshot clone", explicitCloneRole, true, false, snapshotCloneMutateFunc, false),
 				Entry("with explicit role (one namespace) snapshot clone", explicitCloneRole, false, true, snapshotCloneMutateFunc, false),
 			)
-
-			It("should skip authorization when DataVolume already exists", func() {


Why is this test irrelevant anymore? shouldn't we test it even after dropping the webhook validation?

This was part of a PR of Micheal which is mostly since he added this logic to the webhook, but I guess I can keep this test since it can test the vm watch as well

arnongilboa · 2024-10-15T10:20:55Z

pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter_test.go

@@ -1602,48 +1594,6 @@ var _ = Describe("Validating VM Admitter", func() {
 			Entry("when everything suppied with 'sa' service account", "ns1", "ns2", "ns3", "sa", "ns3", "ns2", "sa"),
 		)

-		It("should successfully authorize clone with existing DataVolume", func() {


Why is this utest irrelevant anymore? shouldn't we test it even after dropping the webhook validation?

the auth is checked in the vm watch ut. no need to test it here anymore since the webhook doesnt take it into account anymore

arnongilboa · 2024-10-15T10:25:14Z

pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter_test.go

@@ -1537,195 +1532,6 @@ var _ = Describe("Validating VM Admitter", func() {
 			Expect(causes).To(HaveLen(1))
 			Expect(causes[0].Field).To(Equal("fake"))
 		})
-
-		DescribeTable("should successfully authorize clone", func(arNamespace, vmNamespace, sourceNamespace,


Why is this utest irrelevant anymore? shouldn't we check clone authorization from same / differnt ns?

This is being tested in the vm watch

arnongilboa · 2024-10-15T10:38:27Z

tests/storage/datavolume.go


-				// stop vm
-				vm = libvmops.StopVirtualMachine(vm)
+				libvmops.StartVirtualMachine(vm)
 			},


intentionally removed the DV succeeded check covered by createVMSuccess? no need to stop as the ns is deleted?

I dont need to create the vm anymore since the webhook doesnt fail the creation, instead the VM is created I see that the condition I expect is there and then I start the vm in that function there is a check the VM started successfully which assures the dv succeeded.

…taVolume already exists" Since the change that I will be making removes the auth check alltogether from the webhook we dont need some of the changes that were recently in PR kubevirt#12547 Signed-off-by: Shelly Kagan <[email protected]>

When having this check in the webhook we are preventing the creation of VMs which they datasource might not exist yet. Furthermore there are cases like in restore of backup where the vms volumes are already populated and dont event need to create the datavolume hence for sure no neede to prevent the VM from being created in absence of such source. This fix aims to fix bug: https://issues.redhat.com/browse/CNV-43272 While also make our product more aligned with eventual consistency model. Signed-off-by: Shelly Kagan <[email protected]>

Signed-off-by: Shelly Kagan <[email protected]>

Now we expect the create action to succeed and have a condition to represent that there are permission issues with the creation of the DV. After adding the permission we see that the we can be started and running successfully. Signed-off-by: Shelly Kagan <[email protected]>

Signed-off-by: Shelly Kagan <[email protected]>

arnongilboa · 2024-10-27T14:25:41Z

/lgtm

kubevirt-commenter-bot · 2024-10-27T14:25:48Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-1.30-vgpu
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.30-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.29-sig-network
/test pull-kubevirt-e2e-k8s-1.29-sig-storage
/test pull-kubevirt-e2e-k8s-1.29-sig-compute
/test pull-kubevirt-e2e-k8s-1.29-sig-operator
/test pull-kubevirt-e2e-k8s-1.30-sig-network
/test pull-kubevirt-e2e-k8s-1.30-sig-storage
/test pull-kubevirt-e2e-k8s-1.30-sig-compute
/test pull-kubevirt-e2e-k8s-1.30-sig-operator

ShellyKa13 · 2024-10-28T10:22:57Z

/unhold
Was approved by @mhenriks which put the hold originally

ShellyKa13 · 2024-10-28T12:13:15Z

/retest

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Aug 19, 2024

kubevirt-bot requested review from 0xFelix and AlonaKaplan August 19, 2024 14:48

kubevirt-bot added the size/L label Aug 19, 2024

kubevirt-bot added sig/storage kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Aug 19, 2024

kubevirt-bot requested a review from mhenriks August 19, 2024 14:49

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 21, 2024

ShellyKa13 mentioned this pull request Aug 26, 2024

virt-api: skip clone auth check when DataVolume already exists #12547

Merged

8 tasks

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 28, 2024

kubevirt-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 28, 2024

ShellyKa13 force-pushed the remove-auth-web branch from 8cf8f61 to 504b467 Compare September 1, 2024 13:27

ShellyKa13 force-pushed the remove-auth-web branch from 504b467 to 4695584 Compare September 1, 2024 13:30

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Sep 1, 2024

mhenriks reviewed Oct 8, 2024

View reviewed changes

ShellyKa13 force-pushed the remove-auth-web branch from 83929f3 to 78b5c1c Compare October 9, 2024 11:29

ShellyKa13 requested a review from mhenriks October 9, 2024 11:30

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 13, 2024

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 13, 2024

arnongilboa reviewed Oct 15, 2024

View reviewed changes

ShellyKa13 added 5 commits October 27, 2024 16:16

tests: add to conditions matcher a match with expected message

25a8978

Signed-off-by: Shelly Kagan <[email protected]>

tests: some cosmetics and improvements

1f8de69

Signed-off-by: Shelly Kagan <[email protected]>

ShellyKa13 force-pushed the remove-auth-web branch from 78b5c1c to 1f8de69 Compare October 27, 2024 14:19

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 27, 2024

kubevirt-bot assigned arnongilboa Oct 27, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 27, 2024

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 28, 2024

kubevirt-bot merged commit 180c938 into kubevirt:main Oct 28, 2024
40 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vms-admitter: remove validation of vm clone volume from webhook #12628

vms-admitter: remove validation of vm clone volume from webhook #12628

ShellyKa13 commented Aug 19, 2024 •

edited

Loading

ShellyKa13 commented Aug 19, 2024

ShellyKa13 commented Aug 19, 2024

mhenriks commented Aug 21, 2024

ShellyKa13 commented Aug 28, 2024

mhenriks commented Aug 28, 2024

ShellyKa13 commented Aug 28, 2024

mhenriks Oct 8, 2024

ShellyKa13 Oct 9, 2024

ShellyKa13 Oct 9, 2024

mhenriks commented Oct 10, 2024

mhenriks commented Oct 12, 2024

mhenriks commented Oct 13, 2024

kubevirt-bot commented Oct 13, 2024

EdDev commented Oct 14, 2024

arnongilboa commented Oct 14, 2024

arnongilboa Oct 15, 2024

ShellyKa13 Oct 27, 2024

arnongilboa Oct 15, 2024

ShellyKa13 Oct 27, 2024

arnongilboa Oct 15, 2024

ShellyKa13 Oct 27, 2024

arnongilboa Oct 15, 2024 •

edited

Loading

ShellyKa13 Oct 27, 2024

arnongilboa commented Oct 27, 2024

kubevirt-commenter-bot commented Oct 27, 2024

ShellyKa13 commented Oct 28, 2024

ShellyKa13 commented Oct 28, 2024

vms-admitter: remove validation of vm clone volume from webhook #12628

vms-admitter: remove validation of vm clone volume from webhook #12628

Conversation

ShellyKa13 commented Aug 19, 2024 • edited Loading

Why we need it and why it was done in this way

Special notes for your reviewer

Checklist

Release note

ShellyKa13 commented Aug 19, 2024

ShellyKa13 commented Aug 19, 2024

mhenriks commented Aug 21, 2024

ShellyKa13 commented Aug 28, 2024

mhenriks commented Aug 28, 2024

ShellyKa13 commented Aug 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhenriks commented Oct 10, 2024

mhenriks commented Oct 12, 2024

mhenriks commented Oct 13, 2024

kubevirt-bot commented Oct 13, 2024

EdDev commented Oct 14, 2024

arnongilboa commented Oct 14, 2024

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

arnongilboa Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnongilboa commented Oct 27, 2024

kubevirt-commenter-bot commented Oct 27, 2024

ShellyKa13 commented Oct 28, 2024

ShellyKa13 commented Oct 28, 2024

ShellyKa13 commented Aug 19, 2024 •

edited

Loading

arnongilboa Oct 15, 2024 •

edited

Loading