Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make ConfigMap volume readable as non-root #23793

Merged
merged 1 commit into from
Apr 6, 2016
Merged

Make ConfigMap volume readable as non-root #23793

merged 1 commit into from
Apr 6, 2016
+183 −147

Conversation

pmorie
Copy link
Member

@pmorie pmorie commented Apr 2, 2016

Found by @mikedanese

cc @thockin

@k8s-github-robot
Copy link

Labelling this PR as size/L

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Apr 2, 2016
@k8s-bot
Copy link

k8s-bot commented Apr 2, 2016

GCE e2e build/test passed for commit e4fee9396e0fdc63696e9474522e821c651c53d9.

@pmorie pmorie force-pushed the configmap-nonroot branch from e4fee93 to fecffcf Compare April 4, 2016 04:10
thockin
thockin reviewed Apr 4, 2016
View reviewed changes
pkg/volume/util/atomic_writer.go
@@ -341,6 +341,12 @@ func (w *AtomicWriter) newTimestampDir() (string, error) {
return "", err
}

err = os.Chmod(tsDir, 0655)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rw- r-x r-x

Shouldn't this be 0755 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could get away with 0655 since owner is always going to be root and will have dac override capability, but 0755 is more correct, probably.

@thockin thockin assigned thockin and unassigned saad-ali Apr 4, 2016
@thockin
Copy link
Member

thockin commented Apr 4, 2016

Set yourself a release note label and feel free to self-LGTM. Looks good just a wrong (IMO) mode value.

@pmorie pmorie force-pushed the configmap-nonroot branch from fecffcf to 3a4992c Compare April 4, 2016 04:40
@k8s-bot
Copy link

k8s-bot commented Apr 4, 2016

GCE e2e build/test passed for commit fecffcf00fe109b8e2637c5cba224679a7d5cbff.

@k8s-bot
Copy link

k8s-bot commented Apr 4, 2016

GCE e2e build/test passed for commit 3a4992cee3ce6eadd855297dced5b5ab58ec3974.

@mikedanese mikedanese mentioned this pull request Apr 4, 2016
Closed
@mikedanese
Copy link
Member

I opened #23825 to discuss whether this should be cherry-picked (IIUC can't cherry-pick a PR without a corresponding issue in v1.2 milestone).

@pmorie pmorie force-pushed the configmap-nonroot branch from 3a4992c to 33081fb Compare April 5, 2016 16:15
@pmorie pmorie force-pushed the configmap-nonroot branch from 33081fb to e838ff2 Compare April 5, 2016 16:20
@pmorie pmorie added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Apr 5, 2016
@k8s-bot
Copy link

k8s-bot commented Apr 5, 2016

GCE e2e build/test passed for commit 33081fb2b94ae196777f0254cad5c4a66bbf5bea.

@k8s-bot
Copy link

k8s-bot commented Apr 5, 2016

GCE e2e build/test passed for commit e838ff2.

@saad-ali saad-ali assigned saad-ali and unassigned thockin Apr 5, 2016
@pmorie
Copy link
Member Author

pmorie commented Apr 5, 2016

@k8s-bot test this issue: #23873

@k8s-bot
Copy link

k8s-bot commented Apr 5, 2016

GCE e2e build/test failed for commit e838ff2.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@nickschuch
Copy link

Just rolled this on a local cluster and it works! Thanks @pmorie!

www-data@config-test:/$ ls -l /etc/config/..data/
total 8
-rw-r--r-- 1 root root 5 Apr  6 00:38 example.property.1
-rw-r--r-- 1 root root 5 Apr  6 00:38 example.property.2

@pmorie
Copy link
Member Author

pmorie commented Apr 6, 2016

@k8s-bot test this issue: #21367

@pmorie
Copy link
Member Author

pmorie commented Apr 6, 2016

@nickschuch thanks for the test, glad it worked for you!

@k8s-bot
Copy link

k8s-bot commented Apr 6, 2016

GCE e2e build/test passed for commit e838ff2.

@pmorie
Copy link
Member Author

pmorie commented Apr 6, 2016

By the power vested in me by Tim in #23793 (comment), I declare this PR to LGTM.

@pmorie pmorie added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 6, 2016
@mikedanese mikedanese added this to the v1.2 milestone Apr 6, 2016
@mikedanese mikedanese added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. cherrypick-candidate labels Apr 6, 2016
@mikedanese mikedanese mentioned this pull request Apr 6, 2016
Merged
@zmerlynn
Copy link
Member

zmerlynn commented Apr 6, 2016

@k8s-oncall: Manual merge, please. I'd like this on master for a little while before we cherrypick.

@saad-ali saad-ali merged commit d7b069f into kubernetes:master Apr 6, 2016
@zmerlynn zmerlynn added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Apr 6, 2016
zmerlynn added a commit that referenced this pull request Apr 6, 2016
@zmerlynn
Merge pull request #23935 from mikedanese/automated-cherry-pick-of-#2…
3d9cf1e 
…3793-upstream-release-1.2

Automated cherry pick of #23793
@k8s-cherrypick-bot
Copy link

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

@pmorie pmorie mentioned this pull request Apr 7, 2016
Merged
@yvespp yvespp mentioned this pull request Apr 30, 2016
Closed
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
@zmerlynn
Merge pull request kubernetes#23935 from mikedanese/automated-cherry-…
276df9d 
…pick-of-#23793-upstream-release-1.2

Automated cherry pick of kubernetes#23793
shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017
@zmerlynn
Merge pull request kubernetes#23935 from mikedanese/automated-cherry-…
6abd053 
…pick-of-#23793-upstream-release-1.2

Automated cherry pick of kubernetes#23793
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@saad-ali saad-ali

Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.