libnetwork/netavark: dealloc ips on setup error #2341
Conversation
While we already did clean up the allocated ips on the regular setup error this did not worked for errors from the rootlessNetns.Setup() call. To ensure we dealloc the ips on all error paths use a defer function that checks the return error value. Fixes containers/podman#25422 Signed-off-by: Paul Holzinger <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
LGTM |
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
LGTM going just by the structure of the one function, with zero understanding of libnetwork in general.
The question is almost certainly non-blocking, but I figured I’ll let the experts discuss.
| return n.execNetavark([]string{"setup", namespacePath}, needPlugin, netavarkOpts, &result) | ||
| } | ||
|
|
||
| if n.rootlessNetns != nil { |
There was a problem hiding this comment.
Looking at the len(result) != len(options.Networks) failure — deallocating the IP addresses without doing anything else might be wrong?
But, also, is there anything at all we can reliably do in such a situation?
There was a problem hiding this comment.
The nice thing about kernel network namespaces is if you delete it it removes all virtual interfaces (i.e. veth/bridge, etc... from it). As such it is safe to assume once podman unmounts the netns path the ip is free to reuse.
(There would still be small race here I suppose until podman unmounts it)
Netavark is written in a way that it should tear down everything cleanly on any error it encounters to not leak interfaces/firewall rules. That of course will not apply to that exact condition because netavark itslef didn't return an error.
The check is more of a it cannot happen thing, if it does happen we made a major mistake in netavark and hopefully CI would notice way before any users do. So I don't think there is anything we can do and deallocating is fine
|
/hold |
|
/hold cancel |
4 participants
While we already did clean up the allocated ips on the regular setup error this did not worked for errors from the rootlessNetns.Setup() call. To ensure we dealloc the ips on all error paths use a defer function that checks the return error value.
Fixes containers/podman#25422