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

Support selinux options with bind mounts play/gen #11793

Merged
merged 1 commit into from
Sep 30, 2021
Merged

Support selinux options with bind mounts play/gen #11793

merged 1 commit into from
Sep 30, 2021

Conversation

baude
Copy link
Member

@baude baude commented Sep 29, 2021

When using play kube and generate kube, we need to support if bind
mounts have selinux options. As kubernetes does not support selinux in
this way, we tuck the selinux values into a pod annotation for
generation of the kube yaml. Then on play, we check annotations to see
if a value for the mount exists and apply it.

Fixes BZ #1984081

Signed-off-by: Brent Baude [email protected]

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 29, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baude

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 29, 2021
@rhatdan
Copy link
Member

rhatdan commented Sep 29, 2021

What does the annotation look like?

@baude
Copy link
Member Author

baude commented Sep 29, 2021

What does the annotation look like?

# Created with podman-4.0.0-dev
apiVersion: v1
kind: Pod
metadata:
  annotations:
    /home/baude/my-lamp-project/mydbstuff: Z
    /home/baude/my-lamp-project/mywebstuff: Z

@baude baude force-pushed the playgenkubeselinux branch from ff14410 to 893a484 Compare September 29, 2021 20:17
@ashley-cui
Copy link
Member

LGTM

TomSweeneyRedHat
TomSweeneyRedHat reviewed Sep 29, 2021
View reviewed changes
pkg/specgen/generate/kube/kube.go Outdated
// a selinux mount option exists for it
for k, v := range opts.Annotations {
// Make sure the z/Z option is not already there (from editing the YAML)
if k == volumeSource.Source && !util.StringInSlice("z", options) && !util.StringInSlice("Z", options) {
Copy link
Member

Choose a reason for hiding this comment

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

I believe, but I'm just checking, z and Z are unique values in options. I.e. there's no chance one of them would be part of another string value in options right?

Copy link
Member Author

Choose a reason for hiding this comment

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

there are unique

@TomSweeneyRedHat
Copy link
Member

LGTM
assuming my assumption is correct and tests are happy.

@mheon
Copy link
Member

mheon commented Sep 29, 2021

/lgtm
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 29, 2021
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 29, 2021
jwhonce
jwhonce reviewed Sep 29, 2021
View reviewed changes
Copy link
Member

@jwhonce jwhonce left a comment

Choose a reason for hiding this comment

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

Minor nits in gomega matchers.

test/e2e/generate_kube_test.go Outdated Show resolved Hide resolved
test/e2e/generate_kube_test.go Outdated Show resolved Hide resolved
@mheon
Copy link
Member

mheon commented Sep 29, 2021

Tests are red

@baude baude force-pushed the playgenkubeselinux branch from 893a484 to 760b86d Compare September 29, 2021 21:09
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 29, 2021
@baude
Copy link
Member Author

baude commented Sep 29, 2021

fixed requests for clearer error messages.

@rhatdan
Copy link
Member

rhatdan commented Sep 29, 2021 

/home/baude/my-lamp-project/mydbstuff: Z
/home/baude/my-lamp-project/mywebstuff: Z

I am not crazy about these from a human perspective. Hard to know from looking at the Yaml what these mean. What happens if other annotations are in the code
/my/path: ro
foo: bar
/abc: /def

@Luap99
Copy link
Member

Luap99 commented Sep 30, 2021

Would it make sense to extend this to support all volume options?

/home/baude/my-lamp-project/mydbstuff: Z
/home/baude/my-lamp-project/mywebstuff: Z

I am not crazy about these from a human perspective. Hard to know from looking at the Yaml what these mean. What happens if other annotations are in the code /my/path: ro foo: bar /abc: /def

I agree, I think the path should be prefixed, e.g volume-options-/home/baude/my-lamp-project/mydbstuff: Z

@baude
Copy link
Member Author

baude commented Sep 30, 2021

Would it make sense to extend this to support all volume options?

/home/baude/my-lamp-project/mydbstuff: Z
/home/baude/my-lamp-project/mywebstuff: Z

I am not crazy about these from a human perspective. Hard to know from looking at the Yaml what these mean. What happens if other annotations are in the code /my/path: ro foo: bar /abc: /def

I agree, I think the path should be prefixed, e.g volume-options-/home/baude/my-lamp-project/mydbstuff: Z

I dont love the idea of having to split the annotation key however...this is what iw as talking about yesterday

@mheon
Copy link
Member

mheon commented Sep 30, 2021

Prefix is a good idea, we do use annotations for other things

@baude baude force-pushed the playgenkubeselinux branch from 760b86d to 8ea932f Compare September 30, 2021 15:42
@baude
Support selinux options with bind mounts play/gen
1ff6a50 
When using play kube and generate kube, we need to support if bind
mounts have selinux options.  As kubernetes does not support selinux in
this way, we tuck the selinux values into a pod annotation for
generation of the kube yaml.  Then on play, we check annotations to see
if a value for the mount exists and apply it.

Fixes BZ #1984081

Signed-off-by: Brent Baude <[email protected]>
@baude baude force-pushed the playgenkubeselinux branch from 8ea932f to 1ff6a50 Compare September 30, 2021 15:49
Luap99
Luap99 reviewed Sep 30, 2021
View reviewed changes
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

@mheon
Copy link
Member

mheon commented Sep 30, 2021

/lgtm
/hold

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 30, 2021
@baude
Copy link
Member Author

baude commented Sep 30, 2021

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 30, 2021
@openshift-merge-robot openshift-merge-robot merged commit 3d08c40 into containers:main Sep 30, 2021
@edsantiago edsantiago mentioned this pull request Sep 30, 2021
Merged
@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 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Luap99 Luap99 Luap99 left review comments

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

@rhatdan rhatdan rhatdan left review comments

@jwhonce jwhonce jwhonce left review comments

Assignees

@mheon mheon

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@baude @rhatdan @ashley-cui @TomSweeneyRedHat @mheon @Luap99 @jwhonce @openshift-merge-robot