Address review comments

Signed-off-by: Alvaro Romero <alromero@redhat.com>
kubevirt · Jun 30, 2023 · cde28c5 · cde28c5
1 parent b836759
commit cde28c5
Show file tree

Hide file tree

Showing 4 changed files with 40 additions and 58 deletions.
diff --git a/pkg/apiserver/webhooks/BUILD.bazel b/pkg/apiserver/webhooks/BUILD.bazel
@@ -38,6 +38,7 @@ go_library(
         "//vendor/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
         "//vendor/k8s.io/client-go/kubernetes:go_default_library",
         "//vendor/k8s.io/klog/v2:go_default_library",
+        "//vendor/k8s.io/utils/pointer:go_default_library",
         "//vendor/kubevirt.io/controller-lifecycle-operator-sdk/api:go_default_library",
     ],
 )

diff --git a/pkg/apiserver/webhooks/populators-validate.go b/pkg/apiserver/webhooks/populators-validate.go
@@ -133,46 +133,13 @@ func (wh *populatorValidatingWebhook) validateVolumeImportSource(ar admissionv1.
 		return nil, err
 	}
 
-	// Reject spec updates
 	if ar.Request.Operation == admissionv1.Update {
-<<<<<<< Updated upstream
-		oldSource := cdiv1.VolumeImportSource{}
-		err = json.Unmarshal(ar.Request.OldObject.Raw, &oldSource)
-		if err != nil {
-			return nil, err
-		}
-
-		// Always admit checkpoint updates for multi-stage migrations.
-		multiStageAdmitted := false
-		isMultiStage := volumeImportSource.Spec.Source != nil && len(volumeImportSource.Spec.Checkpoints) > 0 &&
-			(volumeImportSource.Spec.Source.VDDK != nil || volumeImportSource.Spec.Source.Imageio != nil)
-		if isMultiStage {
-			oldSpec := oldSource.Spec.DeepCopy()
-			oldSpec.FinalCheckpoint = false
-			oldSpec.Checkpoints = nil
-
-			newSpec := volumeImportSource.Spec.DeepCopy()
-			newSpec.FinalCheckpoint = false
-			newSpec.Checkpoints = nil
-
-			multiStageAdmitted = apiequality.Semantic.DeepEqual(newSpec, oldSpec)
-		}
-
-		if !multiStageAdmitted && !apiequality.Semantic.DeepEqual(volumeImportSource.Spec, oldSource.Spec) {
-			klog.Errorf("Cannot update spec for VolumeImportSource %s/%s", volumeImportSource.GetNamespace(), volumeImportSource.GetName())
-			return []metav1.StatusCause{{
-				Type:    metav1.CauseTypeFieldValueDuplicate,
-				Message: "Cannot update VolumeImportSource Spec",
-				Field:   k8sfield.NewPath("VolumeImportSource").Child("Spec").String(),
-			}}, nil
-=======
 		cause, err := wh.validateVolumeImportSourceUpdate(ar, &volumeImportSource)
 		if err != nil {
 			return nil, err
 		}
 		if cause != nil {
 			return cause, nil
->>>>>>> Stashed changes
 		}
 	}
 
@@ -201,13 +168,7 @@ func (wh *populatorValidatingWebhook) validateVolumeImportSourceSpec(field *k8sf
 	}
 
 	// validate multi-stage import
-<<<<<<< Updated upstream
-	isMultiStage := spec.Source != nil && len(spec.Checkpoints) > 0 &&
-		(spec.Source.VDDK != nil || spec.Source.Imageio != nil)
-	if isMultiStage && spec.TargetClaim == "" {
-=======
 	if isMultiStageImport(spec) && (spec.TargetClaim == nil || *spec.TargetClaim == "") {
->>>>>>> Stashed changes
 		return []metav1.StatusCause{{
 			Type:    metav1.CauseTypeFieldValueInvalid,
 			Message: "Unable to do multi-stage import without specifying a target claim",
@@ -240,8 +201,6 @@ func (wh *populatorValidatingWebhook) validateVolumeImportSourceSpec(field *k8sf
 	// Should never reach this return
 	return nil
 }
-<<<<<<< Updated upstream
-=======
 
 func (wh *populatorValidatingWebhook) validateVolumeImportSourceUpdate(ar admissionv1.AdmissionReview, volumeImportSource *cdiv1.VolumeImportSource) ([]metav1.StatusCause, error) {
 	oldSource := cdiv1.VolumeImportSource{}
@@ -277,4 +236,3 @@ func isMultiStageImport(spec *cdiv1.VolumeImportSourceSpec) bool {
 	return spec.Source != nil && len(spec.Checkpoints) > 0 &&
 		(spec.Source.VDDK != nil || spec.Source.Imageio != nil)
 }
->>>>>>> Stashed changes
diff --git a/pkg/apiserver/webhooks/populators-validate_test.go b/pkg/apiserver/webhooks/populators-validate_test.go
@@ -112,6 +112,43 @@ var _ = Describe("Validating Webhook", func() {
 			Expect(resp.Allowed).To(BeFalse())
 		})
 
+		It("should accept VolumeImportSource spec checkpoints update", func() {
+			source := &cdiv1.ImportSourceType{
+				HTTP: &cdiv1.DataVolumeSourceHTTP{
+					URL: "http://www.example.com",
+				},
+			}
+			importCR := newVolumeImportSource(cdiv1.DataVolumeKubeVirt, source)
+			importCR.Spec.Checkpoints = []cdiv1.DataVolumeCheckpoint{
+				{Current: "test", Previous: ""},
+			}
+			newBytes, _ := json.Marshal(&importCR)
+
+			oldSource := importCR.DeepCopy()
+			oldSource.Spec.Source.HTTP.URL = "http://www.example.es"
+			oldSource.Spec.Checkpoints = nil
+			oldBytes, _ := json.Marshal(oldSource)
+
+			ar := &admissionv1.AdmissionReview{
+				Request: &admissionv1.AdmissionRequest{
+					Operation: admissionv1.Update,
+					Resource: metav1.GroupVersionResource{
+						Group:    cdiv1.SchemeGroupVersion.Group,
+						Version:  cdiv1.SchemeGroupVersion.Version,
+						Resource: "volumeimportsources",
+					},
+					Object: runtime.RawExtension{
+						Raw: newBytes,
+					},
+					OldObject: runtime.RawExtension{
+						Raw: oldBytes,
+					},
+				},
+			}
+			resp := validatePopulatorsAdmissionReview(ar)
+			Expect(resp.Allowed).To(BeFalse())
+		})
+
 		It("should accept VolumeImportSource with HTTP source on create", func() {
 			source := &cdiv1.ImportSourceType{
 				HTTP: &cdiv1.DataVolumeSourceHTTP{
@@ -145,13 +182,7 @@ var _ = Describe("Validating Webhook", func() {
 			Expect(resp.Allowed).To(BeFalse())
 		})
 
-<<<<<<< Updated upstream
-		It("should VolumeImportSource with incomplete VDDK source", func() {
-=======
-<<<<<<< Updated upstream
-=======
 		It("should reject VolumeImportSource with incomplete VDDK source", func() {
->>>>>>> Stashed changes
 			source := &cdiv1.ImportSourceType{
 				VDDK: &cdiv1.DataVolumeSourceVDDK{
 					BackingFile: "",
@@ -198,20 +229,12 @@ var _ = Describe("Validating Webhook", func() {
 			importCR.Spec.Checkpoints = []cdiv1.DataVolumeCheckpoint{
 				{Current: "test", Previous: ""},
 			}
-<<<<<<< Updated upstream
-			importCR.Spec.TargetClaim = "test-pvc"
-=======
 			targetClaim := "test-pvc"
 			importCR.Spec.TargetClaim = &targetClaim
->>>>>>> Stashed changes
 			resp := validateVolumeImportSourceCreate(importCR)
 			Expect(resp.Allowed).To(BeTrue())
 		})
 
-<<<<<<< Updated upstream
-=======
->>>>>>> Stashed changes
->>>>>>> Stashed changes
 		It("should accept VolumeImportSource with Registry source URL on create", func() {
 			url := "docker://registry:5000/test"
 			source := &cdiv1.ImportSourceType{

diff --git a/pkg/controller/datavolume/import-controller.go b/pkg/controller/datavolume/import-controller.go
@@ -197,7 +197,7 @@ func (r *ImportReconciler) syncImport(log logr.Logger, req reconcile.Request) (d
 	pvcModifier := r.updateAnnotations
 	if syncState.usePopulator {
 		if syncState.dvMutated.Status.Phase != cdiv1.Succeeded {
-			err := r.createVolumeImportSourceCR(&syncState)
+			err := r.reconcileVolumeImportSourceCR(&syncState)
 			if err != nil {
 				return syncState, err
 			}
@@ -315,7 +315,7 @@ func volumeImportSourceName(dv *cdiv1.DataVolume) string {
 	return fmt.Sprintf("%s-%s", volumeImportSourcePrefix, dv.UID)
 }
 
-func (r *ImportReconciler) createVolumeImportSourceCR(syncState *dvSyncState) error {
+func (r *ImportReconciler) reconcileVolumeImportSourceCR(syncState *dvSyncState) error {
 	dv := syncState.dvMutated
 	importSource := &cdiv1.VolumeImportSource{}
 	importSourceName := volumeImportSourceName(dv)