set fsGroup by securityContext.fsGroup in azure file #58316

andyzhangx · 2018-01-16T05:21:19Z

What this PR does / why we need it:
set fsGroup by securityContext.fsGroup in azure file
This PR respects securityContext.fsGroup, while if user both sets gid=xxx in mountOptions in azure storage class and securityContext.fsGroup, gid=xxx setting in mountOptions will win.

apiVersion: v1
kind: Pod
metadata:
  name: security-context-demo
spec:
  securityContext:
    fsGroup: 2000
  volumes:
  - name: sec-ctx-vol
    emptyDir: {}
  containers:
  - name: sec-ctx-demo
    image: gcr.io/google-samples/node-hello:1.0
    volumeMounts:
    - name: sec-ctx-vol
      mountPath: /data/demo
    securityContext:
      allowPrivilegeEscalation: false

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #44085

Special notes for your reviewer:

Release note:

set fsGroup by securityContext.fsGroup in azure file

/assign @karataliu
cc @rootfs @avaranovich

andyzhangx · 2018-01-16T05:22:00Z

/sig azure

karataliu · 2018-01-16T06:34:24Z

pkg/volume/azure_file/azure_file.go

@@ -225,9 +225,11 @@ func (b *azureFileMounter) SetUpAt(dir string, fsGroup *int64) error {
 		options := []string{fmt.Sprintf("username=%s,password=%s", accountName, accountKey)}
 		if b.readOnly {
 			options = append(options, "ro")
+			// we don't respect fsGroup value if b.readOnly = true
+			fsGroup = nil


I think we can make use of fsGroup here. Some implementations do mount with ro first, so that gid could not be changed later.
Here it should be fine as it is passed via mount option,

according to https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/aws_ebs/aws_ebs.go#L379-L381
no fsGroup value will be set if readOnly = true

if !b.readOnly { volume.SetVolumeOwnership(b, fsGroup) }

Anyway I reserved fsGroup value and use groupID var instead, not change the passing parameter value.

karataliu · 2018-01-16T06:34:53Z

pkg/volume/azure_file/azure_util.go

@@ -96,10 +97,11 @@ func (s *azureSvc) SetAzureCredentials(host volume.VolumeHost, nameSpace, accoun
 }

 // check whether mountOptions contain file_mode and dir_mode, if not, append default mode


Consider updating comment since add gid logic.

andyzhangx

@karataliu PTAL

andyzhangx · 2018-01-17T03:12:23Z

pkg/volume/azure_file/azure_file.go

@@ -225,9 +225,11 @@ func (b *azureFileMounter) SetUpAt(dir string, fsGroup *int64) error {
 		options := []string{fmt.Sprintf("username=%s,password=%s", accountName, accountKey)}
 		if b.readOnly {
 			options = append(options, "ro")
+			// we don't respect fsGroup value if b.readOnly = true
+			fsGroup = nil


according to https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/aws_ebs/aws_ebs.go#L379-L381
no fsGroup value will be set if readOnly = true

if !b.readOnly { volume.SetVolumeOwnership(b, fsGroup) }

Anyway I reserved fsGroup value and use groupID var instead, not change the passing parameter value.

andyzhangx · 2018-01-17T03:13:38Z

pkg/volume/azure_file/azure_util.go

@@ -96,10 +97,11 @@ func (s *azureSvc) SetAzureCredentials(host volume.VolumeHost, nameSpace, accoun
 }

 // check whether mountOptions contain file_mode and dir_mode, if not, append default mode


andyzhangx · 2018-01-17T06:33:59Z

/test pull-kubernetes-kubemark-e2e-gce

andyzhangx · 2018-01-17T06:34:07Z

/test pull-kubernetes-e2e-kops-aws

karataliu · 2018-01-17T07:11:35Z

pkg/volume/azure_file/azure_file_test.go

+			expected: []string{"file_mode=0777",
+				fmt.Sprintf("%s=%s", dirMode, defaultDirMode),
+				fmt.Sprintf("%s=%s", vers, defaultVers),
+				fmt.Sprintf("%s=0", gid)},


Can we have a test where input string contains 'gid' and fsGroup is set, to verify gid takes precedence over fsGroup?

Others looks good. Please also do squash commits.

andyzhangx · 2018-01-17T09:07:24Z

/test pull-kubernetes-e2e-kops-aws

fix comments fix comments fix comments

andyzhangx

@karataliu PTAL

andyzhangx · 2018-01-17T09:25:46Z

pkg/volume/azure_file/azure_file_test.go

+			expected: []string{"file_mode=0777",
+				fmt.Sprintf("%s=%s", dirMode, defaultDirMode),
+				fmt.Sprintf("%s=%s", vers, defaultVers),
+				fmt.Sprintf("%s=0", gid)},


karataliu · 2018-01-17T10:33:49Z

/lgtm

k8s-ci-robot · 2018-01-17T10:34:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, karataliu

Associated issue: #44085

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~~ [andyzhangx,karataliu]

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 · 2018-01-17T11:45:00Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-01-17T12:29:02Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot assigned karataliu Jan 16, 2018

k8s-ci-robot added the sig/azure label Jan 16, 2018

karataliu reviewed Jan 16, 2018

View reviewed changes

andyzhangx commented Jan 17, 2018

View reviewed changes

andyzhangx force-pushed the azurefile-setfsgroup branch from 6973118 to e94a988 Compare January 17, 2018 05:52

karataliu reviewed Jan 17, 2018

View reviewed changes

set fsGroup by securityContext.fsGroup in azure file

c38e758

fix comments fix comments fix comments

andyzhangx force-pushed the azurefile-setfsgroup branch from e94a988 to c38e758 Compare January 17, 2018 09:25

andyzhangx commented Jan 17, 2018

View reviewed changes

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

k8s-github-robot merged commit 20f27a7 into kubernetes:master Jan 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

set fsGroup by securityContext.fsGroup in azure file #58316

set fsGroup by securityContext.fsGroup in azure file #58316

andyzhangx commented Jan 16, 2018

andyzhangx commented Jan 16, 2018

karataliu Jan 16, 2018

andyzhangx Jan 17, 2018

karataliu Jan 16, 2018

andyzhangx Jan 17, 2018

andyzhangx left a comment

andyzhangx Jan 17, 2018

andyzhangx Jan 17, 2018

andyzhangx commented Jan 17, 2018

andyzhangx commented Jan 17, 2018

karataliu Jan 17, 2018

andyzhangx Jan 17, 2018

andyzhangx commented Jan 17, 2018

andyzhangx left a comment

andyzhangx Jan 17, 2018

karataliu commented Jan 17, 2018

k8s-ci-robot commented Jan 17, 2018

k8s-github-robot commented Jan 17, 2018

k8s-github-robot commented Jan 17, 2018

		@@ -96,10 +97,11 @@ func (s *azureSvc) SetAzureCredentials(host volume.VolumeHost, nameSpace, accoun
		}

		// check whether mountOptions contain file_mode and dir_mode, if not, append default mode

set fsGroup by securityContext.fsGroup in azure file #58316

set fsGroup by securityContext.fsGroup in azure file #58316

Conversation

andyzhangx commented Jan 16, 2018

andyzhangx commented Jan 16, 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

andyzhangx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyzhangx commented Jan 17, 2018

andyzhangx commented Jan 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyzhangx commented Jan 17, 2018

andyzhangx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karataliu commented Jan 17, 2018

k8s-ci-robot commented Jan 17, 2018

k8s-github-robot commented Jan 17, 2018

k8s-github-robot commented Jan 17, 2018