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

patch for pod host networking & other host namespace handling #14480

Merged
merged 1 commit into from
Jun 9, 2022
Merged

patch for pod host networking & other host namespace handling #14480

merged 1 commit into from
Jun 9, 2022

Conversation

cdoern
Copy link
Contributor

@cdoern cdoern commented Jun 3, 2022 •
edited
Loading

this patch included additonal host namespace checks when creating a ctr as well
as fixing of the tests to check /proc/self/ns/net

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

see #14461

Does this PR introduce a user-facing change?

None

@cdoern
Copy link
Contributor Author

cdoern commented Jun 3, 2022 •
edited
Loading

@Luap99 PTAL. Not sure if the new helper functions for getting the NS type are as easy as I made them to be

Luap99
Luap99 reviewed Jun 3, 2022
View reviewed changes
test/e2e/pod_infra_container_test.go Outdated Show resolved Hide resolved
libpod/pod.go Outdated Show resolved Hide resolved
pkg/specgen/generate/namespaces.go Outdated Show resolved Hide resolved
@rhatdan
Copy link
Member

rhatdan commented Jun 3, 2022

Please fix your checkin messages. I don't want to have to look up 14461

@cdoern cdoern changed the title patch for #14461 patch for pod host networking & other host namespace handling Jun 3, 2022
Luap99
Luap99 reviewed Jun 3, 2022
View reviewed changes
libpod/pod.go Outdated Show resolved Hide resolved
@Luap99 Luap99 mentioned this pull request Jun 7, 2022
Merged
@cdoern
Copy link
Contributor Author

cdoern commented Jun 7, 2022

circling back to this in the morning, I figured out the issue. @umohnani8 is there any way currently to delineate between host and auto userns once the pod is created? If not, I think here the ctr is assuming the host userns even when --auto is defined on pod creation

@rhatdan
Copy link
Member

rhatdan commented Jun 8, 2022

Host User Namespace would have either the UID 0 or the Current UID mapped into the idmappings, or no idmappings, while auto would always have UID 0 NOT in the mappings.

@cdoern
Copy link
Contributor Author

cdoern commented Jun 8, 2022

@containers/podman-maintainers PTAL

Luap99
Luap99 reviewed Jun 8, 2022
View reviewed changes
pkg/specgen/generate/namespaces.go Outdated Show resolved Hide resolved
pkg/specgen/generate/namespaces.go Outdated Show resolved Hide resolved
libpod/pod_api.go Outdated Show resolved Hide resolved
test/e2e/pod_infra_container_test.go Outdated Show resolved Hide resolved
@cdoern
patch for pod host networking & other host namespace handling
b13fc1b 
this patch included additonal host namespace checks when creating a ctr as well
as fixing of the tests to check /proc/self/ns/net

see containers#14461

Signed-off-by: cdoern <cdoern@redhat.com>
Luap99
Luap99 reviewed Jun 9, 2022
View reviewed changes
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

@cdoern
Copy link
Contributor Author

cdoern commented Jun 9, 2022

@rhatdan @mheon PTAL

@baude
Copy link
Member

baude commented Jun 9, 2022

lgtm

@mheon please give this a quick once over and merge

@rhatdan
Copy link
Member

rhatdan commented Jun 9, 2022

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 9, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 9, 2022

[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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 9, 2022
@openshift-merge-robot openshift-merge-robot merged commit e7db6d4 into containers:main Jun 9, 2022
@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 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 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

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. release-note-none
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cdoern @rhatdan @baude @Luap99 @openshift-merge-robot