Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Create PVC if possible even if the StorageClass is missing #2683

Merged
merged 2 commits into from
May 2, 2023
Merged

Create PVC if possible even if the StorageClass is missing #2683

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea9bc21
Create PVC if possible even if SC is missing
arnongilboa Mar 30, 2023
137ec17
CR fixes
arnongilboa Apr 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 6 additions & 4 deletions pkg/apiserver/webhooks/dataimportcron-validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -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)
}

Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any functional difference in the diff of this file or just style?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we moved from returning a slice of causes to a single cause in validateNameLength. This just changes the way that is handled, but functionally does look the same.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the previous one was a bit clumsy.

return causes
}

Expand Down
35 changes: 28 additions & 7 deletions pkg/apiserver/webhooks/datavolume-validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -564,6 +567,22 @@ 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 || *sc == "" {
return nil
}

return validateNameLength(*sc, kvalidation.DNS1123SubdomainMaxLength)
}

func validateStorageSize(spec *cdiv1.DataVolumeSpec, field *k8sfield.Path) (*metav1.StatusCause, bool) {
var name string
var resources v1.ResourceRequirements
Expand Down Expand Up @@ -633,6 +652,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 {
Expand Down Expand Up @@ -682,9 +703,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)
}

Expand Down
44 changes: 37 additions & 7 deletions pkg/apiserver/webhooks/datavolume-validate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"fmt"
"net/http"
"net/http/httptest"
"strings"

. "github.com/onsi/ginkgo"
. "github.com/onsi/ginkgo/extensions/table"
Expand All @@ -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"
Expand All @@ -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")
Expand Down Expand Up @@ -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 accept DataVolume with Storage spec blank StorageClassName", pointer.String(""), true),
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 accept DataVolume with PVC spec blank StorageClassName", pointer.String(""), true),
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)
Expand Down Expand Up @@ -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() {
Expand Down
7 changes: 5 additions & 2 deletions pkg/controller/common/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,8 +354,11 @@ func GetStorageClassByName(client client.Client, name *string) (*storagev1.Stora
if name != nil {
storageClass := &storagev1.StorageClass{}
if err := client.Get(context.TODO(), types.NamespacedName{Name: *name}, storageClass); err != nil {
if k8serrors.IsNotFound(err) {
return nil, 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.Errorf("unable to retrieve storage class %s", *name)
}
return storageClass, nil
}
Expand Down Expand Up @@ -392,7 +395,7 @@ func GetFilesystemOverheadForStorageClass(client client.Client, storageClassName
}

targetStorageClass, err := GetStorageClassByName(client, storageClassName)
if err != nil {
if err != nil || targetStorageClass == nil {
klog.V(3).Info("Storage class", storageClassName, "not found, trying default storage class")
targetStorageClass, err = GetStorageClassByName(client, nil)
if err != nil {
Expand Down
7 changes: 5 additions & 2 deletions pkg/controller/datavolume/conditions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a unit test for this change where message != ""?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

if pvc != nil {
pvcCondition := getPVCCondition(pvc.GetAnnotations())
switch pvc.Status.Phase {
Expand All @@ -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
Expand Down
30 changes: 21 additions & 9 deletions pkg/controller/datavolume/conditions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -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))
Expand All @@ -156,11 +156,23 @@ var _ = Describe("updateBoundCondition", func() {
Expect(condition.Status).To(Equal(corev1.ConditionUnknown))
})

It("should create condition with message if one passed", func() {
message := "message"
conditions := make([]cdiv1.DataVolumeCondition, 0)
conditions = updateBoundCondition(conditions, nil, message, "")
Expect(len(conditions)).To(Equal(2))
condition := FindConditionByType(cdiv1.DataVolumeBound, conditions)
Expect(condition.Type).To(Equal(cdiv1.DataVolumeBound))
Expect(condition.Message).To(Equal(message))
Expect(condition.Reason).To(Equal(NotFound))
Expect(condition.Status).To(Equal(corev1.ConditionUnknown))
})

It("should be bound if PVC bound", 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))
Expand All @@ -173,7 +185,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))
Expand All @@ -186,7 +198,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))
Expand All @@ -204,7 +216,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))
Expand All @@ -222,7 +234,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))
Expand All @@ -241,7 +253,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))
Expand All @@ -259,7 +271,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))
Expand Down