install: add --delete-karg and --append-karg

src/cmdline.rs

@@ -43,6 +43,8 @@ pub struct InstallConfig {
     pub ignition_hash: Option<IgnitionHash>,
     pub platform: Option<String>,
     pub firstboot_kargs: Option<String>,
+    pub append_kargs: Option<Vec<String>>,
+    pub delete_kargs: Option<Vec<String>>,
     pub insecure: bool,
     pub preserve_on_error: bool,
     pub network_config: Option<String>,
@@ -187,7 +189,29 @@ pub fn parse_args() -> Result<Config> {
                         .long("firstboot-args")
                         .value_name("args")
                         .help("Additional kernel args for the first boot")
-                        .takes_value(true),
+                        .takes_value(true)
+                        // This used to be for configuring networking from the cmdline, but it has
+                        // been obsoleted by the nicer `--copy-network` approach. We still need it
+                        // for now though. It's used at least by `coreos-installer.service`.
+                        .hidden(true),
+                )
+                .arg(
+                    Arg::with_name("append-karg")
+                        .long("append-karg")
+                        .value_name("arg")
+                        .help("Append default kernel arg")
+                        .takes_value(true)
+                        .number_of_values(1)
+                        .multiple(true),
+                )
+                .arg(
+                    Arg::with_name("delete-karg")
+                        .long("delete-karg")
+                        .value_name("arg")
+                        .help("Delete default kernel arg")
+                        .takes_value(true)
+                        .number_of_values(1)
+                        .multiple(true),
                 )
                 .arg(
                     Arg::with_name("copy-network")
@@ -640,6 +664,12 @@ fn parse_install(matches: &ArgMatches) -> Result<Config> {
             .chain_err(|| "parsing Ignition config hash")?,
         platform: matches.value_of("platform").map(String::from),
         firstboot_kargs: matches.value_of("firstboot-kargs").map(String::from),
+        append_kargs: matches
+            .values_of("append-karg")
+            .map(|v| v.map(String::from).collect()),
+        delete_kargs: matches
+            .values_of("delete-karg")
+            .map(|v| v.map(String::from).collect()),
         insecure: matches.is_present("insecure"),
         preserve_on_error: matches.is_present("preserve-on-error"),
         network_config,

src/install.rs

@@ -152,6 +152,8 @@ fn write_disk(config: &InstallConfig, source: &mut ImageSource, dest: &mut File)
     // postprocess
     if config.ignition.is_some()
         || config.firstboot_kargs.is_some()
+        || config.append_kargs.is_some()
+        || config.delete_kargs.is_some()
         || config.platform.is_some()
         || config.network_config.is_some()
     {
@@ -164,6 +166,18 @@ fn write_disk(config: &InstallConfig, source: &mut ImageSource, dest: &mut File)
             write_firstboot_kargs(mount.mountpoint(), firstboot_kargs)
                 .chain_err(|| "writing firstboot kargs")?;
         }
+        if config.append_kargs.is_some() || config.delete_kargs.is_some() {
+            eprintln!("Modifying kernel arguments");
+
+            edit_bls_entries(mount.mountpoint(), |orig_contents: &str| {
+                bls_entry_delete_and_append_kargs(
+                    orig_contents,
+                    config.delete_kargs.as_ref(),
+                    config.append_kargs.as_ref(),
+                )
+            })
+            .chain_err(|| "deleting and appending kargs")?;
+        }
         if let Some(platform) = config.platform.as_ref() {
             write_platform(mount.mountpoint(), platform).chain_err(|| "writing platform ID")?;
         }
@@ -236,6 +250,56 @@ fn write_firstboot_kargs(mountpoint: &Path, args: &str) -> Result<()> {
     Ok(())
 }
 
+// This is split out so that we can unit test it.
+fn bls_entry_delete_and_append_kargs(
+    orig_contents: &str,
+    delete_args: Option<&Vec<String>>,
+    append_args: Option<&Vec<String>>,
+) -> Result<String> {
+    let mut new_contents = String::with_capacity(orig_contents.len());
+    let mut found_options = false;
+    for line in orig_contents.lines() {
+        if !line.starts_with("options ") {
+            new_contents.push_str(line.trim_end());
+        } else if found_options {
+            bail!("Multiple 'options' lines found");
+        } else {
+            // XXX: Need a proper parser here and share it with afterburn. The approach we use here
+            // is to just do a dumb substring search and replace. This is naive (e.g. doesn't
+            // handle occurrences in quoted args) but will work for now (one thing that saves us is
+            // that we're acting on our baked configs, which have straight-forward kargs).
+            new_contents.push_str("options ");
+            let mut line: String = add_whitespaces(&line["options ".len()..]);
+            if let Some(args) = delete_args {
+                for arg in args {
+                    let arg = add_whitespaces(&arg);
+                    line = line.replace(&arg, " ");
+                }
+            }
+            new_contents.push_str(line.trim_start().trim_end());
+            if let Some(args) = append_args {
+                for arg in args {
+                    new_contents.push(' ');
+                    new_contents.push_str(&arg);
+                }
+            }
+            found_options = true;
+        }
+        new_contents.push('\n');
+    }
+    if !found_options {
+        bail!("Couldn't locate 'options' line");
+    }
+    Ok(new_contents)
+}
+
+fn add_whitespaces(s: &str) -> String {
+    let mut r: String = s.into();
+    r.insert(0, ' ');
+    r.push(' ');
+    r
+}
+
 /// Override the platform ID.
 fn write_platform(mountpoint: &Path, platform: &str) -> Result<()> {
     // early return if setting the platform to the default value, since
@@ -245,7 +309,29 @@ fn write_platform(mountpoint: &Path, platform: &str) -> Result<()> {
     }
 
     eprintln!("Setting platform to {}", platform);
+    edit_bls_entries(mountpoint, |orig_contents: &str| {
+        bls_entry_write_platform(orig_contents, platform)
+    })?;
+
+    Ok(())
+}
 
+/// Modifies the BLS config, only changing the `ignition.platform.id`. This assumes that we will
+/// only install from metal images and that the bootloader configs will always set
+/// ignition.platform.id.  Fail if those assumptions change.  This is deliberately simplistic.
+fn bls_entry_write_platform(orig_contents: &str, platform: &str) -> Result<String> {
+    let new_contents = orig_contents.replace(
+        "ignition.platform.id=metal",
+        &format!("ignition.platform.id={}", platform),
+    );
+    if orig_contents == new_contents {
+        bail!("Couldn't locate platform ID");
+    }
+    Ok(new_contents)
+}
+
+/// Apply a transforming function on each BLS entry found.
+fn edit_bls_entries(mountpoint: &Path, f: impl Fn(&str) -> Result<String>) -> Result<()> {
     // walk /boot/loader/entries/*.conf
     let mut config_path = mountpoint.to_path_buf();
     config_path.push("loader/entries");
@@ -262,22 +348,16 @@ fn write_platform(mountpoint: &Path, platform: &str) -> Result<()> {
                 .write(true)
                 .open(&path)
                 .chain_err(|| format!("opening bootloader config {}", path.display()))?;
-            let mut orig_contents = String::new();
-            config
-                .read_to_string(&mut orig_contents)
-                .chain_err(|| format!("reading {}", path.display()))?;
-
-            // Rewrite the config.  Assume that we will only install
-            // from metal images and that their bootloader configs will
-            // always set ignition.platform.id.  Fail if those
-            // assumptions change.  This is deliberately simplistic.
-            let new_contents = orig_contents.replace(
-                "ignition.platform.id=metal",
-                &format!("ignition.platform.id={}", platform),
-            );
-            if orig_contents == new_contents {
-                bail!("Couldn't locate platform ID in {}", path.display());
-            }
+            let orig_contents = {
+                let mut s = String::new();
+                config
+                    .read_to_string(&mut s)
+                    .chain_err(|| format!("reading {}", path.display()))?;
+                s
+            };
+
+            let new_contents =
+                f(&orig_contents).chain_err(|| format!("modifying {}", path.display()))?;
 
             // write out the modified data
             config
@@ -366,4 +446,84 @@ mod tests {
         let mut rd = std::io::Cursor::new(input);
         hasher.validate(&mut rd).unwrap();
     }
+
+    #[test]
+    fn test_platform_id() {
+        let orig_content = "options ignition.platform.id=metal foo bar";
+        let new_content = bls_entry_write_platform(orig_content, "openstack").unwrap();
+        assert_eq!(
+            new_content,
+            "options ignition.platform.id=openstack foo bar"
+        );
+
+        let orig_content = "options foo ignition.platform.id=metal bar";
+        let new_content = bls_entry_write_platform(orig_content, "openstack").unwrap();
+        assert_eq!(
+            new_content,
+            "options foo ignition.platform.id=openstack bar"
+        );
+
+        let orig_content = "options foo bar ignition.platform.id=metal";
+        let new_content = bls_entry_write_platform(orig_content, "openstack").unwrap();
+        assert_eq!(
+            new_content,
+            "options foo bar ignition.platform.id=openstack"
+        );
+    }
+
+    #[test]
+    fn test_options_edit() {
+        let orig_content = "options foo bar foobar";
+
+        let delete_kargs = vec!["foo".into()];
+        let new_content =
+            bls_entry_delete_and_append_kargs(orig_content, Some(&delete_kargs), None).unwrap();
+        assert_eq!(new_content, "options bar foobar\n");
+
+        let delete_kargs = vec!["bar".into()];
+        let new_content =
+            bls_entry_delete_and_append_kargs(orig_content, Some(&delete_kargs), None).unwrap();
+        assert_eq!(new_content, "options foo foobar\n");
+
+        let delete_kargs = vec!["foobar".into()];
+        let new_content =
+            bls_entry_delete_and_append_kargs(orig_content, Some(&delete_kargs), None).unwrap();
+        assert_eq!(new_content, "options foo bar\n");
+
+        let delete_kargs = vec!["bar".into(), "foo".into()];
+        let new_content =
+            bls_entry_delete_and_append_kargs(orig_content, Some(&delete_kargs), None).unwrap();
+        assert_eq!(new_content, "options foobar\n");
+
+        let orig_content = "options foo=val bar baz=val";
+
+        let delete_kargs = vec!["foo=val".into()];
+        let new_content =
+            bls_entry_delete_and_append_kargs(orig_content, Some(&delete_kargs), None).unwrap();
+        assert_eq!(new_content, "options bar baz=val\n");
+
+        let delete_kargs = vec!["baz=val".into()];
+        let new_content =
+            bls_entry_delete_and_append_kargs(orig_content, Some(&delete_kargs), None).unwrap();
+        assert_eq!(new_content, "options foo=val bar\n");
+
+        let orig_content =
+            "options foo mitigations=auto,nosmt console=tty0 bar console=ttyS0,115200n8 baz";
+
+        let delete_kargs = vec![
+            "mitigations=auto,nosmt".into(),
+            "console=ttyS0,115200n8".into(),
+        ];
+        let append_kargs = vec!["console=ttyS1,115200n8".into()];
+        let new_content = bls_entry_delete_and_append_kargs(
+            orig_content,
+            Some(&delete_kargs),
+            Some(&append_kargs),
+        )
+        .unwrap();
+        assert_eq!(
+            new_content,
+            "options foo console=tty0 bar baz console=ttyS1,115200n8\n"
+        );
+    }
 }