config: use built-in TOML merge and adhere to label setting #5225

vrothberg · 2020-02-17T09:23:54Z

Podman did not check the label setting in the libpod.conf and ignored if the libpod.conf set it to false to disable process labels in the container. While working on that, I found a bug in the config-merge code related to a false merging of booleans. To fix that, use the built-in features of the TOML library to merge/extend config instead of manually working around that.

Fixes: #5087

openshift-ci-robot · 2020-02-17T09:23:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [vrothberg]

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

vrothberg · 2020-02-17T10:45:09Z

@mheon @rhatdan PTAL

rhatdan · 2020-02-17T13:37:47Z

I would rather get in containers.conf, but I have not figured out the last Test failure.
LGTM

TomSweeneyRedHat · 2020-02-17T13:53:05Z

libpod/config/config.go

nit interatively -> iteratively

TomSweeneyRedHat · 2020-02-17T13:56:06Z

libpod/config/config.go

I wonder if we should make a backlog jira card to help to remember to remove this.

TomSweeneyRedHat · 2020-02-17T13:59:24Z

LGTM

pkg/spec/security.go

vrothberg · 2020-02-18T14:04:41Z

@rhatdan @mheon please take another look. I moved the non-CLI label setting into pkg/spec which to me seems the right place. If you want it somewhere else, please let me know where and I can move it.

mheon · 2020-02-18T15:47:07Z

My thinking was mostly that, if I set libpod.conf to disable labelling, we should not use SELinux, no matter where the container originated - so for APIv2 containers, we should not use SELinux even if the remote host requests it.

vrothberg · 2020-02-18T15:53:28Z

My thinking was mostly that, if I set libpod.conf to disable labelling, we should not use SELinux, no matter where the container originated - so for APIv2 containers, we should not use SELinux even if the remote host requests it.

I don't know any other flag where we're doing that as the CLI always (?) wins. Especially in case of booleans we had to distinguish between unset, set and unspecified in the configurations to know if the admin cares and compare that with the CLI but we don't have that.

mheon · 2020-02-18T15:56:24Z

I view this less as a default setting for the label flag and more as a configuration bool entirely disabling labelling. I'm not too firm on this, but it might help to have a tiebreaker here. @rhatdan @baude Thoughts?

rhatdan · 2020-02-18T20:10:34Z

I disagree, I might want to run the bulk of my containers without SELinux separation, but a couple of untrusted ones, I want to run with SELinux.

rhatdan · 2020-02-18T20:12:52Z

LGTM

vrothberg · 2020-02-19T09:54:16Z

Let me add a commit on top to clarify that in the libpod.conf. The man page is not strictly clear on that and I can understand Matt's idea as well.

Instead of manually merging the configs, use the built-in features of TOMP to merge/extend the fields of a data type when encoding a file. This erases the need for the merge code in libpod/config and also addresses issues when merging booleans. Signed-off-by: Valentin Rothberg <[email protected]>

Set the (default) process labels in `pkg/spec`. This way, we can also query libpod.conf and disable labeling if needed. Fixes: containers#5087 Signed-off-by: Valentin Rothberg <[email protected]>

Clarify that the label option sets the defaults which can still be overriden by the CLI. Signed-off-by: Valentin Rothberg <[email protected]>

vrothberg · 2020-02-19T10:04:26Z

Done ✔️

mheon · 2020-02-19T14:54:26Z

/lgtm

mheon · 2020-02-19T16:02:44Z

/lgtm

openshift-ci-robot requested review from mheon and mrunalp February 17, 2020 09:23

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL labels Feb 17, 2020

vrothberg force-pushed the fix-5087 branch from a4834d7 to e8b2a85 Compare February 17, 2020 09:30

TomSweeneyRedHat reviewed Feb 17, 2020

View reviewed changes

libpod/config/config.go Outdated

Copy link

Member

TomSweeneyRedHat Feb 17, 2020

Choose a reason for hiding this comment

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

nit interatively -> iteratively

TomSweeneyRedHat reviewed Feb 17, 2020

View reviewed changes

libpod/config/config.go Outdated

Copy link

Member

TomSweeneyRedHat Feb 17, 2020

Choose a reason for hiding this comment

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

I wonder if we should make a backlog jira card to help to remember to remove this.

mheon reviewed Feb 17, 2020

View reviewed changes

pkg/spec/security.go Outdated Show resolved Hide resolved

vrothberg force-pushed the fix-5087 branch from e8b2a85 to 983d156 Compare February 18, 2020 14:03

vrothberg added 3 commits February 19, 2020 11:04

set process labels in pkg/spec

58cbbbc

Set the (default) process labels in `pkg/spec`. This way, we can also query libpod.conf and disable labeling if needed. Fixes: containers#5087 Signed-off-by: Valentin Rothberg <[email protected]>

libpod.conf: clarify label description

0fcaa93

Clarify that the label option sets the defaults which can still be overriden by the CLI. Signed-off-by: Valentin Rothberg <[email protected]>

vrothberg force-pushed the fix-5087 branch from 983d156 to 0fcaa93 Compare February 19, 2020 10:04

openshift-ci-robot assigned mheon Feb 19, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 19, 2020

openshift-merge-robot merged commit da249e2 into containers:master Feb 19, 2020

This was referenced Feb 19, 2020

apiv2 container create using specgen #5204

Merged

WIP: Add support for pinned namespaces #5210

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 25, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2023

config: use built-in TOML merge and adhere to label setting #5225

config: use built-in TOML merge and adhere to label setting #5225

Uh oh!

Conversation

vrothberg commented Feb 17, 2020

Uh oh!

openshift-ci-robot commented Feb 17, 2020

Uh oh!

vrothberg commented Feb 17, 2020

Uh oh!

rhatdan commented Feb 17, 2020

Uh oh!

TomSweeneyRedHat Feb 17, 2020

Choose a reason for hiding this comment

Uh oh!

TomSweeneyRedHat Feb 17, 2020

Choose a reason for hiding this comment

Uh oh!

TomSweeneyRedHat commented Feb 17, 2020

Uh oh!

Uh oh!

vrothberg commented Feb 18, 2020

Uh oh!

mheon commented Feb 18, 2020

Uh oh!

vrothberg commented Feb 18, 2020

Uh oh!

mheon commented Feb 18, 2020

Uh oh!

rhatdan commented Feb 18, 2020

Uh oh!

rhatdan commented Feb 18, 2020

Uh oh!

vrothberg commented Feb 19, 2020

Uh oh!

vrothberg commented Feb 19, 2020

Uh oh!

mheon commented Feb 19, 2020

Uh oh!

mheon commented Feb 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants