Improve Mac Volume Curing (#362)

* Improve Mac Volume Curing

* Add test

* Improve error

* Speeling

* Remove comment

* Fix escaping issue

* Reflect comments

src/action/macos/create_fstab_entry.rs

@@ -1,16 +1,13 @@
 use uuid::Uuid;
 
-use super::CreateApfsVolume;
+use super::{get_uuid_for_label, CreateApfsVolume};
-use crate::{
+use crate::action::{
-    action::{Action, ActionDescription, ActionError, ActionState, ActionTag, StatefulAction},
+    Action, ActionDescription, ActionError, ActionState, ActionTag, StatefulAction,
-    execute_command,
 };
-use serde::Deserialize;
 use std::{io::SeekFrom, path::Path};
 use tokio::{
     fs::OpenOptions,
     io::{AsyncReadExt, AsyncSeekExt, AsyncWriteExt},
-    process::Command,
 };
 use tracing::{span, Span};
 
@@ -269,23 +266,6 @@ impl Action for CreateFstabEntry {
     }
 }
 
-async fn get_uuid_for_label(apfs_volume_label: &str) -> Result<Uuid, ActionError> {
-    let output = execute_command(
-        Command::new("/usr/sbin/diskutil")
-            .process_group(0)
-            .arg("info")
-            .arg("-plist")
-            .arg(apfs_volume_label)
-            .stdin(std::process::Stdio::null())
-            .stdout(std::process::Stdio::piped()),
-    )
-    .await?;
-
-    let parsed: DiskUtilApfsInfoOutput = plist::from_bytes(&output.stdout)?;
-
-    Ok(parsed.volume_uuid)
-}
-
 fn fstab_lines(uuid: &Uuid, apfs_volume_label: &str) -> String {
     let prelude_comment = fstab_prelude_comment(apfs_volume_label);
     let fstab_entry = fstab_entry(uuid);
@@ -308,10 +288,3 @@ pub enum CreateFstabEntryError {
     #[error("The `/etc/fstab` entry (previously created by the official install scripts) detected during planning disappeared between planning and executing. Cannot update `/etc/fstab` as planned")]
     ExistingForeignEntryDisappeared,
 }
-
-#[derive(Deserialize, Clone, Debug)]
-#[serde(rename_all = "PascalCase")]
-struct DiskUtilApfsInfoOutput {
-    #[serde(rename = "VolumeUUID")]
-    volume_uuid: Uuid,
-}

src/action/macos/create_nix_volume.rs

@@ -1,5 +1,5 @@
 use crate::action::{
-    base::{create_or_insert_into_file, CreateFile, CreateOrInsertIntoFile},
+    base::{create_or_insert_into_file, CreateOrInsertIntoFile},
     macos::{
         BootstrapLaunchctlService, CreateApfsVolume, CreateSyntheticObjects, EnableOwnership,
         EncryptApfsVolume, UnmountApfsVolume,
@@ -13,7 +13,7 @@ use std::{
 use tokio::process::Command;
 use tracing::{span, Span};
 
-use super::{create_fstab_entry::CreateFstabEntry, KickstartLaunchctlService};
+use super::{create_fstab_entry::CreateFstabEntry, CreateVolumeService, KickstartLaunchctlService};
 
 pub const NIX_VOLUME_MOUNTD_DEST: &str = "/Library/LaunchDaemons/org.nixos.darwin-store.plist";
 
@@ -30,7 +30,7 @@ pub struct CreateNixVolume {
     create_volume: StatefulAction<CreateApfsVolume>,
     create_fstab_entry: StatefulAction<CreateFstabEntry>,
     encrypt_volume: Option<StatefulAction<EncryptApfsVolume>>,
-    setup_volume_daemon: StatefulAction<CreateFile>,
+    setup_volume_daemon: StatefulAction<CreateVolumeService>,
     bootstrap_volume: StatefulAction<BootstrapLaunchctlService>,
     kickstart_launchctl_service: StatefulAction<KickstartLaunchctlService>,
     enable_ownership: StatefulAction<EnableOwnership>,
@@ -78,44 +78,15 @@ impl CreateNixVolume {
             None
         };
 
-        let name_with_qoutes = format!("\"{name}\"");
+        let setup_volume_daemon = CreateVolumeService::plan(
-        let encrypted_command;
+            NIX_VOLUME_MOUNTD_DEST,
-        let mount_command = if encrypt {
+            "org.nixos.darwin-store",
-            encrypted_command = format!("/usr/bin/security find-generic-password -s {name_with_qoutes} -w |  /usr/sbin/diskutil apfs unlockVolume {name_with_qoutes} -mountpoint /nix -stdinpassphrase");
+            name.clone(),
-            vec!["/bin/sh", "-c", encrypted_command.as_str()]
+            "/nix",
-        } else {
+            encrypt,
-            vec![
+        )
-                "/usr/sbin/diskutil",
+        .await
-                "mount",
+        .map_err(|e| ActionError::Child(CreateVolumeService::action_tag(), Box::new(e)))?;
-                "-mountPoint",
-                "/nix",
-                name.as_str(),
-            ]
-        };
-        // TODO(@hoverbear): Use plist lib we have in tree...
-        let mount_plist = format!(
-            "\
-            <?xml version=\"1.0\" encoding=\"UTF-8\"?>\n\
-            <!DOCTYPE plist PUBLIC \"-//Apple Computer//DTD PLIST 1.0//EN\" \"http://www.apple.com/DTDs/PropertyList-1.0.dtd\">\n\
-            <plist version=\"1.0\">\n\
-            <dict>\n\
-            <key>RunAtLoad</key>\n\
-            <true/>\n\
-            <key>Label</key>\n\
-            <string>org.nixos.darwin-store</string>\n\
-            <key>ProgramArguments</key>\n\
-            <array>\n\
-                {}\
-            </array>\n\
-            </dict>\n\
-            </plist>\n\
-        \
-        ", mount_command.iter().map(|v| format!("<string>{v}</string>\n")).collect::<Vec<_>>().join("\n")
-        );
-        let setup_volume_daemon =
-            CreateFile::plan(NIX_VOLUME_MOUNTD_DEST, None, None, None, mount_plist, false)
-                .await
-                .map_err(|e| ActionError::Child(CreateFile::action_tag(), Box::new(e)))?;
 
         let bootstrap_volume = BootstrapLaunchctlService::plan(
             "system",

src/action/macos/create_volume_service.rs

@@ -0,0 +1,210 @@
+use serde::{Deserialize, Serialize};
+use tracing::{span, Span};
+
+use std::path::{Path, PathBuf};
+use tokio::{
+    fs::{remove_file, OpenOptions},
+    io::AsyncWriteExt,
+};
+
+use crate::action::{Action, ActionDescription, ActionError, ActionTag, StatefulAction};
+
+use super::get_uuid_for_label;
+
+/** Create a plist for a `launchctl` service to mount the given `apfs_volume_label` on the given `mount_point`.
+ */
+#[derive(Debug, serde::Deserialize, serde::Serialize, Clone)]
+pub struct CreateVolumeService {
+    pub(crate) path: PathBuf,
+    apfs_volume_label: String,
+    mount_service_label: String,
+    mount_point: PathBuf,
+    encrypt: bool,
+}
+
+impl CreateVolumeService {
+    #[tracing::instrument(level = "debug", skip_all)]
+    pub async fn plan(
+        path: impl AsRef<Path>,
+        mount_service_label: impl Into<String>,
+        apfs_volume_label: String,
+        mount_point: impl AsRef<Path>,
+        encrypt: bool,
+    ) -> Result<StatefulAction<Self>, ActionError> {
+        let path = path.as_ref().to_path_buf();
+        let mount_point = mount_point.as_ref().to_path_buf();
+        let mount_service_label = mount_service_label.into();
+        let this = Self {
+            path,
+            apfs_volume_label,
+            mount_service_label,
+            mount_point,
+            encrypt,
+        };
+
+        if this.path.exists() {
+            let discovered_plist: LaunchctlMountPlist = plist::from_file(&this.path)?;
+            let expected_plist = generate_mount_plist(
+                &this.mount_service_label,
+                &this.apfs_volume_label,
+                &this.mount_point,
+                encrypt,
+            )
+            .await?;
+            if discovered_plist != expected_plist {
+                tracing::trace!(
+                    ?discovered_plist,
+                    ?expected_plist,
+                    "Parsed plists not equal"
+                );
+                return Err(ActionError::Custom(Box::new(
+                    CreateVolumeServiceError::DifferentPlist {
+                        expected: expected_plist,
+                        discovered: discovered_plist,
+                        path: this.path.clone(),
+                    },
+                )));
+            }
+
+            tracing::debug!("Creating file `{}` already complete", this.path.display());
+            return Ok(StatefulAction::completed(this));
+        }
+
+        Ok(StatefulAction::uncompleted(this))
+    }
+}
+
+#[async_trait::async_trait]
+#[typetag::serde(name = "create_volume_service")]
+impl Action for CreateVolumeService {
+    fn action_tag() -> ActionTag {
+        ActionTag("setup_volume_daemon")
+    }
+    fn tracing_synopsis(&self) -> String {
+        format!(
+            "Create a `launchctl` plist to mount the APFS volume `{}`",
+            self.path.display()
+        )
+    }
+
+    fn tracing_span(&self) -> Span {
+        let span = span!(
+            tracing::Level::DEBUG,
+            "setup_volume_daemon",
+            path = tracing::field::display(self.path.display()),
+            buf = tracing::field::Empty,
+        );
+
+        if tracing::enabled!(tracing::Level::TRACE) {
+            span.record("buf", &self.apfs_volume_label);
+        }
+        span
+    }
+
+    fn execute_description(&self) -> Vec<ActionDescription> {
+        vec![ActionDescription::new(self.tracing_synopsis(), vec![])]
+    }
+
+    #[tracing::instrument(level = "debug", skip_all)]
+    async fn execute(&mut self) -> Result<(), ActionError> {
+        let Self {
+            path,
+            mount_service_label,
+            apfs_volume_label,
+            mount_point,
+            encrypt,
+        } = self;
+
+        let generated_plist = generate_mount_plist(
+            &mount_service_label,
+            &apfs_volume_label,
+            mount_point,
+            *encrypt,
+        )
+        .await?;
+
+        let mut options = OpenOptions::new();
+        options.create_new(true).write(true).read(true);
+
+        let mut file = options
+            .open(&path)
+            .await
+            .map_err(|e| ActionError::Open(path.to_owned(), e))?;
+
+        let mut buf = Vec::new();
+        plist::to_writer_xml(&mut buf, &generated_plist)?;
+        file.write_all(&buf)
+            .await
+            .map_err(|e| ActionError::Write(path.to_owned(), e))?;
+
+        Ok(())
+    }
+
+    fn revert_description(&self) -> Vec<ActionDescription> {
+        vec![ActionDescription::new(
+            format!("Delete file `{}`", self.path.display()),
+            vec![format!("Delete file `{}`", self.path.display())],
+        )]
+    }
+
+    #[tracing::instrument(level = "debug", skip_all)]
+    async fn revert(&mut self) -> Result<(), ActionError> {
+        remove_file(&self.path)
+            .await
+            .map_err(|e| ActionError::Remove(self.path.to_owned(), e))?;
+
+        Ok(())
+    }
+}
+
+/// This function must be able to operate at both plan and execute time.
+async fn generate_mount_plist(
+    mount_service_label: &str,
+    apfs_volume_label: &str,
+    mount_point: &Path,
+    encrypt: bool,
+) -> Result<LaunchctlMountPlist, ActionError> {
+    let apfs_volume_label_with_quotes = format!("\"{apfs_volume_label}\"");
+    let uuid = get_uuid_for_label(&apfs_volume_label).await?;
+    // The official Nix scripts uppercase the UUID, so we do as well for compatibility.
+    let uuid_string = uuid.to_string().to_uppercase();
+    let mount_command = if encrypt {
+        let encrypted_command = format!("/usr/bin/security find-generic-password -s {apfs_volume_label_with_quotes} -w |  /usr/sbin/diskutil apfs unlockVolume {apfs_volume_label_with_quotes} -mountpoint {mount_point:?} -stdinpassphrase");
+        vec!["/bin/sh".into(), "-c".into(), encrypted_command]
+    } else {
+        vec![
+            "/usr/sbin/diskutil".into(),
+            "mount".into(),
+            "-mountPoint".into(),
+            mount_point.display().to_string(),
+            uuid_string,
+        ]
+    };
+
+    let mount_plist = LaunchctlMountPlist {
+        run_at_load: true,
+        label: mount_service_label.into(),
+        program_arguments: mount_command,
+    };
+
+    Ok(mount_plist)
+}
+
+#[derive(Deserialize, Clone, Debug, Serialize, PartialEq)]
+#[serde(rename_all = "PascalCase")]
+pub struct LaunchctlMountPlist {
+    run_at_load: bool,
+    label: String,
+    program_arguments: Vec<String>,
+}
+
+#[non_exhaustive]
+#[derive(Debug, thiserror::Error)]
+pub enum CreateVolumeServiceError {
+    #[error("`{path}` contents differs, planned `{expected:?}`, discovered `{discovered:?}`")]
+    DifferentPlist {
+        expected: LaunchctlMountPlist,
+        discovered: LaunchctlMountPlist,
+        path: PathBuf,
+    },
+}

src/action/macos/kickstart_launchctl_service.rs

@@ -76,7 +76,7 @@ impl Action for KickstartLaunchctlService {
         ActionTag("kickstart_launchctl_service")
     }
     fn tracing_synopsis(&self) -> String {
-        format!("Run `launchctl kickstart {}`", self.service)
+        format!("Run `launchctl kickstart -k {}`", self.service)
     }
 
     fn tracing_span(&self) -> Span {

src/action/macos/mod.rs

@@ -6,6 +6,7 @@ pub(crate) mod create_apfs_volume;
 pub(crate) mod create_fstab_entry;
 pub(crate) mod create_nix_volume;
 pub(crate) mod create_synthetic_objects;
+pub(crate) mod create_volume_service;
 pub(crate) mod enable_ownership;
 pub(crate) mod encrypt_apfs_volume;
 pub(crate) mod kickstart_launchctl_service;
@@ -15,7 +16,39 @@ pub use bootstrap_launchctl_service::BootstrapLaunchctlService;
 pub use create_apfs_volume::CreateApfsVolume;
 pub use create_nix_volume::{CreateNixVolume, NIX_VOLUME_MOUNTD_DEST};
 pub use create_synthetic_objects::{CreateSyntheticObjects, CreateSyntheticObjectsError};
+pub use create_volume_service::CreateVolumeService;
 pub use enable_ownership::{EnableOwnership, EnableOwnershipError};
 pub use encrypt_apfs_volume::EncryptApfsVolume;
 pub use kickstart_launchctl_service::KickstartLaunchctlService;
+use serde::Deserialize;
+use tokio::process::Command;
 pub use unmount_apfs_volume::UnmountApfsVolume;
+use uuid::Uuid;
+
+use crate::execute_command;
+
+use super::ActionError;
+
+async fn get_uuid_for_label(apfs_volume_label: &str) -> Result<Uuid, ActionError> {
+    let output = execute_command(
+        Command::new("/usr/sbin/diskutil")
+            .process_group(0)
+            .arg("info")
+            .arg("-plist")
+            .arg(apfs_volume_label)
+            .stdin(std::process::Stdio::null())
+            .stdout(std::process::Stdio::piped()),
+    )
+    .await?;
+
+    let parsed: DiskUtilApfsInfoOutput = plist::from_bytes(&output.stdout)?;
+
+    Ok(parsed.volume_uuid)
+}
+
+#[derive(Deserialize, Clone, Debug)]
+#[serde(rename_all = "PascalCase")]
+struct DiskUtilApfsInfoOutput {
+    #[serde(rename = "VolumeUUID")]
+    volume_uuid: Uuid,
+}

src/lib.rs

@@ -93,7 +93,7 @@ use tokio::process::Command;
 
 #[tracing::instrument(level = "debug", skip_all, fields(command = %format!("{:?}", command.as_std())))]
 async fn execute_command(command: &mut Command) -> Result<Output, ActionError> {
-    tracing::trace!("Executing `{:?}`", command.as_std());
+    tracing::trace!("Executing");
     let output = command
         .output()
         .await

tests/fixtures/macos/macos.json

@@ -55,11 +55,10 @@
         "setup_volume_daemon": {
           "action": {
             "path": "/Library/LaunchDaemons/org.nixos.darwin-store.plist",
-            "user": null,
+            "apfs_volume_label": "Nix Store",
-            "group": null,
+            "mount_service_label": "org.nixos.darwin-store",
-            "mode": null,
+            "mount_point": "/nix",
-            "buf": "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<!DOCTYPE plist PUBLIC \"-//Apple Computer//DTD PLIST 1.0//EN\" \"http://www.apple.com/DTDs/PropertyList-1.0.dtd\">\n<plist version=\"1.0\">\n<dict>\n<key>RunAtLoad</key>\n<true/>\n<key>Label</key>\n<string>org.nixos.darwin-store</string>\n<key>ProgramArguments</key>\n<array>\n<string>/bin/sh</string>\n\n<string>-c</string>\n\n<string>/usr/bin/security find-generic-password -s \"Nix Store\" -w |  /usr/sbin/diskutil apfs unlockVolume \"Nix Store\" -mountpoint /nix -stdinpassphrase</string>\n</array>\n</dict>\n</plist>\n",
+            "encrypt": false
-            "force": false
           },
           "state": "Uncompleted"
         },