fix permissions when using fsGroup #37009

sjenning · 2016-11-17T17:45:37Z

Currently, when an fsGroup is specified, the permissions of the defaultMode are not respected and all files created by the atomic writer have mode 777. This is because in SetVolumeOwnership() the filepath.Walk includes the symlinks created by the atomic writer. The symlinks have mode 777 when read from info.Mode(). However, when they are chmod'ed later, the chmod applies to the file the symlink points to, not the symlink itself, resulting in the wrong mode for the underlying file.

This PR skips chmod/chown for symlinks in the walk since those operations are carried out on the underlying file which will be included elsewhere in the walk.

xref https://bugzilla.redhat.com/show_bug.cgi?id=1384458

@derekwaynecarr @pmorie

This change is

k8s-ci-robot · 2016-11-17T18:21:24Z

Jenkins GCI GCE e2e failed for commit a349fa7. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

sjenning · 2016-11-17T18:29:54Z

@k8s-bot gci gce e2e test this

pmorie · 2016-11-17T19:55:32Z

I think this fix is good, but we need to encode some context about why it is okay to skip symlinks. Remember, this code might also run against any number of block device-provided volumes, so we need a broad understanding of why the code is the way it is to be communicated with this change as comments.

derekwaynecarr · 2016-11-17T20:00:41Z

test case?

pmorie · 2016-11-17T20:03:20Z

Yes, this needs E2E coverage added for configmap/secret/downward API

pmorie · 2016-11-17T20:14:21Z

cc @kubernetes/sig-storage

derekwaynecarr · 2016-11-17T20:14:25Z

@saad-ali -- i would like this to be a cherry-pick for 1.5 once a test case is added.

sjenning · 2016-11-17T21:35:33Z

@derekwaynecarr @pmorie added comments and e2e tests

saad-ali · 2016-11-18T05:22:35Z

@saad-ali -- i would like this to be a cherry-pick for 1.5 once a test case is added.

The issue it is fixing, was it introduced in 1.5? How bad is the issue it is fixing, can we ship 1.5 with it? How risky is the fix to the rest of the 1.5 release?

sjenning · 2016-11-18T17:03:58Z

@saad-ali it isn't risky as chowning/chmoding symlinks was only corrupting the mode of underlying files before this PR. I'm not sure when it was introduced exactly but it predates the 1.5 cycle.

pmorie · 2016-11-18T18:59:14Z

This has been an issue since 1.3 afaict

On Friday, November 18, 2016, Seth Jennings notifications@github.com
wrote:

@saad-ali https://github.com/saad-ali it isn't risky as
chowning/chmoding symlinks was only corrupting the mode of underlying files
before this PR. I'm not sure when it was introduced exactly but it predates
the 1.5 cycle.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#37009 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWXmPU9_qkNI9ylfpzKAo-iRbNRLeOPks5q_dqCgaJpZM4K1mlg
.

saad-ali · 2016-11-22T23:21:06Z

@saad-ali it isn't risky as chowning/chmoding symlinks was only corrupting the mode of underlying files before this PR. I'm not sure when it was introduced exactly but it predates the 1.5 cycle.

Ack. Ok for post-code freeze merge.

As an FYI, last chance for automatic merge to 1.5 branch will Nov 23 10 AM PST. At that point: https://groups.google.com/forum/#!topic/kubernetes-dev/n-vqlX-HHSM

derekwaynecarr · 2016-11-29T17:29:34Z

@pmorie bump

pmorie

Looking closer, but I thought we were going to add tests for downward API volume as well.

pmorie · 2016-11-29T17:35:09Z

pkg/volume/volume_linux.go

@@ -51,6 +51,17 @@ func SetVolumeOwnership(mounter Mounter, fsGroup *int64) error {
 			return err
 		}

+		// chown and chmod pass through to the underlying file for symlinks.


weird rendering error, disregard

pmorie · 2016-11-29T17:37:12Z

test/e2e/common/configmap.go

@@ -41,6 +41,11 @@ var _ = framework.KubeDescribe("ConfigMap", func() {
 		doConfigMapE2EWithoutMappings(f, 0, 0, &defaultMode)
 	})

+	It("should be consumable from pods in volume with defaultMode and fsGroup set [Conformance]", func() {


how about a test for non-root, default mode, and fsgroup?

after thinking about it, this should probably be a non-root only test

pmorie · 2016-11-29T17:37:48Z

test/e2e/common/secrets.go

+		doSecretE2EWithoutMapping(f, &defaultMode, "secret-test-"+string(uuid.NewUUID()), nil)
+	})
+
+	It("should be consumable from pods in volume with defaultMode and fsGroup set [Conformance]", func() {


same comment for secrets -- i'd like to see nonroot, fsgroup, and default mode in a test case.

k8s-github-robot · 2016-12-07T17:31:43Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-ci-robot · 2016-12-07T17:52:03Z

Jenkins unit/integration failed for commit 51ae5a3. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-github-robot · 2016-12-07T18:45:15Z

Automatic merge from submit-queue (batch tested with PRs 38294, 37009, 36778, 38130, 37835)

@derekwaynecarr

Automatic merge from submit-queue (batch tested with PRs 38294, 37009, 36778, 38130, 37835) fix permissions when using fsGroup Currently, when an fsGroup is specified, the permissions of the defaultMode are not respected and all files created by the atomic writer have mode 777. This is because in `SetVolumeOwnership()` the `filepath.Walk` includes the symlinks created by the atomic writer. The symlinks have mode 777 when read from `info.Mode()`. However, when the are chmod'ed later, the chmod applies to the file the symlink points to, not the symlink itself, resulting in the wrong mode for the underlying file. This PR skips chmod/chown for symlinks in the walk since those operations are carried out on the underlying file which will be included elsewhere in the walk. xref https://bugzilla.redhat.com/show_bug.cgi?id=1384458 @derekwaynecarr @pmorie

@thockin

#38276-#37009-upstream-release-1.5 Automatic merge from submit-queue Automated cherry pick of #37594 #38276 #37009 upstream release 1.5 Batch cherry pick PRs #37594 #38276 #37009 from master to release-1.5 branch. PR #37009 had merge conflicts that needed to be resolved. CC Original PR Authors: @thockin @mbohlool @mwielgus @sjenning

k8s-cherrypick-bot · 2016-12-08T02:45:36Z

Commit found in the "release-1.5" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

googlebot added the cla: yes label Nov 17, 2016

k8s-github-robot assigned pmorie Nov 17, 2016

k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Nov 17, 2016

pmorie added sig/storage Categorizes an issue or PR as relevant to SIG Storage. release-note-none Denotes a PR that doesn't merit a release note. labels Nov 17, 2016

k8s-github-robot removed the release-note-label-needed label Nov 17, 2016

This was referenced Nov 17, 2016

UPSTREAM: 37009: fix permissions when using fsGroup openshift/origin#11959

Merged

UPSTREAM: 37009: fix permissions when using fsGroup openshift/origin#11960

Merged

derekwaynecarr added the cherrypick-candidate label Nov 17, 2016

derekwaynecarr added this to the v1.5 milestone Nov 17, 2016

derekwaynecarr 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 Nov 17, 2016

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 17, 2016

sjenning force-pushed the fix-perms-with-fsgroup branch from 63314ed to f898967 Compare November 17, 2016 21:35

pmorie reviewed Nov 29, 2016

View reviewed changes

fix permissions when using fsGroup

51ae5a3

sjenning force-pushed the fix-perms-with-fsgroup branch from f898967 to 51ae5a3 Compare December 6, 2016 20:50

pmorie added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 7, 2016

k8s-github-robot merged commit 0f2c6fc into kubernetes:master Dec 7, 2016

saad-ali added a commit to saad-ali/kubernetes that referenced this pull request Dec 7, 2016

Fix merge issues with PR kubernetes#37009

b1f5a0d

saad-ali mentioned this pull request Dec 7, 2016

Automated cherry pick of #37594 #38219 #38276 #37009 upstream release 1.5 #38335

Closed

saad-ali added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Dec 8, 2016

saad-ali mentioned this pull request Dec 8, 2016

Automated cherry pick of #37594 #38276 #37009 upstream release 1.5 #38343

Merged

k8s-cherrypick-bot removed the cherrypick-candidate label Dec 8, 2016

peebs mentioned this pull request Dec 16, 2016

v1.5.1 single conformance test failure kubernetes-retired/bootkube#237

Closed

sjenning deleted the fix-perms-with-fsgroup branch August 16, 2017 02:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix permissions when using fsGroup #37009

fix permissions when using fsGroup #37009

sjenning commented Nov 17, 2016 •

edited

Loading

k8s-ci-robot commented Nov 17, 2016

sjenning commented Nov 17, 2016

pmorie commented Nov 17, 2016

derekwaynecarr commented Nov 17, 2016

pmorie commented Nov 17, 2016

pmorie commented Nov 17, 2016

derekwaynecarr commented Nov 17, 2016

sjenning commented Nov 17, 2016

saad-ali commented Nov 18, 2016

sjenning commented Nov 18, 2016

pmorie commented Nov 18, 2016

saad-ali commented Nov 22, 2016

derekwaynecarr commented Nov 29, 2016

pmorie left a comment

pmorie Nov 29, 2016

pmorie Nov 29, 2016

pmorie Nov 29, 2016

sjenning Dec 6, 2016

pmorie Nov 29, 2016

k8s-github-robot commented Dec 7, 2016

k8s-ci-robot commented Dec 7, 2016

k8s-github-robot commented Dec 7, 2016

k8s-cherrypick-bot commented Dec 8, 2016

fix permissions when using fsGroup #37009

fix permissions when using fsGroup #37009

Conversation

sjenning commented Nov 17, 2016 • edited Loading

k8s-ci-robot commented Nov 17, 2016

sjenning commented Nov 17, 2016

pmorie commented Nov 17, 2016

derekwaynecarr commented Nov 17, 2016

pmorie commented Nov 17, 2016

pmorie commented Nov 17, 2016

derekwaynecarr commented Nov 17, 2016

sjenning commented Nov 17, 2016

saad-ali commented Nov 18, 2016

sjenning commented Nov 18, 2016

pmorie commented Nov 18, 2016

saad-ali commented Nov 22, 2016

derekwaynecarr commented Nov 29, 2016

pmorie left a comment

Choose a reason for hiding this comment

pmorie Nov 29, 2016

Choose a reason for hiding this comment

pmorie Nov 29, 2016

Choose a reason for hiding this comment

pmorie Nov 29, 2016

Choose a reason for hiding this comment

sjenning Dec 6, 2016

Choose a reason for hiding this comment

pmorie Nov 29, 2016

Choose a reason for hiding this comment

k8s-github-robot commented Dec 7, 2016

k8s-ci-robot commented Dec 7, 2016

k8s-github-robot commented Dec 7, 2016

k8s-cherrypick-bot commented Dec 8, 2016

sjenning commented Nov 17, 2016 •

edited

Loading