[release-v1.56] Annotation to check for statically provisioned PVs wh…

…en creating DataVolumes (#2605) * function should return dataVolumeSyncResult, take *dataVolumeSyncResult as a parameter Signed-off-by: Michael Henriksen <[email protected]> * checkStaticVolume implemetation for import DataVolume Signed-off-by: Michael Henriksen <[email protected]> * upload support for checkStaticVolume Signed-off-by: Michael Henriksen <[email protected]> * checkStaticVolume for clone datavolumes Signed-off-by: Michael Henriksen <[email protected]> * checkStaticVolume for snapshot clone Signed-off-by: Michael Henriksen <[email protected]> * checkStaticVolume for external populator source Signed-off-by: Michael Henriksen <[email protected]> * tignten up static volume check Signed-off-by: Michael Henriksen <[email protected]> * expand functional test to compare creation timestamps Signed-off-by: Michael Henriksen <[email protected]> * updates from code review mostly add md5 verification to test and refacto common index creation Signed-off-by: Michael Henriksen <[email protected]> * webhook changes, allow clone source DataVolumes (with special annotations) even if source does not exist or user has no permission BUT no token is added so this is really just for the static/prepopulate cases Signed-off-by: Michael Henriksen <[email protected]> --------- Signed-off-by: Michael Henriksen <[email protected]> Co-authored-by: Michael Henriksen <[email protected]>
kubevirt · Feb 23, 2023 · 9611f39 · 9611f39
1 parent d0fea1b
commit 9611f39
Show file tree

Hide file tree

Showing 29 changed files with 1,262 additions and 138 deletions.
diff --git a/cmd/cdi-controller/controller.go b/cmd/cdi-controller/controller.go
@@ -204,6 +204,11 @@ func start(ctx context.Context, cfg *rest.Config) {
 		os.Exit(1)
 	}
 
+	if err := dvc.CreateCommonIndexes(mgr); err != nil {
+		klog.Errorf("Unable to create shared indexes: %v", err)
+		os.Exit(1)
+	}
+
 	// TODO: Current DV controller had threadiness 3, should we do the same here, defaults to one thread.
 	if _, err := dvc.NewImportController(ctx, mgr, log, installerLabels); err != nil {
 		klog.Errorf("Unable to setup datavolume import controller: %v", err)

diff --git a/go.mod b/go.mod
@@ -47,6 +47,7 @@ require (
 	k8s.io/client-go v12.0.0+incompatible
 	k8s.io/cluster-bootstrap v0.0.0
 	k8s.io/code-generator v0.23.3
+	k8s.io/component-helpers v0.23.0
 	k8s.io/klog/v2 v2.70.1
 	k8s.io/kube-aggregator v0.23.0
 	k8s.io/kube-openapi v0.0.0-20220803162953-67bda5d908f1

diff --git a/go.sum b/go.sum
@@ -1980,6 +1980,8 @@ k8s.io/code-generator v0.23.0 h1:lhyd2KJVCEmpjaCpuoooGs+e3xhPwpYvupnNRidO0Ds=
 k8s.io/code-generator v0.23.0/go.mod h1:vQvOhDXhuzqiVfM/YHp+dmg10WDZCchJVObc9MvowsE=
 k8s.io/component-base v0.23.0 h1:UAnyzjvVZ2ZR1lF35YwtNY6VMN94WtOnArcXBu34es8=
 k8s.io/component-base v0.23.0/go.mod h1:DHH5uiFvLC1edCpvcTDV++NKULdYYU6pR9Tt3HIKMKI=
+k8s.io/component-helpers v0.23.0 h1:qNbqN10QTefiWcCOPkHL/0nn81sdKVv6ZgEXcSyot/U=
+k8s.io/component-helpers v0.23.0/go.mod h1:liXMh6FZS4qamKtMJQ7uLHnFe3tlC86RX5mJEk/aerg=
 k8s.io/cri-api v0.23.0/go.mod h1:2edENu3/mkyW3c6fVPPPaVGEFbLRacJizBbSp7ZOLOo=
 k8s.io/gengo v0.0.0-20181113154421-fd15ee9cc2f7/go.mod h1:ezvh/TsK7cY6rbqRK0oQQ8IAqLxYwwyPxAX1Pzy0ii0=
 k8s.io/gengo v0.0.0-20201113003025-83324d819ded/go.mod h1:FiNAH4ZV3gBg2Kwh89tzAEV2be7d5xI0vBa/VySYy3E=

diff --git a/pkg/apiserver/webhooks/datavolume-mutate.go b/pkg/apiserver/webhooks/datavolume-mutate.go
@@ -26,6 +26,7 @@ import (
 	snapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1"
 	admissionv1 "k8s.io/api/admission/v1"
 	authv1 "k8s.io/api/authorization/v1"
+	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	k8sfield "k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/client-go/kubernetes"
@@ -85,7 +86,7 @@ func (p *sarProxy) Create(sar *authv1.SubjectAccessReview) (*authv1.SubjectAcces
 }
 
 func (wh *dataVolumeMutatingWebhook) Admit(ar admissionv1.AdmissionReview) *admissionv1.AdmissionResponse {
-	var dataVolume, oldDataVolume cdiv1.DataVolume
+	dataVolume := &cdiv1.DataVolume{}
 
 	klog.V(3).Infof("Got AdmissionReview %+v", ar)
 
@@ -102,22 +103,7 @@ func (wh *dataVolumeMutatingWebhook) Admit(ar admissionv1.AdmissionReview) *admi
 		return allowedAdmissionResponse()
 	}
 
-	cloneSourceHandler, err := newCloneSourceHandler(&dataVolume, wh.cdiClient)
-	if err != nil {
-		return toAdmissionResponseError(err)
-	}
-
-	targetNamespace, targetName := dataVolume.Namespace, dataVolume.Name
-	if targetNamespace == "" {
-		targetNamespace = ar.Request.Namespace
-	}
-
-	if targetName == "" {
-		targetName = ar.Request.Name
-	}
-
 	modifiedDataVolume := dataVolume.DeepCopy()
-	modified := false
 
 	if ar.Request.Operation == admissionv1.Create {
 		config, err := wh.cdiClient.CdiV1beta1().CDIConfigs().Get(context.TODO(), common.ConfigName, metav1.GetOptions{})
@@ -130,17 +116,41 @@ func (wh *dataVolumeMutatingWebhook) Admit(ar admissionv1.AdmissionReview) *admi
 			}
 			if modifiedDataVolume.Annotations[cc.AnnDeleteAfterCompletion] != "false" {
 				modifiedDataVolume.Annotations[cc.AnnDeleteAfterCompletion] = "true"
-				modified = true
 			}
 		}
 	}
 
-	if cloneSourceHandler.cloneType == noClone {
-		klog.V(3).Infof("DataVolume %s/%s not cloning", targetNamespace, targetName)
-		if modified {
+	_, prePopulated := dataVolume.Annotations[cc.AnnPrePopulated]
+	_, checkStaticVolume := dataVolume.Annotations[cc.AnnCheckStaticVolume]
+	noTokenOkay := prePopulated || checkStaticVolume
+
+	targetNamespace, targetName := dataVolume.Namespace, dataVolume.Name
+	if targetNamespace == "" {
+		targetNamespace = ar.Request.Namespace
+	}
+
+	if targetName == "" {
+		targetName = ar.Request.Name
+	}
+
+	cloneSourceHandler, err := newCloneSourceHandler(dataVolume, wh.cdiClient)
+	if err != nil {
+		if k8serrors.IsNotFound(err) && noTokenOkay {
+			// no token needed, likely since no datasource
+			klog.V(3).Infof("DataVolume %s/%s is pre/static populated, not adding token, no datasource", targetNamespace, targetName)
 			return toPatchResponse(dataVolume, modifiedDataVolume)
 		}
-		return allowedAdmissionResponse()
+		return toAdmissionResponseError(err)
+	}
+
+	if cloneSourceHandler.cloneType == noClone {
+		klog.V(3).Infof("DataVolume %s/%s not cloning", targetNamespace, targetName)
+		return toPatchResponse(dataVolume, modifiedDataVolume)
+	}
+
+	// only add token at create time
+	if ar.Request.Operation != admissionv1.Create {
+		return toPatchResponse(dataVolume, modifiedDataVolume)
 	}
 
 	sourceName, sourceNamespace := cloneSourceHandler.sourceName, cloneSourceHandler.sourceNamespace
@@ -150,19 +160,12 @@ func (wh *dataVolumeMutatingWebhook) Admit(ar admissionv1.AdmissionReview) *admi
 
 	_, err = wh.k8sClient.CoreV1().Namespaces().Get(context.TODO(), sourceNamespace, metav1.GetOptions{})
 	if err != nil {
-		return toAdmissionResponseError(err)
-	}
-
-	if ar.Request.Operation == admissionv1.Update {
-		if err := json.Unmarshal(ar.Request.OldObject.Raw, &oldDataVolume); err != nil {
-			return toAdmissionResponseError(err)
-		}
-
-		_, ok := oldDataVolume.Annotations[cc.AnnCloneToken]
-		if ok {
-			klog.V(3).Infof("DataVolume %s/%s already has clone token", targetNamespace, targetName)
-			return allowedAdmissionResponse()
+		if k8serrors.IsNotFound(err) && noTokenOkay {
+			// no token needed, likely since no source namespace
+			klog.V(3).Infof("DataVolume %s/%s is pre/static populated, not adding token, no source namespace", targetNamespace, targetName)
+			return toPatchResponse(dataVolume, modifiedDataVolume)
 		}
+		return toAdmissionResponseError(err)
 	}
 
 	ok, reason, err := cloneSourceHandler.cloneAuthFunc(wh.proxy, sourceNamespace, sourceName, targetNamespace, ar.Request.UserInfo)
@@ -171,6 +174,11 @@ func (wh *dataVolumeMutatingWebhook) Admit(ar admissionv1.AdmissionReview) *admi
 	}
 
 	if !ok {
+		if noTokenOkay {
+			klog.V(3).Infof("DataVolume %s/%s is pre/static populated, not adding token, auth failed", targetNamespace, targetName)
+			return toPatchResponse(dataVolume, modifiedDataVolume)
+		}
+
 		causes := []metav1.StatusCause{
 			{
 				Type:    metav1.CauseTypeFieldValueInvalid,

diff --git a/pkg/apiserver/webhooks/datavolume-mutate_test.go b/pkg/apiserver/webhooks/datavolume-mutate_test.go
@@ -125,13 +125,40 @@ var _ = Describe("Mutating DataVolume Webhook", func() {
 			Expect(resp.Patch).To(BeNil())
 		})
 
+		DescribeTable("should accept a DataVolume with sourceRef to non-existing DataSource", func(anno string) {
+			dataVolume := newDataSourceDataVolume("testDV", nil, "test")
+			Expect(dataVolume.Annotations).To(BeNil())
+			dataVolume.Annotations = map[string]string{anno: "whatever"}
+			dvBytes, _ := json.Marshal(&dataVolume)
+			ar := &admissionv1.AdmissionReview{
+				Request: &admissionv1.AdmissionRequest{
+					Operation: admissionv1.Create,
+					Resource: metav1.GroupVersionResource{
+						Group:    cdicorev1.SchemeGroupVersion.Group,
+						Version:  cdicorev1.SchemeGroupVersion.Version,
+						Resource: "datavolumes",
+					},
+					Object: runtime.RawExtension{
+						Raw: dvBytes,
+					},
+				},
+			}
+			resp := mutateDVs(key, ar, true)
+			Expect(resp.Allowed).To(BeTrue())
+			Expect(resp.Patch).ToNot(BeNil())
+		},
+			Entry("with static volume annotation", cc.AnnCheckStaticVolume),
+			Entry("with prePopulated volume annotation", cc.AnnPrePopulated),
+		)
+
 		It("should allow a DataVolume with sourceRef to existing DataSource", func() {
 			dataVolume := newDataSourceDataVolume("testDV", nil, "test")
 			Expect(dataVolume.Annotations).To(BeNil())
 			dvBytes, _ := json.Marshal(&dataVolume)
 
 			ar := &admissionv1.AdmissionReview{
 				Request: &admissionv1.AdmissionRequest{
+					Operation: admissionv1.Create,
 					Resource: metav1.GroupVersionResource{
 						Group:    cdicorev1.SchemeGroupVersion.Group,
 						Version:  cdicorev1.SchemeGroupVersion.Version,
@@ -207,6 +234,7 @@ var _ = Describe("Mutating DataVolume Webhook", func() {
 
 			ar := &admissionv1.AdmissionReview{
 				Request: &admissionv1.AdmissionRequest{
+					Operation: admissionv1.Create,
 					Resource: metav1.GroupVersionResource{
 						Group:    cdicorev1.SchemeGroupVersion.Group,
 						Version:  cdicorev1.SchemeGroupVersion.Version,
@@ -223,12 +251,41 @@ var _ = Describe("Mutating DataVolume Webhook", func() {
 			Expect(resp.Patch).To(BeNil())
 		})
 
+		DescribeTable("should accept a clone DataVolume", func(anno string) {
+			dataVolume := newPVCDataVolume("testDV", "testNamespace", "test")
+			Expect(dataVolume.Annotations).To(BeNil())
+			dataVolume.Annotations = map[string]string{anno: "whatever"}
+			dvBytes, _ := json.Marshal(&dataVolume)
+
+			ar := &admissionv1.AdmissionReview{
+				Request: &admissionv1.AdmissionRequest{
+					Operation: admissionv1.Create,
+					Resource: metav1.GroupVersionResource{
+						Group:    cdicorev1.SchemeGroupVersion.Group,
+						Version:  cdicorev1.SchemeGroupVersion.Version,
+						Resource: "datavolumes",
+					},
+					Object: runtime.RawExtension{
+						Raw: dvBytes,
+					},
+				},
+			}
+
+			resp := mutateDVs(key, ar, false)
+			Expect(resp.Allowed).To(BeTrue())
+			Expect(resp.Patch).ToNot(BeNil())
+		},
+			Entry("with static volume annotation", cc.AnnCheckStaticVolume),
+			Entry("with prePopulated volume annotation", cc.AnnPrePopulated),
+		)
+
 		It("should reject a clone if the source PVC's namespace doesn't exist", func() {
 			dataVolume := newPVCDataVolume("testDV", "noNamespace", "test")
 			dvBytes, _ := json.Marshal(&dataVolume)
 
 			ar := &admissionv1.AdmissionReview{
 				Request: &admissionv1.AdmissionRequest{
+					Operation: admissionv1.Create,
 					Resource: metav1.GroupVersionResource{
 						Group:    cdicorev1.SchemeGroupVersion.Group,
 						Version:  cdicorev1.SchemeGroupVersion.Version,
@@ -245,12 +302,41 @@ var _ = Describe("Mutating DataVolume Webhook", func() {
 			Expect(resp.Patch).To(BeNil())
 		})
 
+		DescribeTable("should accept a clone if the source PVC's namespace doesn't exist", func(anno string) {
+			dataVolume := newPVCDataVolume("testDV", "noNamespace", "test")
+			Expect(dataVolume.Annotations).To(BeNil())
+			dataVolume.Annotations = map[string]string{anno: "whatever"}
+			dvBytes, _ := json.Marshal(&dataVolume)
+
+			ar := &admissionv1.AdmissionReview{
+				Request: &admissionv1.AdmissionRequest{
+					Operation: admissionv1.Create,
+					Resource: metav1.GroupVersionResource{
+						Group:    cdicorev1.SchemeGroupVersion.Group,
+						Version:  cdicorev1.SchemeGroupVersion.Version,
+						Resource: "datavolumes",
+					},
+					Object: runtime.RawExtension{
+						Raw: dvBytes,
+					},
+				},
+			}
+
+			resp := mutateDVs(key, ar, false)
+			Expect(resp.Allowed).To(BeTrue())
+			Expect(resp.Patch).ToNot(BeNil())
+		},
+			Entry("with static volume annotation", cc.AnnCheckStaticVolume),
+			Entry("with prePopulated volume annotation", cc.AnnPrePopulated),
+		)
+
 		DescribeTable("should", func(srcNamespace string) {
 			dataVolume := newPVCDataVolume("testDV", srcNamespace, "test")
 			dvBytes, _ := json.Marshal(&dataVolume)
 
 			ar := &admissionv1.AdmissionReview{
 				Request: &admissionv1.AdmissionRequest{
+					Operation: admissionv1.Create,
 					Resource: metav1.GroupVersionResource{
 						Group:    cdicorev1.SchemeGroupVersion.Group,
 						Version:  cdicorev1.SchemeGroupVersion.Version,

diff --git a/pkg/apiserver/webhooks/datavolume-validate.go b/pkg/apiserver/webhooks/datavolume-validate.go
@@ -592,6 +592,8 @@ func validateExternalPopulation(spec *cdiv1.DataVolumeSpec, field *k8sfield.Path
 }
 
 func (wh *dataVolumeValidatingWebhook) Admit(ar admissionv1.AdmissionReview) *admissionv1.AdmissionResponse {
+	klog.V(3).Infof("Got AdmissionReview %+v", ar)
+
 	if err := validateDataVolumeResource(ar); err != nil {
 		return toAdmissionResponseError(err)
 	}

diff --git a/pkg/apiserver/webhooks/handler.go b/pkg/apiserver/webhooks/handler.go
@@ -25,6 +25,7 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"reflect"
 	"time"
 
 	"github.com/appscode/jsonpatch"
@@ -197,6 +198,10 @@ func validateDataVolumeResource(ar admissionv1.AdmissionReview) error {
 }
 
 func toPatchResponse(original, current interface{}) *admissionv1.AdmissionResponse {
+	if reflect.DeepEqual(original, current) {
+		return allowedAdmissionResponse()
+	}
+
 	patchType := admissionv1.PatchTypeJSONPatch
 
 	ob, err := json.Marshal(original)
@@ -219,7 +224,7 @@ func toPatchResponse(original, current interface{}) *admissionv1.AdmissionRespon
 		return toAdmissionResponseError(err)
 	}
 
-	klog.V(3).Infof("sending patches\n%s", pb)
+	klog.V(1).Infof("sending patches\n%s", pb)
 
 	return &admissionv1.AdmissionResponse{
 		Allowed:   true,

diff --git a/pkg/controller/common/util.go b/pkg/controller/common/util.go
@@ -177,6 +177,13 @@ const (
 	// AnnUploadRequest marks that a PVC should be made available for upload
 	AnnUploadRequest = AnnAPIGroup + "/storage.upload.target"
 
+	// AnnCheckStaticVolume checks if a statically allocated PV exists before creating the target PVC.
+	// If so, PVC is still created but population is skipped
+	AnnCheckStaticVolume = AnnAPIGroup + "/storage.checkStaticVolume"
+
+	// AnnPersistentVolumeList is an annotation storing a list of PV names
+	AnnPersistentVolumeList = AnnAPIGroup + "/storage.persistentVolumeList"
+
 	//AnnDefaultStorageClass is the annotation indicating that a storage class is the default one.
 	AnnDefaultStorageClass = "storageclass.kubernetes.io/is-default-class"
 

diff --git a/pkg/controller/datavolume/BUILD.bazel b/pkg/controller/datavolume/BUILD.bazel
@@ -42,6 +42,7 @@ go_library(
         "//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library",
         "//vendor/k8s.io/client-go/tools/cache:go_default_library",
         "//vendor/k8s.io/client-go/tools/record:go_default_library",
+        "//vendor/k8s.io/component-helpers/storage/volume:go_default_library",
         "//vendor/sigs.k8s.io/controller-runtime/pkg/client:go_default_library",
         "//vendor/sigs.k8s.io/controller-runtime/pkg/controller:go_default_library",
         "//vendor/sigs.k8s.io/controller-runtime/pkg/controller/controllerutil:go_default_library",
@@ -64,6 +65,7 @@ go_test(
         "pvc-clone-controller_test.go",
         "smart-clone-controller_test.go",
         "snapshot-clone-controller_test.go",
+        "static-volume_test.go",
         "upload-controller_test.go",
         "util_test.go",
     ],

diff --git a/pkg/controller/datavolume/clone-controller-base.go b/pkg/controller/datavolume/clone-controller-base.go
@@ -545,7 +545,7 @@ func (r *CloneReconcilerBase) syncCloneStatusPhase(syncRes *dataVolumeCloneSyncR
 	return r.syncDataVolumeStatusPhaseWithEvent(syncRes, phase, pvc, event)
 }
 
-func (r CloneReconcilerBase) updateStatusPhase(pvc *corev1.PersistentVolumeClaim, dataVolumeCopy *cdiv1.DataVolume, event *Event) error {
+func (r *CloneReconcilerBase) updateStatusPhase(pvc *corev1.PersistentVolumeClaim, dataVolumeCopy *cdiv1.DataVolume, event *Event) error {
 	phase, ok := pvc.Annotations[cc.AnnPodPhase]
 	if phase != string(corev1.PodSucceeded) {
 		_, ok = pvc.Annotations[cc.AnnCloneRequest]