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.

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

storage: document Windows projected volume limitations #30366

Merged
merged 2 commits into from Nov 17, 2021

Conversation

aravindhp
Copy link
Contributor

Document the limitation wrt file ownership with Windows Pods that have a projected volume and RunAsUsername set in it's SecurityContext

xref: kubernetes/kubernetes#102849

/sig storage
/sig windows
/cc @sftim @marosset @jsturtevant @zshihang

@k8s-ci-robot k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/windows Categorizes an issue or PR as relevant to SIG Windows. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Nov 4, 2021
@marosset
Copy link
Contributor

marosset commented Nov 4, 2021

/cc @brasmith-ms

@k8s-ci-robot
Copy link
Contributor

@marosset: GitHub didn't allow me to request PR reviews from the following users: brasmith-ms.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @brasmith-ms

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.

@netlify
Copy link

netlify bot commented Nov 4, 2021

✔️ Deploy Preview for kubernetes-io-main-staging ready!

🔨 Explore the source changes: 4c296c6

🔍 Inspect the deploy log: https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/61943c0b7b69390007d33a2e

😎 Browse the preview: https://deploy-preview-30366--kubernetes-io-main-staging.netlify.app

items:
- key: config
path: my-group/my-config
```
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest we move this and the following YAML files out of the page, into the 'examples' directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or somewhere under tasks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will move the text into configure-projected-volume-storage.md with yaml in the storage examples folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like tasks are more like for trying out rather than just for explanations. I will just move the yaml to the examples folders and link to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can use a codenew shortcode rather than a link, if that's relevant / useful.

* [`secret`](/docs/concepts/storage/volumes/#secret)
* [`downwardAPI`](/docs/concepts/storage/volumes/#downwardapi)
* [`configMap`](/docs/concepts/storage/volumes/#configmap)
* `serviceAccountToken`
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a link for this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could not find a link for this in volumes so I just moved it over as is.

content/en/docs/concepts/storage/projected-volumes.md Outdated Show resolved Hide resolved
In Windows pods that have a projected volume and `RunAsUsername` is set in the
Pod `SecurityContext`, the ownership is not enforced due to the way user
accounts are managed in Windows. Windows stores and manages local user and group
accounts in a database file called Security Account Manager (SAM). This database
Copy link
Contributor

Choose a reason for hiding this comment

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

@brasmith-ms should we use SID or account in this documentation?

Copy link
Contributor

Choose a reason for hiding this comment

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

SAM tracks both, so either term is fine. In this case I would opt for account since the SIDs in the container are not necessarily unique between containers.

read, write and execute access while, non-administrator users will have read and
execute access.

{{< note >}}
Copy link
Contributor

@marosset marosset Nov 5, 2021

Choose a reason for hiding this comment

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

kubernetes/kubernetes#104693 (part ofhttps://github.com/kubernetes/enhancements/issues/2802) will deny pods with RunAsUser specified if os=windows is also set and the feature is enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

kubernetes/kubernetes#104693 will not be backported. So my plan was to get this merged and backported and then open a PR removing or updating the note. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good!

Move inline examples into the examples folder
@aravindhp
Copy link
Contributor Author

@tengqm @marosset @jsturtevant @brasmith-ms I have addressed your comments. PTAL.

@marosset
Copy link
Contributor

marosset commented Nov 9, 2021

/lgtm
Thanks @aravindhp!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 9, 2021
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 71500498d9d844db0450113e63d10bfdad884263

@brasmith-ms
Copy link
Contributor

LGTM! Thank you @aravindhp!!

@tengqm
Copy link
Contributor

tengqm commented Nov 10, 2021

/lgtm

aravindhp added a commit to aravindhp/windows-machine-config-operator that referenced this pull request Nov 10, 2021
aravindhp added a commit to aravindhp/windows-machine-config-operator that referenced this pull request Nov 10, 2021
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 16, 2021
@linux-foundation-easycla
Copy link

CLA Not Signed

@marosset
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 16, 2021
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 46bd88d0cd209dde9a5001600d6c31ccc381b014

@jsturtevant
Copy link
Contributor

/lgtm

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

This is an improvement. Let's merge it.

Comment on lines +74 to +76
The [proposal for file permission handling in projected service account volume](https://github.com/kubernetes/enhancements/pull/1598)
enhancement introduced the projected files having the the correct owner
permissions set.
Copy link
Contributor

Choose a reason for hiding this comment

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


By default, the projected files will have the following ownership as shown for
an example projected volume file:
```powershell
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a valid PowerShell script.

receive updates for those volume sources.
{{< /note >}}
A projected volume maps several existing volume sources into the same
directory. For more details, see [projected volumes](/docs/concepts/storage/projected-volumes/)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
directory. For more details, see [projected volumes](/docs/concepts/storage/projected-volumes/)
directory. For more details, see [projected volumes](/docs/concepts/storage/projected-volumes/).

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sftim

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 17, 2021
@k8s-ci-robot k8s-ci-robot merged commit cee6c71 into kubernetes:main Nov 17, 2021
@aravindhp aravindhp deleted the windows-projected-volume branch November 18, 2021 23:36
@aravindhp
Copy link
Contributor Author

@sftim this PR along with #30559 needs to be backported for older K8s versions that has this problem. What is the process for doing that with docs? Use the cherry-pick bot?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants