Skip to content

Commit

Permalink
Remove datavolume clone source validation (#3331)
Browse files Browse the repository at this point in the history 
* Remove sourceref validation

The source can be changed and updated, we want the error to be
presented by an event and have the datavolume pending instead
of preventing the creation of the datavolume.
We are already handling the option of the source not existing yet
so lets also handle all the validation in the controller.

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

* Move pvc clone specific functions to pvc-clone-controller

Now that the validation is no longer done in the webhook
no need for that code to be in the controller common util
file. Moved the UT accordingly.

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

* Move function to new Describe node

Will add in future commits other tests to that section

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

* pvc-clone-controller: make better event and update dv if validation fails

In order to make it more clear that the validation failed
we should add the reason of validation failure to the event
Added an update watch to the clone controller so if something changes
in the source the clone will reconcile. Hence no need to return an
error, the reconcile will be triggered if someting will change in the
source.

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

* snap-clone: always validate snapshot source

This commit adds missing validation due to the remove validation
from the dv webhook, but also fixes an existing missing validation
in cases where the clone was created before the source. In such case
the webhook would not validate the source since it didnt exist yet,
and then if the clone happened with populators then we would not
validate the source size at all.
We should validate the snapshot source before continuing with the clone
whether it is populator/legacy clones.
Moved all the validation to one function.
Updated the event to include the reason for the validation failure and
updated dv status accordingly.

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

* Cloner_test: remove clone to small size test

The test checks the rejection of the webhook
which no longer exists then clone to appropriate
size dv which should succeed which is done in
any other clone test.
Im not adding a test to check the reconcile loop
since it is covered in the UT.

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

* tests/datasource: remove wait for pvc

Now since we added the validation also in the populators
path we are not creating the pvc until the source exists.

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

---------

Signed-off-by: Shelly Kagan <[email protected]>
  • Loading branch information
@ShellyKa13
ShellyKa13 authored Jul 26, 2024
1 parent 5063ec6 commit 8dab107
Show file tree
Hide file tree
Showing 14 changed files with 500 additions and 598 deletions.
1 change: 0 additions & 1 deletion pkg/apiserver/webhooks/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ go_test(
"//pkg/controller/common:go_default_library",
"//staging/src/kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1:go_default_library",
"//vendor/github.com/appscode/jsonpatch:go_default_library",
"//vendor/github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1:go_default_library",
"//vendor/github.com/kubernetes-csi/external-snapshotter/client/v6/clientset/versioned/fake:go_default_library",
"//vendor/github.com/onsi/ginkgo/v2:go_default_library",
"//vendor/github.com/onsi/gomega:go_default_library",
Expand Down
90 changes: 0 additions & 90 deletions pkg/apiserver/webhooks/datavolume-validate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -218,12 +218,6 @@ func (wh *dataVolumeValidatingWebhook) validateDataVolumeSpec(request *admission
})
return causes
}
if request.Operation == admissionv1.Create {
cause := wh.validateDataVolumeSourcePVC(spec.Source.PVC, field.Child("source", "PVC"), spec)
if cause != nil {
causes = append(causes, *cause)
}
}
}
if spec.Source.Snapshot != nil {
if spec.Source.Snapshot.Namespace == "" || spec.Source.Snapshot.Name == "" {
Expand All @@ -234,12 +228,6 @@ func (wh *dataVolumeValidatingWebhook) validateDataVolumeSpec(request *admission
})
return causes
}
if request.Operation == admissionv1.Create {
cause := wh.validateDataVolumeSourceSnapshot(spec.Source.Snapshot, field.Child("source", "Snapshot"), spec)
if cause != nil {
causes = append(causes, *cause)
}
}
}

return causes
Expand Down Expand Up @@ -267,61 +255,6 @@ func (wh *dataVolumeValidatingWebhook) validateSourceRef(request *admissionv1.Ad
Field: field.Child("sourceRef", "Name").String(),
}
}
if request.Operation != admissionv1.Create {
return nil
}
ns := namespace
if spec.SourceRef.Namespace != nil && *spec.SourceRef.Namespace != "" {
ns = spec.SourceRef.Namespace
}
dataSource, err := wh.cdiClient.CdiV1beta1().DataSources(*ns).Get(context.TODO(), spec.SourceRef.Name, metav1.GetOptions{})
if err != nil {
if k8serrors.IsNotFound(err) {
return &metav1.StatusCause{
Type: metav1.CauseTypeFieldValueNotFound,
Message: fmt.Sprintf("SourceRef %s/%s/%s not found", spec.SourceRef.Kind, *ns, spec.SourceRef.Name),
Field: field.Child("sourceRef").String(),
}
}
return &metav1.StatusCause{
Message: err.Error(),
Field: field.Child("sourceRef").String(),
}
}
switch {
case dataSource.Spec.Source.PVC != nil:
return wh.validateDataVolumeSourcePVC(dataSource.Spec.Source.PVC, field.Child("sourceRef"), spec)
case dataSource.Spec.Source.Snapshot != nil:
return wh.validateDataVolumeSourceSnapshot(dataSource.Spec.Source.Snapshot, field.Child("sourceRef"), spec)
}

return &metav1.StatusCause{
Message: fmt.Sprintf("Empty source field in '%s'. DataSource may not be ready yet", dataSource.Name),
Field: field.Child("sourceRef").String(),
}
}

func (wh *dataVolumeValidatingWebhook) validateDataVolumeSourcePVC(PVC *cdiv1.DataVolumeSourcePVC, field *k8sfield.Path, spec *cdiv1.DataVolumeSpec) *metav1.StatusCause {
sourcePVC, err := wh.k8sClient.CoreV1().PersistentVolumeClaims(PVC.Namespace).Get(context.TODO(), PVC.Name, metav1.GetOptions{})
if err != nil {
// We allow the creation of a clone DV even if the source PVC doesn't exist.
// The validation will be completed once the source PVC is created.
if k8serrors.IsNotFound(err) {
return nil
}
return &metav1.StatusCause{
Message: err.Error(),
Field: field.String(),
}
}

if err := cc.ValidateClone(sourcePVC, spec); err != nil {
return &metav1.StatusCause{
Type: metav1.CauseTypeFieldValueInvalid,
Message: err.Error(),
Field: field.String(),
}
}

return nil
}
Expand Down Expand Up @@ -400,29 +333,6 @@ func validateDataSourceRef(dataSource *v1.TypedObjectReference, field *k8sfield.
return causes
}

func (wh *dataVolumeValidatingWebhook) validateDataVolumeSourceSnapshot(snapshot *cdiv1.DataVolumeSourceSnapshot, field *k8sfield.Path, spec *cdiv1.DataVolumeSpec) *metav1.StatusCause {
sourceSnapshot, err := wh.snapClient.SnapshotV1().VolumeSnapshots(snapshot.Namespace).Get(context.TODO(), snapshot.Name, metav1.GetOptions{})
if err != nil {
if k8serrors.IsNotFound(err) {
return nil
}
return &metav1.StatusCause{
Message: err.Error(),
Field: field.String(),
}
}

if err := cc.ValidateSnapshotClone(sourceSnapshot, spec); err != nil {
return &metav1.StatusCause{
Type: metav1.CauseTypeFieldValueInvalid,
Message: err.Error(),
Field: field.String(),
}
}

return nil
}

func validateStorageClassName(spec *cdiv1.DataVolumeSpec, field *k8sfield.Path) *metav1.StatusCause {
var sc *string

Expand Down
58 changes: 4 additions & 54 deletions pkg/apiserver/webhooks/datavolume-validate_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import (
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"

snapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1"
snapclientfake "github.com/kubernetes-csi/external-snapshotter/client/v6/clientset/versioned/fake"

admissionv1 "k8s.io/api/admission/v1"
Expand Down Expand Up @@ -620,55 +619,6 @@ var _ = Describe("Validating Webhook", func() {
Expect(resp.Allowed).To(BeTrue())
})

It("should reject snapshot clone when input size is lower than recommended restore size", func() {
size := resource.MustParse("1G")
snapshot := &snapshotv1.VolumeSnapshot{
ObjectMeta: metav1.ObjectMeta{
Name: "testsnap",
Namespace: testNamespace,
},
Status: &snapshotv1.VolumeSnapshotStatus{
RestoreSize: &size,
},
}
storage := &cdiv1.StorageSpec{
Resources: corev1.VolumeResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceStorage: resource.MustParse("500Mi"),
},
},
}
snapSource := &cdiv1.DataVolumeSource{
Snapshot: &cdiv1.DataVolumeSourceSnapshot{
Namespace: snapshot.Namespace,
Name: snapshot.Name,
},
}
dv := newDataVolumeWithStorageSpec("testDV", snapSource, nil, storage)
resp := validateDataVolumeCreateEx(dv, nil, nil, []runtime.Object{snapshot}, nil)
Expect(resp.Allowed).To(BeFalse())
})

It("should reject snapshot clone when no input size/recommended restore size", func() {
snapshot := &snapshotv1.VolumeSnapshot{
ObjectMeta: metav1.ObjectMeta{
Name: "testsnap",
Namespace: testNamespace,
},
Status: &snapshotv1.VolumeSnapshotStatus{},
}
storage := &cdiv1.StorageSpec{}
snapSource := &cdiv1.DataVolumeSource{
Snapshot: &cdiv1.DataVolumeSourceSnapshot{
Namespace: snapshot.Namespace,
Name: snapshot.Name,
},
}
dv := newDataVolumeWithStorageSpec("testDV", snapSource, nil, storage)
resp := validateDataVolumeCreateEx(dv, nil, nil, []runtime.Object{snapshot}, nil)
Expect(resp.Allowed).To(BeFalse())
})

DescribeTable("should", func(oldFinalCheckpoint bool, oldCheckpoints []string, newFinalCheckpoint bool, newCheckpoints []string, modifyDV func(*cdiv1.DataVolume), expectedSuccess bool, sourceFunc func() *cdiv1.DataVolumeSource) {
oldDV := newMultistageDataVolume("multi-stage", oldFinalCheckpoint, oldCheckpoints, sourceFunc)
oldBytes, _ := json.Marshal(&oldDV)
Expand Down Expand Up @@ -844,14 +794,14 @@ var _ = Describe("Validating Webhook", func() {
Entry("accept DataVolume with PVC and sourceRef missing namespace on create", &emptyNamespace),
)

It("should reject DataVolume with SourceRef on create if DataSource does not exist", func() {
It("should allow DataVolume with SourceRef on create if DataSource does not exist", func() {
ns := "testNamespace"
dataVolume := newDataSourceDataVolume("testDV", &ns, "test")
resp := validateDataVolumeCreate(dataVolume)
Expect(resp.Allowed).To(BeFalse())
Expect(resp.Allowed).To(BeTrue())
})

It("should reject DataVolume with SourceRef on create if DataSource exists but its PVC field is not populated", func() {
It("should allow DataVolume with SourceRef on create if DataSource exists but its PVC field is not populated", func() {
dataVolume := newDataSourceDataVolume("testDV", &testNamespace, "test")
dataSource := &cdiv1.DataSource{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -865,7 +815,7 @@ var _ = Describe("Validating Webhook", func() {
},
}
resp := validateDataVolumeCreateEx(dataVolume, nil, []runtime.Object{dataSource}, nil, nil)
Expect(resp.Allowed).To(BeFalse())
Expect(resp.Allowed).To(BeTrue())
})

It("should accept DataVolume with SourceRef on create if DataSource exists but PVC does not exist", func() {
Expand Down
84 changes: 13 additions & 71 deletions pkg/controller/common/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -966,83 +966,25 @@ func validateTokenData(tokenData *token.Payload, srcNamespace, srcName, targetNa
return nil
}

// validateContentTypes compares the content type of a clone DV against its source PVC's one
func validateContentTypes(sourcePVC *corev1.PersistentVolumeClaim, spec *cdiv1.DataVolumeSpec) (bool, cdiv1.DataVolumeContentType, cdiv1.DataVolumeContentType) {
sourceContentType := GetPVCContentType(sourcePVC)
targetContentType := spec.ContentType
if targetContentType == "" {
targetContentType = cdiv1.DataVolumeKubeVirt
}
return sourceContentType == targetContentType, sourceContentType, targetContentType
}

// ValidateClone compares a clone spec against its source PVC to validate its creation
func ValidateClone(sourcePVC *corev1.PersistentVolumeClaim, spec *cdiv1.DataVolumeSpec) error {
var targetResources corev1.VolumeResourceRequirements

valid, sourceContentType, targetContentType := validateContentTypes(sourcePVC, spec)
if !valid {
msg := fmt.Sprintf("Source contentType (%s) and target contentType (%s) do not match", sourceContentType, targetContentType)
return errors.New(msg)
}

isSizelessClone := false
explicitPvcRequest := spec.PVC != nil
if explicitPvcRequest {
targetResources = spec.PVC.Resources
} else {
targetResources = spec.Storage.Resources
// The storage size in the target DV can be empty
// when cloning using the 'Storage' API
if _, ok := targetResources.Requests[corev1.ResourceStorage]; !ok {
isSizelessClone = true
}
}

// TODO: Spec.Storage API needs a better more complex check to validate clone size - to account for fsOverhead
// simple size comparison will not work here
if (!isSizelessClone && GetVolumeMode(sourcePVC) == corev1.PersistentVolumeBlock) || explicitPvcRequest {
if err := ValidateRequestedCloneSize(sourcePVC.Spec.Resources, targetResources); err != nil {
return err
}
}

return nil
}

// ValidateSnapshotClone compares a snapshot clone spec against its source snapshot to validate its creation
func ValidateSnapshotClone(sourceSnapshot *snapshotv1.VolumeSnapshot, spec *cdiv1.DataVolumeSpec) error {
var sourceResources, targetResources corev1.VolumeResourceRequirements

// IsSnapshotValidForClone returns an error if the passed snapshot is not valid for cloning
func IsSnapshotValidForClone(sourceSnapshot *snapshotv1.VolumeSnapshot) error {
if sourceSnapshot.Status == nil {
return fmt.Errorf("no status on source snapshot, not possible to proceed")
return fmt.Errorf("no status on source snapshot yet")
}
size := sourceSnapshot.Status.RestoreSize
restoreSizeAvailable := size != nil && size.Sign() > 0
if restoreSizeAvailable {
sourceResources.Requests = corev1.ResourceList{corev1.ResourceStorage: *size}
if !IsSnapshotReady(sourceSnapshot) {
klog.V(3).Info("snapshot not ReadyToUse, while we allow this, probably going to be an issue going forward", "namespace", sourceSnapshot.Namespace, "name", sourceSnapshot.Name)
}

isSizelessClone := false
explicitPvcRequest := spec.PVC != nil
if explicitPvcRequest {
targetResources = spec.PVC.Resources
} else {
targetResources = spec.Storage.Resources
if _, ok := targetResources.Requests["storage"]; !ok {
isSizelessClone = true
if sourceSnapshot.Status.Error != nil {
errMessage := "no details"
if msg := sourceSnapshot.Status.Error.Message; msg != nil {
errMessage = *msg
}
return fmt.Errorf("snapshot in error state with msg: %s", errMessage)
}

if !isSizelessClone && restoreSizeAvailable {
// Sizes available, make sure user picked something bigger than minimal
if err := ValidateRequestedCloneSize(sourceResources, targetResources); err != nil {
return err
}
} else if isSizelessClone && !restoreSizeAvailable {
return fmt.Errorf("size not specified by user/provisioner, can't tell how much needed for restore")
if sourceSnapshot.Spec.VolumeSnapshotClassName == nil ||
*sourceSnapshot.Spec.VolumeSnapshotClassName == "" {
return fmt.Errorf("snapshot %s/%s does not have volume snap class populated, can't clone", sourceSnapshot.Name, sourceSnapshot.Namespace)
}

return nil
}

Expand Down
30 changes: 0 additions & 30 deletions pkg/controller/common/util_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,36 +30,6 @@ var _ = Describe("GetRequestedImageSize", func() {
})
})

var _ = Describe("validateContentTypes", func() {
getContentType := func(contentType string) cdiv1.DataVolumeContentType {
if contentType == "" {
return cdiv1.DataVolumeKubeVirt
}
return cdiv1.DataVolumeContentType(contentType)
}

DescribeTable("should return", func(sourceContentType, targetContentType string, expectedResult bool) {
sourcePvc := CreatePvc("testPVC", "default", map[string]string{AnnContentType: sourceContentType}, nil)
dvSpec := &cdiv1.DataVolumeSpec{}
dvSpec.ContentType = cdiv1.DataVolumeContentType(targetContentType)

validated, sourceContent, targetContent := validateContentTypes(sourcePvc, dvSpec)
Expect(validated).To(Equal(expectedResult))
Expect(sourceContent).To(Equal(getContentType(sourceContentType)))
Expect(targetContent).To(Equal(getContentType(targetContentType)))
},
Entry("true when using archive in source and target", string(cdiv1.DataVolumeArchive), string(cdiv1.DataVolumeArchive), true),
Entry("false when using archive in source and KubeVirt in target", string(cdiv1.DataVolumeArchive), string(cdiv1.DataVolumeKubeVirt), false),
Entry("false when using KubeVirt in source and archive in target", string(cdiv1.DataVolumeKubeVirt), string(cdiv1.DataVolumeArchive), false),
Entry("true when using KubeVirt in source and target", string(cdiv1.DataVolumeKubeVirt), string(cdiv1.DataVolumeKubeVirt), true),
Entry("true when using default in source and target", "", "", true),
Entry("true when using default in source and KubeVirt (explicit) in target", "", string(cdiv1.DataVolumeKubeVirt), true),
Entry("true when using KubeVirt (explicit) in source and default in target", string(cdiv1.DataVolumeKubeVirt), "", true),
Entry("false when using default in source and archive in target", "", string(cdiv1.DataVolumeArchive), false),
Entry("false when using archive in source and default in target", string(cdiv1.DataVolumeArchive), "", false),
)
})

var _ = Describe("GetStorageClassByName", func() {
It("Should return the default storage class name", func() {
client := CreateClient(
Expand Down
8 changes: 6 additions & 2 deletions pkg/controller/datavolume/clone-controller-base.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ const (
// CloneValidationFailed reports that a clone wasn't admitted by our validation mechanism (reason)
CloneValidationFailed = "CloneValidationFailed"
// MessageCloneValidationFailed reports that a clone wasn't admitted by our validation mechanism (message)
MessageCloneValidationFailed = "The clone doesn't meet the validation requirements"
MessageCloneValidationFailed = "The clone doesn't meet the validation requirements: %s"
// CloneWithoutSource reports that the source of a clone doesn't exists (reason)
CloneWithoutSource = "CloneWithoutSource"
// MessageCloneWithoutSource reports that the source of a clone doesn't exists (message)
Expand Down Expand Up @@ -519,6 +519,10 @@ func (r *CloneReconcilerBase) populateSourceIfSourceRef(dv *cdiv1.DataVolume) er
if err := r.client.Get(context.TODO(), types.NamespacedName{Name: dv.Spec.SourceRef.Name, Namespace: ns}, dataSource); err != nil {
return err
}
if dataSource.Spec.Source.PVC == nil && dataSource.Spec.Source.Snapshot == nil {
return errors.Errorf("Empty source field in '%s'. DataSource may not be ready yet", dataSource.Name)
}

dv.Spec.Source = &cdiv1.DataVolumeSource{
PVC: dataSource.Spec.Source.PVC,
Snapshot: dataSource.Spec.Source.Snapshot,
Expand Down Expand Up @@ -572,7 +576,7 @@ func addCloneWithoutSourceWatch(mgr manager.Manager, datavolumeController contro
predicate.Funcs{
CreateFunc: func(e event.CreateEvent) bool { return true },
DeleteFunc: func(e event.DeleteEvent) bool { return false },
UpdateFunc: func(e event.UpdateEvent) bool { return false },
UpdateFunc: func(e event.UpdateEvent) bool { return true },
})); err != nil {
return err
}
Expand Down
2 changes: 2 additions & 0 deletions pkg/controller/datavolume/controller-base.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"time"

"github.com/go-logr/logr"
snapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1"
"github.com/pkg/errors"

corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -111,6 +112,7 @@ type dvSyncState struct {
dvMutated *cdiv1.DataVolume
pvc *corev1.PersistentVolumeClaim
pvcSpec *corev1.PersistentVolumeClaimSpec
snapshot *snapshotv1.VolumeSnapshot
dvSyncResult
usePopulator bool
}
Expand Down
Loading

0 comments on commit 8dab107

Please sign in to comment.