-
Notifications
You must be signed in to change notification settings - Fork 26
coreos-teardown-initramfs-network: propagate hostname, support coreos.no_persist_ip #174
coreos-teardown-initramfs-network: propagate hostname, support coreos.no_persist_ip #174
Conversation
47ab606 to
7d5b9e5
Compare
|
added two more small commits:
|
darkmuggle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
|
||
| # Use tmpfiles.d to schedule SELinux relabel of files | ||
| schedule_selinux_relabel() { | ||
| echo "Z $1 - - -" >> "/run/tmpfiles.d/$(basename $0)-relabel.conf" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use https://github.com/coreos/fedora-coreos-config/blob/testing-devel/overlay.d/05core/usr/lib/dracut/modules.d/40ignition-ostree/coreos-relabel instead so we relabel from the initrd on FCOS?
It's awkward since it's in a different repo. But really everything in this repo that's not strictly about Ignition should be moved to overlays eventually and then this repo merged into https://github.com/coreos/ignition.
(And eventually RHEL8 will also get the patch for relabeling from the initrd, so it'll do the better thing on RHCOS too.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally was using coreos-relabel but I didn't want to introduce another dep here in this repo. I agree we should eventually move this into an overlay or something because it's really blurring the line between what should be in ignition-dracut vs in an overlay.
Maybe we should leave as and if we move it to the overlay switch it to coreos-relabel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, though... a lot of work went into not having to do these hacks anymore. 😢
How about just in this function do something like command -v coreos-relabel and then fallback to what you're doing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
| # -*- mode: shell-script; indent-tabs-mode: nil; sh-basic-offset: 4; -*- | ||
| # ex: ts=8 sw=4 sts=4 et filetype=sh | ||
|
|
||
| set -euo pipefail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ouuff. What issues were you hitting with set -euo pipefail left on? Can we just turn it on after importing the library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unset variables, commands that error, etc. We'd have to turn it off/on when we import the library and also each time we called a function in the library. Believe me I started doing it that way but declared bankruptcy. It's ugly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh. Hmm, one thing we could do is have a wrapper function like:
function dracut_fn() {
set +euo pipefail
"$@"
set -euo pipefail
}then call it as e.g. dracut_fn ip_to_var ....
Anyway, IMO worth trying something along those lines if it works to maintain our sanity, but will leave it up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok added a wrapper function!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have this dream that someday 30 years from now, something will win and software engineers will see shell script like we see COBOL...
7d5b9e5 to
657eacd
Compare
The files in the real root ended up with unlabled_t file context. I'm suprised we didn't see any issues with this before. Let's fix it now.
Not skipping it means that we get an extra error message in our output during bootup. Let's skip it.
…for RHCOS For RHCOS we still support ifcfg files so let's consider the ifcfg directory (/sysroot/etc/sysconfig/network-scripts/) when checking to see if a user has provided networking configuration via Ignition.
Add coreos.no_persist_ip to allow for explicitly disabling the network config propagation. This is supported on the spec2x branch of ignition-dracut (i.e. for RHCOS) right now so we might as well have it in the master branch so we can keep the maintenance burden lower. Note that this adds two new functions load_dracut_libs() and dracut_func(). We needed to create these new functions in order to wrap in set +/- euo pipefail because dracut was written with set -eu in mind and there are unset variables and commands that fail in a lot of places.
This is a forward port of 1f8184e (coreos#156). If the admin specified a hostname in the ip= karg static networking config and no hostname was specified via Ignition then we'll make sure the hostname from the ip= karg gets applied to the real root (persistently). Ideally in the future there will be better support for this in NetworkManager itself as the `network-legacy` dracut module did at least provide more support for setting the hostname via ip= kargs than `network-manager` currently does. The discussion about this problem is in [1]. The fix for that will most likely implicate changes to the propagation code introduced here. Fixes: coreos/fedora-coreos-tracker#466 [1] https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/419
657eacd to
d6805df
Compare
|
ok i've addressed all code review comments I believe |
jlebon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
| type getargbool &>/dev/null || . /lib/dracut-lib.sh | ||
| type ip_to_var &>/dev/null || . /lib/net-lib.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, it's totally fine IMO to just source these upfront and only doing our set -euo pipefail afterwards. This works fine too though of course.
| ) | ||
| hostname=$(<$hostnamefile); rm $hostnamefile | ||
| if [ -n "$hostname" ]; then | ||
| echo "info: propagating initramfs hostname (${hostname}) to the real root" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally there'd be some way for us to note that the "origin" of /etc/hostname was this beyond just this journal message that will rotate out eventually. I guess we could use an xattr but...eh.
Not important.
cgwalters
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me!
|
testing looks good! 🚂 |
This is a backport of the merge commits f67d587 (coreos#159), and 390779d (coreos#174). The upstream commits persisted networking configuration and hostname as part of the teardown service so we also remove `persist-ifcfg.sh`.
This gets us new networking improvements added in coreos/ignition-dracut#174.
This gets us new networking improvements added in coreos/ignition-dracut#174.
This gets us new networking improvements added in coreos/ignition-dracut#174.
There are several improvements to the coreos-teardown-initramfs-network script in this patchset. The most significant are: support for propagating the hostname to the real root if it was provided via static networking ip= kargs and also adding support for the coreos.no_persist_ip karg. Please see each commit for the full details on each change.