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 support for selecting kvm and systemd labels #5690

Merged
merged 1 commit into from
Apr 16, 2020

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Mar 31, 2020

In order to better support kata containers and systemd containers
container-selinux has added new types. Podman should execute the
container with an SELinux process label to match the container type.

Traditional Container process : container_t
KVM Container Process: containre_kvm_t
PID 1 Init process: container_init_t

Signed-off-by: Daniel J Walsh [email protected]

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 31, 2020
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 31, 2020
@giuseppe
Copy link
Member

giuseppe commented Apr 1, 2020

code LGTM

@rhatdan rhatdan changed the title [wip] Add support for selecting kvm and systemd labels Add support for selecting kvm and systemd labels Apr 4, 2020
@openshift-ci-robot openshift-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 4, 2020
@vrothberg
Copy link
Member

Gating is now yet happy:

[+0012s] libpod/container_internal.go:1934:17: undefined: selinux.KVMContainerLabels
[+0012s] libpod/container_internal.go:1944:18: undefined: selinux.InitContainerLabels

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

Just minor nits. Once they're fixed, LGTM.

return swapTypes(initLabel, processLabel)
}

func swapTypes(source, dest string) (string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we rename it to swapSELinuxTypes? libpod is a really big package and swapTypes sounds very generic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

for _, r := range runtime.config.Engine.RuntimeSupportsJSON {
supportsJSON[r] = true
}
for _, r := range runtime.config.Engine.RuntimeSupportsNoCgroups {
supportsNoCgroups[r] = true
}

/* for _, r := range runtime.config.Engine.RuntimeSupportsKVM {
Copy link
Member

Choose a reason for hiding this comment

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

What's going on here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops had not implemented, it is now in containers/common.

libpod/oci.go Outdated
@@ -102,6 +102,8 @@ type OCIRuntime interface {
// SupportsNoCgroups is whether the runtime supports running containers
// without cgroups.
SupportsNoCgroups() bool
// SupportsKVM indicates container will use KVM separation
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: extra space between use and KVM

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@@ -379,7 +383,7 @@ func makeRuntime(ctx context.Context, runtime *Runtime) (err error) {
json := supportsJSON[name]
nocgroups := supportsNoCgroups[name]

ociRuntime, err := newConmonOCIRuntime(name, paths, runtime.conmonPath, runtime.config, json, nocgroups)
ociRuntime, err := newConmonOCIRuntime(name, paths, runtime.conmonPath, runtime.config, json, nocgroups, supportsKVM[name])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should save the value of this like we do nocgroups and json above

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed and figured it out inside of the function.

@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #5727) made this pull request unmergeable. Please resolve the merge conflicts.

@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #5478) made this pull request unmergeable. Please resolve the merge conflicts.

@rhatdan rhatdan force-pushed the selinux branch 2 times, most recently from d33b1ed to 7e922c6 Compare April 7, 2020 09:02
@rhatdan rhatdan changed the title Add support for selecting kvm and systemd labels [WIP] Add support for selecting kvm and systemd labels Apr 7, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 7, 2020
@rhatdan rhatdan force-pushed the selinux branch 4 times, most recently from 11b1a95 to 79f34a5 Compare April 7, 2020 18:20
@rhatdan rhatdan changed the title [WIP] Add support for selecting kvm and systemd labels Add support for selecting kvm and systemd labels Apr 7, 2020
@openshift-ci-robot openshift-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 7, 2020
@rhatdan
Copy link
Member Author

rhatdan commented Apr 7, 2020

@haircommander @vrothberg @umohnani8 PTAL

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #5756) made this pull request unmergeable. Please resolve the merge conflicts.

@TomSweeneyRedHat
Copy link
Member

LGTM once the unhappy tests are happy

@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #5785) made this pull request unmergeable. Please resolve the merge conflicts.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 10, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2020
@umohnani8 umohnani8 removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 14, 2020
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 14, 2020
@rhatdan rhatdan changed the title Add support for selecting kvm and systemd labels [WIP] Add support for selecting kvm and systemd labels Apr 14, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 14, 2020
@rhatdan
Copy link
Member Author

rhatdan commented Apr 14, 2020

I just destroyed this PR> Will need to recreate it tomorrow.

@rhatdan rhatdan changed the title [WIP] Add support for selecting kvm and systemd labels Add support for selecting kvm and systemd labels Apr 15, 2020
@openshift-ci-robot openshift-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 15, 2020
@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #5786) made this pull request unmergeable. Please resolve the merge conflicts.

@rhatdan rhatdan force-pushed the selinux branch 4 times, most recently from c62bedb to da807e6 Compare April 15, 2020 20:36
In order to better support kata containers and systemd containers
container-selinux has added new types. Podman should execute the
container with an SELinux process label to match the container type.

Traditional Container process : container_t
KVM Container Process: containre_kvm_t
PID 1 Init process: container_init_t

Signed-off-by: Daniel J Walsh <[email protected]>
@rhatdan
Copy link
Member Author

rhatdan commented Apr 16, 2020

@baude @mheon @giuseppe @vrothberg @TomSweeneyRedHat @QiWang19 @jwhonce this one is ready to go in. PTAL

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@baude
Copy link
Member

baude commented Apr 16, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 16, 2020
@openshift-merge-robot openshift-merge-robot merged commit 09e821a into containers:master Apr 16, 2020
@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 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 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.