ScaleIO - Ability to specify Secret's name and namespace #54013

vladimirvivien · 2017-10-16T21:16:13Z

What this PR does / why we need it:
This PR is to decouple the ScaleIO secret from the same namespace as that of the StorageClass/PVC/PV that uses it (#53619). Currently, authorized non-admin k8s user, who creates volumes, may end up having unauthorized access to ScaleIO secret information. This PR introduces secret parameter that allows specification of secret's namespace.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #53619

Release note:

ScaleIO persistent volumes now support referencing a secret in a namespace other than the bound persistent volume claim's namespace; this is controlled during provisioning with the `secretNamespace` storage class parameter; StoragePool and ProtectionDomain attributes no longer defaults to the value `default`

k8s-ci-robot · 2017-10-16T21:16:21Z

Hi @vladimirvivien. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

vladimirvivien · 2017-10-16T21:17:19Z

/assign @liggitt

liggitt · 2017-10-16T23:59:33Z

pkg/api/validation/validation.go

+		if sio.SecretRef.Name == "" {
+			allErrs = append(allErrs, field.Required(fldPath.Child("secretRef.Name"), ""))
+		}
+		if sio.SecretRef.Namespace == "" {


can't make this required, existing objects do not have a namespace

liggitt · 2017-10-17T00:00:19Z

pkg/api/validation/validation.go

+		allErrs = append(allErrs, field.Required(fldPath.Child("volumeName"), ""))
+	}
+	if sio.SecretRef != nil {
+		if sio.SecretRef.Name == "" {


can't tighten validation to make this required without breaking update of existing objects that didn't require this

still outstanding, should not tighten validation on existing objects. basically, the validation for this new object has to be identical to the existing validation on ScaleIOVolumeSource

liggitt · 2017-10-17T00:01:14Z

cc @kubernetes/sig-storage-api-reviews

rootfs · 2017-10-17T14:59:05Z

@vladimirvivien no generated files?

rootfs · 2017-10-17T14:59:18Z

/ok-to-test

liggitt · 2017-10-17T15:14:07Z

@rootfs this is still WIP, needs generated files and volume plugin updates

vladimirvivien · 2017-10-19T03:54:24Z

@liggitt @rootfs

Addressed changes pointed out earlier
Added generated files
Made changes to the plugin code
Update test files

liggitt · 2017-10-19T04:40:05Z

pkg/api/persistentvolume/util.go

+		if source.ScaleIO.SecretRef != nil && len(source.ScaleIO.SecretRef.Namespace) > 0 {
+			ns = source.ScaleIO.SecretRef.Namespace
+		}
+		if !visitor(ns, source.ScaleIO.SecretRef.Name) {


Keep this inside a if source.ScaleIO.SecretRef != nil check

liggitt · 2017-10-19T04:41:50Z

pkg/api/validation/validation.go

@@ -1231,6 +1231,30 @@ func validateScaleIOVolumeSource(sio *api.ScaleIOVolumeSource, fldPath *field.Pa
 	if sio.VolumeName == "" {
 		allErrs = append(allErrs, field.Required(fldPath.Child("volumeName"), ""))
 	}
+	if sio.SecretRef != nil {
+		if sio.SecretRef.Name == "" {


Same here, this is tightening validation of existing objects, which isn’t allowed

liggitt · 2017-10-19T04:45:18Z

pkg/volume/scaleio/sio_util.go

 		sdcGuid string
 	}{
 		gateway:          "gateway",
 		sslEnabled:       "sslEnabled",
-		secretRef:        "secretRef",
+		secretName:       "secretRef",


+1 for keeping the same config key for compatibility

liggitt · 2017-10-21T04:15:37Z

pkg/volume/scaleio/sio_volume.go

@@ -448,19 +452,20 @@ func (v *sioVolume) setSioMgrFromConfig() error {
 	if v.sioMgr == nil {
 		configData := v.configData
 		applyConfigDefaults(configData)
+
+		configData[confKey.volSpecName] = v.volSpecName


is there a reason this isn't setting these:

configData[confKey.secretName] = v.secretName configData[confKey.secretNamespace] = v.secretNamespace

@liggitt yes, secret and namespace comes from options.Prameters=v.configData during NewProvisioner. But, will change code to make that clearer.

liggitt · 2017-10-21T04:18:02Z

--- FAIL: TestTypeTags (0.15s)
	import_known_versions_test.go:163: External types should have json tags. schema.GroupVersionKind{Group:"", Version:"v1", Kind:"PersistentVolume"} tags on field Gateway are: protobuf:"bytes,1,opt,name=gateway"
	import_known_versions_test.go:165: v1.PersistentVolume
	import_known_versions_test.go:165:   v1.PersistentVolumeSpec
	import_known_versions_test.go:165:     v1.PersistentVolumeSource
	import_known_versions_test.go:165:       *v1.ScaleIOPersistentVolumeSource
	import_known_versions_test.go:165:         v1.ScaleIOPersistentVolumeSource
	import_known_versions_test.go:163: External types should have json tags.

liggitt · 2017-10-21T04:19:08Z

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

@@ -1375,6 +1375,42 @@ type ScaleIOVolumeSource struct {
 	ReadOnly bool `json:"readOnly,omitempty" protobuf:"varint,10,opt,name=readOnly"`
 }

+// ScaleIOPersistentVolumeSource represents a persistent ScaleIO volume that can be defined
+// by a an admin via a storage class, for instance.
+type ScaleIOPersistentVolumeSource struct {


this should be an exact copy of v1.ScaleIOVolumeSource with SecretRef changed to *SecretReference. That will pick up all the json/protobuf tags verbatim:

// ScaleIOPersistentVolumeSource represents a persistent ScaleIO volume type ScaleIOPersistentVolumeSource struct { // The host address of the ScaleIO API Gateway. Gateway string `json:"gateway" protobuf:"bytes,1,opt,name=gateway"` // The name of the storage system as configured in ScaleIO. System string `json:"system" protobuf:"bytes,2,opt,name=system"` // SecretRef references to the secret for ScaleIO user and other // sensitive information. If this is not provided, Login operation will fail. SecretRef *SecretReference `json:"secretRef" protobuf:"bytes,3,opt,name=secretRef"` // Flag to enable/disable SSL communication with Gateway, default false // +optional SSLEnabled bool `json:"sslEnabled,omitempty" protobuf:"varint,4,opt,name=sslEnabled"` // The name of the Protection Domain for the configured storage (defaults to "default"). // +optional ProtectionDomain string `json:"protectionDomain,omitempty" protobuf:"bytes,5,opt,name=protectionDomain"` // The Storage Pool associated with the protection domain (defaults to "default"). // +optional StoragePool string `json:"storagePool,omitempty" protobuf:"bytes,6,opt,name=storagePool"` // Indicates whether the storage for a volume should be thick or thin (defaults to "thin"). // +optional StorageMode string `json:"storageMode,omitempty" protobuf:"bytes,7,opt,name=storageMode"` // The name of a volume already created in the ScaleIO system // that is associated with this volume source. VolumeName string `json:"volumeName,omitempty" protobuf:"bytes,8,opt,name=volumeName"` // 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. // +optional FSType string `json:"fsType,omitempty" protobuf:"bytes,9,opt,name=fsType"` // Defaults to false (read/write). ReadOnly here will force // the ReadOnly setting in VolumeMounts. // +optional ReadOnly bool `json:"readOnly,omitempty" protobuf:"varint,10,opt,name=readOnly"` }

@liggitt ok, thanks. I think I only changed SecretRef's type in ScaleIOPersistentVolumeSource. I will make sure they are the same in both ScaleIOVolumeSource as well. Thanks.

I re-read your comment. Ok, I will ensure the types are the same in staging and v1.

yeah, leave ScaleIOVolumeSource as is

@liggitt
I got confused because your comment above has ScaleIOVolumeSource using SecretReference. Please confirm:

XXXVolumeSource -> LocalObjectReference (in pkg/v1 and staging/v1)

XXXPersistentVolumeSource -> SecretReference (in pkg/v1 and staging/v1)

The above is how I see other types are done, but confirm anyway.

Oops, fixed.

liggitt · 2017-10-21T05:14:40Z

pkg/api/v1/zz_generated.defaults.go

@@ -145,9 +145,6 @@ func SetObjectDefaults_PersistentVolume(in *v1.PersistentVolume) {
 	if in.Spec.PersistentVolumeSource.AzureDisk != nil {
 		SetDefaults_AzureDiskVolumeSource(in.Spec.PersistentVolumeSource.AzureDisk)
 	}
-	if in.Spec.PersistentVolumeSource.ScaleIO != nil {
-		SetDefaults_ScaleIOVolumeSource(in.Spec.PersistentVolumeSource.ScaleIO)


need to copy SetDefaults_ScaleIOVolumeSource to SetDefaults_ScaleIOPersistentVolumeSource (changing the types), which will regenerate this to call a defaulting function

which also means copying the registered fuzzing function in pkg/api/fuzzer/fuzzer.go for func(sio *api.ScaleIOVolumeSource, c fuzz.Continue) for ScaleIOPersistentVolumeSource

ok will do.

liggitt · 2017-10-24T04:32:09Z

pkg/api/fuzzer/fuzzer.go

-				sio.StoragePool = "default"
+			sio.FSType = c.RandString()
+			if sio.FSType == "" {
+				sio.FSType = "xfs"
 			}


why did ProtectionDomain and StoragePool defaults get dropped? should be present in both fuzzing funcs for ScaleIOVolumeSource and ScaleIOPersistentVolumeSource, right?

@liggitt the assumption that these two params would default to value default was a bug. Because if those default were actually applied the volume would fail. This was partially fixed in the last release. So I removed them here in accordance with what was done then (#49973).

hmm, ok. changing defaults is typically considered a breaking API change, but if the defaults caused the object to never work, that doesn't seem good either.

would like agreement from @kubernetes/sig-storage-api-reviews on the removal of the defaulting behavior on those fields, and if agreed on, remove the (defaults to "default") godoc from those fields and add a description of the default change to the release note.

@liggitt thank you. Basically this is taking care of a bug in the API validation. If those values are applied by default as default, chances are they would break storage operations since, most likely, these values will not match values on the backing storage system.

Ok. Validation doesn't require those values to be set (and can't, without breaking existing API create calls), so with defaulting removed, if someone leaves them empty, they will just be unset now. Is that acceptable?

@liggitt Yes. That is acceptable.

liggitt · 2017-10-24T04:33:16Z

pkg/api/v1/defaults.go

-	if obj.StoragePool == "" {
-		obj.StoragePool = "default"
+	if obj.FSType == "" {
+		obj.FSType = "xfs"
 	}


restore these defaults to both SetDefaults_ScaleIOVolumeSource and SetDefaults_ScaleIOPersistentVolumeSource?

See comment above. Thanks!

liggitt · 2017-10-24T04:50:35Z

a couple last changes, then LGTM

one outstanding comment on not tightening validation
~~restore defaulting and fuzzing of StoragePool and ProtectionDomain~~ remove (defaults to "default") doc from StoragePool and ProtectionDomain fields and add change in defaulting behavior to the release note

vladimirvivien · 2017-10-25T00:24:01Z

/test pull-kubernetes-bazel-test

vladimirvivien · 2017-10-25T16:25:01Z

@liggitt

Update comment on API object to sync up with default values
Update release note

k8s-ci-robot · 2017-10-25T16:55:33Z

@vladimirvivien: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-bazel	ebb5abae5bd580e25f7c0f1a640bdb305cd7a74d	link	`/test pull-kubernetes-e2e-gce-bazel`
pull-kubernetes-e2e-gce-etcd3	ebb5abae5bd580e25f7c0f1a640bdb305cd7a74d	link	`/test pull-kubernetes-e2e-gce-etcd3`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

This commit tracks all human-generated code for API source updates.

vladimirvivien · 2017-10-25T23:36:56Z

@liggitt
Ready for LGTM

liggitt · 2017-10-26T03:14:37Z

/lgtm

liggitt · 2017-10-26T03:19:42Z

/assign @rootfs @saad-ali
for /pkg/volume approval

rootfs · 2017-10-26T17:52:04Z

/approve

childsb · 2017-10-26T17:53:47Z

/approve

k8s-github-robot · 2017-10-26T17:54:39Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: childsb, liggitt, rootfs, vladimirvivien
We suggest the following additional approvers:

Assign the PR to them by writing /assign in a comment when ready.

Associated issue: 53619

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~api/OWNERS~~ [liggitt]
~~docs/api-reference/OWNERS~~ [liggitt]
federation/OWNERS
~~pkg/api/OWNERS~~ [liggitt]
~~pkg/printers/OWNERS~~ [liggitt]
~~pkg/volume/OWNERS~~ [childsb,rootfs]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

liggitt · 2017-10-26T18:44:03Z

changes under federation/ are generated files only, tagging

k8s-github-robot · 2017-10-26T20:24:32Z

Automatic merge from submit-queue (batch tested with PRs 49865, 53731, 54013, 54513, 51502). If you want to cherry-pick this change to another branch, please follow the instructions here.

…f-#54013-upstream-release-1.8 Automatic merge from submit-queue. Automated cherry pick of #54013 Cherry pick of #54013 on release-1.8. #54013: ScaleIO - API source code update ```release-note ScaleIO persistent volumes now support referencing a secret in a namespace other than the bound persistent volume claim's namespace; this is controlled during provisioning with the `secretNamespace` storage class parameter; StoragePool and ProtectionDomain attributes no longer defaults to the value `default` ```

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 16, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 16, 2017

k8s-github-robot assigned vishh and gmarek Oct 16, 2017

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Oct 16, 2017

k8s-ci-robot assigned liggitt Oct 16, 2017

liggitt reviewed Oct 16, 2017

View reviewed changes

liggitt reviewed Oct 17, 2017

View reviewed changes

liggitt assigned rootfs and saad-ali and unassigned gmarek and vishh Oct 17, 2017

k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 17, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 17, 2017

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 18, 2017

liggitt reviewed Oct 19, 2017

View reviewed changes

liggitt reviewed Oct 21, 2017

View reviewed changes

liggitt reviewed Oct 24, 2017

View reviewed changes

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 24, 2017

vladimirvivien added 2 commits October 25, 2017 16:05

ScaleIO - API source code update

3fb3cc7

This commit tracks all human-generated code for API source updates.

ScaleIO - Generated files

1c9aff8

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2017

liggitt added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2017

k8s-github-robot merged commit 7d5dc52 into kubernetes:master Oct 26, 2017

This was referenced Nov 3, 2017

Automated cherry pick of #54013 #55077

Merged

Automated cherry pick of #54013 #55087

Closed

Automated cherry pick of #54013 #55142

Closed

vladimirvivien mentioned this pull request Jan 11, 2018

ScaleIO - decouple secret namspace from PVC for dynamic provisioning #51555

Closed

ScaleIO - Ability to specify Secret's name and namespace #54013

ScaleIO - Ability to specify Secret's name and namespace #54013

Conversation

vladimirvivien commented Oct 16, 2017 • edited by liggitt

k8s-ci-robot commented Oct 16, 2017

vladimirvivien commented Oct 16, 2017

Choose a reason for hiding this comment

liggitt Oct 17, 2017 • edited

Choose a reason for hiding this comment

liggitt Oct 21, 2017 • edited

Choose a reason for hiding this comment

liggitt commented Oct 17, 2017

rootfs commented Oct 17, 2017

rootfs commented Oct 17, 2017

liggitt commented Oct 17, 2017

vladimirvivien commented Oct 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Oct 21, 2017

liggitt Oct 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Oct 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Oct 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Oct 24, 2017 • edited

vladimirvivien commented Oct 25, 2017

vladimirvivien commented Oct 25, 2017

k8s-ci-robot commented Oct 25, 2017 • edited

vladimirvivien commented Oct 25, 2017

liggitt commented Oct 26, 2017

liggitt commented Oct 26, 2017

rootfs commented Oct 26, 2017

childsb commented Oct 26, 2017

k8s-github-robot commented Oct 26, 2017

liggitt commented Oct 26, 2017

k8s-github-robot commented Oct 26, 2017

vladimirvivien commented Oct 16, 2017 •

edited by liggitt

liggitt Oct 17, 2017 •

edited

liggitt Oct 21, 2017 •

edited

liggitt Oct 21, 2017 •

edited

liggitt Oct 21, 2017 •

edited

liggitt Oct 24, 2017 •

edited

liggitt commented Oct 24, 2017 •

edited

k8s-ci-robot commented Oct 25, 2017 •

edited