Auto-detect DHCP client with systemd-networkd (closes #2375)

[OliverO2]

Pull Request Details:

• Type: Enhancement

• Impact: Normal

• Reference to related issue (URL): Add auto-detection of DHCP client with systemd-networkd #2375

• How was this pull request tested? On Ubuntu 18.04.4 LTS

• Brief description of the changes in this pull request: See issue.

[jsmeix]

I cannot really review it because I know nothing about
how that things work on Ubuntu (in general am not a Ubuntu user
and in particulare I am neither a DHCP nor a systemd expert)
but as far as I see this is purely additional code
so there cannot be regressions for existing use cases and
the additional code looks good to me from plain looking at it
so that I can "almost blindly" approve it in particular
because in general code from @OliverO2 is "just right".

[jsmeix]

I would appreciate it if another ReaR maintainer
could additionally have a look and also approve it
or request changes if needed.

[OliverO2]

After doing some more testing with overrides, I found a bug: The present code will accept an enabled DHCP client on any network connection. However, with Docker there may be additional network connections configured with DHCP enabled. These should be ignored. Fix upcoming.

[jsmeix]

@OliverO2
while you are working on that file could you
by the way also improve

COPY_AS_IS=( "${COPY_AS_IS[@]}" "/etc/localtime" "/usr/lib/dhcpcd/*" )
PROGS=( "${PROGS[@]}" arping ipcalc usleep "${dhclients[@]}" )

to

COPY_AS_IS+=( "/etc/localtime" "/usr/lib/dhcpcd/*" )
PROGS+=( arping ipcalc usleep "${dhclients[@]}" )

cf. #2364

[OliverO2]

@jsmeix
Sure, also dhclients+=("$x") in line 19, I guess...

[jsmeix]

@OliverO2
yes of course!
You know my current regexp doesn't find all ;-)
By the way: I am finished with what my current regexp found :-)

[OliverO2]

@jsmeix

Done. I wanted to clean up the entire dhclients variable setting but didn't dare to: I'm still not sure what dhclients changes in array/string type across the file are all about.

For me it looks like that the define_dhclients_variable function could be replaced by something like

define_dhclients_variable() {
    dhclients=$(printf "%s\n" "${DHCLIENT_BIN##*/}" "${DHCLIENT6_BIN##*/}" dhcpcd dhclient dhcp6c dhclient6 | grep . | sort -u)
}

[jsmeix]

I think @gdha is our DHCP expert here ;-)

[jsmeix]

Ouch! It seems this issue gets nasty because
it looks as if a more general cleanup is needed.

As fas as I see now it seems the dhclients variable
is used inconsistently both as an array and as a string
according to those code excerpts:

define_dhclients_variable()
{   ...
    dhclients=()
    ...
        for d in "${dhclients[@]}" ; do
        ...
        dhclients+=("$x")
    ...
    dhclients="${dhclients[*]}"
}

...

    for x in ${dhclients} ; do
...
PROGS+=( arping ipcalc usleep "${dhclients[@]}" )

@OliverO2
I know this messed up dhclients variable usage is not your fault.
But now I noticed it and because you are working on that code
I would very much appreciate it if you could also
clean up the messed up dhclients variable usage
to be consistently either an array or a string.

Or do I perhaps somehow misunderstand the current
dhclients variable usage and actually it is ok?
Even if it is technically ok at least to me its usage
looks weird.

[OliverO2]

@jsmeix
Yes, these type changes between string and array are what I was wondering about. I'll come up with a commit trying to clean up that dhclient stuff.

[OliverO2]

@jsmeix The old code was a bit too messy for my taste that I re-wrote it to what I think was really intended. Also added a few checks which were not there originally. See commit remarks and code.

What still remains: The distinction between DHCLIENT_BIN and DHCLIENT6_BIN is not really necessary as even the code picking up these variables in 58-start-dhclient.sh does not really need it. However, these variables (although still undocumented in default.conf) might be present in some user's local configuration files.

[jsmeix]

According to

localhost:~/rear.github.master # find . -type f | xargs grep -l 'DHCLIENT_BIN'

./usr/share/rear/skel/default/etc/scripts/system-setup.d/58-start-dhclient.sh
./usr/share/rear/prep/GNU/Linux/210_include_dhclient.sh

DHCLIENT_BIN and DHCLIENT6_BIN are internal variables.

In general what is not documented in default.conf
is not meant as a user config variable.

We have tons of internal variables with uppercase names
that are not meant as user config variables,
cf. the section "Variables" in
https://github.com/rear/rear/wiki/Coding-Style

All variables that are used in more than a single script must be all-caps:
$FOO instead of $foo or $Foo.

Of course we also have globally used variables
that do not have uppercase names e.g. backuparchive:

# find usr/sbin/rear usr/share/rear -type f | xargs egrep -l 'backuparchive=|\$backuparchive' | wc -l
18

:-(

[OliverO2]

@jsmeix The problem is that configuring DHCLIENT[6]_BIN in local.conf was suggested in comments of 210_include_dhclient.sh before. Comments and actual code were also not consistent as the comments suggested one could define dhclient binaries beyond what was included in the known clients list. However, 58-start-dhclient.sh could not deal with such an unknown client.

[jsmeix]

@OliverO2
personally I would say feel free to clean it up mercilessly
to make things straightforward and consistent.

Personally I was too often completely confused
with the DHCP setup code so I would so much appreciate
a generic cleanup in this area.

With a more straightforward code we could much more
reliably maintain that code in the future.

@gdha
what do you think about a generic cleanup of the DHCP setup code?

[jsmeix]

@OliverO2
in particular you can safely remove all what is not supported by
skel/default/etc/scripts/system-setup.d/58-start-dhclient.sh
because no user reported that something is missing in this area
which indicates that no user uses more than what is supported by
skel/default/etc/scripts/system-setup.d/58-start-dhclient.sh

[gdha]

@OliverO2 @jsmeix the dhcp code was an ugly copy/paste from ?? (old RH code I guess) and does indeed require a serious facelift.

[OliverO2]

@gdha @jsmeix OK, so I'll clean up the last bits and then ask you to have a final look.

[OliverO2]

@gdha @jsmeix

• Did the final cleanup (as far as I am concerned).
• Tested on Ubuntu 18.04.4 LTS Desktiop and Server edition.

Over to you!

[jsmeix]

This test does not work:

# dhcp_clients=(dhcpcd dhclient dhcp6c dhclient6)

# for DHCLIENT_BIN in ${dhcp_clients[*]} ; do [[ "${dhcp_clients[*]}" == *" $DHCLIENT_BIN "* ]] || echo "DHCLIENT_BIN='$DHCLIENT_BIN' is not among known DHCP clients (${dhcp_clients[*]})" ; done

DHCLIENT_BIN='dhcpcd' is not among known DHCP clients (dhcpcd dhclient dhcp6c dhclient6)
DHCLIENT_BIN='dhclient6' is not among known DHCP clients (dhcpcd dhclient dhcp6c dhclient6)

because it tests with leading and trailing blanks which is magic characters coding.
I suggest the more straifgtforward test:

IsInArray "$DHCLIENT_BIN" "${dhcp_clients[@]}" || Error "..."

[OliverO2]

You are right, I forgot to add blanks: [[ " ${dhcp_clients[*]} " == *" $DHCLIENT_BIN "* ]]

Anyway, I've changed it to use IsInArray. Occasionally, I miss a rear library function because I'm just a casual user and there would be so many lib sources to look at. In this case, I just didn't remember. However, I may deliberately choose not to use a lib function if it does not add sufficient value to compensate a major downside: These functions make manual unit testing harder. I tend to write test beds like this and it can be hard to pull in ReaR lib files due to dependency hell:

#!/bin/bash

. usr/share/rear/lib/global-functions.sh

function has_binary () {
    for bin in $@ ; do
        # Suppress success output via stdout which is crucial when has_binary is called
        # in other functions that provide their intended function results via stdout
        # to not pollute intended function results with intermixed has_binary stdout
        # (e.g. the RequiredSharedObjects function) but keep failure output via stderr:
        type $bin 1>/dev/null && return 0
    done
    return 1
}

function IsInArray() {
    element="$1"
    shift
    [[ " $* " == *" $element "* ]]
}

function Log() {
    echo "Log: $*"
}

function Error() {
    echo "Error: $*"
    exit 1
}

function test() {
    . usr/share/rear/prep/GNU/Linux/210_include_dhclient.sh
}

#set -x

ROOTFS_DIR=.

test

echo "$ROOTFS_DIR/etc/rear/rescue.conf:"
cat "$ROOTFS_DIR/etc/rear/rescue.conf"
rm "$ROOTFS_DIR/etc/rear/rescue.conf"

[jsmeix]

Now all looks OK to me from plain looking at the code so I approve it.

@rear/contributors
I would appreciate it if another ReaR maintainer could also have a look
and either also approve it or request changes.

If there are no objections I would like to merge it tomorrow afternoon.

@OliverO2
thank you very much for your continuous improvements of ReaR!

[OliverO2]

@jsmeix
Thanks for your thorough review. Always appreciated.