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

play kube selinux label issue #9205

Merged
merged 4 commits into from
Feb 5, 2021
Merged

play kube selinux label issue #9205

merged 4 commits into from
Feb 5, 2021

Conversation

st1971
Copy link
Contributor

@st1971 st1971 commented Feb 2, 2021

play kube function not respecting selinux options in kube yaml, all options were
being mapped to role.

fixes issue 8710

Signed-off-by: Steven Taylor [email protected]

@st1971
play kube selinux label issue
c68b59f 
play kube function not respecting selinux options in kube yaml, all options were
being mapped to role.

fixes issue 8710

Signed-off-by: Steven Taylor <[email protected]>
@rhatdan
Copy link
Member

rhatdan commented Feb 2, 2021

Nice catch.
LGTM
Could you add a test. Basically specify an SELinux label, and then create a kube container, and make sure the label is correct.

@rhatdan
Copy link
Member

rhatdan commented Feb 2, 2021

Nice catch.
LGTM
Could you add a test. Basically specify an SELinux label, and then create a kube container, and make sure the label is correct.

@containers/podman-maintainers PTAL

@mheon
Copy link
Member

mheon commented Feb 2, 2021

Code changes LGTM

@mheon
Copy link
Member

mheon commented Feb 2, 2021

Would like to get this in for 3.0 final

@TomSweeneyRedHat
Copy link
Member

LGTM
assuming happy tests

@st1971
play kube selinux label test case
432ee04 
test case added to e2e test suite to validate process label being correctly set
on play kube

Signed-off-by: Steven Taylor <[email protected]>
@st1971
Copy link
Contributor Author

st1971 commented Feb 3, 2021

@rhatdan have added an additional test case to play_kube_test to test this, it's not pretty but validates that the process label is set correctly

@zhangguanzhang
Copy link
Collaborator

Fixes: #8710

rhatdan
rhatdan reviewed Feb 3, 2021
View reviewed changes
test/e2e/play_kube_test.go Outdated
inspect.WaitWithDefaultTimeout()
label := inspect.OutputToString()

Expect(label).To(ContainSubstring("nconfined_u:system_r:spc_t:s0"))
Copy link
Member

Choose a reason for hiding this comment

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

Missing a u unconfined_u

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry my bad have just correct

@rhatdan
Copy link
Member

rhatdan commented Feb 3, 2021

LGTM
Lets get this in and back ported to 3.0.

@st1971
play kube selinux test case
96adf0e 
fixed typo in the label comparison

Signed-off-by: Steven Taylor <[email protected]>
rhatdan
rhatdan approved these changes Feb 4, 2021
View reviewed changes
test/e2e/play_kube_test.go
@@ -803,6 +846,21 @@ var _ = Describe("Podman play kube", func() {

})

It("podman play kube fail with custom selinux label", func() {
Copy link
Member

Choose a reason for hiding this comment

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

You need to add.

	if !selinux.GetEnabled() {
		Skip("SELinux not 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.

sorry hadn't considered cases where selinux is not enabled, have updated the test

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

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

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 4, 2021
Luap99
Luap99 reviewed Feb 4, 2021
View reviewed changes
test/e2e/play_kube_test.go
@@ -803,6 +846,21 @@ var _ = Describe("Podman play kube", func() {

})

It("podman play kube fail with custom selinux label", func() {
Copy link
Member

Choose a reason for hiding this comment

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

You have to add this to get pass the CI:

if !selinux.GetEnabled() {
	Skip("SELinux not enabled")
}

And make sure you import github.com/opencontainers/selinux/go-selinux at the top.

The test doesn't work on systems without selinux support like ubuntu.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry hadn't considered cases where selinux is not enabled, have updated the test

@st1971
play kube selinux test case
6c71398 
added skip to test case where selinux not enabled

Signed-off-by: Steven Taylor <[email protected]>
@rhatdan
Copy link
Member

rhatdan commented Feb 4, 2021

/lgtm
/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 4, 2021
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 4, 2021
@rhatdan
Copy link
Member

rhatdan commented Feb 4, 2021

Thanks @st1971

@rhatdan
Copy link
Member

rhatdan commented Feb 5, 2021

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 5, 2021
@openshift-merge-robot openshift-merge-robot merged commit 69ddbde into containers:master Feb 5, 2021
@st1971 st1971 deleted the issue-8710 branch February 21, 2021 11:01
@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 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 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

@rhatdan rhatdan rhatdan approved these changes

Assignees

@rhatdan rhatdan

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
@st1971 @rhatdan @mheon @TomSweeneyRedHat @zhangguanzhang @openshift-ci-robot @Luap99 @openshift-merge-robot