Pod Security Option support and Infra Inheritance changes #12208

cdoern · 2021-11-08T02:35:07Z

What this PR does / why we need it:

Added support for pod security options. These are applied to infra and passed down to the
containers as added (unless overridden).

Modified the inheritance process from infra, creating a new function Inherit() which reads the config, and marshals the compatible options into an intermediate struct InfraInherit
This is then unmarshaled into a container config and all of this is added to the CtrCreateOptions. Removes the need (mostly) for special additions which complicate the Container_create
code and pod creation.

Which issue(s) this PR fixes:

resolves #12173

Signed-off-by: cdoern [email protected]

docs/source/markdown/podman-pod-create.1.md

pkg/specgen/generate/container_create.go

libpod/container_config.go

cdoern · 2021-11-17T03:44:52Z

@mheon any idea why the play kube seccomp tests are failing? looks like something that is not supposed to be permitted is being allowed. Could this be something that doesn't apply now since security options are allowed?

mheon · 2021-11-17T16:06:25Z

Can you link a failure? Gitvalidation is failing now, so I can't see the failures

cdoern · 2021-11-17T17:22:48Z

@mheon here is a link: https://storage.googleapis.com/cirrus-ci-6707778565701632-fcae48/artifacts/containers/podman/6536676483792896/html/int-podman-fedora-33-rootless-host.log.html

I am pushing now to fix the validation, trying something new to see if that fixes the seccomp issues

EDIT: seeing same failures in most recent test runs.

mheon · 2021-11-17T19:14:18Z

It's complaining we don't have a private mount namespace?

Error is coming out of crun, definitely, but I think we're passing an invalid config - I don't see how Podman can create a container without a private mount namespace though.

libpod/container_inspect.go

mheon · 2021-11-17T19:16:32Z

I think you're mangling the spec such that we no longer have a mount namespace, but I'm not sure how that's happening. Maybe we're trying to inherit the pod's mount namespace?

cdoern · 2021-11-22T20:31:51Z

@containers/podman-maintainers PTAL I think this is good to go

test/e2e/pod_create_test.go

rhatdan · 2021-11-23T14:00:22Z

While you are in fixing the SELinux check could you add a --security-opt seccomp=unconfined check, also.

cdoern · 2021-11-23T17:28:51Z

While you are in fixing the SELinux check could you add a --security-opt seccomp=unconfined check, also.

the seccomp=unconfined is in the first test, I do it simultaneously with label=type:spc_t @rhatdan

cdoern · 2021-12-03T15:54:37Z

@containers/podman-maintainers PTAL

rhatdan · 2021-12-03T18:19:17Z

LGTM
/approve
@mheon PTAL

openshift-ci · 2021-12-03T18:19:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cdoern, 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:

~~OWNERS~~ [rhatdan]

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

cdoern · 2021-12-08T13:58:51Z

@containers/podman-maintainers PTAL

mheon · 2021-12-08T16:03:57Z

pkg/specgen/generate/container_create.go

+		}
+	}
+	if compatibleOptions != nil {
+		options = append(options, libpod.WithInfraConfig(*compatibleOptions))


Instead of doing this, can we return the InfraInherit and do the unmarshal over the spec within pkg/specgen/generate/oci.go as we create the spec itself? Localizes the changes to pkg/spec which seems simpler, though you will have to pass the InfraInherit around a bit

Actually, it looks like you're passing it into SpecGenToOCI below, so it probably isn't even necessary to change how it's passed around

so is this good as is? theSpecGenToOCI bit is just for config elements that get overwritten. This works for like 90% of options.

I'd prefer to remove the option and just set the values in the spec in SpecGenToOCI - most options alter more than just the spec, if we're just changing the spec we can do it during spec generation

ok, I am trying to have this just be done in the spec, but for some reason all of the tests are failing even though the runtime spec has the proper values. I am going to investigate a bit,

Huh...

Can you verify if it has the correct values after it gets put into the DB? I wonder if some other part of container creation is overwriting it.

ok @mheon I did some digging, I do not think the issue is with the runtime spec, I think the container config needs to be modified as well. some options like volumes are not explicitly in the spec and will be dropped.

Instead of doing options, I could make a path that opts to do something like a checkpoint where we load the config from infra (or at least what matches). Could help delineate this from regular create options

OK, if we need config changes, options are fine - I'd just prefer to keep things split (the options modify the container config, while state modification is done without options in SpecGen). Doesn't need to be more than one option, either - the old approach of a single option to grab all relevant config blobs from the infra works fine in my book.

ok @mheon, then is this PR good as is? I never pushed the new changes.

rhatdan · 2021-12-14T21:30:54Z

@cdoern Back in your hands.

cdoern · 2021-12-15T14:54:55Z

@cdoern Back in your hands.

sorry I will circle back to this later tonight.

cdoern · 2021-12-21T19:02:22Z

@containers/podman-maintainers PTAL

Added support for pod security options. These are applied to infra and passed down to the containers as added (unless overridden). Modified the inheritance process from infra, creating a new function Inherit() which reads the config, and marshals the compatible options into an intermediate struct `InfraInherit` This is then unmarshaled into a container config and all of this is added to the CtrCreateOptions. Removes the need (mostly) for special additons which complicate the Container_create code and pod creation. resolves containers#12173 Signed-off-by: cdoern <[email protected]>

rhatdan · 2021-12-28T10:48:07Z

@mheon PTAL

rhatdan · 2022-01-05T16:54:58Z

/lgtm

cdoern force-pushed the podSecurityOpt branch from e17296f to d03eb0e Compare November 8, 2021 14:00

TomSweeneyRedHat reviewed Nov 8, 2021

View reviewed changes

docs/source/markdown/podman-pod-create.1.md Show resolved Hide resolved

TomSweeneyRedHat reviewed Nov 8, 2021

View reviewed changes

docs/source/markdown/podman-pod-create.1.md Outdated Show resolved Hide resolved

pkg/specgen/generate/container_create.go Outdated Show resolved Hide resolved

cdoern force-pushed the podSecurityOpt branch from d03eb0e to 95a321c Compare November 9, 2021 02:31

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 9, 2021

cdoern force-pushed the podSecurityOpt branch from 95a321c to 40a2b43 Compare November 9, 2021 02:43

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 9, 2021

Luap99 reviewed Nov 9, 2021

View reviewed changes

libpod/container_config.go Outdated Show resolved Hide resolved

cdoern marked this pull request as draft November 9, 2021 19:51

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 9, 2021

cdoern force-pushed the podSecurityOpt branch 2 times, most recently from bbdf94a to 52c829e Compare November 12, 2021 14:05

cdoern changed the title ~~Pod Security Option support~~ Pod Security Option support and Infra Inheritance changes Nov 12, 2021

cdoern force-pushed the podSecurityOpt branch 4 times, most recently from 14154c6 to 1b1866d Compare November 17, 2021 03:43

cdoern force-pushed the podSecurityOpt branch 2 times, most recently from 0e49ee8 to 2215e29 Compare November 17, 2021 15:48

cdoern force-pushed the podSecurityOpt branch from 2215e29 to 121d066 Compare November 17, 2021 17:23

mheon reviewed Nov 17, 2021

View reviewed changes

libpod/container_inspect.go Outdated Show resolved Hide resolved

cdoern force-pushed the podSecurityOpt branch 4 times, most recently from 6d24cd2 to 5cd899b Compare November 18, 2021 17:28

giuseppe reviewed Nov 23, 2021

View reviewed changes

test/e2e/pod_create_test.go Show resolved Hide resolved

cdoern force-pushed the podSecurityOpt branch 2 times, most recently from 63b0f4a to 6c31371 Compare December 2, 2021 19:34

cdoern force-pushed the podSecurityOpt branch from 6c31371 to 5c51adb Compare December 3, 2021 15:58

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 3, 2021

mheon reviewed Dec 8, 2021

View reviewed changes

cdoern force-pushed the podSecurityOpt branch from 5c51adb to 9385749 Compare December 21, 2021 19:02

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 23, 2021

cdoern force-pushed the podSecurityOpt branch from 9385749 to bc0e45b Compare December 27, 2021 16:37

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 27, 2021

cdoern force-pushed the podSecurityOpt branch from bc0e45b to 2892703 Compare December 27, 2021 18:40

openshift-ci bot assigned rhatdan Jan 5, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 5, 2022

openshift-merge-robot merged commit 50e156b into containers:main Jan 5, 2022

edsantiago mentioned this pull request Jan 18, 2022

Return of the Evil Dead Pprof #12624

Closed

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 bot locked as resolved and limited conversation to collaborators Sep 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pod Security Option support and Infra Inheritance changes #12208

Pod Security Option support and Infra Inheritance changes #12208

cdoern commented Nov 8, 2021 •

edited

Loading

cdoern commented Nov 17, 2021

mheon commented Nov 17, 2021

cdoern commented Nov 17, 2021 •

edited

Loading

mheon commented Nov 17, 2021

mheon commented Nov 17, 2021

cdoern commented Nov 22, 2021

rhatdan commented Nov 23, 2021

cdoern commented Nov 23, 2021 •

edited

Loading

cdoern commented Dec 3, 2021

rhatdan commented Dec 3, 2021

openshift-ci bot commented Dec 3, 2021

cdoern commented Dec 8, 2021

mheon Dec 8, 2021

mheon Dec 8, 2021

cdoern Dec 10, 2021

mheon Dec 10, 2021

cdoern Dec 16, 2021

mheon Dec 16, 2021

cdoern Dec 17, 2021

mheon Dec 17, 2021

cdoern Dec 19, 2021

rhatdan commented Dec 14, 2021

cdoern commented Dec 15, 2021

cdoern commented Dec 21, 2021

rhatdan commented Dec 28, 2021

rhatdan commented Jan 5, 2022

Pod Security Option support and Infra Inheritance changes #12208

Pod Security Option support and Infra Inheritance changes #12208

Conversation

cdoern commented Nov 8, 2021 • edited Loading

What this PR does / why we need it:

Which issue(s) this PR fixes:

cdoern commented Nov 17, 2021

mheon commented Nov 17, 2021

cdoern commented Nov 17, 2021 • edited Loading

mheon commented Nov 17, 2021

mheon commented Nov 17, 2021

cdoern commented Nov 22, 2021

rhatdan commented Nov 23, 2021

cdoern commented Nov 23, 2021 • edited Loading

cdoern commented Dec 3, 2021

rhatdan commented Dec 3, 2021

openshift-ci bot commented Dec 3, 2021

cdoern commented Dec 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Dec 14, 2021

cdoern commented Dec 15, 2021

cdoern commented Dec 21, 2021

rhatdan commented Dec 28, 2021

rhatdan commented Jan 5, 2022

cdoern commented Nov 8, 2021 •

edited

Loading

cdoern commented Nov 17, 2021 •

edited

Loading

cdoern commented Nov 23, 2021 •

edited

Loading