add source UID to VMSnapshot status and verify source matches target …

…when restoring

Signed-off-by: Michael Henriksen <[email protected]>

api/openapi-spec/swagger.json

@@ -11196,6 +11196,9 @@
      "readyToUse": {
       "type": "boolean"
      },
+     "sourceUID": {
+      "type": "string"
+     },
      "virtualMachineSnapshotContentName": {
       "type": "string"
      }

go.mod

@@ -1,7 +1,5 @@
 module kubevirt.io/kubevirt
 
-go 1.12
-
 require (
 	github.com/Azure/go-autorest/autorest v0.9.1 // indirect
 	github.com/Azure/go-autorest/autorest/adal v0.6.0 // indirect

pkg/virt-api/webhooks/validating-webhook/admitters/BUILD.bazel

@@ -33,6 +33,7 @@ go_library(
         "//vendor/k8s.io/apimachinery/pkg/api/validation:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/labels:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/types:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/validation:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
         "//vendor/kubevirt.io/containerized-data-importer/pkg/clone:go_default_library",
@@ -78,6 +79,7 @@ go_test(
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/types:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/intstr:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/validation:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",

pkg/virt-api/webhooks/validating-webhook/admitters/vmrestore-admitter.go

@@ -27,6 +27,7 @@ import (
 	"k8s.io/api/admission/v1beta1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
 	k8sfield "k8s.io/apimachinery/pkg/util/validation/field"
 
 	v1 "kubevirt.io/client-go/api/v1"
@@ -72,6 +73,7 @@ func (admitter *VMRestoreAdmitter) Admit(ar *v1beta1.AdmissionReview) *v1beta1.A
 
 	switch ar.Request.Operation {
 	case v1beta1.Create:
+		var targetUID *types.UID
 		targetField := k8sfield.NewPath("spec", "target")
 
 		if vmRestore.Spec.Target.APIGroup == nil {
@@ -82,40 +84,40 @@ func (admitter *VMRestoreAdmitter) Admit(ar *v1beta1.AdmissionReview) *v1beta1.A
 					Field:   targetField.Child("apiGroup").String(),
 				},
 			}
-			break
-		}
-
-		switch *vmRestore.Spec.Target.APIGroup {
-		case v1.GroupName:
-			switch vmRestore.Spec.Target.Kind {
-			case "VirtualMachine":
-				causes, err = admitter.validateCreateVM(targetField.Child("name"), ar.Request.Namespace, vmRestore.Spec.Target.Name)
-				if err != nil {
-					return webhookutils.ToAdmissionResponseError(err)
+		} else {
+			switch *vmRestore.Spec.Target.APIGroup {
+			case v1.GroupName:
+				switch vmRestore.Spec.Target.Kind {
+				case "VirtualMachine":
+					causes, targetUID, err = admitter.validateCreateVM(targetField.Child("name"), ar.Request.Namespace, vmRestore.Spec.Target.Name)
+					if err != nil {
+						return webhookutils.ToAdmissionResponseError(err)
+					}
+				default:
+					causes = []metav1.StatusCause{
+						{
+							Type:    metav1.CauseTypeFieldValueInvalid,
+							Message: "invalid kind",
+							Field:   targetField.Child("kind").String(),
+						},
+					}
 				}
 			default:
 				causes = []metav1.StatusCause{
 					{
 						Type:    metav1.CauseTypeFieldValueInvalid,
-						Message: "invalid kind",
-						Field:   targetField.Child("kind").String(),
+						Message: "invalid apiGroup",
+						Field:   targetField.Child("apiGroup").String(),
 					},
 				}
 			}
-		default:
-			causes = []metav1.StatusCause{
-				{
-					Type:    metav1.CauseTypeFieldValueInvalid,
-					Message: "invalid apiGroup",
-					Field:   targetField.Child("apiGroup").String(),
-				},
-			}
 		}
 
 		snapshotCauses, err := admitter.validateSnapshot(
 			k8sfield.NewPath("spec", "virtualMachineSnapshotName"),
 			ar.Request.Namespace,
 			vmRestore.Spec.VirtualMachineSnapshotName,
+			targetUID,
 		)
 		if err != nil {
 			return webhookutils.ToAdmissionResponseError(err)
@@ -153,7 +155,7 @@ func (admitter *VMRestoreAdmitter) Admit(ar *v1beta1.AdmissionReview) *v1beta1.A
 	return &reviewResponse
 }
 
-func (admitter *VMRestoreAdmitter) validateCreateVM(field *k8sfield.Path, namespace, name string) ([]metav1.StatusCause, error) {
+func (admitter *VMRestoreAdmitter) validateCreateVM(field *k8sfield.Path, namespace, name string) ([]metav1.StatusCause, *types.UID, error) {
 	vm, err := admitter.Client.VirtualMachine(namespace).Get(name, &metav1.GetOptions{})
 	if errors.IsNotFound(err) {
 		return []metav1.StatusCause{
@@ -162,11 +164,11 @@ func (admitter *VMRestoreAdmitter) validateCreateVM(field *k8sfield.Path, namesp
 				Message: fmt.Sprintf("VirtualMachine %q does not exist", name),
 				Field:   field.String(),
 			},
-		}, nil
+		}, nil, nil
 	}
 
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
 	var causes []metav1.StatusCause
@@ -180,10 +182,10 @@ func (admitter *VMRestoreAdmitter) validateCreateVM(field *k8sfield.Path, namesp
 		causes = append(causes, cause)
 	}
 
-	return causes, nil
+	return causes, &vm.UID, nil
 }
 
-func (admitter *VMRestoreAdmitter) validateSnapshot(field *k8sfield.Path, namespace, name string) ([]metav1.StatusCause, error) {
+func (admitter *VMRestoreAdmitter) validateSnapshot(field *k8sfield.Path, namespace, name string, targetUID *types.UID) ([]metav1.StatusCause, error) {
 	snapshot, err := admitter.Client.VirtualMachineSnapshot(namespace).Get(name, metav1.GetOptions{})
 	if errors.IsNotFound(err) {
 		return []metav1.StatusCause{
@@ -210,5 +212,14 @@ func (admitter *VMRestoreAdmitter) validateSnapshot(field *k8sfield.Path, namesp
 		causes = append(causes, cause)
 	}
 
+	if targetUID != nil && snapshot.Status != nil && snapshot.Status.SourceUID != nil && *targetUID != *snapshot.Status.SourceUID {
+		cause := metav1.StatusCause{
+			Type:    metav1.CauseTypeFieldValueInvalid,
+			Message: fmt.Sprintf("VirtualMachineSnapshot source UID is %q but target UID is %q", *snapshot.Status.SourceUID, *targetUID),
+			Field:   field.String(),
+		}
+		causes = append(causes, cause)
+	}
+
 	return causes, nil
 }

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

@@ -33,6 +33,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/types"
 
 	v1 "kubevirt.io/client-go/api/v1"
 	snapshotv1 "kubevirt.io/client-go/apis/snapshot/v1alpha1"
@@ -49,6 +50,7 @@ var _ = Describe("Validating VirtualMachineRestore Admitter", func() {
 		vmSnapshotName = "snapshot"
 	)
 
+	var vmUID types.UID = "vm-uid"
 	apiGroup := "kubevirt.io"
 
 	t := true
@@ -60,6 +62,7 @@ var _ = Describe("Validating VirtualMachineRestore Admitter", func() {
 			Namespace: "default",
 		},
 		Status: &snapshotv1.VirtualMachineSnapshotStatus{
+			SourceUID:  &vmUID,
 			ReadyToUse: &t,
 		},
 	}
@@ -112,11 +115,17 @@ var _ = Describe("Validating VirtualMachineRestore Admitter", func() {
 
 		It("should reject missing apigroup", func() {
 			restore := &snapshotv1.VirtualMachineRestore{
-				Spec: snapshotv1.VirtualMachineRestoreSpec{},
+				Spec: snapshotv1.VirtualMachineRestoreSpec{
+					Target: corev1.TypedLocalObjectReference{
+						Kind: "VirtualMachine",
+						Name: vmName,
+					},
+					VirtualMachineSnapshotName: vmSnapshotName,
+				},
 			}
 
 			ar := createRestoreAdmissionReview(restore)
-			resp := createTestVMRestoreAdmitter(config, nil, nil).Admit(ar)
+			resp := createTestVMRestoreAdmitter(config, nil, snapshot).Admit(ar)
 			Expect(resp.Allowed).To(BeFalse())
 			Expect(len(resp.Result.Details.Causes)).To(Equal(1))
 			Expect(resp.Result.Details.Causes[0].Field).To(Equal("spec.target.apiGroup"))
@@ -227,6 +236,7 @@ var _ = Describe("Validating VirtualMachineRestore Admitter", func() {
 				vm = &v1.VirtualMachine{
 					ObjectMeta: metav1.ObjectMeta{
 						Name: vmName,
+						UID:  vmUID,
 					},
 				}
 			})
@@ -339,6 +349,27 @@ var _ = Describe("Validating VirtualMachineRestore Admitter", func() {
 				Expect(resp.Result.Details.Causes[0].Field).To(Equal("spec.target.apiGroup"))
 			})
 
+			It("should reject invalid source ID", func() {
+				restore := &snapshotv1.VirtualMachineRestore{
+					Spec: snapshotv1.VirtualMachineRestoreSpec{
+						Target: corev1.TypedLocalObjectReference{
+							APIGroup: &apiGroup,
+							Kind:     "VirtualMachine",
+							Name:     vmName,
+						},
+						VirtualMachineSnapshotName: vmSnapshotName,
+					},
+				}
+
+				vm.UID = "foo"
+
+				ar := createRestoreAdmissionReview(restore)
+				resp := createTestVMRestoreAdmitter(config, vm, snapshot).Admit(ar)
+				Expect(resp.Allowed).To(BeFalse())
+				Expect(len(resp.Result.Details.Causes)).To(Equal(1))
+				Expect(resp.Result.Details.Causes[0].Field).To(Equal("spec.virtualMachineSnapshotName"))
+			})
+
 			It("should accept when VM is not running", func() {
 				restore := &snapshotv1.VirtualMachineRestore{
 					Spec: snapshotv1.VirtualMachineRestoreSpec{

pkg/virt-controller/watch/snapshot/BUILD.bazel

@@ -24,6 +24,7 @@ go_library(
         "//vendor/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/types:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/runtime:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/wait:go_default_library",
@@ -63,6 +64,7 @@ go_test(
         "//vendor/k8s.io/apimachinery/pkg/api/resource:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/types:go_default_library",
         "//vendor/k8s.io/client-go/kubernetes/fake:go_default_library",
         "//vendor/k8s.io/client-go/testing:go_default_library",
         "//vendor/k8s.io/client-go/tools/cache:go_default_library",

pkg/virt-controller/watch/snapshot/restore_test.go

@@ -64,7 +64,7 @@ var _ = Describe("Snapshot controlleer", func() {
 			{
 				VolumeName:                "disk1",
 				PersistentVolumeClaimName: "restore-uid-disk1",
-				VolumeSnapshotName:        "vmsnapshot-uid-volume-disk1",
+				VolumeSnapshotName:        "vmsnapshot-snapshot-uid-volume-disk1",
 			},
 		}
 	}
@@ -319,23 +319,6 @@ var _ = Describe("Snapshot controlleer", func() {
 				controller.processVMRestoreWorkItem()
 			})
 
-			It("should go to error state when restore deleted", func() {
-				r := createRestore()
-				rc := r.DeepCopy()
-				rc.ResourceVersion = "1"
-				rc.Status = &snapshotv1.VirtualMachineRestoreStatus{
-					Complete: &f,
-					Conditions: []snapshotv1.Condition{
-						newProgressingCondition(corev1.ConditionTrue, "Creating new PVCs"),
-						newReadyCondition(corev1.ConditionFalse, "Waiting for new PVCs"),
-					},
-				}
-				addVolumeRestores(rc)
-				expectVMRestoreUpdate(kubevirtClient, rc)
-				addVirtualMachineRestore(r)
-				controller.processVMRestoreWorkItem()
-			})
-
 			It("should create restore PVCs", func() {
 				r := createRestore()
 				r.Status = &snapshotv1.VirtualMachineRestoreStatus{

pkg/virt-controller/watch/snapshot/snapshot.go

@@ -30,6 +30,7 @@ import (
 	storagev1 "k8s.io/api/storage/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
 
 	kubevirtv1 "kubevirt.io/client-go/api/v1"
@@ -57,6 +58,7 @@ const (
 )
 
 type snapshotSource interface {
+	UID() types.UID
 	Locked() bool
 	Lock() (bool, error)
 	Unlock() error
@@ -114,7 +116,7 @@ func (ctrl *VMSnapshotController) updateVMSnapshot(vmSnapshot *snapshotv1.Virtua
 
 	// Make sure status is initialized
 	if vmSnapshot.Status == nil {
-		return 0, ctrl.updateSnapshotStatus(vmSnapshot)
+		return 0, ctrl.updateSnapshotStatus(vmSnapshot, nil)
 	}
 
 	source, err := ctrl.getSnapshotSource(vmSnapshot)
@@ -167,7 +169,7 @@ func (ctrl *VMSnapshotController) updateVMSnapshot(vmSnapshot *snapshotv1.Virtua
 		}
 	}
 
-	if err = ctrl.updateSnapshotStatus(vmSnapshot); err != nil {
+	if err = ctrl.updateSnapshotStatus(vmSnapshot, source); err != nil {
 		return 0, err
 	}
 
@@ -388,7 +390,7 @@ func (ctrl *VMSnapshotController) cleanupVMSnapshot(vmSnapshot *snapshotv1.Virtu
 
 	if vmSnapshotProgressing(vmSnapshot) {
 		// will put the snapshot in error state
-		return ctrl.updateSnapshotStatus(vmSnapshot)
+		return ctrl.updateSnapshotStatus(vmSnapshot, nil)
 	}
 
 	content, err := ctrl.getContent(vmSnapshot)
@@ -563,7 +565,7 @@ func (ctrl *VMSnapshotController) getVolumeSnapshotClass(storageClassName string
 	return "", fmt.Errorf("%d matching VolumeSnapshotClasses for %s", len(matches), storageClassName)
 }
 
-func (ctrl *VMSnapshotController) updateSnapshotStatus(vmSnapshot *snapshotv1.VirtualMachineSnapshot) error {
+func (ctrl *VMSnapshotController) updateSnapshotStatus(vmSnapshot *snapshotv1.VirtualMachineSnapshot, source snapshotSource) error {
 	f := false
 	vmSnapshotCpy := vmSnapshot.DeepCopy()
 	if vmSnapshotCpy.Status == nil {
@@ -572,6 +574,11 @@ func (ctrl *VMSnapshotController) updateSnapshotStatus(vmSnapshot *snapshotv1.Vi
 		}
 	}
 
+	if source != nil {
+		uid := source.UID()
+		vmSnapshotCpy.Status.SourceUID = &uid
+	}
+
 	if vmSnapshotCpy.DeletionTimestamp != nil {
 		// go into error state
 		if vmSnapshotProgressing(vmSnapshotCpy) {
@@ -660,6 +667,10 @@ func (ctrl *VMSnapshotController) getContent(vmSnapshot *snapshotv1.VirtualMachi
 	return obj.(*snapshotv1.VirtualMachineSnapshotContent), nil
 }
 
+func (s *vmSnapshotSource) UID() types.UID {
+	return s.vm.UID
+}
+
 func (s *vmSnapshotSource) Locked() bool {
 	return s.vm.Status.SnapshotInProgress != nil &&
 		*s.vm.Status.SnapshotInProgress == s.snapshot.Name &&