support mount options in azure file #54674

andyzhangx · 2017-10-27T05:41:55Z

What this PR does / why we need it:
support mount options in azure file

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

Special notes for your reviewer:
@rootfs @karataliu @feiskyer
By default, the dir_mode and file_mode would be 0700, vers would be 3.0, while if user specify dir_mode, file_mode, vers in storage class in mountOptions field(see below), then azure file should use user specified mount options.

---
kind: StorageClass
apiVersion: storage.k8s.io/v1
metadata:
  name: azurefile
provisioner: kubernetes.io/azure-file
mountOptions:
  - dir_mode=0377
  - file_mode=0350
  - vers=2.1
  - uid=1000
  - gid=1000
parameters:
  skuName: Standard_LRS
  location: westus2

Release note:

Support mount options in azure file

/sig azure

andyzhangx · 2017-10-27T06:53:15Z

/test pull-kubernetes-unit

andyzhangx · 2017-10-27T06:53:23Z

/test pull-kubernetes-e2e-gce

dixudx · 2017-10-27T07:59:54Z

pkg/volume/azure_file/azure_file.go

@@ -48,6 +48,10 @@ var _ volume.PersistentVolumePlugin = &azureFilePlugin{}

 const (
 	azureFilePluginName = "kubernetes.io/azure-file"
+	fileModeName        = "file_mode"
+	dirModeName         = "dir_mode"
+	defaultFileMode     = "700"


defaultFileMode is used only in tests. Why declaring here?

Also I suggest moving these const vars to azure_util.go.

feiskyer · 2017-10-27T08:05:02Z

pkg/volume/azure_file/azure_util.go

@@ -84,3 +85,29 @@ func (s *azureSvc) SetAzureCredentials(host volume.VolumeHost, nameSpace, accoun
 	}
 	return secretName, err
 }
+
+// check whether mountOptions contains file_mode and dir_mode, if not, append default mode


nit: appendDefaultMountOptions checks

feiskyer · 2017-10-27T08:06:13Z

pkg/volume/azure_file/azure_file_test.go

+		{
+			options:  []string{"file_mode=0777"},
+			expected: []string{"file_mode=0777", fmt.Sprintf("%s=%s", dirModeName, defaultDirMode)},
+		},


nit: add another case of setting both

ccojocar · 2017-10-27T11:25:54Z

pkg/volume/azure_file/azure_file.go

@@ -222,11 +226,12 @@ func (b *azureFileMounter) SetUpAt(dir string, fsGroup *int64) error {
 	} else {
 		os.MkdirAll(dir, 0700)


You should use the dir_mode value if defined, otherwise defaultFileMode when the root folder is created.

actually here dir is the k8s path in the agent node, e.g. /var/lib/kubelet/plugins/kubernetes.io/azure-disk/mounts/b1246717734, not the container mount path, k8s would then use volume mapping between dir and container mount path. So dir_mode is different here, we should not set os.MkdirAll(dir, 0700) as dir_mode in mountOptions.

gnufied · 2017-10-27T15:46:38Z

mostly lgtm to me. Please fix the gofmt issue..

feiskyer

/lgtm

andyzhangx · 2017-11-06T05:47:33Z

/test pull-kubernetes-bazel-test

andyzhangx · 2017-11-06T08:01:11Z

@rootfs could you help review, thx

andyzhangx · 2017-11-11T05:12:41Z

/test pull-kubernetes-bazel-test

andyzhangx · 2017-11-11T05:12:47Z

/test pull-kubernetes-e2e-gce

andyzhangx · 2017-11-15T15:07:16Z

@rootfs @brendanburns

rootfs · 2017-11-15T15:31:29Z

pkg/volume/azure_file/azure_util.go


 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/kubernetes/pkg/volume"
 )

+const (
+	fileModeName    = "file_mode"


a bit paranoid but file_mode= is safer to me (and dir_mode=) too

fileMode is better, underscore is not good in go var naming convention

rootfs · 2017-11-15T15:31:53Z

pkg/volume/azure_file/azure_util.go

+const (
+	fileModeName    = "file_mode"
+	dirModeName     = "dir_mode"
+	defaultFileMode = "700"


thx for check

rootfs · 2017-11-15T15:32:03Z

pkg/volume/azure_file/azure_util.go

+	fileModeName    = "file_mode"
+	dirModeName     = "dir_mode"
+	defaultFileMode = "700"
+	defaultDirMode  = "700"


rootfs · 2017-11-15T15:32:27Z

pkg/volume/azure_file/azure_util.go

@@ -84,3 +92,29 @@ func (s *azureSvc) SetAzureCredentials(host volume.VolumeHost, nameSpace, accoun
 	}
 	return secretName, err
 }
+
+// check whether mountOptions contains file_mode and dir_mode, if not, append default mode


rootfs · 2017-11-15T15:39:57Z

pkg/volume/azure_file/azure_file.go

@@ -222,11 +222,12 @@ func (b *azureFileMounter) SetUpAt(dir string, fsGroup *int64) error {
 	} else {
 		os.MkdirAll(dir, 0700)
 		// parameters suggested by https://azure.microsoft.com/en-us/documentation/articles/storage-how-to-use-files-linux/
-		options := []string{fmt.Sprintf("vers=3.0,username=%s,password=%s,dir_mode=0700,file_mode=0700", accountName, accountKey)}


allow vers override too?this example uses vers=2.1 (not good idea though).

I have added vers as a mount option

use JoinMountOptions func add a new test case move const vars fix fmt issue

andyzhangx · 2017-11-17T14:41:34Z

/assign @rootfs

rootfs · 2017-11-17T17:48:07Z

/approve
/lgtm

k8s-github-robot · 2017-11-17T17:48:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, feiskyer, rootfs

Associated issue: 37005

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

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

~~pkg/volume/azure_file/OWNERS~~ [rootfs]

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-11-17T21:34:13Z

Automatic merge from submit-queue (batch tested with PRs 55254, 55525, 50108, 54674, 55263). If you want to cherry-pick this change to another branch, please follow the instructions here.

…4674-upstream-release-1.8 Automatic merge from submit-queue. Automated cherry pick of #54674 Cherry pick of #54674 on release-1.8. #54674: support mount options in azure file

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/azure 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 27, 2017

k8s-github-robot assigned justinsb and derekwaynecarr Oct 27, 2017

This was referenced Oct 27, 2017

azureFile volume mode too strict for container with non root user #54610

Closed

azure_file volumes should allow setting of dir_mode and file_mode #37005

Closed

dixudx reviewed Oct 27, 2017

View reviewed changes

feiskyer reviewed Oct 27, 2017

View reviewed changes

andyzhangx force-pushed the azurefile-mountoptions branch from 3836db6 to 557822b Compare October 27, 2017 09:54

ccojocar reviewed Oct 27, 2017

View reviewed changes

andyzhangx force-pushed the azurefile-mountoptions branch from 557822b to be97c91 Compare October 27, 2017 23:25

feiskyer approved these changes Oct 28, 2017

View reviewed changes

k8s-ci-robot assigned feiskyer Oct 28, 2017

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

rootfs reviewed Nov 15, 2017

View reviewed changes

andyzhangx force-pushed the azurefile-mountoptions branch from be97c91 to 5f20ffb Compare November 16, 2017 07:14

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

andyzhangx added 2 commits November 16, 2017 07:15

support mount options in azure file

badefda

use JoinMountOptions func add a new test case move const vars fix fmt issue

add vers mountoptions and fix comments

e316bbc

andyzhangx force-pushed the azurefile-mountoptions branch from 5f20ffb to e316bbc Compare November 16, 2017 07:15

k8s-ci-robot assigned rootfs Nov 17, 2017

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

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

k8s-github-robot merged commit e4a6d42 into kubernetes:master Nov 17, 2017

andyzhangx mentioned this pull request Nov 27, 2017

Automated cherry pick of #54674 #56384

Merged

duizendnegen mentioned this pull request Dec 24, 2017

AzureFile not working with non-root user, unable to upgrade to fixed version of K8S Azure/AKS#97

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support mount options in azure file #54674

support mount options in azure file #54674

andyzhangx commented Oct 27, 2017 •

edited by jpbetz

Loading

andyzhangx commented Oct 27, 2017

andyzhangx commented Oct 27, 2017

dixudx Oct 27, 2017

dixudx Oct 27, 2017

andyzhangx Oct 27, 2017

feiskyer Oct 27, 2017

feiskyer Oct 27, 2017

andyzhangx Oct 27, 2017

ccojocar Oct 27, 2017

andyzhangx Oct 27, 2017 •

edited

Loading

gnufied commented Oct 27, 2017

feiskyer left a comment

andyzhangx commented Nov 6, 2017

andyzhangx commented Nov 6, 2017

andyzhangx commented Nov 11, 2017

andyzhangx commented Nov 11, 2017

andyzhangx commented Nov 15, 2017 •

edited

Loading

rootfs Nov 15, 2017

andyzhangx Nov 16, 2017 •

edited

Loading

rootfs Nov 15, 2017

andyzhangx Nov 16, 2017

rootfs Nov 15, 2017

andyzhangx Nov 16, 2017

rootfs Nov 15, 2017

andyzhangx Nov 16, 2017

rootfs Nov 15, 2017

andyzhangx Nov 16, 2017

andyzhangx commented Nov 17, 2017

rootfs commented Nov 17, 2017

k8s-github-robot commented Nov 17, 2017

k8s-github-robot commented Nov 17, 2017

		@@ -222,11 +226,12 @@ func (b azureFileMounter) SetUpAt(dir string, fsGroup int64) error {
		} else {
		os.MkdirAll(dir, 0700)

support mount options in azure file #54674

support mount options in azure file #54674

Conversation

andyzhangx commented Oct 27, 2017 • edited by jpbetz Loading

andyzhangx commented Oct 27, 2017

andyzhangx commented Oct 27, 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

andyzhangx Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

gnufied commented Oct 27, 2017

feiskyer left a comment

Choose a reason for hiding this comment

andyzhangx commented Nov 6, 2017

andyzhangx commented Nov 6, 2017

andyzhangx commented Nov 11, 2017

andyzhangx commented Nov 11, 2017

andyzhangx commented Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

andyzhangx Nov 16, 2017 • edited Loading

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

andyzhangx commented Nov 17, 2017

rootfs commented Nov 17, 2017

k8s-github-robot commented Nov 17, 2017

k8s-github-robot commented Nov 17, 2017

andyzhangx commented Oct 27, 2017 •

edited by jpbetz

Loading

andyzhangx Oct 27, 2017 •

edited

Loading

andyzhangx commented Nov 15, 2017 •

edited

Loading

andyzhangx Nov 16, 2017 •

edited

Loading