rootless netns: resolve all path components for resolv.conf #12524
Conversation
|
[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
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
We need to follow all symlinks in the /etc/resolv.conf path. Currently we would only check the last file but it is possible that any directory before that is also a link. Unfortunately this code is very hard to maintain and not well tested. I will try to come up with a unit test when I have more time. I think we could utilize some for of chroot for this. For now we are stucked with the default setup in the fedora/ubunutu test VMs. [NO NEW TESTS NEEDED] Fixes containers#12461 Signed-off-by: Paul Holzinger <[email protected]>
Luap99
force-pushed
the
resolve-symlink
branch
from
861a93e to
3753347
Compare
| // We also need to resolve all path components not just the last file. | ||
| // see https://github.com/containers/podman/issues/12461 | ||
|
|
||
| if resolvePath[i] != '/' { |
There was a problem hiding this comment.
Can't we strip any potential trailing / and then do a filepath.Dir()?
There was a problem hiding this comment.
That would be the wrong order. You have to resolve from left to right.
There was a problem hiding this comment.
Maybe split the string on / to get an array of path components, and then iterate through them, adding each to the path and checking for a symlink?
There was a problem hiding this comment.
I do not see how this would be better than what I am doing now. This would just complicate things because I now have to constantly use strings.Split() and strings.Join().
There was a problem hiding this comment.
Fair - I just see the existing code as not exactly clear in what it's trying to do.
There was a problem hiding this comment.
If I have more time after podman 4.0 I will spend some time to add a test for this and maybe rework this to make it more readable.
In theory we could drop this when we are full netavark without iptables backend. In this case we would not need to bind mount /run and /var/lib/cni because netavark does not need write access to those directories.
|
LGTM |
|
/lgtm |
|
Oh well. I was trying to figure out what would happen if |
|
@edsantiago That works already this is the default setup on fedora with systemd-resolved |
github-actions
bot
added
the
locked - please file new issue/PR
What this PR does / why we need it:
We need to follow all symlinks in the /etc/resolv.conf path. Currently
we would only check the last file but it is possible that any directory
before that is also a link.
Unfortunately this code is very hard to maintain and not well tested. I
will try to come up with a unit test when I have more time. I think we
could utilize some for of chroot for this. For now we are stucked with
the default setup in the fedora/ubunutu test VMs.
How to verify it
Which issue(s) this PR fixes:
Fixes #12461
Special notes for your reviewer: