Option to preserve default file mode bit in atomicwriter volumes #2606

abhisheksinghbaghel · 2021-04-07T20:07:24Z

xref #2605
/sig storage

k8s-ci-robot · 2021-04-07T20:07:32Z

Welcome @abhisheksinghbaghel!

It looks like this is your first PR to kubernetes/enhancements 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/enhancements has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-04-07T20:07:32Z

Hi @abhisheksinghbaghel. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Jiawei0227 · 2021-04-07T20:24:26Z

/ok-to-test

Jiawei0227 · 2021-04-07T20:24:41Z

/assign

abhisheksinghbaghel · 2021-04-07T21:04:43Z

/test pull-enhancements-verify

k8s-ci-robot · 2021-04-07T21:24:16Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: abhisheksinghbaghel
To complete the pull request process, please assign deads2k, jsafrane after the PR has been reviewed.
You can assign the PR to them by writing /assign @deads2k @jsafrane in a comment when ready.

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

Needs approval from an approver in each of these files:

keps/prod-readiness/OWNERS
keps/sig-storage/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

abhisheksinghbaghel · 2021-04-07T21:24:23Z

/test pull-enhancements-verify

abhisheksinghbaghel · 2021-04-07T22:29:40Z

/test pull-enhancements-verify

Jiawei0227

I think this looks good! Thanks a lot for putting this together. I am cc more folks to get some more reviews on this and see what their feedbacks are.

/cc @msau42 @jingxu97 @gnufied @jsafrane

.../sig-storage/2605-option-to-preserve-default-file-mode-bit-in-atomicwriter-volumes/README.md

gnufied · 2021-04-20T19:07:29Z

Can you make it non-draft PR please?

gnufied

Can the use case be solved by a CSI driver implementation that specifically opts-out of fsgroup based change via #1682 ?

.../sig-storage/2605-option-to-preserve-default-file-mode-bit-in-atomicwriter-volumes/README.md

gnufied · 2021-04-20T19:12:36Z

.../sig-storage/2605-option-to-preserve-default-file-mode-bit-in-atomicwriter-volumes/README.md

+  - We would have to pass the payload information along with PreserveDefaultMode to the SetVolumeOwnership routine
+        and skip chown\chmod if PreserveDefaultMode is set to true for the file.
+  - This will be more invasive change, but would allow more granular control at per file level
+  - If users want to preserve Mode for one file in the volume and not for another they can achieve that by creating


I think another alternative would have been to use field introduced in #695 to respect FSGroupChangePolicy for secrets/configmaps/emptydirs too.

FSGroupChangePolicy is a pod level setting whereas in our case we do want to only specify the atomicwriter volume file permission.

Yeah, I think the difference here is that if user wants the behavior for a specific volume and not all volumes for the pod, which appears to be a common grievance in the discussion.

Could there be a case where user is using secret volume with more than one container in pod but still wants to preserve mode bits? It would require either both containers to run with same UID or containers running as root.

My main concern is - do we need per-volume granularity? Because when we discussed this with @msau42 @tallclair - while @tallclair proposed, it might be helpful to have per-volume granularity , in practical terms either pods that use fsgroup will want to apply it to all volumes or not. Also per-volume granularity is confusing for end users.

@gnufied I think these are the ones which were being highlighted in the discussions. Take a look at this one. In certain scenarios users apparently want to preserve the fileMode bit set and that's the motivation here.
istio/istio#32217

I am not sure if there are workarounds available for issues such as above. Just following the conversation and making an educated attempt to address.

abhisheksinghbaghel · 2021-04-21T07:37:22Z

Can the use case be solved by a CSI driver implementation that specifically opts-out of fsgroup based change via #1682 ?

We are trying to address the issue for the existing AtomicWriter Volumes like configmaps etc. I am not sure if the CSI driver is in picture here.
Are you suggesting we should have a separate CSI driver implementation for Secrets/ConfigMaps etc and take care of this problem using fsGroupPolicyChange?

abhisheksinghbaghel · 2021-04-21T07:42:47Z

Can you make it non-draft PR please?

done!

.../sig-storage/2605-option-to-preserve-default-file-mode-bit-in-atomicwriter-volumes/README.md

Jiawei0227 · 2021-04-28T07:12:12Z

I think this is looking good. @msau42 to take a look.

jsafrane · 2021-04-28T13:31:34Z

.../sig-storage/2605-option-to-preserve-default-file-mode-bit-in-atomicwriter-volumes/README.md

+    // the volume is modified to be owned by the fsGroup.
+    // If PreservePermissions is not specified in the volume spec, it defaults to false.
+    // +optional
+    PreservePermissions *bool


Now that I am looking at the API, would it make sense to move the flag into struct Volume? This way, it could be used with any volume, incl. in-line ephemeral CSI volumes and, if someone really needs, to opt-out any volume in a pod from fsGroup.

I am not sure if it makes things easier or harder on kubelet side.

gnufied · 2021-04-28T18:34:21Z

Also CC @liggitt because he has been tracking similar issue for awhile now.

gnufied · 2021-04-28T19:34:07Z

.../sig-storage/2605-option-to-preserve-default-file-mode-bit-in-atomicwriter-volumes/README.md

+
+For the volumes that use AtomicWriter today the files are created with the Mode set in the KeyToPath struct.
+If the Mode is not specified then the DefaultMode set for the source is used.
+If DefaultMode is not specified then the file is created with mode bit set to 0600.


From definition of DefaultMode in SecretVolumeSource:

// YAML accepts both octal and decimal values, JSON requires decimal values
// for mode bits. Defaults to 0644.

gnufied · 2021-04-28T19:35:39Z

.../sig-storage/2605-option-to-preserve-default-file-mode-bit-in-atomicwriter-volumes/README.md

+    This is the default case.
+- *Case 2*: The volume has PreservePermissions set to true.
+    In this case the payload/volume/files are created using Mode/Default mode.
+    Then the call to SetVolumeOwnership is skipped, in turn preserving the Default file mode on the files.


Have we tried commenting out call to SetVolumeOwnership and just setting the mode on files? It sounds like this will cause files within secret to become inaccessible to the container's process (unless container runs as root). I am not sure if setting the mode is enough.

gnufied · 2021-05-05T17:24:26Z

@abhisheksinghbaghel if we want to preserve the mode, we will have to apply user based ownership to the volume. Please take a look at - #1598

deads2k · 2021-05-07T20:02:50Z

If you're targeting alpha for 1.22, you need to answer the enable/disable questions in the production readiness review template: https://github.com/kubernetes/enhancements/blame/master/keps/NNNN-kep-template/README.md#L359-L667

For simplicity, I suggest copying the entire thing and only filling in the sections required for alpha.

cbandy · 2021-05-18T21:46:30Z

.../sig-storage/2605-option-to-preserve-default-file-mode-bit-in-atomicwriter-volumes/README.md

+    This is the default case.
+- *Case 2*: The volume has PreservePermissions set to true.
+    In this case the payload/volume/files are created using Mode/Default mode.
+    Then the call to SetVolumeOwnership is skipped, in turn preserving the Default file mode on the files.


Ownership and Permissions are different concepts. When PreservePermissions is true, should the group ownership continue to be driven by fsGroup?

…t.fsGroup (kubernetes/enhancements#2606)

k8s-triage-robot · 2021-08-16T22:34:12Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2021-09-15T23:02:05Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2021-10-15T23:27:42Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen
Mark this issue or PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2021-10-15T23:27:56Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen

Mark this issue or PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

benjaminxie · 2022-05-12T20:50:14Z

/reopen

k8s-ci-robot · 2022-05-12T20:50:30Z

@benjaminxie: You can't reopen an issue/PR unless you authored it or you are a collaborator.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. sig/storage Categorizes an issue or PR as relevant to SIG Storage. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 7, 2021

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory labels Apr 7, 2021

k8s-ci-robot requested review from saad-ali and xing-yang April 7, 2021 20:07

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 7, 2021

abhisheksinghbaghel mentioned this pull request Apr 7, 2021

Option to Preserve default file mode bit (Mode\DefaultMode) set in AtomicWriter volumes #2605

Closed

4 tasks

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 7, 2021

k8s-ci-robot assigned Jiawei0227 Apr 7, 2021

abhisheksinghbaghel changed the title ~~option to preserve default file mode bit in atomicwriter volumes~~ Option to preserve default file mode bit in atomicwriter volumes Apr 7, 2021

abhisheksinghbaghel mentioned this pull request Apr 12, 2021

feat: Option to preserve default file mode bit in atomicwriter volumes kubernetes/kubernetes#101016

Closed

1 task

Jiawei0227 reviewed Apr 20, 2021

View reviewed changes

.../sig-storage/2605-option-to-preserve-default-file-mode-bit-in-atomicwriter-volumes/README.md Outdated Show resolved Hide resolved

.../sig-storage/2605-option-to-preserve-default-file-mode-bit-in-atomicwriter-volumes/README.md Outdated Show resolved Hide resolved

k8s-ci-robot requested review from gnufied, jingxu97, jsafrane and msau42 April 20, 2021 18:52

Jiawei0227 mentioned this pull request Apr 20, 2021

Setting defaultMode is not Fully Respected When Pod.spec.securityContext.runAsUser is Set kubernetes/kubernetes#57923

Open

gnufied reviewed Apr 20, 2021

View reviewed changes

fix typo on toc

9c0fc42

abhisheksinghbaghel closed this Apr 21, 2021

abhisheksinghbaghel reopened this Apr 21, 2021

address review comments

2211163

abhisheksinghbaghel force-pushed the enahancement_preserve_file_mode branch from fb63434 to 2211163 Compare April 21, 2021 07:53

abhisheksinghbaghel marked this pull request as ready for review April 21, 2021 07:57

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 21, 2021

jsafrane reviewed Apr 21, 2021

View reviewed changes

.../sig-storage/2605-option-to-preserve-default-file-mode-bit-in-atomicwriter-volumes/README.md Show resolved Hide resolved

.../sig-storage/2605-option-to-preserve-default-file-mode-bit-in-atomicwriter-volumes/README.md Outdated Show resolved Hide resolved

k8s-ci-robot requested a review from jsafrane April 23, 2021 06:23

Address review comments

130a965

jsafrane reviewed Apr 28, 2021

View reviewed changes

gnufied reviewed Apr 28, 2021

View reviewed changes

cbandy reviewed May 18, 2021

View reviewed changes

JorritSalverda added a commit to estafette/estafette-ci-api that referenced this pull request Jul 16, 2021

run as root until volume defaultMode gets respected by securityContex…

34c7576

…t.fsGroup (kubernetes/enhancements#2606)

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 16, 2021

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Sep 15, 2021

k8s-ci-robot closed this Oct 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Option to preserve default file mode bit in atomicwriter volumes #2606

Option to preserve default file mode bit in atomicwriter volumes #2606

abhisheksinghbaghel commented Apr 7, 2021

k8s-ci-robot commented Apr 7, 2021

k8s-ci-robot commented Apr 7, 2021

Jiawei0227 commented Apr 7, 2021

Jiawei0227 commented Apr 7, 2021

abhisheksinghbaghel commented Apr 7, 2021

k8s-ci-robot commented Apr 7, 2021

abhisheksinghbaghel commented Apr 7, 2021

abhisheksinghbaghel commented Apr 7, 2021

Jiawei0227 left a comment

gnufied commented Apr 20, 2021

gnufied left a comment

gnufied Apr 20, 2021

Jiawei0227 Apr 20, 2021

abhisheksinghbaghel Apr 21, 2021

gnufied Apr 28, 2021

abhisheksinghbaghel Apr 28, 2021

abhisheksinghbaghel commented Apr 21, 2021

abhisheksinghbaghel commented Apr 21, 2021

Jiawei0227 commented Apr 28, 2021

jsafrane Apr 28, 2021

gnufied commented Apr 28, 2021

gnufied Apr 28, 2021 •

edited

Loading

gnufied Apr 28, 2021 •

edited

Loading

gnufied commented May 5, 2021

deads2k commented May 7, 2021

cbandy May 18, 2021

k8s-triage-robot commented Aug 16, 2021

k8s-triage-robot commented Sep 15, 2021

k8s-triage-robot commented Oct 15, 2021

k8s-ci-robot commented Oct 15, 2021

benjaminxie commented May 12, 2022

k8s-ci-robot commented May 12, 2022

Option to preserve default file mode bit in atomicwriter volumes #2606

Option to preserve default file mode bit in atomicwriter volumes #2606

Conversation

abhisheksinghbaghel commented Apr 7, 2021

k8s-ci-robot commented Apr 7, 2021

k8s-ci-robot commented Apr 7, 2021

Jiawei0227 commented Apr 7, 2021

Jiawei0227 commented Apr 7, 2021

abhisheksinghbaghel commented Apr 7, 2021

k8s-ci-robot commented Apr 7, 2021

abhisheksinghbaghel commented Apr 7, 2021

abhisheksinghbaghel commented Apr 7, 2021

Jiawei0227 left a comment

Choose a reason for hiding this comment

gnufied commented Apr 20, 2021

gnufied left a comment

Choose a reason for hiding this comment

gnufied Apr 20, 2021

Choose a reason for hiding this comment

Jiawei0227 Apr 20, 2021

Choose a reason for hiding this comment

abhisheksinghbaghel Apr 21, 2021

Choose a reason for hiding this comment

gnufied Apr 28, 2021

Choose a reason for hiding this comment

abhisheksinghbaghel Apr 28, 2021

Choose a reason for hiding this comment

abhisheksinghbaghel commented Apr 21, 2021

abhisheksinghbaghel commented Apr 21, 2021

Jiawei0227 commented Apr 28, 2021

jsafrane Apr 28, 2021

Choose a reason for hiding this comment

gnufied commented Apr 28, 2021

gnufied Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

gnufied Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

gnufied commented May 5, 2021

deads2k commented May 7, 2021

cbandy May 18, 2021

Choose a reason for hiding this comment

k8s-triage-robot commented Aug 16, 2021

k8s-triage-robot commented Sep 15, 2021

k8s-triage-robot commented Oct 15, 2021

k8s-ci-robot commented Oct 15, 2021

benjaminxie commented May 12, 2022

k8s-ci-robot commented May 12, 2022

gnufied Apr 28, 2021 •

edited

Loading

gnufied Apr 28, 2021 •

edited

Loading