iSCSI CHAP support #43396

rootfs · 2017-03-20T17:32:12Z

What this PR does / why we need it:
To support CHAP authentication in a multi-tenant setup
Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

Support iSCSI CHAP authentication

k8s-reviewable · 2017-03-20T17:32:17Z

This change is

rootfs · 2017-03-20T17:34:53Z

@fabiand

rootfs · 2017-03-20T17:55:12Z

@kubernetes/sig-storage-misc

rootfs · 2017-03-20T17:56:52Z

/assign @jsafrane

jsafrane · 2017-03-21T08:04:59Z

pkg/api/types.go

+	// Optional: whether support iSCSI Session CHAP authentication
+	// +optional
+	SessionCHAPAuth bool
+	// Optional: CHAP secret for iSCSI target and initiator authentication


Please add a note when this parameter is required.

jsafrane · 2017-03-21T08:07:45Z

pkg/api/validation/validation.go

@@ -580,6 +580,9 @@ func validateISCSIVolumeSource(iscsi *api.ISCSIVolumeSource, fldPath *field.Path
 	if iscsi.Lun < 0 || iscsi.Lun > 255 {
 		allErrs = append(allErrs, field.Invalid(fldPath.Child("lun"), iscsi.Lun, validation.InclusiveRangeError(0, 255)))
 	}
+	if iscsi.DiscoveryCHAPAuth == true && iscsi.SessionCHAPAuth == true && iscsi.SecretRef == nil {


the unit test below expects (iscsi.DiscoveryCHAPAuth == true || iscsi.SessionCHAPAuth == true) && iscsi.SecretRef == nil. And checking for boolVariable == true looks weird...

jsafrane · 2017-03-21T09:21:32Z

pkg/volume/iscsi/iscsi_util.go

+		}
+
+	}
+	for _, k := range chap_st {


do we want to fill the username/secret when b.chap_discovery is false?

jsafrane · 2017-03-21T09:21:52Z

pkg/volume/iscsi/iscsi_util.go

+		}
+
+	}
+	for _, k := range chap_sess {


do we want to fill the username/secret when b.chap_session is false?

jsafrane · 2017-03-21T09:22:58Z

pkg/volume/iscsi/iscsi_util.go

+			// build discoverydb and discover iscsi target
+			b.plugin.execCommand("iscsiadm", []string{"-m", "discoverydb", "-t", "sendtargets", "-p", tp, "-I", b.iface, "-o", "new"})
+			// update discoverydb with CHAP secret
+			err = updateISCSIDiscoverydb(b, tp)


You should clear username/password on error

jsafrane · 2017-03-21T09:23:15Z

pkg/volume/iscsi/iscsi_util.go

+				last_err = fmt.Errorf("iscsi: failed to sendtargets to portal %s output: %s, err %v", tp, string(out), err)
+				continue
+			}
+			err = updateISCSINode(b, tp)


You should clear username/password on error

jsafrane · 2017-03-21T09:31:57Z

pkg/volume/iscsi/iscsi_util.go

+			// update discoverydb with CHAP secret
+			err = updateISCSIDiscoverydb(b, tp)
+			if err != nil {
+				last_err = fmt.Errorf("iscsi: failed to update discoverydb to portal %s error: %v", tp, err)


IMO you should log the errors here (and everywhere where last_err is used), maybe as warnings or info, so we can see all the errors and not only the last one in the logs.

jsafrane · 2017-03-21T09:33:14Z

pkg/volume/iscsi/iscsi_util.go

@@ -133,16 +188,28 @@ func (util *ISCSIUtil) AttachDisk(b iscsiDiskMounter) error {
 		}
 		exist := waitForPathToExist(devicePath, 1, iscsiTransport)


Can this waitForPathToExist ever succeed if we know that CHAP is enabled and we did not set username / password? IMO it adds unnecessary 1 second delay.

this is to detect if the devices are already created. 1 is retry.

jsafrane · 2017-03-21T09:34:01Z

pkg/volume/iscsi/iscsi_util.go

@@ -307,6 +307,13 @@ func (util *ISCSIUtil) DetachDisk(c iscsiDiskUnmounter, mntPath string) error {
 					glog.Errorf("iscsi: failed to detach disk Error: %s", string(out))
 				}
 			}
+			// delete the node record from database


this does not look like README change

humblec · 2017-03-24T06:45:45Z

pkg/api/v1/generated.proto

+  // whether support iSCSI Discovery CHAP authentication
+  // +optional
+  optional bool chapAuthDiscovery = 8;
+


@rootfs did we deliberately left "9" or Am I missing something here?

this is generated code

ah.. yes. missed that part.

humblec · 2017-03-24T06:51:38Z

pkg/volume/iscsi/iscsi.go

+	if source.SecretRef != nil {
+		if secret, err = ioutil.GetSecretForPod(pod, source.SecretRef.Name, plugin.host.GetKubeClient()); err != nil {
+			glog.Errorf("Couldn't get secret from %v/%v", pod.Namespace, source.SecretRef)
+			return nil, err


@rootfs checking secret in pod's namespace is good enough ? or do we need to have an option for specifying a 'namespace' for this secret?

This uses pod's namespace. Referencing across namespaces is not guaranteed to succeed.

rootfs · 2017-03-28T20:44:13Z

@k8s-bot bazel test this

humblec · 2017-04-05T11:08:01Z

LGTM.

Signed-off-by: Huamin Chen <hchen@redhat.com>

rootfs · 2017-04-05T13:54:27Z

@k8s-bot non-cri e2e test this

thockin · 2017-04-05T15:34:31Z

/approve

rootfs · 2017-04-05T23:29:59Z

thank you @thockin

rootfs · 2017-04-05T23:31:22Z

@jsafrane PTAL, thanks!

saad-ali · 2017-04-06T01:02:01Z

lgtm will defer final lgtm to @jsafrane

jsafrane · 2017-04-06T08:17:01Z

pkg/volume/iscsi/iscsi_util.go

+			}
+			err = updateISCSINode(b, tp)
+			if err != nil {
+				last_err = fmt.Errorf("iscsi: failed to update iscsi node to portal %s error: %v", tp, err)


missing iscsiadm -m discoverydb .. -o delete?

keep the discoverydb there in case there is another open session using the same sendtarget.

jsafrane · 2017-04-06T08:20:39Z

pkg/volume/iscsi/iscsi_util.go

-				glog.Errorf("iscsi: failed to attach disk:Error: %s (%v)", string(out), err)
+				// delete the node record from database
+				b.plugin.execCommand("iscsiadm", []string{"-m", "node", "-p", tp, "-I", b.iface, "-T", b.iqn, "-o", "delete"})
+				last_err = fmt.Errorf("iscsi: failed to attach disk:Error: %s (%v)", string(out), err)


five lines below, there is "Could not attach disk: Timeout after 10s" error. Should it update last_err? Should it delete node records?

jsafrane · 2017-04-06T08:21:04Z

pkg/volume/iscsi/iscsi_util.go

@@ -105,6 +157,7 @@ func (util *ISCSIUtil) AttachDisk(b iscsiDiskMounter) error {
 	var devicePath string
 	var devicePaths []string
 	var iscsiTransport string
+	var last_err error


jsafrane · 2017-04-06T08:25:37Z

examples/volumes/iscsi/README.md

+
+### CHAP Secret
+
+As illustrated in [chap-secret.yaml](chap-secret.yaml), the secret consists of the following keys:


"must have type kubernetes.io/iscsi-chap and consists of the following keys"?

jsafrane · 2017-04-06T08:44:28Z

examples/volumes/iscsi/README.md

+
+These keys map to those used by Open-iSCSI initiator. Detailed documents on these keys can be found at [Open-iSCSI](https://github.com/open-iscsi/open-iscsi/blob/master/etc/iscsid.conf)
+
+Note, all user names and passwords must be base64 encoded in the secret.


That's IMO misleading. We use base64 to encode all secret values in the yaml just for easier transport, it's not something CHAP specific. E.g. on command line we use plaintext:

$ kubectl create secret generic my-secret --from-literal=discovery.sendtargets.auth.username=alibaba --from-literal=discovery.sendtargets.auth.password=opensesame -o yaml kind: Secret data: discovery.sendtargets.auth.password: b3BlbnNlc2FtZQ== discovery.sendtargets.auth.username: YWxpYmFiYQ==

It's not like RBD, where the passwords must be encoded into base64 twice.

Signed-off-by: Huamin Chen <hchen@redhat.com>

jsafrane · 2017-04-07T16:42:28Z

/lgtm

k8s-github-robot · 2017-04-07T16:42:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, rootfs, thockin

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

~~OWNERS~~ [thockin]

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

rootfs · 2017-04-07T16:44:46Z

thanks @jsafrane great reviews!

rootfs · 2017-04-07T18:38:32Z

@k8s-bot non-cri e2e test this

k8s-github-robot · 2017-04-07T21:09:41Z

Automatic merge from submit-queue (batch tested with PRs 44119, 42538, 43802, 42336, 43396)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 20, 2017

rootfs changed the title ~~Iscsi CHAP support~~ iSCSI CHAP support Mar 20, 2017

k8s-github-robot assigned piosz Mar 20, 2017

rootfs force-pushed the iscsi-chap branch from e45a03d to 93cfd8b Compare March 20, 2017 17:34

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 20, 2017

k8s-ci-robot assigned jsafrane Mar 20, 2017

rootfs force-pushed the iscsi-chap branch 2 times, most recently from e0c259c to 5c4c306 Compare March 20, 2017 20:12

jsafrane reviewed Mar 21, 2017

View reviewed changes

piosz assigned saad-ali and unassigned piosz Mar 22, 2017

humblec reviewed Mar 24, 2017

View reviewed changes

rootfs force-pushed the iscsi-chap branch from 5c4c306 to 6974577 Compare March 27, 2017 16:50

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2017

rootfs force-pushed the iscsi-chap branch from 6d1c037 to 1e916f2 Compare March 28, 2017 20:25

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 5, 2017

rootfs added 2 commits April 5, 2017 12:43

add iSCSI CHAP API

4b7e084

Signed-off-by: Huamin Chen <hchen@redhat.com>

generated files

777eb32

Signed-off-by: Huamin Chen <hchen@redhat.com>

rootfs force-pushed the iscsi-chap branch from 1e916f2 to 321cb5d Compare April 5, 2017 12:47

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 5, 2017

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

jsafrane reviewed Apr 6, 2017

View reviewed changes

rootfs force-pushed the iscsi-chap branch 2 times, most recently from 73f8920 to f9ee03a Compare April 6, 2017 14:04

rootfs added 2 commits April 7, 2017 16:38

Add iSCSI CHAP authentication

9298217

Signed-off-by: Huamin Chen <hchen@redhat.com>

update iSCSI README with CHAP instruction

8eb6d6c

Signed-off-by: Huamin Chen <hchen@redhat.com>

rootfs force-pushed the iscsi-chap branch from f9ee03a to 8eb6d6c Compare April 7, 2017 16:38

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

k8s-github-robot merged commit 98a4c6b into kubernetes:master Apr 7, 2017

humblec mentioned this pull request May 12, 2017

Enable Chap Authentication in gluster block kubernetes-retired/external-storage#105

Merged

		@@ -133,16 +188,28 @@ func (util *ISCSIUtil) AttachDisk(b iscsiDiskMounter) error {
		}
		exist := waitForPathToExist(devicePath, 1, iscsiTransport)


		### CHAP Secret

		As illustrated in [chap-secret.yaml](chap-secret.yaml), the secret consists of the following keys:


		These keys map to those used by Open-iSCSI initiator. Detailed documents on these keys can be found at [Open-iSCSI](https://github.com/open-iscsi/open-iscsi/blob/master/etc/iscsid.conf)

		Note, all user names and passwords must be base64 encoded in the secret.

iSCSI CHAP support #43396

iSCSI CHAP support #43396

Conversation

rootfs commented Mar 20, 2017

k8s-reviewable commented Mar 20, 2017

rootfs commented Mar 20, 2017

rootfs commented Mar 20, 2017

rootfs commented Mar 20, 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

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

rootfs commented Mar 28, 2017

humblec commented Apr 5, 2017

rootfs commented Apr 5, 2017

thockin commented Apr 5, 2017

rootfs commented Apr 5, 2017

rootfs commented Apr 5, 2017

saad-ali commented Apr 6, 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

Choose a reason for hiding this comment

jsafrane commented Apr 7, 2017

k8s-github-robot commented Apr 7, 2017

rootfs commented Apr 7, 2017

rootfs commented Apr 7, 2017

k8s-github-robot commented Apr 7, 2017