Merge pull request #58762 from musse/make-volume-attr-first-class

Automatic merge from submit-queue (batch tested with PRs 60214, 58762, 59898, 59897, 60204). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Make CSI volume attributes first class

**What this PR does / why we need it**:
Move CSI volume attributes from PV annotation to CSI volume source first class field

**Release note**:
```release-note
NONE
```

api/openapi-spec/swagger.json

@@ -75241,6 +75241,13 @@
       "description": "Optional: The value to pass to ControllerPublishVolumeRequest. Defaults to false (read/write).",
       "type": "boolean"
      },
+     "volumeAttributes": {
+      "description": "Attributes of the volume to publish.",
+      "type": "object",
+      "additionalProperties": {
+       "type": "string"
+      }
+     },
      "volumeHandle": {
       "description": "VolumeHandle is the unique volume name returned by the CSI volume plugin’s CreateVolume to refer to the volume on all subsequent calls. Required.",
       "type": "string"

api/swagger-spec/v1.json

@@ -19980,6 +19980,10 @@
      "fsType": {
       "type": "string",
       "description": "Filesystem type to mount. Must be a filesystem type supported by the host operating system. Ex. \"ext4\", \"xfs\", \"ntfs\". Implicitly inferred to be \"ext4\" if unspecified."
+     },
+     "volumeAttributes": {
+      "type": "object",
+      "description": "Attributes of the volume to publish."
      }
     }
    },

docs/api-reference/v1/definitions.html

@@ -7878,6 +7878,13 @@ Examples:<br>
 <td class="tableblock halign-left valign-top"><p class="tableblock">string</p></td>
 <td class="tableblock halign-left valign-top"></td>
 </tr>
+<tr>
+<td class="tableblock halign-left valign-top"><p class="tableblock">volumeAttributes</p></td>
+<td class="tableblock halign-left valign-top"><p class="tableblock">Attributes of the volume to publish.</p></td>
+<td class="tableblock halign-left valign-top"><p class="tableblock">false</p></td>
+<td class="tableblock halign-left valign-top"><p class="tableblock">object</p></td>
+<td class="tableblock halign-left valign-top"></td>
+</tr>
 </tbody>
 </table>
 

pkg/apis/core/types.go

@@ -1634,6 +1634,10 @@ type CSIPersistentVolumeSource struct {
 	// Ex. "ext4", "xfs", "ntfs". Implicitly inferred to be "ext4" if unspecified.
 	// +optional
 	FSType string
+
+	// Attributes of the volume to publish.
+	// +optional
+	VolumeAttributes map[string]string
 }
 
 // ContainerPort represents a network port in a single container

pkg/apis/core/v1/zz_generated.conversion.go

@@ -622,6 +622,7 @@ func autoConvert_v1_CSIPersistentVolumeSource_To_core_CSIPersistentVolumeSource(
 	out.VolumeHandle = in.VolumeHandle
 	out.ReadOnly = in.ReadOnly
 	out.FSType = in.FSType
+	out.VolumeAttributes = *(*map[string]string)(unsafe.Pointer(&in.VolumeAttributes))
 	return nil
 }
 
@@ -635,6 +636,7 @@ func autoConvert_core_CSIPersistentVolumeSource_To_v1_CSIPersistentVolumeSource(
 	out.VolumeHandle = in.VolumeHandle
 	out.ReadOnly = in.ReadOnly
 	out.FSType = in.FSType
+	out.VolumeAttributes = *(*map[string]string)(unsafe.Pointer(&in.VolumeAttributes))
 	return nil
 }
 

pkg/apis/core/zz_generated.deepcopy.go

@@ -249,6 +249,13 @@ func (in *Binding) DeepCopyObject() runtime.Object {
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *CSIPersistentVolumeSource) DeepCopyInto(out *CSIPersistentVolumeSource) {
 	*out = *in
+	if in.VolumeAttributes != nil {
+		in, out := &in.VolumeAttributes, &out.VolumeAttributes
+		*out = make(map[string]string, len(*in))
+		for key, val := range *in {
+			(*out)[key] = val
+		}
+	}
 	return
 }
 
@@ -3314,7 +3321,7 @@ func (in *PersistentVolumeSource) DeepCopyInto(out *PersistentVolumeSource) {
 			*out = nil
 		} else {
 			*out = new(CSIPersistentVolumeSource)
-			**out = **in
+			(*in).DeepCopyInto(*out)
 		}
 	}
 	return

pkg/volume/csi/csi_mounter.go

@@ -151,14 +151,7 @@ func (c *csiMountMgr) SetUpAt(dir string, fsGroup *int64) error {
 		c.volumeInfo = attachment.Status.AttachmentMetadata
 	}
 
-	// get volume attributes
-	// TODO: for alpha vol attributes are passed via PV.Annotations
-	// Beta will fix that
-	attribs, err := getVolAttribsFromSpec(c.spec)
-	if err != nil {
-		glog.Error(log("mounter.SetUpAt failed to extract volume attributes from PV annotations: %v", err))
-		return err
-	}
+	attribs := csiSource.VolumeAttributes
 
 	// create target_dir before call to NodePublish
 	if err := os.MkdirAll(dir, 0750); err != nil {
@@ -295,29 +288,6 @@ func (c *csiMountMgr) TearDownAt(dir string) error {
 	return nil
 }
 
-// getVolAttribsFromSpec extracts CSI VolumeAttributes information from PV.Annotations
-// using key csi.kubernetes.io/volume-attributes.  The annotation value is expected
-// to be a JSON-encoded object of form {"key0":"val0",...,"keyN":"valN"}
-func getVolAttribsFromSpec(spec *volume.Spec) (map[string]string, error) {
-	if spec == nil {
-		return nil, errors.New("missing volume spec")
-	}
-	annotations := spec.PersistentVolume.GetAnnotations()
-	if annotations == nil {
-		return nil, nil // no annotations found
-	}
-	jsonAttribs := annotations[csiVolAttribsAnnotationKey]
-	if jsonAttribs == "" {
-		return nil, nil // csi annotation not found
-	}
-	attribs := map[string]string{}
-	if err := json.Unmarshal([]byte(jsonAttribs), &attribs); err != nil {
-		glog.Error(log("error parsing csi PV.Annotation [%s]=%s: %v", csiVolAttribsAnnotationKey, jsonAttribs, err))
-		return nil, err
-	}
-	return attribs, nil
-}
-
 // saveVolumeData persists parameter data as json file using the location
 // generated by /var/lib/kubelet/pods/<podID>/volumes/kubernetes.io~csi/<specVolId>/volume_data.json
 func saveVolumeData(p *csiPlugin, podUID types.UID, specVolID string, data map[string]string) error {

pkg/volume/csi/csi_mounter_test.go

@@ -202,53 +202,6 @@ func TestUnmounterTeardown(t *testing.T) {
 
 }
 
-func TestGetVolAttribsFromSpec(t *testing.T) {
-	testCases := []struct {
-		name        string
-		annotations map[string]string
-		attribs     map[string]string
-		shouldFail  bool
-	}{
-		{
-			name:        "attribs ok",
-			annotations: map[string]string{"key0": "val0", csiVolAttribsAnnotationKey: `{"k0":"attr0","k1":"attr1","k2":"attr2"}`, "keyN": "valN"},
-			attribs:     map[string]string{"k0": "attr0", "k1": "attr1", "k2": "attr2"},
-		},
-
-		{
-			name:        "missing attribs",
-			annotations: map[string]string{"key0": "val0", "keyN": "valN"},
-		},
-		{
-			name: "missing annotations",
-		},
-		{
-			name:        "bad json",
-			annotations: map[string]string{"key0": "val0", csiVolAttribsAnnotationKey: `{"k0""attr0","k1":"attr1,"k2":"attr2"`, "keyN": "valN"},
-			attribs:     map[string]string{"k0": "attr0", "k1": "attr1", "k2": "attr2"},
-			shouldFail:  true,
-		},
-	}
-	spec := volume.NewSpecFromPersistentVolume(makeTestPV("test-pv", 10, testDriver, testVol), false)
-	for _, tc := range testCases {
-		t.Logf("test case: %s", tc.name)
-		spec.PersistentVolume.Annotations = tc.annotations
-		attribs, err := getVolAttribsFromSpec(spec)
-		if !tc.shouldFail && err != nil {
-			t.Errorf("test case should not fail, but err != nil: %v", err)
-		}
-		eq := true
-		for k, v := range attribs {
-			if tc.attribs[k] != v {
-				eq = false
-			}
-		}
-		if !eq {
-			t.Errorf("expecting attribs %#v, but got %#v", tc.attribs, attribs)
-		}
-	}
-}
-
 func TestSaveVolumeData(t *testing.T) {
 	plug, tmpDir := newTestPlugin(t)
 	defer os.RemoveAll(tmpDir)

pkg/volume/csi/csi_plugin.go

@@ -31,8 +31,7 @@ import (
 )
 
 const (
-	csiPluginName              = "kubernetes.io/csi"
-	csiVolAttribsAnnotationKey = "csi.volume.kubernetes.io/volume-attributes"
+	csiPluginName = "kubernetes.io/csi"
 
 	// TODO (vladimirvivien) implement a more dynamic way to discover
 	// the unix domain socket path for each installed csi driver.

staging/src/k8s.io/api/core/v1/generated.proto

@@ -191,6 +191,10 @@ message CSIPersistentVolumeSource {
   // Ex. "ext4", "xfs", "ntfs". Implicitly inferred to be "ext4" if unspecified.
   // +optional
   optional string fsType = 4;
+
+  // Attributes of the volume to publish.
+  // +optional
+  map<string, string> volumeAttributes = 5;
 }
 
 // Adds and removes POSIX capabilities from running containers.

staging/src/k8s.io/api/core/v1/types.go

@@ -1748,6 +1748,10 @@ type CSIPersistentVolumeSource struct {
 	// Ex. "ext4", "xfs", "ntfs". Implicitly inferred to be "ext4" if unspecified.
 	// +optional
 	FSType string `json:"fsType,omitempty" protobuf:"bytes,4,opt,name=fsType"`
+
+	// Attributes of the volume to publish.
+	// +optional
+	VolumeAttributes map[string]string `json:"volumeAttributes,omitempty" protobuf:"bytes,5,rep,name=volumeAttributes"`
 }
 
 // ContainerPort represents a network port in a single container.

staging/src/k8s.io/api/core/v1/types_swagger_doc_generated.go

@@ -117,11 +117,12 @@ func (Binding) SwaggerDoc() map[string]string {
 }
 
 var map_CSIPersistentVolumeSource = map[string]string{
-	"":             "Represents storage that is managed by an external CSI volume driver (Beta feature)",
-	"driver":       "Driver is the name of the driver to use for this volume. Required.",
-	"volumeHandle": "VolumeHandle is the unique volume name returned by the CSI volume plugin’s CreateVolume to refer to the volume on all subsequent calls. Required.",
-	"readOnly":     "Optional: The value to pass to ControllerPublishVolumeRequest. Defaults to false (read/write).",
-	"fsType":       "Filesystem type to mount. Must be a filesystem type supported by the host operating system. Ex. \"ext4\", \"xfs\", \"ntfs\". Implicitly inferred to be \"ext4\" if unspecified.",
+	"":                 "Represents storage that is managed by an external CSI volume driver (Beta feature)",
+	"driver":           "Driver is the name of the driver to use for this volume. Required.",
+	"volumeHandle":     "VolumeHandle is the unique volume name returned by the CSI volume plugin’s CreateVolume to refer to the volume on all subsequent calls. Required.",
+	"readOnly":         "Optional: The value to pass to ControllerPublishVolumeRequest. Defaults to false (read/write).",
+	"fsType":           "Filesystem type to mount. Must be a filesystem type supported by the host operating system. Ex. \"ext4\", \"xfs\", \"ntfs\". Implicitly inferred to be \"ext4\" if unspecified.",
+	"volumeAttributes": "Attributes of the volume to publish.",
 }
 
 func (CSIPersistentVolumeSource) SwaggerDoc() map[string]string {

staging/src/k8s.io/api/core/v1/zz_generated.deepcopy.go

@@ -249,6 +249,13 @@ func (in *Binding) DeepCopyObject() runtime.Object {
 // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
 func (in *CSIPersistentVolumeSource) DeepCopyInto(out *CSIPersistentVolumeSource) {
 	*out = *in
+	if in.VolumeAttributes != nil {
+		in, out := &in.VolumeAttributes, &out.VolumeAttributes
+		*out = make(map[string]string, len(*in))
+		for key, val := range *in {
+			(*out)[key] = val
+		}
+	}
 	return
 }
 
@@ -3300,7 +3307,7 @@ func (in *PersistentVolumeSource) DeepCopyInto(out *PersistentVolumeSource) {
 			*out = nil
 		} else {
 			*out = new(CSIPersistentVolumeSource)
-			**out = **in
+			(*in).DeepCopyInto(*out)
 		}
 	}
 	return