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

Allow setting permission mode bits on secrets, configmaps and downwardAPI files #28936

Merged

k8s-github-robot merged 3 commits into kubernetes:master from rata:secret-configmap-file-mode

Aug 18, 2016

Member

rata commented Jul 14, 2016 •

edited by thockin

Loading

cc @thockin @pmorie

Here is the first round to implement: #28733.

I made two commits: one with the actual change and the other with the auto-generated code. I think it's easier to review this way, but let me know if you prefer in some other way.

I haven't written any tests yet, I wanted to have a first glance and not write them till this (and the API) are more close to the "LGTM" :)

There are some things:

I'm not sure where to do the "AND 0777". I'll try to look better in the code base, but suggestions are always welcome :)
The write permission on group and others is not set when you do an ls -l on the running container. It does work with write permissions to the owner. Debugging seems to show that is something happening after this is correctly set on creation. Will look closer.
The default permission (when the new fields are not specified) are the same that on kubernetes v1.3
I do realize there are conflicts with master, but I think this is good enough to have a look. The conflicts is with the autog-enerated code, so the actual code is actually the same (and it takes like ~30 minutes to generate it here)
I didn't generate the docs (generated-docs and generated-swagger-docs from hack/update-all.sh) because my machine runs out of mem. So that's why it isn't in this first PR, will try to investigate and see why it happens.

Other than that, this works fine here with some silly scripts I did to create a secret&configmap&downwardAPI, a pod and check the file permissions. Tested the "defaultMode" and "mode" for all. But of course, will write tests once this is looking fine :)

Thanks a lot again!
Rodrigo

This change is

googlebot added the cla: yes label

k8s-bot commented Jul 14, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

1 similar comment

k8s-bot commented Jul 14, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-github-robot assigned bgrant0607

k8s-github-robot added needs-rebase kind/api-change size/L release-note-label-needed labels

k8s-bot commented Jul 14, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

2 similar comments

k8s-bot commented Jul 14, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot commented Jul 14, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

rata changed the title ~~[[NOT MERGE] Allow to set permission mode bits on secrets, configmaps and downwardAPI files~~ [NOT MERGE] Allow to set permission mode bits on secrets, configmaps and downwardAPI files

mqliang mentioned this pull request

Allow user set mode for Secret/ConfigMap #24199

Closed

rata mentioned this pull request

Support mounting host directories into pods kubernetes/minikube#2

Closed

Member

bgrant0607 commented Jul 21, 2016

ok to test

bgrant0607 added the do-not-merge label

bgrant0607 assigned pmorie and unassigned bgrant0607

Member

bgrant0607 commented Jul 21, 2016

This is an example of a PR that sat for over a week without me noticing.

We need to explicitly notify assignees in order to break through email filters.
We need to ping again when an assignee doesn't respond.
We need to reassign if the assignee still doesn't respond. People may be OOO, too busy, etc.

cc @apelisse @kubernetes/contributor-experience

bgrant0607 mentioned this pull request

More PR automation kubernetes-retired/contrib#869

Closed

Member Author

rata commented Jul 21, 2016

@bgrant0607: thanks!
The test seem to failed for a network issue on jenkins:

ERROR: Error fetching remote repo 'remote1'
hudson.plugins.git.GitException: Failed to fetch from https://github.com/kubernetes/kubernetes

Can you ask to re-test, please? :)

rata force-pushed the secret-configmap-file-mode branch from ba0159d to 24f078d Compare

July 23, 2016 00:28

Member Author

rata commented Jul 23, 2016 •

edited

Loading

Well, there was a typo that I was using hexa instead of octal. That fixed, everything is working fine now :-)

The only "weird" thing is that the mode in the yaml needs to be in base 10 or 16, I didn't find how to write in octal in the yaml and get that number when reading it. Probably just because I don't know about protobufs.

Also, checking the code for fsGroup, the "OR'd" is done in the lower layer in volume_linux.go so I guess the AND in atomic_writer.go is fine (I guess there isn't a "higher level" where I should do this). But please confirm. The comments are still in the code to make it easy to see what I refer to.

Also, it seems a validation is not needed, so I didn't wrote one.

@pmorie can you please have a look? If it looks mostly fine, I'd like to proceed with some tests (suggestions welcome!), improve some comments and have this merged =)

rata changed the title ~~[NOT MERGE] Allow to set permission mode bits on secrets, configmaps and downwardAPI files~~ WIP: Allow to set permission mode bits on secrets, configmaps and downwardAPI files

gregory-lyons mentioned this pull request

Add support for SSH in git-sync kubernetes-retired/contrib#1431

Merged

Member

thockin commented Jul 29, 2016

@k8s-bot test this: github issue #IGNORE

thockin reviewed

View reviewed changes

pkg/api/types.go

    
            @@ -607,6 +607,10 @@ type SecretVolumeSource struct {
          
              	// the volume setup will error. Paths must be relative and may not contain

              	// the '..' path or start with '..'.

              	Items []KeyToPath `json:"items,omitempty"`

              	// Mode bits to use on created files by default. The used mode bits will

Member

thockin Jul 29, 2016

s/used/final (same in all cases)

rata added 2 commits

August 17, 2016 14:44


          Check return value from volume.SetVolumeOwnership() in downwardAPI

The function can fail, so we must check the return code.


          Update generated code

4e9a14a

pmorie force-pushed the secret-configmap-file-mode branch from 377743d to 4e9a14a Compare

August 17, 2016 18:50

k8s-github-robot commented Aug 17, 2016

PR changed after LGTM, removing LGTM. @pmorie @rata

k8s-github-robot removed lgtm needs-rebase labels

thockin added the lgtm label

k8s-bot commented Aug 17, 2016

GCE e2e build/test passed for commit 4e9a14a.

Member Author

rata commented Aug 17, 2016

Should this have the milestone v1.4? It is in the queue, but after all the v1.4 PRs. But if it will make it anyways, no problem :)

pmorie added this to the v1.4 milestone

Member Author

rata commented Aug 17, 2016

@thockin @pmorie thanks again! :-)

Member

pmorie commented Aug 18, 2016

Reviewed 3 of 12 files at r1, 6 of 13 files at r3, 3 of 3 files at r5, 10 of 22 files at r6, 12 of 12 files at r7.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

Comments from Reviewable

Member

pmorie commented Aug 18, 2016

Review status: all files reviewed at latest revision, all discussions resolved.

pkg/volume/configmap/configmap_test.go, line 39 [r6] (raw file):

Previously, pmorie (Paul Morie) wrote…

caseMappingMode is the prefered style in golang

OK

Comments from Reviewable

k8s-github-robot commented Aug 18, 2016

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

k8s-bot commented Aug 18, 2016

GCE e2e build/test passed for commit 4e9a14a.

k8s-github-robot commented Aug 18, 2016

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

k8s-bot commented Aug 18, 2016

GCE e2e build/test passed for commit 4e9a14a.

k8s-github-robot commented Aug 18, 2016

Automatic merge from submit-queue

k8s-github-robot merged commit 6824f4c into kubernetes:master

rata added a commit to rata/kubernetes.github.io that referenced this pull request


          Add documentation for secret, configmap and downwardAPI file permissions

59e9261

The patch that adds this feature in core kubernetes,
kubernetes/kubernetes#28936, was merged and will be
released with kubernetes 1.4.

rata added a commit to rata/kubernetes.github.io that referenced this pull request


          Add documentation for secret, configmap and downwardAPI file permissions

7e1c6ea

The patch that adds this feature in core kubernetes,
kubernetes/kubernetes#28936, was merged and will be
released with kubernetes 1.4.

rata added a commit to rata/kubernetes.github.io that referenced this pull request


          Add documentation for secret, configmap and downwardAPI file permissions

b547bc4

The patch that adds this feature in core kubernetes,
kubernetes/kubernetes#28936, was merged and will be
released with kubernetes 1.4.

rata mentioned this pull request

Add documentation for secret, configmap and downwardAPI file permissions for 1.4 release kubernetes/website#1178

Merged

rata added a commit to rata/kubernetes.github.io that referenced this pull request


          Add documentation for secret, configmap and downwardAPI file permissions

7ae4ea8

The patch that adds this feature in core kubernetes,
kubernetes/kubernetes#28936, was merged and will be
released with kubernetes 1.4.

rata added a commit to rata/kubernetes.github.io that referenced this pull request


          Add documentation for secret, configmap and downwardAPI file permissions

7f8b65d

The patch that adds this feature in core kubernetes,
kubernetes/kubernetes#28936, was merged and will be
released with kubernetes 1.4.

rata mentioned this pull request

Remove duplicated code in secret e2e tests #33978

Merged

k8s-github-robot pushed a commit that referenced this pull request


          Merge pull request #33978 from rata/simplify-e2e-secret

f352e67

Automatic merge from submit-queue

Remove duplicated code in secret e2e tests

<!--  Thanks for sending a pull request!  Here are some tips for you:
1. If this is your first time, read our contributor guidelines https://github.com/kubernetes/kubernetes/blob/master/CONTRIBUTING.md and developer guide https://github.com/kubernetes/kubernetes/blob/master/docs/devel/development.md
2. If you want *faster* PR reviews, read how: https://github.com/kubernetes/kubernetes/blob/master/docs/devel/faster_reviews.md
3. Follow the instructions for writing a release note: https://github.com/kubernetes/kubernetes/blob/master/docs/devel/pull-requests.md#release-notes
-->

**What this PR does / why we need it**: This come up when writing another PR: #28936 as a comment from @thockin. 

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

**Special notes for your reviewer**:

**Release note**:
<!--  Steps to write your release note:
1. Use the release-note-* labels to set the release note state (if you have access) 
2. Enter your extended release note in the below block; leaving it blank means using the PR title as the release note. If no release note is required, just write `NONE`. 
-->
```NONE
```

This patch just removes duplicated code in secret e2e tests.

ApsOps added a commit to ApsOps/git-sync that referenced this pull request


          Remove chmod command since we can specify permissions for a Secret Vo…

c01a9a4

…lume

- Fixed because of kubernetes/kubernetes#28936

ApsOps mentioned this pull request

Remove chmod command since we can specify permissions for a Secret Volume kubernetes/git-sync#25

Merged

ApsOps added a commit to ApsOps/git-sync that referenced this pull request


          Remove chmod command since we can specify permissions for Secret Volumes

ea98806

- Fixed because of kubernetes/kubernetes#28936

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/api-change lgtm release-note size/XXL