diff --git a/pkg/apiserver/webhooks/BUILD.bazel b/pkg/apiserver/webhooks/BUILD.bazel index a77af35dc2..a4f296f226 100644 --- a/pkg/apiserver/webhooks/BUILD.bazel +++ b/pkg/apiserver/webhooks/BUILD.bazel @@ -55,6 +55,7 @@ go_test( "datavolume-mutate_test.go", "datavolume-validate_test.go", "populators-validate_test.go", + "pvc-mutate_test.go", "transfer-validate_test.go", "webhook_suite_test.go", ], @@ -71,6 +72,7 @@ go_test( "//vendor/k8s.io/api/admission/v1:go_default_library", "//vendor/k8s.io/api/authorization/v1:go_default_library", "//vendor/k8s.io/api/core/v1:go_default_library", + "//vendor/k8s.io/api/storage/v1:go_default_library", "//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", @@ -78,6 +80,7 @@ go_test( "//vendor/k8s.io/client-go/testing:go_default_library", "//vendor/k8s.io/utils/ptr:go_default_library", "//vendor/kubevirt.io/controller-lifecycle-operator-sdk/api:go_default_library", + "//vendor/sigs.k8s.io/controller-runtime/pkg/client:go_default_library", "//vendor/sigs.k8s.io/controller-runtime/pkg/client/fake:go_default_library", ], ) diff --git a/pkg/apiserver/webhooks/pvc-mutate_test.go b/pkg/apiserver/webhooks/pvc-mutate_test.go new file mode 100644 index 0000000000..02dad345b3 --- /dev/null +++ b/pkg/apiserver/webhooks/pvc-mutate_test.go @@ -0,0 +1,185 @@ +/* + * This file is part of the CDI project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * Copyright 2024 Red Hat, Inc. + * + */ + +package webhooks + +import ( + "encoding/json" + "fmt" + "sort" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/appscode/jsonpatch" + + admissionv1 "k8s.io/api/admission/v1" + corev1 "k8s.io/api/core/v1" + storagev1 "k8s.io/api/storage/v1" + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/utils/ptr" + + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + cdiv1 "kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1" + "kubevirt.io/containerized-data-importer/pkg/common" +) + +var _ = Describe("Mutating PVC Webhook", func() { + Context("with PVC admission review", func() { + It("should reject review without request", func() { + ar := &admissionv1.AdmissionReview{} + resp := mutatePvc(ar) + Expect(resp.Allowed).To(BeFalse()) + Expect(resp.Result.Message).Should(Equal("AdmissionReview.Request is nil")) + }) + + const testStorageClassName = "sc_test" + + var ( + storageClass = storagev1.StorageClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: testStorageClassName, + }, + } + defaultStorageClass = storagev1.StorageClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: testStorageClassName, + Annotations: map[string]string{ + "storageclass.kubernetes.io/is-default-class": "true", + }, + }, + } + storageProfile = cdiv1.StorageProfile{ + ObjectMeta: metav1.ObjectMeta{Name: testStorageClassName}, + Status: cdiv1.StorageProfileStatus{ + ClaimPropertySets: []cdiv1.ClaimPropertySet{{ + VolumeMode: ptr.To[corev1.PersistentVolumeMode](corev1.PersistentVolumeBlock), + AccessModes: []corev1.PersistentVolumeAccessMode{corev1.ReadWriteMany}, + }}, + }, + } + partialStorageProfile = cdiv1.StorageProfile{ + ObjectMeta: metav1.ObjectMeta{Name: testStorageClassName}, + Status: cdiv1.StorageProfileStatus{ + ClaimPropertySets: []cdiv1.ClaimPropertySet{{ + VolumeMode: ptr.To[corev1.PersistentVolumeMode](corev1.PersistentVolumeBlock), + }}, + }, + } + ) + + DescribeTable("should", func(allowed bool, message string, objs ...client.Object) { + pvc := newPvc() + dvBytes, _ := json.Marshal(&pvc) + + ar := &admissionv1.AdmissionReview{ + Request: &admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Resource: metav1.GroupVersionResource{ + Group: corev1.SchemeGroupVersion.Group, + Version: corev1.SchemeGroupVersion.Version, + Resource: "persistentvolumeclaims", + }, + Object: runtime.RawExtension{ + Raw: dvBytes, + }, + }, + } + + resp := mutatePvc(ar, objs...) + + if !allowed { + Expect(resp.Allowed).To(BeFalse()) + Expect(resp.Result).ToNot(BeNil()) + Expect(resp.Result.Message).To(Equal(message)) + return + } + + Expect(resp.Allowed).To(BeTrue()) + Expect(resp.Result).To(BeNil()) + Expect(resp.Patch).ToNot(BeNil()) + + var patchObjs []jsonpatch.Operation + err := json.Unmarshal(resp.Patch, &patchObjs) + Expect(err).ToNot(HaveOccurred()) + Expect(patchObjs).Should(HaveLen(3)) + + sort.Slice(patchObjs, func(i, j int) bool { + return patchObjs[i].Path < patchObjs[j].Path + }) + + Expect(patchObjs[0].Operation).Should(Equal("add")) + Expect(patchObjs[0].Path).Should(Equal("/spec/accessModes")) + accessModes, ok := patchObjs[0].Value.([]interface{}) + Expect(ok).Should(BeTrue()) + Expect(accessModes).Should(HaveLen(1)) + Expect(accessModes[0]).Should(Equal("ReadWriteMany")) + + Expect(patchObjs[1].Operation).Should(Equal("add")) + Expect(patchObjs[1].Path).Should(Equal("/spec/storageClassName")) + Expect(patchObjs[1].Value).Should(Equal(testStorageClassName)) + + Expect(patchObjs[2].Operation).Should(Equal("add")) + Expect(patchObjs[2].Path).Should(Equal("/spec/volumeMode")) + Expect(patchObjs[2].Value).Should(Equal("Block")) + }, + Entry("fail with no storage classes", false, + "PVC spec is missing accessMode and no storageClass to choose profile"), + Entry("fail with no default storage classes", false, + "PVC spec is missing accessMode and no storageClass to choose profile", &storageClass, &storageProfile), + Entry("fail with default storage classes but with partial storage profile", false, + fmt.Sprintf("no accessMode specified in StorageProfile %s", testStorageClassName), &defaultStorageClass, &partialStorageProfile), + Entry("succeed with default storage classes and complete storage profile", true, "", &defaultStorageClass, &storageProfile), + ) + }) +}) + +func newPvc() *corev1.PersistentVolumeClaim { + pvc := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testPvc", + Labels: map[string]string{common.PvcApplyStorageProfileLabel: "true"}, + }, + Spec: corev1.PersistentVolumeClaimSpec{ + Resources: corev1.VolumeResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceStorage: resource.MustParse("1G"), + }, + }, + }, + } + + return pvc +} + +func mutatePvc(ar *admissionv1.AdmissionReview, objs ...client.Object) *admissionv1.AdmissionResponse { + _ = storagev1.AddToScheme(scheme) + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(objs...). + Build() + + wh := NewPvcMutatingWebhook(fakeClient) + + return serve(ar, wh) +} diff --git a/pkg/controller/datavolume/util.go b/pkg/controller/datavolume/util.go index 906c521391..c92e1946d0 100644 --- a/pkg/controller/datavolume/util.go +++ b/pkg/controller/datavolume/util.go @@ -31,7 +31,6 @@ import ( k8serrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/cache" "k8s.io/client-go/tools/record" @@ -129,21 +128,22 @@ func pvcFromStorage(client client.Client, recorder record.EventRecorder, log log func renderPvcSpecVolumeModeAndAccessModesAndStorageClass(client client.Client, recorder record.EventRecorder, log *logr.Logger, dv *cdiv1.DataVolume, pvcSpec *v1.PersistentVolumeClaimSpec, dvContentType cdiv1.DataVolumeContentType) error { logInfo := func(msg string, keysAndValues ...interface{}) { - if log != nil { - log.V(1).Info(msg, keysAndValues...) + if log != nil && dv != nil { + keysAndValuesWithDv := append(keysAndValues, "namespace", dv.Namespace, "name", dv.Name) + log.V(1).Info(msg, keysAndValuesWithDv...) } } - recordEventf := func(object runtime.Object, eventtype, reason, messageFmt string, args ...interface{}) { - if recorder != nil { - recorder.Eventf(object, eventtype, reason, messageFmt, args...) + recordEventf := func(eventtype, reason, messageFmt string, args ...interface{}) { + if recorder != nil && dv != nil { + recorder.Eventf(dv, eventtype, reason, messageFmt, args...) } } if dvContentType == cdiv1.DataVolumeArchive { if pvcSpec.VolumeMode != nil && *pvcSpec.VolumeMode == v1.PersistentVolumeBlock { - logInfo("ContentType Archive cannot have block volumeMode", "namespace", dv.Namespace, "name", dv.Name) - recordEventf(dv, v1.EventTypeWarning, cc.ErrClaimNotValid, "ContentType Archive cannot have block volumeMode") + logInfo("ContentType Archive cannot have block volumeMode") + recordEventf(v1.EventTypeWarning, cc.ErrClaimNotValid, "ContentType Archive cannot have block volumeMode") return errors.Errorf("ContentType Archive cannot have block volumeMode") } pvcSpec.VolumeMode = ptr.To[v1.PersistentVolumeMode](v1.PersistentVolumeFilesystem) @@ -159,8 +159,8 @@ func renderPvcSpecVolumeModeAndAccessModesAndStorageClass(client client.Client, } // Not even default storageClass on the cluster, cannot apply the defaults, verify spec is ok if len(pvcSpec.AccessModes) == 0 { - logInfo("Cannot set accessMode for new pvc", "namespace", dv.Namespace, "name", dv.Name) - recordEventf(dv, v1.EventTypeWarning, cc.ErrClaimNotValid, MessageErrStorageClassNotFound) + logInfo("Cannot set accessMode for new pvc") + recordEventf(v1.EventTypeWarning, cc.ErrClaimNotValid, MessageErrStorageClassNotFound) return ErrStorageClassNotFound } return nil @@ -171,8 +171,8 @@ func renderPvcSpecVolumeModeAndAccessModesAndStorageClass(client client.Client, if (pvcSpec.VolumeMode == nil || *pvcSpec.VolumeMode == "") && len(pvcSpec.AccessModes) == 0 { accessModes, volumeMode, err := getDefaultVolumeAndAccessMode(client, storageClass) if err != nil { - logInfo("Cannot set accessMode and volumeMode for new pvc", "namespace", dv.Namespace, "name", dv.Name, "Error", err) - recordEventf(dv, v1.EventTypeWarning, cc.ErrClaimNotValid, + logInfo("Cannot set accessMode and volumeMode for new pvc", "Error", err) + recordEventf(v1.EventTypeWarning, cc.ErrClaimNotValid, fmt.Sprintf("Spec is missing accessMode and volumeMode, cannot get access mode from StorageProfile %s", getName(storageClass))) return err } @@ -181,8 +181,8 @@ func renderPvcSpecVolumeModeAndAccessModesAndStorageClass(client client.Client, } else if len(pvcSpec.AccessModes) == 0 { accessModes, err := getDefaultAccessModes(client, storageClass, pvcSpec.VolumeMode) if err != nil { - logInfo("Cannot set accessMode for new pvc", "namespace", dv.Namespace, "name", dv.Name, "Error", err) - recordEventf(dv, v1.EventTypeWarning, cc.ErrClaimNotValid, + logInfo("Cannot set accessMode for new pvc", "Error", err) + recordEventf(v1.EventTypeWarning, cc.ErrClaimNotValid, fmt.Sprintf("Spec is missing accessMode and cannot get access mode from StorageProfile %s", getName(storageClass))) return err } @@ -360,7 +360,7 @@ func renderPvcSpecFromAvailablePv(c client.Client, pvcSpec *v1.PersistentVolumeC func getDefaultVolumeAndAccessMode(c client.Client, storageClass *storagev1.StorageClass) ([]v1.PersistentVolumeAccessMode, *v1.PersistentVolumeMode, error) { if storageClass == nil { - return nil, nil, errors.Errorf("no accessMode defined on DV and no StorageProfile") + return nil, nil, errors.Errorf("no accessMode specified and StorageClass not found") } storageProfile := &cdiv1.StorageProfile{} @@ -377,7 +377,7 @@ func getDefaultVolumeAndAccessMode(c client.Client, storageClass *storagev1.Stor } // no accessMode configured on storageProfile - return nil, nil, errors.Errorf("no accessMode defined DV nor on StorageProfile for %s StorageClass", storageClass.Name) + return nil, nil, errors.Errorf("no accessMode specified in StorageProfile %s", storageProfile.Name) } func getDefaultVolumeMode(c client.Client, storageClass *storagev1.StorageClass, pvcAccessModes []v1.PersistentVolumeAccessMode) (*v1.PersistentVolumeMode, error) { @@ -416,13 +416,13 @@ func getDefaultVolumeMode(c client.Client, storageClass *storagev1.StorageClass, func getDefaultAccessModes(c client.Client, storageClass *storagev1.StorageClass, pvcVolumeMode *v1.PersistentVolumeMode) ([]v1.PersistentVolumeAccessMode, error) { if storageClass == nil { - return nil, errors.Errorf("no accessMode defined on DV, no StorageProfile ") + return nil, errors.Errorf("no accessMode specified and no StorageProfile") } storageProfile := &cdiv1.StorageProfile{} err := c.Get(context.TODO(), types.NamespacedName{Name: storageClass.Name}, storageProfile) if err != nil { - return nil, errors.Wrap(err, "no accessMode defined on DV, cannot get StorageProfile") + return nil, errors.Wrapf(err, "no accessMode specified and cannot get StorageProfile %s", storageClass.Name) } if len(storageProfile.Status.ClaimPropertySets) > 0 { @@ -444,7 +444,7 @@ func getDefaultAccessModes(c client.Client, storageClass *storagev1.StorageClass } // no accessMode configured on storageProfile - return nil, errors.Errorf("no accessMode defined on StorageProfile for %s StorageClass", storageClass.Name) + return nil, errors.Errorf("no accessMode specified in StorageProfile %s", storageProfile.Name) } // storageClassCSIDriverExists returns true if the passed storage class has CSI drivers available