status to optional, comments update

xing-yang · Oct 14, 2019 · 0402020 · 0402020
1 parent 19abc09
commit 0402020
Show file tree

Hide file tree

Showing 8 changed files with 153 additions and 139 deletions.
diff --git a/config/crd/snapshot.storage.k8s.io_volumesnapshotclasses.yaml b/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

diff --git a/config/crd/snapshot.storage.k8s.io_volumesnapshotcontents.yaml b/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:

diff --git a/config/crd/snapshot.storage.k8s.io_volumesnapshots.yaml b/config/crd/snapshot.storage.k8s.io_volumesnapshots.yaml
@@ -41,17 +41,17 @@ 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.
+                    VolumeSnapshotContent object. This field is immutable once specified.
                   type: string
               type: object
             volumeSnapshotClassName:
@@ -72,11 +72,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
@@ -113,10 +116,9 @@ spec:
                 finished all out-of-bound procedures to make a snapshot ready to be
                 used to restore a volume. For a pre-existing snapshot, readyToUse
                 will be set to the value returned from CSI "ListSnapshots" gRPC call
-                if the matching CSI driver exists and supports. Otherwise, if there
-                exists no CSI driver or the driver does not support "ListSnapshots",
-                this field will be set to "True". If not specified, it indicates that
-                the readiness of a snapshot is unknown.
+                if the matching CSI driver exists and supports. Otherwise, this field
+                will be set to "True". If not specified, it indicates that the readiness
+                of a snapshot is unknown.
               type: boolean
             restoreSize:
               description: restoreSize represents the complete size of the snapshot

diff --git a/pkg/apis/volumesnapshot/v1beta1/types.go b/pkg/apis/volumesnapshot/v1beta1/types.go
@@ -1,5 +1,5 @@
 /*
-Copyright 2018 The Kubernetes Authors.
+Copyright 2019 The Kubernetes Authors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -50,7 +50,7 @@ type VolumeSnapshot struct {
 	// Controllers should only use information from the VolumeSnapshotContent object
 	// after verifying that the binding is accurate and complete.
 	// +optional
-	Status VolumeSnapshotStatus `json:"status,omitempty" protobuf:"bytes,3,opt,name=status"`
+	Status *VolumeSnapshotStatus `json:"status,omitempty" protobuf:"bytes,3,opt,name=status"`
 }
 
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
@@ -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.
+	// object.
+	// 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"`
 
@@ -124,8 +129,7 @@ type VolumeSnapshotStatus struct {
 	// be used to restore a volume.
 	// For a pre-existing snapshot, readyToUse will be set to the value returned
 	// from CSI "ListSnapshots" gRPC call if the matching CSI driver exists and supports.
-	// Otherwise, if there exists no CSI driver or the driver does not support "ListSnapshots",
-	// this field will be set to "True".
+	// Otherwise, this field will be set to "True".
 	// If not specified, it indicates that the readiness of a snapshot is unknown.
 	// +optional
 	ReadyToUse *bool `json:"readyToUse,omitempty" protobuf:"varint,3,opt,name=readyToUse"`
@@ -166,6 +170,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 +220,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 +249,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 +277,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 +333,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 +344,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"
 )
 

diff --git a/pkg/apis/volumesnapshot/v1beta1/zz_generated.deepcopy.go b/pkg/apis/volumesnapshot/v1beta1/zz_generated.deepcopy.go
diff --git a/pkg/controller/framework_test.go b/pkg/controller/framework_test.go
@@ -786,7 +786,7 @@ func newContent(contentName, boundToSnapshotUID, boundToSnapshotName, snapshotHa
 			Driver:         mockDriverName,
 			DeletionPolicy: deletionPolicy,
 		},
-		Status: crdv1.VolumeSnapshotContentStatus{
+		Status: &crdv1.VolumeSnapshotContentStatus{
 			CreationTime: creationTime,
 			RestoreSize:  size,
 		},
@@ -869,7 +869,7 @@ func newSnapshot(
 		Spec: crdv1.VolumeSnapshotSpec{
 			VolumeSnapshotClassName: nil,
 		},
-		Status: crdv1.VolumeSnapshotStatus{
+		Status: &crdv1.VolumeSnapshotStatus{
 			CreationTime: creationTime,
 			ReadyToUse:   readyToUse,
 			Error:        err,