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

libpod: restart+userns cleanup netns correctly #20384

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Oct 17, 2023

When a userns and netns is used we need to let the runtime create the netns otherwise the netns is not owned by the right userns and thus the capabilities would not be correct.

The current restart logic tries to reuse the netns which is fine if no userns is used but when one is used we setup a new netns (which is correct) but forgot to cleanup the old netns. This resulted in leaked network namespaces and because no teardown was ever called leaked ipam assignments, thus a quickly restarting container will run out of ip space very fast.

Fixes #18615

Does this PR introduce a user-facing change?

Fixed a bug where a container with a custom user namespace and --restart always/on-failure would not correctly cleanup the netns on restart resulting in leaked ips and network namespaces. 

When a userns and netns is used we need to let the runtime create the
netns otherwise the netns is not owned by the right userns and thus
the capabilities would not be correct.

The current restart logic tries to reuse the netns which is fine if no
userns is used but when one is used we setup a new netns (which is
correct) but forgot to cleanup the old netns. This resulted in leaked
network namespaces and because no teardown was ever called leaked ipam
assignments, thus a quickly restarting container will run out of ip
space very fast.

Fixes containers#18615

Signed-off-by: Paul Holzinger <[email protected]>
@openshift-ci openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 17, 2023
@mheon
Copy link
Member

mheon commented Oct 17, 2023

Do we need to do this in the typical restart codepath, node just restart policy handling?

@Luap99
Copy link
Member Author

Luap99 commented Oct 17, 2023

The typical restart does not preserve the netns, see 6d487c7

@rhatdan
Copy link
Member

rhatdan commented Oct 17, 2023

LGTM

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

fi

local net1=a-$(random_string 10)
# use /29 subnet to limt available ip space, a 29 gives 5 useable addresses (6 - 1 for the gw)
Copy link
Member

Choose a reason for hiding this comment

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

Nits

Suggested change
# use /29 subnet to limt available ip space, a 29 gives 5 useable addresses (6 - 1 for the gw)
# use /29 subnet to limit available ip space, a 29 gives 5 usable addresses (6 - 1 for the gw)

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 18, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 18, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, vrothberg

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 merged commit 6624ccb into containers:main Oct 18, 2023
98 checks passed
@Luap99 Luap99 deleted the double-netns branch October 18, 2023 08:45
@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 Jan 17, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 17, 2024
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. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bad container image consuming all network IP addresses when using userns=keep-id
4 participants