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

Add note about volume with unprivileged container #12301

Merged
merged 1 commit into from
Nov 22, 2021

Conversation

umohnani8
Copy link
Member

NO NEW TESTS NEEDED
Signed-off-by: Urvashi Mohnani [email protected]

What this PR does / why we need it:

Add a note to the generated kube yaml if we detect a
volume is being mounted. The note lets the user know
what needs to be done to avoid permission denied error
when trying to access the volume for an unprivileged
container.
Add the same note to the man pages.

How to verify it

Add volumes to your podman container and call podman generate kube on it - the generated yaml should have a note about volumes in it.

Which issue(s) this PR fixes:

Fixes #11916

Special notes for your reviewer:

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 15, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 15, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, umohnani8

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

@@ -124,6 +124,15 @@ func (ic *ContainerEngine) GenerateKube(ctx context.Context, nameOrIDs []string,
if err != nil {
return nil, err
}
if len(po.Spec.Volumes) != 0 {
warning := `
# NOTE: If you generated this yaml from an unprivileged and rootless podman container on an SELinux
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this something we can know? We know if we're rootless, and AFAIK the SELinux package has the ability to check if we're enabled

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 check for that and add the privileged: true option, but that would be assuming the user wants to run a privileged container. We could add a warning that we made it privileged due to volumes.

Copy link
Member

Choose a reason for hiding this comment

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

Well the spec file could be shared with people on a different system with SELinux enabled.

Copy link
Member

Choose a reason for hiding this comment

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

Why are we doing this in podman generate kube, now that I think about this? This mostly seems like an issue for podman play kube - shouldn't we warn people there, given the problems show up when the YAML is run?

Copy link
Member Author

Choose a reason for hiding this comment

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

It shows up when we run the yaml in a kubernetes cluster, so makes more sense to put the warning on the generate side.

Copy link
Member Author

Choose a reason for hiding this comment

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

The note will be added to the generated yaml, so users should see it when using it in a kubernetes cluster or with play kube.

@TomSweeneyRedHat
Copy link
Member

LGTM
and happy green test buttons.

Add a note to the generated kube yaml if we detect a
volume is being mounted. The note lets the user know
what needs to be done to avoid permission denied error
when trying to access the volume for an unprivileged
container.
Add the same note to the man pages.

NO NEW TESTS NEEDED

Signed-off-by: Urvashi Mohnani <[email protected]>
@umohnani8
Copy link
Member Author

This is ready to merge @rhatdan PTAL

@rhatdan
Copy link
Member

rhatdan commented Nov 22, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 22, 2021
@openshift-merge-robot openshift-merge-robot merged commit 0b7c132 into containers:main Nov 22, 2021
@umohnani8 umohnani8 deleted the table branch February 23, 2023 18:44
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 8, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kube generate: privileged: false is not correct
5 participants