[pve-devel] [PATCH installer v2 0/4] auto-install-assistant: allow specifying 'fetch-from' partition label

[pve-devel] [PATCH installer v2 0/4] auto-install-assistant: allow specifying 'fetch-from' partition label

[Christoph Heiss]

This series allow specifying the partition label the
`proxmox-fetch-answer` tool will search when trying to retrieve an
answer file in the 'partition' fetch mode.

This has been requested by at least one user one user [0] and definitely
makes sense, esp. for BMCs/IPMIs where one might not be able to control
the partition label.

Patch #1 implements the new CLI parameter in the
`proxmox-auto-install-assistant` tool, patch #4 then integrates it into
the `proxmox-fetch-answer` itself. Patch #2 is just a small (but needed)
refactor of the CLI parsing code.

Patch #3 implements also searching for the partition label in the exact
case it was specified by the user. This is something I stumbled over
while testing it - see the patch itself for some more information.

[0] https://forum.proxmox.com/threads/proxmox-ais-question-request.153043/post-695689

History
=======
v1: https://lore.proxmox.com/pve-devel/20241018115943.813243-1-c.heiss@proxmox.com/

Notable Changes v1 -> v2:
 * addressed suggestions by Aaron 
 * added Aaron's T-b & R-b tags, thanks!
 * dropped useless "proxmox-ais" explanation from --prepare-iso
   description

Diffstat
========

Christoph Heiss (4):
  auto-install-assistant: add new parameter to specify partition label
  fetch-answer: refactor cli argument parsing
  fetch-answer: partition: also search for exact-matching partition
    label
  fetch-answer: use partition label from fetch config instead of
    hardcoded

 proxmox-auto-install-assistant/src/main.rs    | 15 ++++-
 proxmox-auto-installer/src/utils.rs           |  1 +
 .../src/fetch_plugins/partition.rs            | 32 ++++++----
 proxmox-fetch-answer/src/main.rs              | 58 ++++++++++++++-----
 4 files changed, 80 insertions(+), 26 deletions(-)

-- 
2.46.0



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH installer v2 1/4] auto-install-assistant: add new parameter to specify partition label

[Christoph Heiss]

.. for the 'partition' fetch method.

Tested-By: Aaron Lauterer <a.lauterer@proxmox.com>
Reviewed-By: Aaron Lauterer <a.lauterer@proxmox.com>
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * dropped useless "proxmox-ais" explanation from --prepare-iso
    command description

 proxmox-auto-install-assistant/src/main.rs | 12 ++++++++++--
 proxmox-auto-installer/src/utils.rs        |  1 +
 proxmox-fetch-answer/src/main.rs           |  1 +
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/proxmox-auto-install-assistant/src/main.rs b/proxmox-auto-install-assistant/src/main.rs
index 1447175..5d11882 100644
--- a/proxmox-auto-install-assistant/src/main.rs
+++ b/proxmox-auto-install-assistant/src/main.rs
@@ -94,8 +94,7 @@ struct CommandValidateAnswer {
 /// The behavior of how to fetch an answer file must be set with the '--fetch-from' parameter. The
 /// answer file can be:{n}
 /// * integrated into the ISO itself ('iso'){n}
-/// * present on a partition / file-system with the label 'PROXMOX-AIS' (Proxmox
-/// Automated Installer Source) ('partition'){n}
+/// * present on a partition / file-system, matched by its label ('partition'){n}
 /// * requested via an HTTP Post request ('http').
 ///
 /// The URL for the HTTP mode can be defined for the ISO with the '--url' argument. If not present,
@@ -110,6 +109,9 @@ struct CommandValidateAnswer {
 /// to retrieve the URL. For example, the DNS TXT record for the fingerprint will only be used, if
 /// no one was configured with the '--cert-fingerprint' parameter and if the URL was retrieved via
 /// the DNS TXT record.
+///
+/// If the 'partition' mode is used, the '--partition-label' parameter can be used to set the
+/// partition label the auto-installer should search for. This defaults to 'proxmox-ais'.
 #[derive(Args, Debug)]
 struct CommandPrepareISO {
     /// Path to the source ISO to prepare
@@ -141,6 +143,11 @@ struct CommandPrepareISO {
     /// input ISO file.
     #[arg(long)]
     tmp: Option<String>,
+
+    /// Can be used in combination with `--fetch-from partition` to set the partition label
+    /// the auto-installer will search for.
+    #[arg(long, default_value_t = { "proxmox-ais".to_owned() } )]
+    partition_label: String,
 }
 
 /// Show the system information that can be used to identify a host.
@@ -323,6 +330,7 @@ fn prepare_iso(args: &CommandPrepareISO) -> Result<()> {
     println!("Preparing ISO...");
     let config = AutoInstSettings {
         mode: args.fetch_from.clone(),
+        partition_label: args.partition_label.clone(),
         http: HttpOptions {
             url: args.url.clone(),
             cert_fingerprint: args.cert_fingerprint.clone(),
diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
index 45ad222..8ff8134 100644
--- a/proxmox-auto-installer/src/utils.rs
+++ b/proxmox-auto-installer/src/utils.rs
@@ -86,6 +86,7 @@ pub struct HttpOptions {
 #[serde(rename_all = "lowercase", deny_unknown_fields)]
 pub struct AutoInstSettings {
     pub mode: FetchAnswerFrom,
+    pub partition_label: String,
     #[serde(default)]
     pub http: HttpOptions,
 }
diff --git a/proxmox-fetch-answer/src/main.rs b/proxmox-fetch-answer/src/main.rs
index 660dc51..86c3270 100644
--- a/proxmox-fetch-answer/src/main.rs
+++ b/proxmox-fetch-answer/src/main.rs
@@ -63,6 +63,7 @@ fn settings_from_cli_args(args: &[String]) -> Result<AutoInstSettings> {
     }
     Ok(AutoInstSettings {
         mode,
+        partition_label: "proxmox-ais".to_owned(),
         http: HttpOptions {
             url: args.get(2).cloned(),
             cert_fingerprint: args.get(3).cloned(),
-- 
2.47.0



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH installer v2 2/4] fetch-answer: refactor cli argument parsing

[Christoph Heiss]

Clean up the actual parsing a bit to make it more easily extensible, and
add a proper help menu.

Tested-By: Aaron Lauterer <a.lauterer@proxmox.com>
Reviewed-By: Aaron Lauterer <a.lauterer@proxmox.com>
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * no changes

 proxmox-fetch-answer/src/main.rs | 40 +++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/proxmox-fetch-answer/src/main.rs b/proxmox-fetch-answer/src/main.rs
index 86c3270..753ab29 100644
--- a/proxmox-fetch-answer/src/main.rs
+++ b/proxmox-fetch-answer/src/main.rs
@@ -16,6 +16,22 @@ mod fetch_plugins;
 static LOGGER: AutoInstLogger = AutoInstLogger;
 static AUTOINST_MODE_FILE: &str = "/cdrom/auto-installer-mode.toml";
 
+const CLI_USAGE_HELPTEXT: &str = concat!(
+    "Usage: ",
+    env!("CARGO_BIN_NAME"),
+    " <command> <additional parameters..>
+
+Commands:
+  iso         Fetch the builtin answer file from the ISO
+  http        Fetch the answer file via HTTP(S)
+              Additional parameters: [<http-url>] [<tls-cert-fingerprint>]
+  partition   Fetch the answer file from a mountable partition
+
+Options:
+  -h, --help  Print this help menu
+"
+);
+
 pub fn init_log() -> Result<()> {
     AutoInstLogger::init("/tmp/fetch_answer.log")?;
     log::set_logger(&LOGGER)
@@ -46,21 +62,27 @@ fn fetch_answer(install_settings: &AutoInstSettings) -> Result<String> {
 }
 
 fn settings_from_cli_args(args: &[String]) -> Result<AutoInstSettings> {
-    // TODO: this was done in a bit of a hurry, needs tidying up
     let mode = match args[1].to_lowercase().as_str() {
         "iso" => FetchAnswerFrom::Iso,
         "http" => FetchAnswerFrom::Http,
         "partition" => FetchAnswerFrom::Partition,
-        "-h" | "--help" => bail!(
-            "usage: {} <http|iso|partition> [<http-url>] [<tls-cert-fingerprint>]",
-            args[0]
-        ),
+        "-h" | "--help" => {
+            eprintln!("{}", CLI_USAGE_HELPTEXT);
+            bail!("invalid usage");
+        }
         _ => bail!("failed to parse fetch-from argument, not one of 'http', 'iso', or 'partition'"),
     };
-    if args.len() > 4 {
-    } else if args.len() > 2 && mode != FetchAnswerFrom::Http {
-        bail!("only 'http' fetch-from mode supports additional url and cert-fingerprint mode");
-    }
+
+    match mode {
+        FetchAnswerFrom::Iso | FetchAnswerFrom::Partition if args.len() > 2 => {
+            bail!("'iso' and 'partition' modes do not take any additional arguments")
+        }
+        FetchAnswerFrom::Http if args.len() > 4 => {
+            bail!("'http' mode takes at most 2 additional arguments")
+        }
+        _ => {}
+    };
+
     Ok(AutoInstSettings {
         mode,
         partition_label: "proxmox-ais".to_owned(),
-- 
2.47.0



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH installer v2 3/4] fetch-answer: partition: also search for exact-matching partition label

[Christoph Heiss]

While some filesystems - such as FAT(32) - might not supported/allow
mixed-case labels, some implementations still handle them correctly,
such as Linux. Thus, also search for that variant.

Tested-By: Aaron Lauterer <a.lauterer@proxmox.com>
Reviewed-By: Aaron Lauterer <a.lauterer@proxmox.com>
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * no changes

 .../src/fetch_plugins/partition.rs               | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/proxmox-fetch-answer/src/fetch_plugins/partition.rs b/proxmox-fetch-answer/src/fetch_plugins/partition.rs
index 4472922..cbfe2d5 100644
--- a/proxmox-fetch-answer/src/fetch_plugins/partition.rs
+++ b/proxmox-fetch-answer/src/fetch_plugins/partition.rs
@@ -43,12 +43,22 @@ fn path_exists_logged(file_name: &str, search_path: &str) -> Option<PathBuf> {
     }
 }
 
-/// Searches for upper and lower case existence of the partlabel in the search_path
+/// Searches for the exact case, upper and finally lower case existence of the partlabel in the
+/// search_path, in that order.
+///
+/// While some filesystems - such as FAT(32) - might not supported/allow mixed-case labels, some
+/// implementations still handle them correctly, such as Linux. Thus, also search for that variant
+/// first.
 ///
 /// # Arguments
-/// * `partlabel_source` - Partition Label, used as upper and lower case
+/// * `partlabel_source` - Partition Label, used for matching, in the exact, upper and lower case
 /// * `search_path` - Path where to search for the partition label
 fn scan_partlabels(partlabel: &str, search_path: &str) -> Result<PathBuf> {
+    if let Some(path) = path_exists_logged(partlabel, search_path) {
+        info!("Found partition with label '{partlabel}'");
+        return Ok(path);
+    }
+
     let partlabel_upper_case = partlabel.to_uppercase();
     if let Some(path) = path_exists_logged(&partlabel_upper_case, search_path) {
         info!("Found partition with label '{partlabel_upper_case}'");
@@ -61,7 +71,7 @@ fn scan_partlabels(partlabel: &str, search_path: &str) -> Result<PathBuf> {
         return Ok(path);
     }
 
-    bail!("Could not detect upper or lower case labels for '{partlabel}'");
+    bail!("Could not find partition for label '{partlabel}'");
 }
 
 /// Will search and mount a partition/FS labeled PARTLABEL (proxmox-ais) in lower or uppercase
-- 
2.47.0



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH installer v2 4/4] fetch-answer: use partition label from fetch config instead of hardcoded

[Christoph Heiss]

This has been requested by at least one user one user [0] and definitely
makes sense, esp. for BMCs/IPMIs where one might not be able to control
the partition label.

[0] https://forum.proxmox.com/threads/proxmox-ais-question-request.153043/post-695689

Tested-By: Aaron Lauterer <a.lauterer@proxmox.com>
Reviewed-By: Aaron Lauterer <a.lauterer@proxmox.com>
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * addressed suggestions made by Aaron

 proxmox-auto-install-assistant/src/main.rs    |  2 ++
 .../src/fetch_plugins/partition.rs            | 16 ++++++-------
 proxmox-fetch-answer/src/main.rs              | 23 +++++++++++++------
 3 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/proxmox-auto-install-assistant/src/main.rs b/proxmox-auto-install-assistant/src/main.rs
index 5d11882..9f0fbec 100644
--- a/proxmox-auto-install-assistant/src/main.rs
+++ b/proxmox-auto-install-assistant/src/main.rs
@@ -146,6 +146,8 @@ struct CommandPrepareISO {
 
     /// Can be used in combination with `--fetch-from partition` to set the partition label
     /// the auto-installer will search for.
+    // FAT can only handle 11 characters (per specification at least, drivers might allow more),
+    // so shorten "Automated Installer Source" to "AIS" to be safe.
     #[arg(long, default_value_t = { "proxmox-ais".to_owned() } )]
     partition_label: String,
 }
diff --git a/proxmox-fetch-answer/src/fetch_plugins/partition.rs b/proxmox-fetch-answer/src/fetch_plugins/partition.rs
index cbfe2d5..f4f8bcc 100644
--- a/proxmox-fetch-answer/src/fetch_plugins/partition.rs
+++ b/proxmox-fetch-answer/src/fetch_plugins/partition.rs
@@ -8,18 +8,16 @@ use std::{
 
 static ANSWER_FILE: &str = "answer.toml";
 static ANSWER_MP: &str = "/mnt/answer";
-// FAT can only handle 11 characters, so shorten Automated Installer Source to AIS
-static PARTLABEL: &str = "proxmox-ais";
 static DISK_BY_ID_PATH: &str = "/dev/disk/by-label";
 
 pub struct FetchFromPartition;
 
 impl FetchFromPartition {
     /// Returns the contents of the answer file
-    pub fn get_answer() -> Result<String> {
+    pub fn get_answer(part_label: &str) -> Result<String> {
         info!("Checking for answer file on partition.");
 
-        let mut mount_path = PathBuf::from(mount_proxmoxinst_part()?);
+        let mut mount_path = PathBuf::from(mount_proxmoxinst_part(part_label)?);
         mount_path.push(ANSWER_FILE);
         let answer = fs::read_to_string(mount_path)
             .map_err(|err| format_err!("failed to read answer file - {err}"))?;
@@ -74,14 +72,16 @@ fn scan_partlabels(partlabel: &str, search_path: &str) -> Result<PathBuf> {
     bail!("Could not find partition for label '{partlabel}'");
 }
 
-/// Will search and mount a partition/FS labeled PARTLABEL (proxmox-ais) in lower or uppercase
-/// to ANSWER_MP
-fn mount_proxmoxinst_part() -> Result<String> {
+/// Searches for a partition/filesystem labeled `part_label` mounts it to `ANSWER_MP`, if found.
+///
+/// # Arguments
+///   * `partlabel` - Partition Label, used for matching, in the exact, upper and lower case
+fn mount_proxmoxinst_part(part_label: &str) -> Result<String> {
     if let Ok(true) = check_if_mounted(ANSWER_MP) {
         info!("Skipping: '{ANSWER_MP}' is already mounted.");
         return Ok(ANSWER_MP.into());
     }
-    let part_path = scan_partlabels(PARTLABEL, DISK_BY_ID_PATH)?;
+    let part_path = scan_partlabels(part_label, DISK_BY_ID_PATH)?;
     info!("Mounting partition at {ANSWER_MP}");
     // create dir for mountpoint
     create_dir_all(ANSWER_MP)?;
diff --git a/proxmox-fetch-answer/src/main.rs b/proxmox-fetch-answer/src/main.rs
index 753ab29..d56f8b9 100644
--- a/proxmox-fetch-answer/src/main.rs
+++ b/proxmox-fetch-answer/src/main.rs
@@ -26,6 +26,7 @@ Commands:
   http        Fetch the answer file via HTTP(S)
               Additional parameters: [<http-url>] [<tls-cert-fingerprint>]
   partition   Fetch the answer file from a mountable partition
+              Additional parameters: [<partition-label>]
 
 Options:
   -h, --help  Print this help menu
@@ -49,10 +50,12 @@ fn fetch_answer(install_settings: &AutoInstSettings) -> Result<String> {
                 Err(err) => info!("Fetching answer file from ISO failed: {err}"),
             }
         }
-        FetchAnswerFrom::Partition => match FetchFromPartition::get_answer() {
-            Ok(answer) => return Ok(answer),
-            Err(err) => info!("Fetching answer file from partition failed: {err}"),
-        },
+        FetchAnswerFrom::Partition => {
+            match FetchFromPartition::get_answer(&install_settings.partition_label) {
+                Ok(answer) => return Ok(answer),
+                Err(err) => info!("Fetching answer file from partition failed: {err}"),
+            }
+        }
         FetchAnswerFrom::Http => match FetchFromHTTP::get_answer(&install_settings.http) {
             Ok(answer) => return Ok(answer),
             Err(err) => info!("Fetching answer file via HTTP failed: {err}"),
@@ -74,18 +77,24 @@ fn settings_from_cli_args(args: &[String]) -> Result<AutoInstSettings> {
     };
 
     match mode {
-        FetchAnswerFrom::Iso | FetchAnswerFrom::Partition if args.len() > 2 => {
-            bail!("'iso' and 'partition' modes do not take any additional arguments")
+        FetchAnswerFrom::Iso if args.len() > 2 => {
+            bail!("'iso' mode does not take any additional arguments")
         }
         FetchAnswerFrom::Http if args.len() > 4 => {
             bail!("'http' mode takes at most 2 additional arguments")
         }
+        FetchAnswerFrom::Partition if args.len() > 3 => {
+            bail!("'partition' mode takes at most 1 additional argument")
+        }
         _ => {}
     };
 
     Ok(AutoInstSettings {
         mode,
-        partition_label: "proxmox-ais".to_owned(),
+        partition_label: args
+            .get(2)
+            .ok_or(format_err!("partition label expected"))
+            .cloned()?,
         http: HttpOptions {
             url: args.get(2).cloned(),
             cert_fingerprint: args.get(3).cloned(),
-- 
2.47.0



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel