Validate DV SC; update conditions if SC is missing

Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

pkg/apiserver/webhooks/dataimportcron-validate.go

@@ -40,6 +40,8 @@ type dataImportCronValidatingWebhook struct {
 }
 
 func (wh *dataImportCronValidatingWebhook) Admit(ar admissionv1.AdmissionReview) *admissionv1.AdmissionResponse {
+	var causes []metav1.StatusCause
+
 	if ar.Request.Resource.Group != cdiv1.CDIGroupVersionKind.Group || ar.Request.Resource.Resource != "dataimportcrons" {
 		klog.V(3).Infof("Got unexpected resource type %s", ar.Request.Resource.Resource)
 		return toAdmissionResponseError(fmt.Errorf("unexpected resource: %s", ar.Request.Resource.Resource))
@@ -52,8 +54,8 @@ func (wh *dataImportCronValidatingWebhook) Admit(ar admissionv1.AdmissionReview)
 		return toAdmissionResponseError(err)
 	}
 
-	causes := validateNameLength(cron.Name, validation.DNS1035LabelMaxLength)
-	if len(causes) > 0 {
+	if cause := validateNameLength(cron.Name, validation.DNS1035LabelMaxLength); cause != nil {
+		causes = append(causes, *cause)
 		return toRejectedAdmissionResponse(causes)
 	}
 
@@ -152,8 +154,8 @@ func (wh *dataImportCronValidatingWebhook) validateDataImportCronSpec(request *a
 		return causes
 	}
 
-	causes = validateNameLength(spec.ManagedDataSource, validation.DNS1035LabelMaxLength)
-	if len(causes) > 0 {
+	if cause := validateNameLength(spec.ManagedDataSource, validation.DNS1035LabelMaxLength); cause != nil {
+		causes = append(causes, *cause)
 		return causes
 	}
 

pkg/apiserver/webhooks/datavolume-validate.go

@@ -63,16 +63,15 @@ func validateSourceURL(sourceURL string) string {
 	return ""
 }
 
-func validateNameLength(name string, maxLen int) []metav1.StatusCause {
-	var causes []metav1.StatusCause
+func validateNameLength(name string, maxLen int) *metav1.StatusCause {
 	if len(name) > maxLen {
-		causes = append(causes, metav1.StatusCause{
+		return &metav1.StatusCause{
 			Type:    metav1.CauseTypeFieldValueInvalid,
 			Message: fmt.Sprintf("Name cannot be longer than %d characters", maxLen),
 			Field:   "",
-		})
+		}
 	}
-	return causes
+	return nil
 }
 
 func (wh *dataVolumeValidatingWebhook) validateDataVolumeSpec(request *admissionv1.AdmissionRequest, field *k8sfield.Path, spec *cdiv1.DataVolumeSpec, namespace *string) []metav1.StatusCause {
@@ -104,6 +103,10 @@ func (wh *dataVolumeValidatingWebhook) validateDataVolumeSpec(request *admission
 		causes = append(causes, *cause)
 		return causes
 	}
+	if cause := validateStorageClassName(spec, field); cause != nil {
+		causes = append(causes, *cause)
+		return causes
+	}
 
 	if spec.PVC != nil {
 		dataSourceRef = spec.PVC.DataSourceRef
@@ -523,6 +526,28 @@ func (wh *dataVolumeValidatingWebhook) validateDataVolumeSourceSnapshot(snapshot
 	return nil
 }
 
+func validateStorageClassName(spec *cdiv1.DataVolumeSpec, field *k8sfield.Path) *metav1.StatusCause {
+	var sc *string
+
+	if spec.PVC != nil {
+		sc = spec.PVC.StorageClassName
+	} else if spec.Storage != nil {
+		sc = spec.Storage.StorageClassName
+	}
+
+	if sc == nil {
+		return nil
+	}
+	if *sc == "" {
+		return &metav1.StatusCause{
+			Type:    metav1.CauseTypeFieldValueInvalid,
+			Message: fmt.Sprintf("storageClassName is empty"),
+			Field:   field.Child("PVC", "storageClassName").String(),
+		}
+	}
+	return validateNameLength(*sc, kvalidation.DNS1123SubdomainMaxLength)
+}
+
 func validateStorageSize(spec *cdiv1.DataVolumeSpec, field *k8sfield.Path) (*metav1.StatusCause, bool) {
 	var name string
 	var resources v1.ResourceRequirements
@@ -592,6 +617,8 @@ func validateExternalPopulation(spec *cdiv1.DataVolumeSpec, field *k8sfield.Path
 }
 
 func (wh *dataVolumeValidatingWebhook) Admit(ar admissionv1.AdmissionReview) *admissionv1.AdmissionResponse {
+	var causes []metav1.StatusCause
+
 	klog.V(3).Infof("Got AdmissionReview %+v", ar)
 
 	if err := validateDataVolumeResource(ar); err != nil {
@@ -641,9 +668,9 @@ func (wh *dataVolumeValidatingWebhook) Admit(ar admissionv1.AdmissionReview) *ad
 		}
 	}
 
-	causes := validateNameLength(dv.Name, kvalidation.DNS1123SubdomainMaxLength)
-	if len(causes) > 0 {
+	if cause := validateNameLength(dv.Name, kvalidation.DNS1123SubdomainMaxLength); cause != nil {
 		klog.Infof("rejected DataVolume admission")
+		causes = append(causes, *cause)
 		return toRejectedAdmissionResponse(causes)
 	}
 

pkg/apiserver/webhooks/datavolume-validate_test.go

@@ -25,6 +25,7 @@ import (
 	"fmt"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/ginkgo/extensions/table"
@@ -38,6 +39,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	fakeclient "k8s.io/client-go/kubernetes/fake"
+	"k8s.io/utils/pointer"
 
 	snapclientfake "github.com/kubernetes-csi/external-snapshotter/client/v6/clientset/versioned/fake"
 	cdiclientfake "kubevirt.io/containerized-data-importer/pkg/client/clientset/versioned/fake"
@@ -51,6 +53,8 @@ var (
 )
 
 var _ = Describe("Validating Webhook", func() {
+	longName := "name-is-longer-than-253-" + strings.Repeat("0123456789", 23)
+
 	Context("with DataVolume admission review", func() {
 		It("should accept DataVolume with HTTP source on create", func() {
 			dataVolume := newHTTPDataVolume("testDV", "http://www.example.com")
@@ -215,16 +219,43 @@ var _ = Describe("Validating Webhook", func() {
 		})
 
 		It("should reject DataVolume with name length greater than 253 characters", func() {
-			longName := "the-name-length-of-this-datavolume-is-greater-then-253-characters" +
-				"123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-" +
-				"123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789-123456789"
-			dataVolume := newHTTPDataVolume(
-				longName,
-				"http://www.example.com")
+			dataVolume := newHTTPDataVolume(longName, "http://www.example.com")
 			resp := validateDataVolumeCreate(dataVolume)
 			Expect(resp.Allowed).To(Equal(false))
 		})
 
+		DescribeTable("should", func(scName *string, expected bool) {
+			httpSource := &cdiv1.DataVolumeSource{
+				HTTP: &cdiv1.DataVolumeSourceHTTP{URL: "http://www.example.com"},
+			}
+			storage := &cdiv1.StorageSpec{
+				Resources: corev1.ResourceRequirements{
+					Requests: corev1.ResourceList{
+						corev1.ResourceStorage: resource.MustParse("500Mi"),
+					},
+				},
+				StorageClassName: scName,
+			}
+			dv := newDataVolumeWithStorageSpec("testDV", httpSource, nil, storage)
+			resp := validateDataVolumeCreate(dv)
+			Expect(resp.Allowed).To(Equal(expected))
+		},
+			Entry("should reject DataVolume with Storage spec blank StorageClassName", pointer.String(""), false),
+			Entry("should reject DataVolume with Storage spec too long StorageClassName", pointer.String(longName), false),
+			Entry("should accept DataVolume with Storage spec empty StorageClassName", nil, true),
+		)
+
+		DescribeTable("should", func(scName *string, expected bool) {
+			dv := newHTTPDataVolume("testDV", "http://www.example.com")
+			dv.Spec.PVC.StorageClassName = scName
+			resp := validateDataVolumeCreate(dv)
+			Expect(resp.Allowed).To(Equal(expected))
+		},
+			Entry("should reject DataVolume with PVC spec blank StorageClassName", pointer.String(""), false),
+			Entry("should reject DataVolume with PVC spec too long StorageClassName", pointer.String(longName), false),
+			Entry("should accept DataVolume with PVC spec empty StorageClassName", nil, true),
+		)
+
 		It("should reject DataVolume source with invalid URL on create", func() {
 			dataVolume := newHTTPDataVolume("testDV", "invalidurl")
 			resp := validateDataVolumeCreate(dataVolume)
@@ -277,7 +308,6 @@ var _ = Describe("Validating Webhook", func() {
 			dataVolume.Spec.ContentType = "invalid"
 			resp := validateDataVolumeCreate(dataVolume)
 			Expect(resp.Allowed).To(Equal(false))
-
 		})
 
 		It("should accept DataVolume with archive contentType", func() {

pkg/controller/common/util.go

@@ -338,7 +338,7 @@ func GetStorageClassByName(client client.Client, name *string) (*storagev1.Stora
 		storageClass := &storagev1.StorageClass{}
 		if err := client.Get(context.TODO(), types.NamespacedName{Name: *name}, storageClass); err != nil {
 			klog.V(3).Info("Unable to retrieve storage class", "storage class name", *name)
-			return nil, errors.New("unable to retrieve storage class")
+			return nil, errors.New(fmt.Sprintf("unable to retrieve storage class %s", *name))
 		}
 		return storageClass, nil
 	}

pkg/controller/datavolume/conditions.go

@@ -122,7 +122,7 @@ func UpdateReadyCondition(conditions []cdiv1.DataVolumeCondition, status corev1.
 	return updateCondition(conditions, cdiv1.DataVolumeReady, status, message, reason)
 }
 
-func updateBoundCondition(conditions []cdiv1.DataVolumeCondition, pvc *corev1.PersistentVolumeClaim, reason string) []cdiv1.DataVolumeCondition {
+func updateBoundCondition(conditions []cdiv1.DataVolumeCondition, pvc *corev1.PersistentVolumeClaim, message, reason string) []cdiv1.DataVolumeCondition {
 	if pvc != nil {
 		pvcCondition := getPVCCondition(pvc.GetAnnotations())
 		switch pvc.Status.Phase {
@@ -149,10 +149,13 @@ func updateBoundCondition(conditions []cdiv1.DataVolumeCondition, pvc *corev1.Pe
 			conditions = UpdateReadyCondition(conditions, corev1.ConditionFalse, "", "")
 		}
 	} else {
+		if message == "" {
+			message = "No PVC found"
+		}
 		if reason == "" {
 			reason = cc.NotFound
 		}
-		conditions = updateCondition(conditions, cdiv1.DataVolumeBound, corev1.ConditionUnknown, "No PVC found", reason)
+		conditions = updateCondition(conditions, cdiv1.DataVolumeBound, corev1.ConditionUnknown, message, reason)
 		conditions = UpdateReadyCondition(conditions, corev1.ConditionFalse, "", "")
 	}
 	return conditions

pkg/controller/datavolume/conditions_test.go

@@ -135,7 +135,7 @@ var _ = Describe("updateReadyCondition", func() {
 var _ = Describe("updateBoundCondition", func() {
 	It("should create condition if it doesn't exist", func() {
 		conditions := make([]cdiv1.DataVolumeCondition, 0)
-		conditions = updateBoundCondition(conditions, nil, "")
+		conditions = updateBoundCondition(conditions, nil, "", "")
 		Expect(len(conditions)).To(Equal(2))
 		condition := FindConditionByType(cdiv1.DataVolumeBound, conditions)
 		Expect(condition.Type).To(Equal(cdiv1.DataVolumeBound))
@@ -147,7 +147,7 @@ var _ = Describe("updateBoundCondition", func() {
 	It("should create condition with reason if it doesn't exist", func() {
 		reason := "exceeded quota"
 		conditions := make([]cdiv1.DataVolumeCondition, 0)
-		conditions = updateBoundCondition(conditions, nil, reason)
+		conditions = updateBoundCondition(conditions, nil, "", reason)
 		Expect(len(conditions)).To(Equal(2))
 		condition := FindConditionByType(cdiv1.DataVolumeBound, conditions)
 		Expect(condition.Type).To(Equal(cdiv1.DataVolumeBound))
@@ -160,7 +160,7 @@ var _ = Describe("updateBoundCondition", func() {
 		conditions := make([]cdiv1.DataVolumeCondition, 0)
 		pvc := CreatePvc("test", corev1.NamespaceDefault, nil, nil)
 		pvc.Status.Phase = corev1.ClaimBound
-		conditions = updateBoundCondition(conditions, pvc, "")
+		conditions = updateBoundCondition(conditions, pvc, "", "")
 		Expect(len(conditions)).To(Equal(1))
 		condition := FindConditionByType(cdiv1.DataVolumeBound, conditions)
 		Expect(condition.Type).To(Equal(cdiv1.DataVolumeBound))
@@ -173,7 +173,7 @@ var _ = Describe("updateBoundCondition", func() {
 		conditions := make([]cdiv1.DataVolumeCondition, 0)
 		pvc := CreatePvc("test", corev1.NamespaceDefault, map[string]string{AnnBoundCondition: "true"}, nil)
 		pvc.Status.Phase = corev1.ClaimBound
-		conditions = updateBoundCondition(conditions, pvc, "")
+		conditions = updateBoundCondition(conditions, pvc, "", "")
 		Expect(len(conditions)).To(Equal(1))
 		condition := FindConditionByType(cdiv1.DataVolumeBound, conditions)
 		Expect(condition.Type).To(Equal(cdiv1.DataVolumeBound))
@@ -186,7 +186,7 @@ var _ = Describe("updateBoundCondition", func() {
 		conditions := make([]cdiv1.DataVolumeCondition, 0)
 		pvc := CreatePvc("test", corev1.NamespaceDefault, map[string]string{AnnBoundCondition: "false", AnnBoundConditionReason: "not bound", AnnBoundConditionMessage: "scratch PVC not bound"}, nil)
 		pvc.Status.Phase = corev1.ClaimBound
-		conditions = updateBoundCondition(conditions, pvc, "")
+		conditions = updateBoundCondition(conditions, pvc, "", "")
 		Expect(len(conditions)).To(Equal(2))
 		condition := FindConditionByType(cdiv1.DataVolumeBound, conditions)
 		Expect(condition.Type).To(Equal(cdiv1.DataVolumeBound))
@@ -204,7 +204,7 @@ var _ = Describe("updateBoundCondition", func() {
 		conditions := make([]cdiv1.DataVolumeCondition, 0)
 		pvc := CreatePvc("test", corev1.NamespaceDefault, nil, nil)
 		pvc.Status.Phase = corev1.ClaimPending
-		conditions = updateBoundCondition(conditions, pvc, "")
+		conditions = updateBoundCondition(conditions, pvc, "", "")
 		Expect(len(conditions)).To(Equal(2))
 		condition := FindConditionByType(cdiv1.DataVolumeBound, conditions)
 		Expect(condition.Type).To(Equal(cdiv1.DataVolumeBound))
@@ -222,7 +222,7 @@ var _ = Describe("updateBoundCondition", func() {
 		conditions := make([]cdiv1.DataVolumeCondition, 0)
 		pvc := CreatePvc("test", corev1.NamespaceDefault, map[string]string{AnnBoundCondition: "true"}, nil)
 		pvc.Status.Phase = corev1.ClaimPending
-		conditions = updateBoundCondition(conditions, pvc, "")
+		conditions = updateBoundCondition(conditions, pvc, "", "")
 		Expect(len(conditions)).To(Equal(2))
 		condition := FindConditionByType(cdiv1.DataVolumeBound, conditions)
 		Expect(condition.Type).To(Equal(cdiv1.DataVolumeBound))
@@ -241,7 +241,7 @@ var _ = Describe("updateBoundCondition", func() {
 		conditions := make([]cdiv1.DataVolumeCondition, 0)
 		pvc := CreatePvc("test", corev1.NamespaceDefault, map[string]string{AnnBoundCondition: "false", AnnBoundConditionReason: reason, AnnBoundConditionMessage: "scratch PVC not bound"}, nil)
 		pvc.Status.Phase = corev1.ClaimPending
-		conditions = updateBoundCondition(conditions, pvc, "")
+		conditions = updateBoundCondition(conditions, pvc, "", "")
 		Expect(len(conditions)).To(Equal(2))
 		condition := FindConditionByType(cdiv1.DataVolumeBound, conditions)
 		Expect(condition.Type).To(Equal(cdiv1.DataVolumeBound))
@@ -259,7 +259,7 @@ var _ = Describe("updateBoundCondition", func() {
 		conditions := make([]cdiv1.DataVolumeCondition, 0)
 		pvc := CreatePvc("test", corev1.NamespaceDefault, nil, nil)
 		pvc.Status.Phase = corev1.ClaimLost
-		conditions = updateBoundCondition(conditions, pvc, "")
+		conditions = updateBoundCondition(conditions, pvc, "", "")
 		Expect(len(conditions)).To(Equal(2))
 		condition := FindConditionByType(cdiv1.DataVolumeBound, conditions)
 		Expect(condition.Type).To(Equal(cdiv1.DataVolumeBound))

pkg/controller/datavolume/controller-base.go

@@ -392,7 +392,7 @@ func (r *ReconcilerBase) syncDvPvcState(log logr.Logger, req reconcile.Request,
 		}
 	}
 
-	syncState.pvcSpec, err = renderPvcSpec(r.client, r.recorder, log, dv)
+	syncState.pvcSpec, err = r.renderPvcSpec(&syncState, dv, log)
 	if err != nil {
 		return syncState, err
 	}
@@ -433,6 +433,14 @@ func (r *ReconcilerBase) syncUpdate(log logr.Logger, syncState *dvSyncState) err
 	return nil
 }
 
+func (r *ReconcilerBase) renderPvcSpec(syncState *dvSyncState, dv *cdiv1.DataVolume, log logr.Logger) (*corev1.PersistentVolumeClaimSpec, error) {
+	pvcSpec, err := renderPvcSpec(r.client, r.recorder, log, dv)
+	if err != nil {
+		r.syncDataVolumeStatusPhaseWithEvent(syncState, cdiv1.PhaseUnset, nil, Event{corev1.EventTypeWarning, cc.ErrClaimNotValid, err.Error()})
+	}
+	return pvcSpec, err
+}
+
 func (r *ReconcilerBase) handleStaticVolume(syncState *dvSyncState, log logr.Logger) error {
 	if _, ok := syncState.dvMutated.Annotations[cc.AnnCheckStaticVolume]; !ok {
 		return nil
@@ -690,10 +698,12 @@ func (r *ReconcilerBase) updateDataVolumeStatusPhaseWithEvent(
 	dataVolumeCopy.Status.Phase = phase
 
 	reason := ""
+	message := ""
 	if pvc == nil {
 		reason = event.reason
+		message = event.message
 	}
-	r.updateConditions(dataVolumeCopy, pvc, reason)
+	r.updateConditions(dataVolumeCopy, pvc, reason, message)
 	return r.emitEvent(dataVolume, dataVolumeCopy, curPhase, dataVolume.Status.Conditions, &event)
 }
 
@@ -780,11 +790,11 @@ func (r ReconcilerBase) updateStatus(req reconcile.Request, phaseSync *statusPha
 
 	currentCond := make([]cdiv1.DataVolumeCondition, len(dataVolumeCopy.Status.Conditions))
 	copy(currentCond, dataVolumeCopy.Status.Conditions)
-	r.updateConditions(dataVolumeCopy, pvc, "")
+	r.updateConditions(dataVolumeCopy, pvc, "", "")
 	return result, r.emitEvent(dv, dataVolumeCopy, curPhase, currentCond, &event)
 }
 
-func (r *ReconcilerBase) updateConditions(dataVolume *cdiv1.DataVolume, pvc *corev1.PersistentVolumeClaim, reason string) {
+func (r *ReconcilerBase) updateConditions(dataVolume *cdiv1.DataVolume, pvc *corev1.PersistentVolumeClaim, reason, message string) {
 	var anno map[string]string
 
 	if dataVolume.Status.Conditions == nil {
@@ -807,8 +817,8 @@ func (r *ReconcilerBase) updateConditions(dataVolume *cdiv1.DataVolume, pvc *cor
 		readyStatus = corev1.ConditionFalse
 	}
 
-	dataVolume.Status.Conditions = updateBoundCondition(dataVolume.Status.Conditions, pvc, reason)
-	dataVolume.Status.Conditions = UpdateReadyCondition(dataVolume.Status.Conditions, readyStatus, "", reason)
+	dataVolume.Status.Conditions = updateBoundCondition(dataVolume.Status.Conditions, pvc, message, reason)
+	dataVolume.Status.Conditions = UpdateReadyCondition(dataVolume.Status.Conditions, readyStatus, message, reason)
 	dataVolume.Status.Conditions = updateRunningCondition(dataVolume.Status.Conditions, anno)
 }