Improve the dhclient hook for OCI compat#2013
Improve the dhclient hook for OCI compat#2013brauner merged 4 commits intolxc:masterfrom 3XX0:oci-dhcp-improvements
Conversation
|
This pull request didn't trigger Jenkins as its author isn't in the whitelist. An organization member must perform one of the following:
Those commands are simple Github comments of the format: "jenkins: COMMAND" |
hooks/dhclient.in
Outdated
| rm -f "${pidfile}" | ||
| } | ||
|
|
||
| if [ "$#" -lt 3 ] || [ "$2" != "lxc" ]; then |
There was a problem hiding this comment.
I'm using the same test in my script, I would be interested to know if you think that makes sense and if it's safe to have (e.g.: are we expecting new arguments to be passed to hooks in the near-future?)
| fi | ||
|
|
||
| if [ -e "${LXC_TEMPLATE_CONFIG}/oci.common.conf" ]; then | ||
| echo "lxc.include = ${LXC_TEMPLATE_CONFIG}/oci.common.conf" >> "${LXC_CONF_FILE}" |
There was a problem hiding this comment.
Should we push oci.common.conf.in in the same commit? Even if it only contains commented hooks for now, e.g.:
# Uncomment if you want to use dhclient...
#lxc.hook.start-host = @LXCHOOKDIR@/dhclient
#lxc.hook.stop = @LXCHOOKDIR@/dhclient
|
Regarding the hook args, quite the opposite - we are hoping to stop sending them as argv and just send them as environment variables. |
|
--dhcp option would be nice. Can it just double-check whether the dhcp hook is already set in the config when the template is called to avoid double-setting it? |
|
See #2026 . |
|
@brauner I updated the hook to work with your PR |
|
Labeling this as "blocked" until #2026 is merged. :) |
brauner
added
Incomplete
brauner
left a comment
brauner
left a comment
There was a problem hiding this comment.
Nice. I just left a comment about attaching to the user namespace. I think the logic needs to change there. Otherwise this lgtm.
hooks/dhclient.in
Outdated
| echo "WARN: DHCP client is already running, skipping start hook." >> "${debugfile}" | ||
| else | ||
| echo "INFO: Starting DHCP client and acquiring a lease..." >> "${debugfile}" | ||
| nsenter --uts --user --net --target "${LXC_PID}" -- \ |
There was a problem hiding this comment.
So I think this won't work unconditionally. The problem is that with --user the host and container need to be in different user namespaces. If they are not this will fail with EINVAL. I think what you want to do is look whether LXC_USER_NS is set if LXC_HOOK_VERSION is set to 1 or if user:/proc/<pid>/fd/<nr> is passed as argument when LXC_HOOK_VERSION is set to 0 or not set at all.
There was a problem hiding this comment.
Right, good catch. Shouldn't we do that for every namespace though since the user could have specified a --share-<ns>?
There was a problem hiding this comment.
If I remember correctly, then only user namespaces have this restriction. The other ones should be fine. Was that your concern or did I misunderstand?
(Fwiw, since I introduced lxc.namespace.[namespace identifier] users can actually even specify this in their config.)
There was a problem hiding this comment.
It works for the stop hook but what about the start-host hook? Do I have to check the userns proc inodes or there is a better way to know whether the userns is shared or not?
There was a problem hiding this comment.
One way to do it would be to compare:
readlink /proc/${LXD_PID}/ns/user == readlink /proc/self/ns/user
There was a problem hiding this comment.
Since by the time the start-hook runs we already know the init pid of the container and we make it available in LXC_PID.
There was a problem hiding this comment.
Oh, and the LXC_PID is guaranteed to be valid until after the start-host hook is run since we place a barrier right after it. Only after the start-hooks are run is the the child allowed to exec*().
hooks/dhclient.in
Outdated
| done | ||
| else | ||
| ns_args+=("--uts=${LXC_UTS_NS}") | ||
| ns_args+=("--user=${LXC_USER_NS}") |
There was a problem hiding this comment.
Same comment applies here: So I think this won't work unconditionally. The problem is that with --user the host and container need to be in different user namespaces. If they are not this will fail with EINVAL. I think what you want to do is look whether LXC_USER_NS is set if LXC_HOOK_VERSION is set to 1 or if user:/proc/<pid>/fd/<nr> is passed as argument when LXC_HOOK_VERSION is set to 0 or not set at all.
- Merge dhclient-start and dhclient-stop into a single hook. - Wait for a lease before returning from the hook. - Generate a logfile when LXC log level is either DEBUG or TRACE. - Rely on namespace file descriptors for the stop hook. - Use settings from /<sysconf>/lxc/dhclient.conf if available. - Attempt to cleanup if dhclient fails to shutdown properly. Signed-off-by: Jonathan Calmels <jcalmels@nvidia.com>
Excerpt from dnsmasq(8): By default, the DHCP server will attempt to ensure that an address in not in use before allocating it to a host. It does this by sending an ICMP echo request (aka "ping") to the address in question. If it gets a reply, then the address must already be in use, and another is tried. This flag disables this check. This is useful if one expects all the containers to get an IP address from the LXC authoritative DHCP server and wants to speed up the process of getting a lease. Signed-off-by: Jonathan Calmels <jcalmels@nvidia.com>
Signed-off-by: Jonathan Calmels <jcalmels@nvidia.com>
Signed-off-by: Jonathan Calmels <jcalmels@nvidia.com>
|
|
jenkins: test this please |
5 participants
Now it becomes:
@stgraber @brauner Do you have any suggestion to address the LSM problems?
Most distribution protect
dhclient, thus it won't have permissions to read or write config/lease/pid files. Should we write Apparmor/SELinux profile overrides or let the users figure it out (e.g. document what to do in/etc/apparmor.d/local/sbin.dhclient)?@hallyn Seems like the hook namespace fds got fixed so I was able to leverage it.
Right now I use
oci.common.confto setup the hooks for all my OCI containers, should we add a--dhcpoption to the OCI template as well? It might be problematic if both are set though, what do you think?