Refactor RBD volume #54302

sbezverk · 2017-10-20T15:46:52Z

Refactor RBD Volume Persistent Volume Spec so RBD PV's SecretRef
allows referencing a secret from a persistent volume in any namespace.
This allows locating credentials for persistent volumes in namespaces
other than the one containing the PVC.
Closes #54432

RBD Persistent Volume Sources can now reference User's Secret in namespaces other than the namespace of the bound Persistent Volume Claim

sbezverk · 2017-10-20T15:47:31Z

/assign @rootfs

rootfs · 2017-10-20T15:53:18Z

/assign @liggitt

rootfs · 2017-10-20T15:54:05Z

please split the PR into 2 commits: one for generated files and one for the rest

rootfs · 2017-10-20T16:30:28Z

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

+        // Default is nil.
+        // More info: https://releases.k8s.io/HEAD/examples/volumes/rbd/README.md#how-to-use-it
+        // +optional
+ 	SecretRef *SecretReference `json:"secretRef,omitempty" protobuf:"bytes,5,opt,name=secretRef"`


thanks a lot. Changed.

liggitt · 2017-10-21T04:58:31Z

pkg/api/v1/zz_generated.defaults.go

@@ -136,9 +136,6 @@ func SetObjectDefaults_PersistentVolume(in *v1.PersistentVolume) {
 	if in.Spec.PersistentVolumeSource.HostPath != nil {
 		SetDefaults_HostPathVolumeSource(in.Spec.PersistentVolumeSource.HostPath)
 	}
-	if in.Spec.PersistentVolumeSource.RBD != nil {
-		SetDefaults_RBDVolumeSource(in.Spec.PersistentVolumeSource.RBD)


need to copy SetDefaults_RBDVolumeSource to SetDefaults_RBDPersistentVolumeSource

which also means copying the registered fuzzing function in pkg/api/fuzzer/fuzzer.go for RBDVolumeSource for RBDPersistentVolumeSource

Done fuzzer, but could you clarify about the first part? Do you want me to manually edit generated file or there is another way to accomplish it without editiing zz file.

SetDefaults_RBDVolumeSource is not in a generated file. Duplicate it there for the new type and rerun generation scripts which will fix up this file

Done, I think at least, please check when you have a second.

liggitt · 2017-10-21T05:00:47Z

pkg/volume/rbd/disk_manager.go

@@ -40,6 +40,8 @@ type diskManager interface {
 	DetachDisk(disk rbdUnmounter, mntPath string) error
 	// Creates a rbd image
 	CreateImage(provisioner *rbdVolumeProvisioner) (r *v1.RBDVolumeSource, volumeSizeGB int, err error)


I only see this called in a single place, from rbdVolumeProvisioner, right? that should only deal with persistentvolumes... should just change the existing function

liggitt · 2017-10-21T05:06:07Z

pkg/volume/rbd/rbd.go

 			return nil, err
 		}
+		for name, data := range secrets.Data {
+			secret = string(data)
+			glog.V(4).Infof("found ceph secret info: %s", name)


too verbose for inner loop, we weren't logging this before in parsePodSecret

liggitt · 2017-10-21T05:08:30Z

pkg/volume/rbd/rbd.go

@@ -486,16 +486,29 @@ func (c *rbdUnmounter) TearDownAt(dir string) error {
 	return diskTearDown(c.manager, *c, dir, c.mounter)
 }

-func getVolumeSource(
-	spec *volume.Spec) (*v1.RBDVolumeSource, bool, error) {
+func getVolumeSource(spec *volume.Spec) ([]string, string, string, string, string, string, bool, error) {


this many return values of identical types is asking for trouble... individual accessors are simple and clear

liggitt · 2017-10-21T05:09:27Z

pkg/volume/rbd/rbd_util.go

@@ -407,6 +407,51 @@ func (util *RBDUtil) CreateImage(p *rbdVolumeProvisioner) (r *v1.RBDVolumeSource
 	}, sz, nil
 }

+func (util *RBDUtil) CreatePersistentVolumeImage(p *rbdVolumeProvisioner) (r *v1.RBDPersistentVolumeSource, size int, err error) {


should be able to change the existing CreateImage method instead of introducing this... there are no other callers

liggitt · 2017-10-21T05:13:15Z

pkg/volume/rbd/rbd.go

 	if err != nil {
 		glog.Errorf("rbd: create volume failed, err: %v", err)
 		return nil, err
 	}
 	glog.Infof("successfully created rbd image %q", image)
 	pv := new(v1.PersistentVolume)
 	metav1.SetMetaDataAnnotation(&pv.ObjectMeta, volumehelper.VolumeDynamicallyCreatedByKey, "rbd-dynamic-provisioner")
-	rbd.SecretRef = new(v1.LocalObjectReference)
+	rbd.SecretRef = new(v1.SecretReference)
 	rbd.SecretRef.Name = secretName


I expected to see rbd.SecretRef.Namespace = secretNamespace set from a case "usersecretnamespace": config param

sbezverk · 2017-10-21T21:34:48Z

/sig storage

liggitt · 2017-10-22T16:14:15Z

pkg/api/persistentvolume/util_test.go

-				RBD: &api.RBDVolumeSource{
-					SecretRef: &api.LocalObjectReference{
+				RBD: &api.RBDPersistentVolumeSource{
+					SecretRef: &api.SecretReference{


Add a second PV spec that specifies a name and namespace in the RBD secretref (suggest namespace of "rbdns") and update expected namespaced secrets below

liggitt · 2017-10-22T16:19:12Z

pkg/volume/rbd/rbd.go

@@ -349,6 +380,9 @@ func (r *rbdVolumeProvisioner) Provision() (*v1.PersistentVolume, error) {
 	if secretName == "" {
 		return nil, fmt.Errorf("missing user secret name")
 	}
+	if secretNamespace == "" {
+		secretNamespace = adminSecretNamespace


This is incorrect. User secret namespace defaults to "", which uses the namespace of the pod/pvc

liggitt · 2017-10-22T16:21:00Z

pkg/volume/rbd/rbd.go

-	spec *volume.Spec) (*v1.RBDVolumeSource, bool, error) {
+func getVolumeSourceMonitors(spec *volume.Spec) ([]string, error) {
+	if spec.Volume != nil && spec.Volume.RBD != nil {
+		mon := spec.Volume.RBD.CephMonitors


for all the accessors, remove all the intermediate assignments and just return spec.Volume.RBD.CephMonitors, nil directly, etc

liggitt · 2017-10-22T16:22:34Z

pkg/volume/rbd/rbd.go

+		return readOnly, nil
+	} else if spec.PersistentVolume != nil &&
+		spec.PersistentVolume.Spec.RBD != nil {
+		readOnly := spec.ReadOnly


Comment here that this is intentional would be good (also worth noting in the API doc for this field that it is ignored in favor of the PV readonly setting would be good)

liggitt · 2017-10-22T18:46:04Z

A few gofmt errors to fix:

I1022 18:39:48.130] Verifying hack/make-rules/../../hack/verify-gofmt.sh
I1022 18:40:00.434] diff ./pkg/api/persistentvolume/util_test.go gofmt/./pkg/api/persistentvolume/util_test.go
I1022 18:40:00.434] --- /tmp/gofmt551719841	2017-10-22 18:39:54.826431042 +0000
I1022 18:40:00.434] +++ /tmp/gofmt114547340	2017-10-22 18:39:54.826431042 +0000
I1022 18:40:00.434] @@ -68,13 +68,13 @@
I1022 18:40:00.435]  				RBD: &api.RBDPersistentVolumeSource{
I1022 18:40:00.435]  					SecretRef: &api.SecretReference{
I1022 18:40:00.435]  						Name: "Spec.PersistentVolumeSource.RBD.SecretRef"}}}}},
I1022 18:40:00.435] -                {Spec: api.PersistentVolumeSpec{
I1022 18:40:00.435] -                        ClaimRef: &api.ObjectReference{Namespace: "claimrefns", Name: "claimrefname"},
I1022 18:40:00.435] -                        PersistentVolumeSource: api.PersistentVolumeSource{
I1022 18:40:00.436] -                                RBD: &api.RBDPersistentVolumeSource{
I1022 18:40:00.436] -                                        SecretRef: &api.SecretReference{
I1022 18:40:00.436] -                                                Name: "Spec.PersistentVolumeSource.RBD.SecretRef",
I1022 18:40:00.436] -                                                Namespace: "rbdns"}}}}},
I1022 18:40:00.436] +		{Spec: api.PersistentVolumeSpec{
I1022 18:40:00.436] +			ClaimRef: &api.ObjectReference{Namespace: "claimrefns", Name: "claimrefname"},
I1022 18:40:00.436] +			PersistentVolumeSource: api.PersistentVolumeSource{
I1022 18:40:00.437] +				RBD: &api.RBDPersistentVolumeSource{
I1022 18:40:00.437] +					SecretRef: &api.SecretReference{
I1022 18:40:00.437] +						Name:      "Spec.PersistentVolumeSource.RBD.SecretRef",
I1022 18:40:00.437] +						Namespace: "rbdns"}}}}},
I1022 18:40:00.437]  		{Spec: api.PersistentVolumeSpec{
I1022 18:40:00.437]  			ClaimRef: &api.ObjectReference{Namespace: "claimrefns", Name: "claimrefname"},
I1022 18:40:00.437]  			PersistentVolumeSource: api.PersistentVolumeSource{
I1022 18:40:00.438] @@ -148,7 +148,7 @@
I1022 18:40:00.438]  		"cephfs/Spec.PersistentVolumeSource.CephFS.SecretRef",
I1022 18:40:00.438]  		"claimrefns/Spec.PersistentVolumeSource.FlexVolume.SecretRef",
I1022 18:40:00.438]  		"claimrefns/Spec.PersistentVolumeSource.RBD.SecretRef",
I1022 18:40:00.438] -                "rbdns/Spec.PersistentVolumeSource.RBD.SecretRef",
I1022 18:40:00.438] +		"rbdns/Spec.PersistentVolumeSource.RBD.SecretRef",
I1022 18:40:00.438]  		"claimrefns/Spec.PersistentVolumeSource.ScaleIO.SecretRef",
I1022 18:40:00.439]  		"claimrefns/Spec.PersistentVolumeSource.ISCSI.SecretRef",
I1022 18:40:00.439]  		"storageosns/Spec.PersistentVolumeSource.StorageOS.SecretRef",
I1022 18:40:00.439] diff ./pkg/volume/rbd/rbd.go gofmt/./pkg/volume/rbd/rbd.go
I1022 18:40:00.439] --- /tmp/gofmt441846395	2017-10-22 18:39:55.218430953 +0000
I1022 18:40:00.439] +++ /tmp/gofmt314242462	2017-10-22 18:39:55.218430953 +0000
I1022 18:40:00.439] @@ -589,8 +589,8 @@
I1022 18:40:00.439]  		return spec.Volume.RBD.ReadOnly, nil
I1022 18:40:00.439]  	} else if spec.PersistentVolume != nil &&
I1022 18:40:00.440]  		spec.PersistentVolume.Spec.RBD != nil {
I1022 18:40:00.440] -                // rbd volumes used as a PersistentVolume gets the ReadOnly flag indirectly through
I1022 18:40:00.440] -                // the persistent-claim volume used to mount the PV
I1022 18:40:00.440] +		// rbd volumes used as a PersistentVolume gets the ReadOnly flag indirectly through
I1022 18:40:00.440] +		// the persistent-claim volume used to mount the PV
I1022 18:40:00.440]  		return spec.ReadOnly, nil
I1022 18:40:00.440]  	}
I1022 18:40:00.441]  
I1022 18:40:00.441] �[0;31mFAILED�[0m   hack/make-rules/../../hack/verify-gofmt.sh	12s

lgtm orherwise

sbezverk · 2017-10-24T01:01:45Z

/test pull-kubernetes-unit

liggitt · 2017-10-24T04:59:57Z

/lgtm
needs a release note and doc update indicating the new storage class secret param

sbezverk · 2017-10-24T12:33:16Z

/release-note
RBD Persistent Volume Sources can now reference User's Secret in namespaces other than the namespace of the bound Persistent Volume Claim

k8s-ci-robot · 2017-10-24T12:33:16Z

@sbezverk: the /release-note and /release-note-action-required commands have been deprecated.
Please edit the release-note block in the PR body text to include the release note. If the release note requires additional action include the string action required in the release note. For example:

```release-note
Some release note with action required.
```

In response to this:

/release-note
RBD Persistent Volume Sources can now reference User's Secret in namespaces other than the namespace of the bound Persistent Volume Claim

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.

sbezverk · 2017-10-24T12:36:18Z

RBD Persistent Volume Sources can now reference User's Secret in namespaces other than the namespace of the bound Persistent Volume Claim.

rootfs · 2017-10-24T13:13:28Z

/lgtm

rootfs · 2017-10-24T13:20:49Z

/assign @thockin @smarterclayton @pwittrock
for doc approval

smarterclayton · 2017-10-24T13:48:12Z

/approve

k8s-github-robot · 2017-10-24T13:49:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, rootfs, sbezverk, smarterclayton

Associated issue: 54432

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,smarterclayton]
~~docs/OWNERS~~ [smarterclayton]
~~pkg/api/OWNERS~~ [liggitt,smarterclayton]
~~pkg/printers/OWNERS~~ [liggitt,smarterclayton]
~~pkg/volume/rbd/OWNERS~~ [rootfs,smarterclayton]
~~staging/src/k8s.io/api/core/OWNERS~~ [liggitt,smarterclayton]

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

k8s-github-robot · 2017-10-24T15:35:08Z

Automatic merge from submit-queue (batch tested with PRs 54229, 54380, 54302, 54454). If you want to cherry-pick this change to another branch, please follow the instructions here.

krmayankk · 2018-01-13T07:54:18Z

@liggitt @rootfs @sbezverk why do most of these PR's not have a milestone, its hard to know which release they are part of. this is a great change. thanks for doing this. Is this also done in the external provisioner

krmayankk · 2018-01-13T07:55:15Z

api/swagger-spec/v1.json

-   "v1.RBDVolumeSource": {
-    "id": "v1.RBDVolumeSource",
+   "v1.RBDPersistentVolumeSource": {
+    "id": "v1.RBDPersistentVolumeSource",


@rootfs is this a backward compatible change ?

It is wire compatible. The type name never appears in serialized proto and json, and the new struct contains identical fields, with the addition of a namespace field in the secret ref

krmayankk · 2018-01-13T07:58:45Z

api/swagger-spec/v1.json

@@ -21674,6 +21674,51 @@
     }
    }
   },
+   "v1.RBDVolumeSource": {


why do we have both RBDPersistentVolumeSource and this ?

Because only the persistent volume version lets you reference a secret in a namespace other than the pod.

k8s-github-robot assigned vishh and justinsb Oct 20, 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 20, 2017

k8s-ci-robot assigned rootfs Oct 20, 2017

k8s-ci-robot assigned liggitt Oct 20, 2017

rootfs reviewed Oct 20, 2017

View reviewed changes

liggitt reviewed Oct 21, 2017

View reviewed changes

k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Oct 21, 2017

liggitt reviewed Oct 22, 2017

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 22, 2017

liggitt added 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 22, 2017

sbezverk added 2 commits October 23, 2017 16:59

Refactor RBD volume

1411c26

Refactor RBD volume (Generated files)

ab32196

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 23, 2017

liggitt added 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 24, 2017

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Oct 24, 2017

k8s-ci-robot added 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 24, 2017

k8s-ci-robot assigned pwittrock and thockin Oct 24, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 24, 2017

k8s-github-robot merged commit 16cdda0 into kubernetes:master Oct 24, 2017

cofyc mentioned this pull request Oct 25, 2017

RBD Plugin: Implement Attacher/Detacher interfaces. #51608

Merged

krmayankk reviewed Jan 13, 2018

View reviewed changes

dependabot-preview bot mentioned this pull request May 8, 2018

Bump kubernetes from 4.0.0 to 6.0.0 ministryofjustice/analytics-platform-unidler#3

Merged

dependabot-preview bot mentioned this pull request May 16, 2018

Bump kubernetes from 3.0.0 to 6.0.0 ministryofjustice/analytics-platform-control-panel#161

Closed

Refactor RBD volume #54302

Refactor RBD volume #54302

Conversation

sbezverk commented Oct 20, 2017 • edited

sbezverk commented Oct 20, 2017

rootfs commented Oct 20, 2017

rootfs commented Oct 20, 2017

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbezverk commented Oct 21, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Oct 22, 2017

sbezverk commented Oct 24, 2017

liggitt commented Oct 24, 2017

sbezverk commented Oct 24, 2017

k8s-ci-robot commented Oct 24, 2017

sbezverk commented Oct 24, 2017

rootfs commented Oct 24, 2017

rootfs commented Oct 24, 2017

smarterclayton commented Oct 24, 2017

k8s-github-robot commented Oct 24, 2017

k8s-github-robot commented Oct 24, 2017

krmayankk commented Jan 13, 2018

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

sbezverk commented Oct 20, 2017 •

edited

liggitt Oct 21, 2017 •

edited

liggitt Oct 21, 2017 •

edited

liggitt Oct 21, 2017 •

edited