status to optional, comments update

remove unnecessary binding call

Author: yuxiangqian

config/crd/snapshot.storage.k8s.io_volumesnapshotclasses.yaml

@@ -42,7 +42,7 @@ spec:
           type: string
         driver:
           description: driver is the name of the storage driver that handles this
-            VolumeSnapshotClass.
+            VolumeSnapshotClass. Required.
           type: string
         kind:
           description: 'Kind is a string value representing the REST resource this

config/crd/snapshot.storage.k8s.io_volumesnapshotcontents.yaml

@@ -35,7 +35,7 @@ spec:
           type: object
         spec:
           description: spec defines properties of a VolumeSnapshotContent created
-            by the underlying storage system.
+            by the underlying storage system. Required.
           properties:
             deletionPolicy:
               allOf:
@@ -68,15 +68,17 @@ spec:
               type: string
             source:
               description: source specifies from where a snapshot will be created.
-                Required.
+                This field is immutable after creation. Required.
               properties:
                 snapshotHandle:
                   description: snapshotHandle specifies the CSI name of a pre-existing
-                    snapshot on the underlying storage system.
+                    snapshot on the underlying storage system. This field is immutable
+                    once specified.
                   type: string
                 volumeHandle:
                   description: volumeHandle specifies the CSI name of the volume from
-                    which a snapshot should be dynamically taken from.
+                    which a snapshot should be dynamically taken from. This field
+                    is immutable once specified.
                   type: string
               type: object
             volumeSnapshotRef:
@@ -85,7 +87,7 @@ spec:
                 field must reference to this VolumeSnapshotContent's name for the
                 bidirectional binding to be valid. For a pre-existing VolumeSnapshotContent
                 object, name and namespace of the VolumeSnapshot object MUST be provided
-                for binding to happen. Required.
+                for binding to happen. This field is immutable after creation. Required.
               properties:
                 apiVersion:
                   description: API version of the referent.
@@ -177,7 +179,6 @@ spec:
           type: object
       required:
       - spec
-      - status
       type: object
   version: v1beta1
   versions:

config/crd/snapshot.storage.k8s.io_volumesnapshots.yaml

@@ -41,17 +41,18 @@ spec:
           properties:
             source:
               description: source specifies where a snapshot will be created from.
-                Required.
+                This field is immutable after creation. Required.
               properties:
                 persistentVolumeClaimName:
                   description: persistentVolumeClaimName specifies the name of the
                     PersistentVolumeClaim object in the same namespace as the VolumeSnapshot
-                    object where the snapshot should be dynamically taken from.
+                    object where the snapshot should be dynamically taken from. This
+                    field is immutable once specified.
                   type: string
                 volumeSnapshotContentName:
                   description: volumeSnapshotContentName specifies the name of a pre-existing
                     VolumeSnapshotContent object a user asks to statically bind the
-                    VolumeSnapshot object to.
+                    VolumeSnapshot object to. This field is immutable once specified.
                   type: string
               type: object
             volumeSnapshotClassName:
@@ -72,11 +73,14 @@ spec:
             is accurate and complete.'
           properties:
             boundVolumeSnapshotContentName:
-              description: boundVolumeSnapshotContentName represents the name of the
-                VolumeSnapshotContent object to which the VolumeSnapshot object is
-                bound. If not specified, it indicates that the VolumeSnapshot object
+              description: 'boundVolumeSnapshotContentName represents the name of
+                the VolumeSnapshotContent object to which the VolumeSnapshot object
+                is bound. If not specified, it indicates that the VolumeSnapshot object
                 has not been successfully bound to a VolumeSnapshotContent object
-                yet.
+                yet. NOTE: Specified boundVolumeSnapshotContentName alone does not
+                mean binding       is valid. Controllers MUST always verify bidirectional
+                binding between       VolumeSnapshot and VolumeSnapshotContent to
+                avoid possible security issues.'
               type: string
             creationTime:
               description: creationTime, if not nil, represents the timestamp when

pkg/apis/volumesnapshot/v1beta1/types.go

@@ -67,6 +67,7 @@ type VolumeSnapshotList struct {
 // VolumeSnapshotSpec describes the common attributes of a volume snapshot.
 type VolumeSnapshotSpec struct {
 	// source specifies where a snapshot will be created from.
+	// This field is immutable after creation.
 	// Required.
 	Source VolumeSnapshotSource `json:"source" protobuf:"bytes,1,opt,name=source"`
 
@@ -78,7 +79,9 @@ type VolumeSnapshotSpec struct {
 	VolumeSnapshotClassName *string `json:"volumeSnapshotClassName,omitempty" protobuf:"bytes,2,opt,name=volumeSnapshotClassName"`
 }
 
-// VolumeSnapshotSource represents the source of a snapshot.
+// VolumeSnapshotSource specifies whether the underlying snapshot should be
+// dynamically taken upon creation or if a pre-existing VolumeSnapshotContent
+// object should be used.
 // Exactly one of its members must be set.
 // Members in VolumeSnapshotSource are immutable.
 // TODO(xiangqian): Add a webhook to ensure that VolumeSnapshotSource members
@@ -87,24 +90,26 @@ type VolumeSnapshotSource struct {
 	// persistentVolumeClaimName specifies the name of the PersistentVolumeClaim
 	// object in the same namespace as the VolumeSnapshot object where the
 	// snapshot should be dynamically taken from.
+	// This field is immutable once specified.
 	// +optional
 	PersistentVolumeClaimName *string `json:"persistentVolumeClaimName,omitempty" protobuf:"bytes,1,opt,name=persistentVolumeClaimName"`
 
 	// volumeSnapshotContentName specifies the name of a pre-existing VolumeSnapshotContent
 	// object a user asks to statically bind the VolumeSnapshot object to.
+	// This field is immutable once specified.
 	// +optional
 	VolumeSnapshotContentName *string `json:"volumeSnapshotContentName,omitempty" protobuf:"bytes,2,opt,name=volumeSnapshotContentName"`
 }
 
 // VolumeSnapshotStatus is the status of the VolumeSnapshot
 type VolumeSnapshotStatus struct {
-	// NOTE: All fields in VolumeSnapshotStatus are informational for user references.
-	// Controllers MUST NOT rely on any fields programmatically.
-
 	// boundVolumeSnapshotContentName represents the name of the VolumeSnapshotContent
 	// object to which the VolumeSnapshot object is bound.
 	// If not specified, it indicates that the VolumeSnapshot object has not been
 	// successfully bound to a VolumeSnapshotContent object yet.
+	// NOTE: Specified boundVolumeSnapshotContentName alone does not mean binding
+	//       is valid. Controllers MUST always verify bidirectional binding between
+	//       VolumeSnapshot and VolumeSnapshotContent to avoid possible security issues.
 	// +optional
 	BoundVolumeSnapshotContentName *string `json:"boundVolumeSnapshotContentName,omitempty" protobuf:"bytes,1,opt,name=boundVolumeSnapshotContentName"`
 
@@ -166,6 +171,7 @@ type VolumeSnapshotClass struct {
 	metav1.ObjectMeta `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"`
 
 	// driver is the name of the storage driver that handles this VolumeSnapshotClass.
+	// Required.
 	Driver string `json:"driver" protobuf:"bytes,2,opt,name=driver"`
 
 	// parameters is a key-value map with storage driver specific parameters for creating snapshots.
@@ -215,10 +221,12 @@ type VolumeSnapshotContent struct {
 	metav1.ObjectMeta `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"`
 
 	// spec defines properties of a VolumeSnapshotContent created by the underlying storage system.
+	// Required.
 	Spec VolumeSnapshotContentSpec `json:"spec" protobuf:"bytes,2,opt,name=spec"`
 
 	// status represents the current information of a snapshot.
-	Status VolumeSnapshotContentStatus `json:"status" protobuf:"bytes,3,opt,name=status"`
+	// +optional
+	Status VolumeSnapshotContentStatus `json:"status,omitempty" protobuf:"bytes,3,opt,name=status"`
 }
 
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
@@ -242,6 +250,7 @@ type VolumeSnapshotContentSpec struct {
 	// this VolumeSnapshotContent's name for the bidirectional binding to be valid.
 	// For a pre-existing VolumeSnapshotContent object, name and namespace of the
 	// VolumeSnapshot object MUST be provided for binding to happen.
+	// This field is immutable after creation.
 	// Required.
 	VolumeSnapshotRef core_v1.ObjectReference `json:"volumeSnapshotRef" protobuf:"bytes,1,opt,name=volumeSnapshotRef"`
 
@@ -269,23 +278,26 @@ type VolumeSnapshotContentSpec struct {
 	SnapshotClassName *string `json:"snapshotClassName,omitempty" protobuf:"bytes,4,opt,name=snapshotClassName"`
 
 	// source specifies from where a snapshot will be created.
+	// This field is immutable after creation.
 	// Required.
 	Source VolumeSnapshotContentSource `json:"source" protobuf:"bytes,5,opt,name=source"`
 }
 
-// VolumeSnapshotContentSource represents the source of a snapshot.
+// VolumeSnapshotContentSource represents the CSI source of a snapshot.
 // Exactly one of its members must be set.
 // Members in VolumeSnapshotContentSource are immutable.
 // TODO(xiangqian): Add a webhook to ensure that VolumeSnapshotContentSource members
-// will not be updated once specified.
+// will be immutable once specified.
 type VolumeSnapshotContentSource struct {
 	// volumeHandle specifies the CSI name of the volume from which a snapshot
 	// should be dynamically taken from.
+	// This field is immutable once specified.
 	// +optional
 	VolumeHandle *string `json:"volumeHandle,omitempty" protobuf:"bytes,1,opt,name=volumeHandle"`
 
 	// snapshotHandle specifies the CSI name of a pre-existing snapshot on the
 	// underlying storage system.
+	// This field is immutable once specified.
 	// +optional
 	SnapshotHandle *string `json:"snapshotHandle,omitempty" protobuf:"bytes,2,opt,name=snapshotHandle"`
 }
@@ -322,6 +334,7 @@ type VolumeSnapshotContentStatus struct {
 	// If not specified, it means the readiness of a snapshot is unknown.
 	// +optional.
 	ReadyToUse *bool `json:"readyToUse,omitempty" protobuf:"varint,4,opt,name=readyToUse"`
+
 	// error is the latest observed error during snapshot creation, if any.
 	// +optional
 	Error *VolumeSnapshotError `json:"error,omitempty" protobuf:"bytes,5,opt,name=error,casttype=VolumeSnapshotError"`
@@ -332,10 +345,12 @@ type VolumeSnapshotContentStatus struct {
 type DeletionPolicy string
 
 const (
-	// volumeSnapshotContentDelete means the snapshot will be deleted from Kubernetes on release from its volume snapshot.
+	// volumeSnapshotContentDelete means the snapshot will be deleted from the
+	// underlying storage system on release from its volume snapshot.
 	VolumeSnapshotContentDelete DeletionPolicy = "Delete"
 
-	// volumeSnapshotContentRetain means the snapshot will be left in its current state on release from its volume snapshot.
+	// volumeSnapshotContentRetain means the snapshot will be left in its current
+	// state on release from its volume snapshot.
 	VolumeSnapshotContentRetain DeletionPolicy = "Retain"
 )
 

pkg/controller/snapshot_controller.go

@@ -274,14 +274,10 @@ func (ctrl *csiSnapshotController) syncUnreadySnapshot(snapshot *crdv1.VolumeSna
 		// find a matching volume snapshot content
 		if contentObj := ctrl.getMatchSnapshotContent(snapshot); contentObj != nil {
 			klog.V(5).Infof("Find VolumeSnapshotContent object %s for snapshot %s", contentObj.Name, uniqueSnapshotName)
-			newSnapshot, err := ctrl.bindandUpdateVolumeSnapshot(contentObj, snapshot)
-			if err != nil {
-				return err
-			}
-			if err = ctrl.checkandUpdateBoundSnapshotStatus(newSnapshot, contentObj); err != nil {
+			if err := ctrl.checkandUpdateBoundSnapshotStatus(snapshot, contentObj); err != nil {
 				return err
 			}
-			klog.V(5).Infof("bindandUpdateVolumeSnapshot %v", newSnapshot)
+			klog.V(5).Infof("checkandUpdateBoundSnapshotStatus %v", snapshot)
 		} else if snapshot.Status.BoundVolumeSnapshotContentName != nil {
 			contentObj, found, err := ctrl.contentStore.GetByKey(*snapshot.Status.BoundVolumeSnapshotContentName)
 			if err != nil {
@@ -686,12 +682,6 @@ func (ctrl *csiSnapshotController) createSnapshotOperation(snapshot *crdv1.Volum
 				VolumeHandle: &volume.Spec.CSI.VolumeHandle,
 			},
 		},
-		Status: crdv1.VolumeSnapshotContentStatus{
-			SnapshotHandle: &snapshotID,
-			CreationTime:   &timestamp,
-			RestoreSize:    &size,
-			ReadyToUse:     &readyToUse,
-		},
 	}
 
 	// Set AnnDeletionSecretRefName and AnnDeletionSecretRefNamespace
@@ -707,15 +697,19 @@ func (ctrl *csiSnapshotController) createSnapshotOperation(snapshot *crdv1.Volum
 	// Try to create the VolumeSnapshotContent object several times
 	for i := 0; i < ctrl.createSnapshotContentRetryCount; i++ {
 		klog.V(5).Infof("createSnapshot [%s]: trying to save volume snapshot content %s", snapshotKey(snapshot), snapshotContent.Name)
-		if _, err = ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Create(snapshotContent); err == nil || apierrs.IsAlreadyExists(err) {
+		var newContent *crdv1.VolumeSnapshotContent
+		if newContent, err = ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Create(snapshotContent); err == nil || apierrs.IsAlreadyExists(err) {
 			// Save succeeded.
 			if err != nil {
 				klog.V(3).Infof("volume snapshot content %q for snapshot %q already exists, reusing", snapshotContent.Name, snapshotKey(snapshot))
 				err = nil
 			} else {
 				klog.V(3).Infof("volume snapshot content %q for snapshot %q saved, %v", snapshotContent.Name, snapshotKey(snapshot), snapshotContent)
 			}
-			break
+			err = ctrl.updateSnapshotContentStatus(newContent, snapshotID, readyToUse, timestamp, size)
+			if err == nil {
+				break
+			}
 		}
 		// Save failed, try again after a while.
 		klog.V(3).Infof("failed to save volume snapshot content %q for snapshot %q: %v", snapshotContent.Name, snapshotKey(snapshot), err)
@@ -731,13 +725,7 @@ func (ctrl *csiSnapshotController) createSnapshotOperation(snapshot *crdv1.Volum
 		ctrl.eventRecorder.Event(newSnapshot, v1.EventTypeWarning, "CreateSnapshotContentFailed", strerr)
 		return nil, newControllerUpdateError(snapshotKey(snapshot), err.Error())
 	}
-
-	// save succeeded, bind and update status for snapshot.
-	result, err := ctrl.bindandUpdateVolumeSnapshot(snapshotContent, newSnapshot)
-	if err != nil {
-		return nil, err
-	}
-	return result, nil
+	return newSnapshot, nil
 }
 
 // Delete a snapshot
@@ -784,37 +772,6 @@ func (ctrl *csiSnapshotController) deleteSnapshotContentOperation(content *crdv1
 	return nil
 }
 
-func (ctrl *csiSnapshotController) bindandUpdateVolumeSnapshot(snapshotContent *crdv1.VolumeSnapshotContent, snapshot *crdv1.VolumeSnapshot) (*crdv1.VolumeSnapshot, error) {
-	klog.V(5).Infof("bindandUpdateVolumeSnapshot for snapshot [%s]: snapshotContent [%s]", snapshot.Name, snapshotContent.Name)
-	snapshotObj, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshots(snapshot.Namespace).Get(snapshot.Name, metav1.GetOptions{})
-	if err != nil {
-		return nil, fmt.Errorf("error get snapshot %s from api server: %v", snapshotKey(snapshot), err)
-	}
-
-	// Copy the snapshot object before updating it
-	snapshotCopy := snapshotObj.DeepCopy()
-
-	if snapshotObj.Status.BoundVolumeSnapshotContentName != nil && *snapshotObj.Status.BoundVolumeSnapshotContentName == snapshotContent.Name {
-		klog.Infof("bindVolumeSnapshotContentToVolumeSnapshot: VolumeSnapshot %s already bind to volumeSnapshotContent [%s]", snapshot.Name, snapshotContent.Name)
-	} else {
-		klog.Infof("bindVolumeSnapshotContentToVolumeSnapshot: before bind VolumeSnapshot %s to volumeSnapshotContent [%s]", snapshot.Name, snapshotContent.Name)
-		snapshotCopy.Status.BoundVolumeSnapshotContentName = &snapshotContent.Name
-		updateSnapshot, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshots(snapshot.Namespace).Update(snapshotCopy)
-		if err != nil {
-			klog.Infof("bindVolumeSnapshotContentToVolumeSnapshot: Error binding VolumeSnapshot %s to volumeSnapshotContent [%s]. Error [%#v]", snapshot.Name, snapshotContent.Name, err)
-			return nil, newControllerUpdateError(snapshotKey(snapshot), err.Error())
-		}
-		snapshotCopy = updateSnapshot
-		_, err = ctrl.storeSnapshotUpdate(snapshotCopy)
-		if err != nil {
-			klog.Errorf("%v", err)
-		}
-	}
-
-	klog.V(5).Infof("bindandUpdateVolumeSnapshot for snapshot completed [%#v]", snapshotCopy)
-	return snapshotCopy, nil
-}
-
 // TODO(xiangqian) This is a temp implementation to make sure the test cases pass
 func (ctrl *csiSnapshotController) updateSnapshotContentStatus(
 	content *crdv1.VolumeSnapshotContent,

pkg/controller/snapshot_ready_test.go

@@ -196,11 +196,11 @@ func TestSync(t *testing.T) {
 			test: testSyncSnapshot,
 		},
 		{
-			name:              "2-9 - bind when snapshot and content matches, fail on status update as there is not pvc provided",
+			name:              "2-9 - fail on status update as there is not pvc provided",
 			initialContents:   newContentArray("content2-9", "snapuid2-9", "snap2-9", "sid2-9", validSecretClass, "", "", deletionPolicy, nil, nil, false),
 			expectedContents:  newContentArray("content2-9", "snapuid2-9", "snap2-9", "sid2-9", validSecretClass, "", "", deletionPolicy, nil, nil, false),
 			initialSnapshots:  newSnapshotArray("snap2-9", "snapuid2-9", "claim2-9", "", validSecretClass, "", &False, nil, nil, nil),
-			expectedSnapshots: newSnapshotArray("snap2-9", "snapuid2-9", "claim2-9", "", validSecretClass, "content2-9", &False, nil, nil, newVolumeError("Failed to check and update snapshot: failed to get input parameters to create snapshot snap2-9: \"failed to retrieve PVC claim2-9 from the lister: \\\"persistentvolumeclaim \\\\\\\"claim2-9\\\\\\\" not found\\\"\"")),
+			expectedSnapshots: newSnapshotArray("snap2-9", "snapuid2-9", "claim2-9", "", validSecretClass, "", &False, nil, nil, newVolumeError("Failed to check and update snapshot: failed to get input parameters to create snapshot snap2-9: \"failed to retrieve PVC claim2-9 from the lister: \\\"persistentvolumeclaim \\\\\\\"claim2-9\\\\\\\" not found\\\"\"")),
 			errors:            noerrors,
 			test:              testSyncSnapshot,
 		},