install: add --copy-network and --network-dir options #212

dustymabe · 2020-04-14T04:46:57Z

This will allow us to copy in networking configuration from a specified
location into a predetermined directory inside of /boot/ of the
installed system (which follows the path of other postprocess steps).
Having this mechanism will allow for a user to interactively configure
networking in the Live ISO environment via nmtui and then have those
settings propagate forward into the installed system.

After the files are copied into the directory within /boot/ a systemd
unit from the initramfs will pick them up on firstboot and propagate
them into the appropriate locations.

Fixes: #205

dustymabe · 2020-04-14T04:48:36Z

This is my first ever rust PR. It took me waaaay too long to get this code working 😜. Please be gentle in code review.

marking as WIP for now as I test things out more

src/install.rs

dustymabe · 2020-04-14T14:35:53Z

src/cmdline.rs

+                        .short("n")
+                        .long("copy-network-config")
+                        .value_name("path")
+                        .default_value("/etc/NetworkManager/system-connections/")


This default value will likely need to be different for RHCOS (for now). I'm thinking we can just carry that as a patch in the RPM in downstream. Does that sound OK?

We also do want to copy that path for RHCOS too. So it's more like disabling ifcfg copying on FCOS. We could do something like look at the target OS install and conditionalize based on that...since we do want to allow people to use the upstream/Fedora coreos-installer binary to install RHCOS.

Or to rephrase, FCOS supports NM config files; RHCOS supports NM config files and ifcfg files. It's not "RHCOS only supports ifcfg files".

Perhaps simplest is just to make this a boolean value, and copy files from both paths if they exist and the option is specified.

We also do want to copy that path for RHCOS too. So it's more like disabling ifcfg copying on FCOS. We could do something like look at the target OS install and conditionalize based on that...since we do want to allow people to use the upstream/Fedora coreos-installer binary to install RHCOS.

Using the FCOS installer to install RHCOS is probably a little risky long term, though for this case the user could still pass -n /etc/sysconfig/network-scripts/ and get it to work.

Or to rephrase, FCOS supports NM config files; RHCOS supports NM config files and ifcfg files. It's not "RHCOS only supports ifcfg files".

Perhaps simplest is just to make this a boolean value, and copy files from both paths if they exist and the option is specified.

Since the use case here is to propagate forward the files generated by nmtui I think we should support the default format that is spit out by nmtui on each platform and not overcomplicate things.

Since the use case here is to propagate forward the files generated by nmtui I think we should support the default format that is spit out by nmtui on each platform and not overcomplicate things.

Hmm. I suppose you're right that NM will save in ifcfg files by default on RHCOS. I wonder how hard that'd be to change though...i.e. make ifcfg files a read source but not a write target.

i.e. make ifcfg files a read source but not a write target.

+1, that's what it does for nm keyfiles today IIUC. I'd have to dig into the plugin to see if it's possible. For now I'm going to stick with this and I'll come back and change it if I have enough time between now and this Friday.

opened this against NM upstream to see if there's any way to do what we want: https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/415

cgwalters · 2020-04-14T14:55:04Z

src/install.rs

@@ -233,6 +236,53 @@ fn write_platform(mountpoint: &Path, platform: &str) -> Result<()> {
    Ok(())
 }

+/// Copy networking config if asked to do so
+fn copy_network_config(mountpoint: &Path, net_config_src: &str) -> Result<()> {
+    eprintln!("Copying networking configuration from {}", net_config_src);


Since this isn't an error, let's use println!.

will look into this. Thanks for the suggestion.

Nope, all of our existing status logging goes to stderr.

So I'll leave this as is in this PR, but it's probably worth us having a followup where we convert our non-error status logging to non stderr, right?

Why? Status reports aren't the primary output of the program, so it makes sense to me that they would go to stderr.

I don't agree. If we ever add e.g. a JSON report describing what actions install has taken, that would go to stdout for piping to another process, and the status reports would need to not be commingled with it.

@lucab, thoughts?

I don't agree. If we ever add e.g. a JSON report describing what actions install has taken, that would go to stdout for piping to another process, and the status reports would need to not be commingled with it.

Sounds like a good use case for a --quiet.

Either way maybe we should move this conversation into an issue since it no longer really reflects a change we are going to make in this PR.

follow up: #214

I don't agree. If we ever add e.g. a JSON report describing what actions install has taken, that would go to stdout for piping to another process, and the status reports would need to not be commingled with it.

What you're talking about is one of the most long-standing things in how Unix pipelines work - the fact they're just connected by bytestreams and there's no common APIs or structure. A lot of successor projects try to address this, from Windows Powershell to nushell to many others. (In fact long ago I tried to write one myself...)

Practically speaking, I think it's better to do e.g. --status-report-json /path/to/status.json. Or alternatively --output=json and that also suppresses internal printing.

src/install.rs

cgwalters · 2020-04-14T15:44:50Z

This will allow us to copy in networking configuration from a specified
location into a predetermined directory inside of /boot/ of the
installed system

Another approach would be to convert the files into an Ignition config that gets merged with any other config provided. Something like having Ignition support a /boot/config.ign.d directory instead of just /boot/config.ign, and then we drop a /boot/config.ign.d/coreos-installer.ign?

dustymabe · 2020-04-14T18:09:49Z

This will allow us to copy in networking configuration from a specified
location into a predetermined directory inside of /boot/ of the
installed system

Another approach would be to convert the files into an Ignition config that gets merged with any other config provided. Something like having Ignition support a /boot/config.ign.d directory instead of just /boot/config.ign, and then we drop a /boot/config.ign.d/coreos-installer.ign?

We discussed this a bit before I think: #205 (comment). We need the networking to be brought up in the initramfs.

dustymabe · 2020-04-15T03:30:36Z

The systemd unit/script that will pick up these networking config files placed by coreos-installer is in coreos/fedora-coreos-config#346

dustymabe · 2020-04-15T03:31:39Z

I'll get to the code review comments/suggestions in this PR first thing tomorrow. I'll be doing a good dose of testing as well.

bgilbert · 2020-04-15T08:28:53Z

src/cmdline.rs

+                        .value_name("path")
+                        .default_value("/etc/NetworkManager/system-connections/")
+                        .takes_value(true)
+                        .help("Optionally copy the network config from <path> in the installer environment"),


This breaks the 80-column limit on readable help text. Copy network config from <path> almost fixes it, but the option description (--copy-network-config <path>) is so wide that it shifts other options' help text to the right.

Which intersects with another issue: I don't think the single option is going to work. coreos-installer install -n /dev/loop0 and coreos-installer install --copy-network-config /dev/loop0 both misinterpret /dev/loop0 as the path to the network configs. Without a more discerning parser, I think we should split this into two options: a boolean, and an option to override the default path.

Dropping <path> from the option description would fix the help text length without needing to rename the option, though it then moves the problem to choosing the name of the second option.

I know the character-counting is painful but we don't really get usable help text otherwise. We could enable the wrap_help clap feature but it's... only slightly less bad:

Install Fedora CoreOS or RHEL CoreOS USAGE: coreos-installer install [OPTIONS] <device> OPTIONS: -s, --stream <name> Fedora CoreOS stream [default: stable] -u, --image-url <URL> Manually specify the image URL -f, --image-file <path> Manually specify a local image file -i, --ignition-file <path> Embed an Ignition config from a file -p, --platform <name> Override the Ignition platform ID --firstboot-args <args> Additional kernel args for the first boot -n, --copy-network-config <path> Optionally copy the network config from <path> in the installer environment [default: /etc/NetworkManager/system-connections/] --insecure Skip signature verification --stream-base-url <URL> Base URL for Fedora CoreOS stream metadata --architecture <name> Target CPU architecture [default: x86_64] --preserve-on-error Don't clear partition table on error -h, --help Prints help information ARGS: <device> Destination device

This breaks the 80-column limit on readable help text. Copy network config from <path> almost fixes it, but the option description (--copy-network-config <path>) is so wide that it shifts other options' help text to the right.

Which intersects with another issue: I don't think the single option is going to work. coreos-installer install -n /dev/loop0 and coreos-installer install --copy-network-config /dev/loop0 both misinterpret /dev/loop0 as the path to the network configs. Without a more discerning parser, I think we should split this into two options: a boolean, and an option to override the default path.

I'll work on adding the second option.

Dropping <path> from the option description would fix the help text length without needing to rename the option, though it then moves the problem to choosing the name of the second option.

I'll try to get the text to under 80 characters.

I know the character-counting is painful but we don't really get usable help text otherwise. We could enable the wrap_help clap feature but it's... only slightly less bad:

On a console readability within 80 characters in important.

The closest I could get was changing the long form of -n to --copy-network, adding a --network-dir option and using .next_line_help(true). Here's what we end up with, which is less than 80 characters and I think a good compromise:

Install Fedora CoreOS or RHEL CoreOS USAGE: coreos-installer install [OPTIONS] <device> OPTIONS: -s, --stream <name> Fedora CoreOS stream [default: stable] -u, --image-url <URL> Manually specify the image URL -f, --image-file <path> Manually specify a local image file -i, --ignition-file <path> Embed an Ignition config from a file -p, --platform <name> Override the Ignition platform ID --firstboot-args <args> Additional kernel args for the first boot -n, --copy-network Copy network config from install environment --network-dir <path> For use with -n. [default: /etc/NetworkManager/system-connections/] --insecure Skip signature verification --stream-base-url <URL> Base URL for Fedora CoreOS stream metadata --architecture <name> Target CPU architecture [default: x86_64] --preserve-on-error Don't clear partition table on error -h, --help Prints help information ARGS: <device> Destination device

We could avoid the second line with e.g. Override default network config path and hide_default_value(true).

I do think showing the user the default is important.

src/install.rs

This will allow us to copy in networking configuration from a specified location into a predetermined directory inside of `/boot/` of the installed system (which follows the path of other postprocess steps). Having this mechanism will allow for a user to interactively configure networking in the Live ISO environment via `nmtui` and then have those settings propagate forward into the installed system. After the files are copied into the directory within `/boot/` a systemd unit from the initramfs will pick them up on firstboot and propagate them into the appropriate locations. Fixes: coreos#205

dustymabe · 2020-04-15T15:34:19Z

based on #212 (comment) I changed this to install: add --copy-network and --network-dir options.

bgilbert

I'm not entirely happy with the help text, but we can always change it later. The substance LGTM!

dustymabe changed the title ~~install: add --copy-network-config option~~ [WIP] install: add --copy-network-config option Apr 14, 2020

dustymabe force-pushed the dusty-copy-network branch from f8cff4b to 97f4560 Compare April 14, 2020 04:52

dustymabe commented Apr 14, 2020

View reviewed changes

src/install.rs Outdated Show resolved Hide resolved

dustymabe commented Apr 14, 2020

View reviewed changes

cgwalters reviewed Apr 14, 2020

View reviewed changes

dustymabe mentioned this pull request Apr 15, 2020

overlay: add 15copy-installer-network dracut module coreos/fedora-coreos-config#346

Merged

bgilbert reviewed Apr 15, 2020

View reviewed changes

dustymabe force-pushed the dusty-copy-network branch from 97f4560 to 29e22ef Compare April 15, 2020 15:29

dustymabe force-pushed the dusty-copy-network branch from 29e22ef to df06c7b Compare April 15, 2020 15:32

dustymabe changed the title ~~[WIP] install: add --copy-network-config option~~ install: add --copy-network and --network-dir options Apr 15, 2020

dustymabe marked this pull request as ready for review April 15, 2020 15:34

bgilbert approved these changes Apr 15, 2020

View reviewed changes

dustymabe mentioned this pull request Apr 15, 2020

default to stderr or stdout for log/print messages #214

Open

dustymabe merged commit 6ae8145 into coreos:master Apr 15, 2020

dustymabe deleted the dusty-copy-network branch May 8, 2020 14:12

jlebon mentioned this pull request Jun 17, 2020

Support disabling initramfs networking via Ignition coreos/ignition#979

Closed

hardys mentioned this pull request Jul 3, 2020

Discuss host network configuration interfaces openshift/enhancements#394

Merged

jlebon mentioned this pull request Dec 3, 2020

Initramfs network configuration coreos/fedora-coreos-tracker#460

Closed

jlebon mentioned this pull request Jul 15, 2021

Add testiso scenario covering --copy-network path coreos/coreos-assembler#1987

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

install: add --copy-network and --network-dir options #212

install: add --copy-network and --network-dir options #212

dustymabe commented Apr 14, 2020

dustymabe commented Apr 14, 2020

dustymabe Apr 14, 2020

cgwalters Apr 14, 2020

cgwalters Apr 14, 2020

dustymabe Apr 14, 2020

cgwalters Apr 14, 2020

dustymabe Apr 14, 2020

dustymabe Apr 21, 2020

cgwalters Apr 14, 2020

dustymabe Apr 14, 2020

bgilbert Apr 15, 2020

dustymabe Apr 15, 2020

bgilbert Apr 15, 2020

bgilbert Apr 15, 2020

dustymabe Apr 15, 2020

dustymabe Apr 15, 2020 •

edited

Loading

dustymabe Apr 15, 2020

cgwalters Apr 15, 2020

cgwalters commented Apr 14, 2020

dustymabe commented Apr 14, 2020

dustymabe commented Apr 15, 2020

dustymabe commented Apr 15, 2020

bgilbert Apr 15, 2020

dustymabe Apr 15, 2020

dustymabe Apr 15, 2020

bgilbert Apr 15, 2020

dustymabe Apr 15, 2020

dustymabe commented Apr 15, 2020

bgilbert left a comment

install: add --copy-network and --network-dir options #212

install: add --copy-network and --network-dir options #212

Conversation

dustymabe commented Apr 14, 2020

dustymabe commented Apr 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustymabe Apr 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters commented Apr 14, 2020

dustymabe commented Apr 14, 2020

dustymabe commented Apr 15, 2020

dustymabe commented Apr 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustymabe commented Apr 15, 2020

bgilbert left a comment

Choose a reason for hiding this comment

dustymabe Apr 15, 2020 •

edited

Loading