Prohibit --uid/gid map and --pod for container create/run

[cdoern]

add a check in namespaceOptions() that ensures the user is not setting a new uid/gid map
if entering or creating a pod that has an infra container

resolves #12669

Signed-off-by: cdoern cdoern@redhat.com

[mheon]

Should we add a note to the manpages about this?

[mheon]

Otherwise LGTM

[cdoern]

sure @mheon do you think it should go in both podman run and podman create?

[mheon]

Yeah, we usually mirror contents of those manpages very closely

[Luap99]

But why do we forbid it if it works?

[cdoern]

@Luap99 sharing of namespaces within a pod makes it so that the uid/gid maps should be on the pod level not overwritten by the container: #12669 (comment)

[rhatdan]

LGTM

[rhatdan]

/approve
/lgtm
/hold

[openshift-ci]

[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

[Luap99]

But why? This clearly works just fine right now. This will be a breaking change for may users since uidmapping didn't work on pod earlier so I assume that there are people using podman run --uidmap with --pod. Also what if I want a different uidmapping for all containers in a pod?

[cdoern]

@Luap99 that kind of defies the entire ways that pods and namespaces are supposed to work, if we allow for overwriting this flag then people will assume the same works for other flags that involve namespaces, which it doesn't. Take sysctls for example: #12836 we want things involving namespaces to be defined on the pod level and pod level only.

[Luap99]

OK but then this check needs to check if the pod has a infra bacause if you use a pod without infra this should still be allowed since there are no shared namespaces in this case. Also the check should be in specgen to also cover api users

[cdoern]

@Luap99, can't do the check after FillOutSpecGen because that function autofills IDMappings if none are provided. I will throw a check in pkg/api/handlers/libpod when the specgen gets decoded. Will also add another check in validate() for if the pod we are entering has an infra container.

[cdoern]

@Luap99 @rhatdan placed the check in specgen so that the change applies to remote and api users as well.

[Luap99]

LGTM, just two small nits

[cdoern]

added the test changes, sorry I didn't see you did the capitalization changed as well @Luap99

[cdoern]

the tests had an issue because I used the same pod name. looks good now @rhatdan @Luap99

[rhatdan]

/lgtm

[rhatdan]

/hold cancel