[pbs-devel] [PATCH proxmox-offline-mirror v2 0/3] improve pom multi-key handling

[pbs-devel] [PATCH proxmox-offline-mirror v2 0/3] improve pom multi-key handling

[Stefan Sterz]

this patch series intends to improve the user experience when a user
adds multiple keys to one mirror and wants to use one medium to activate
multiple products. the first commit removes the `ProductType` trait that
has been moved to `proxmox-subscription` from pom. the second commit
handles media with muliple keys for muliple products more gracefully in
proxmox-offline-mirror-helper. it only tries to activate subscriptions
that have a matching server id. the finally commit just adds some
rustfmt clean ups.

changes since v1 @ (thanks to Fabian Grünbichler):

- moved to a hash map based approach when automatically selecting all
  applicable keys for the current system


Stefan Sterz (3):
  type: move `ProductType` enum to `proxmox-subscription`
  helper: improve handling of multiple keys when activating them
  offline mirror binary: rustfmt clean up

 src/bin/proxmox-offline-mirror-helper.rs      | 101 ++++++++++--------
 src/bin/proxmox-offline-mirror.rs             |  15 ++-
 src/bin/proxmox_offline_mirror_cmds/medium.rs |   4 +-
 .../subscription.rs                           |   4 +-
 src/config.rs                                 |   4 +-
 src/subscription.rs                           |   4 +-
 src/types.rs                                  |  44 +-------
 7 files changed, 75 insertions(+), 101 deletions(-)

--
2.39.2

[pbs-devel] [PATCH proxmox-offline-mirror v2 1/3] type: move `ProductType` enum to `proxmox-subscription`

[Stefan Sterz]

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
 src/bin/proxmox-offline-mirror-helper.rs      |  4 +-
 src/bin/proxmox-offline-mirror.rs             |  3 +-
 src/bin/proxmox_offline_mirror_cmds/medium.rs |  4 +-
 .../subscription.rs                           |  4 +-
 src/config.rs                                 |  4 +-
 src/subscription.rs                           |  4 +-
 src/types.rs                                  | 44 +------------------
 7 files changed, 13 insertions(+), 54 deletions(-)

diff --git a/src/bin/proxmox-offline-mirror-helper.rs b/src/bin/proxmox-offline-mirror-helper.rs
index e62206c..f1b4bd7 100644
--- a/src/bin/proxmox-offline-mirror-helper.rs
+++ b/src/bin/proxmox-offline-mirror-helper.rs
@@ -4,8 +4,8 @@ use std::{collections::HashMap, path::Path};
 
 use anyhow::{bail, format_err, Error};
 
-use proxmox_offline_mirror::types::{ProductType, Snapshot};
-use proxmox_subscription::SubscriptionInfo;
+use proxmox_offline_mirror::types::Snapshot;
+use proxmox_subscription::{ProductType, SubscriptionInfo};
 use proxmox_sys::command::run_command;
 use proxmox_sys::fs::{replace_file, CreateOptions};
 use proxmox_sys::{fs::file_get_contents, linux::tty};
diff --git a/src/bin/proxmox-offline-mirror.rs b/src/bin/proxmox-offline-mirror.rs
index d029d3d..a6d6c49 100644
--- a/src/bin/proxmox-offline-mirror.rs
+++ b/src/bin/proxmox-offline-mirror.rs
@@ -10,6 +10,7 @@ use serde_json::Value;
 use proxmox_router::cli::{run_cli_command, CliCommand, CliCommandMap, CliEnvironment};
 use proxmox_schema::api;
 use proxmox_section_config::SectionConfigData;
+use proxmox_subscription::ProductType;
 use proxmox_sys::linux::tty;
 
 use proxmox_offline_mirror::helpers::tty::{
@@ -18,7 +19,7 @@ use proxmox_offline_mirror::helpers::tty::{
 use proxmox_offline_mirror::{
     config::{save_config, MediaConfig, MirrorConfig, SkipConfig},
     mirror,
-    types::{ProductType, MEDIA_ID_SCHEMA, MIRROR_ID_SCHEMA},
+    types::{MEDIA_ID_SCHEMA, MIRROR_ID_SCHEMA},
 };
 
 mod proxmox_offline_mirror_cmds;
diff --git a/src/bin/proxmox_offline_mirror_cmds/medium.rs b/src/bin/proxmox_offline_mirror_cmds/medium.rs
index 574f748..3f05d43 100644
--- a/src/bin/proxmox_offline_mirror_cmds/medium.rs
+++ b/src/bin/proxmox_offline_mirror_cmds/medium.rs
@@ -6,7 +6,7 @@ use serde_json::Value;
 use proxmox_router::cli::{CliCommand, CliCommandMap, CommandLineInterface, OUTPUT_FORMAT};
 use proxmox_schema::api;
 use proxmox_section_config::SectionConfigData;
-use proxmox_subscription::SubscriptionInfo;
+use proxmox_subscription::{ProductType, SubscriptionInfo};
 use proxmox_time::epoch_to_rfc3339_utc;
 
 use proxmox_offline_mirror::{
@@ -14,7 +14,7 @@ use proxmox_offline_mirror::{
     generate_repo_file_line,
     medium::{self},
     mirror,
-    types::{ProductType, Snapshot, MEDIA_ID_SCHEMA},
+    types::{Snapshot, MEDIA_ID_SCHEMA},
 };
 
 use super::get_config_path;
diff --git a/src/bin/proxmox_offline_mirror_cmds/subscription.rs b/src/bin/proxmox_offline_mirror_cmds/subscription.rs
index e58b049..c5b8f3d 100644
--- a/src/bin/proxmox_offline_mirror_cmds/subscription.rs
+++ b/src/bin/proxmox_offline_mirror_cmds/subscription.rs
@@ -7,9 +7,9 @@ use std::convert::TryFrom;
 use proxmox_offline_mirror::{
     config::{SubscriptionKey, SubscriptionKeyUpdater},
     subscription::{extract_mirror_key, refresh_mirror_key, refresh_offline_keys},
-    types::{ProductType, PROXMOX_SUBSCRIPTION_KEY_SCHEMA},
+    types::PROXMOX_SUBSCRIPTION_KEY_SCHEMA,
 };
-use proxmox_subscription::{files::DEFAULT_SIGNING_KEY, SubscriptionStatus};
+use proxmox_subscription::{files::DEFAULT_SIGNING_KEY, ProductType, SubscriptionStatus};
 use proxmox_sys::fs::file_get_contents;
 use proxmox_time::epoch_to_rfc3339_utc;
 
diff --git a/src/config.rs b/src/config.rs
index d0bc14e..9c75717 100644
--- a/src/config.rs
+++ b/src/config.rs
@@ -7,11 +7,11 @@ use serde::{Deserialize, Serialize};
 
 use proxmox_schema::{api, ApiStringFormat, ApiType, Schema, Updater};
 use proxmox_section_config::{SectionConfig, SectionConfigData, SectionConfigPlugin};
+use proxmox_subscription::ProductType;
 use proxmox_sys::fs::{replace_file, CreateOptions};
 
 use crate::types::{
-    ProductType, MEDIA_ID_SCHEMA, MIRROR_ID_SCHEMA, PROXMOX_SERVER_ID_SCHEMA,
-    PROXMOX_SUBSCRIPTION_KEY_SCHEMA,
+    MEDIA_ID_SCHEMA, MIRROR_ID_SCHEMA, PROXMOX_SERVER_ID_SCHEMA, PROXMOX_SUBSCRIPTION_KEY_SCHEMA,
 };
 
 /// Skip Configuration
diff --git a/src/subscription.rs b/src/subscription.rs
index 91b7bdb..3f0c776 100644
--- a/src/subscription.rs
+++ b/src/subscription.rs
@@ -5,10 +5,10 @@ use proxmox_http::{HttpClient, HttpOptions, ProxyConfig};
 use proxmox_subscription::SubscriptionStatus;
 use proxmox_subscription::{
     sign::{SignRequest, SignedResponse},
-    SubscriptionInfo,
+    ProductType, SubscriptionInfo,
 };
 
-use crate::{config::SubscriptionKey, types::ProductType};
+use crate::config::SubscriptionKey;
 
 // TODO: Update with final, public URL
 const PRODUCT_URL: &str = "-";
diff --git a/src/types.rs b/src/types.rs
index 3098a8d..7544d5e 100644
--- a/src/types.rs
+++ b/src/types.rs
@@ -1,10 +1,9 @@
 use std::{fmt::Display, path::PathBuf, str::FromStr};
 
 use anyhow::Error;
-use proxmox_schema::{api, const_regex, ApiStringFormat, Schema, StringSchema, Updater};
+use proxmox_schema::{api, const_regex, ApiStringFormat, Schema, StringSchema};
 use proxmox_serde::{forward_deserialize_to_from_str, forward_serialize_to_display};
 use proxmox_time::{epoch_i64, epoch_to_rfc3339_utc, parse_rfc3339};
-use serde::{Deserialize, Serialize};
 
 #[rustfmt::skip]
 #[macro_export]
@@ -100,47 +99,6 @@ impl FromStr for Snapshot {
     }
 }
 
-#[api()]
-#[derive(Debug, Clone, Serialize, Deserialize, Updater, PartialEq, Eq)]
-#[serde(rename_all = "lowercase")]
-/// Product type
-pub enum ProductType {
-    /// Proxmox Virtual Environment
-    Pve,
-    /// Proxmox Backup Server
-    Pbs,
-    /// Proxmox Mail Gateway
-    Pmg,
-    /// Proxmox Offline Mirror
-    Pom,
-}
-
-impl Display for ProductType {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        let txt = match self {
-            ProductType::Pve => "pve",
-            ProductType::Pbs => "pbs",
-            ProductType::Pmg => "pmg",
-            ProductType::Pom => "pom",
-        };
-        f.write_str(txt)
-    }
-}
-
-impl FromStr for ProductType {
-    type Err = Error;
-
-    fn from_str(s: &str) -> Result<Self, Self::Err> {
-        match s {
-            "pve" => Ok(ProductType::Pve),
-            "pmg" => Ok(ProductType::Pmg),
-            "pbs" => Ok(ProductType::Pbs),
-            "pom" => Ok(ProductType::Pom),
-            _ => unimplemented!(),
-        }
-    }
-}
-
 /// Entries of Diff
 #[derive(Default)]
 pub struct DiffMember {
-- 
2.39.2

[pbs-devel] [PATCH proxmox-offline-mirror v2 2/3] helper: improve handling of multiple keys when activating them

[Stefan Sterz]

this commit fixes a behavior where pom would applied any subscription
key that matched the provided product. it did not check whether the
server id of the activated subscription matched the current system.
this commit fixes that and only allows applying subscriptions for the
current system.

it also adds a couple of ux improvements:

- the `offline-key` sub-command now does not require the `--product`
  parameter anymore. if there are multiple keys with different
  products for the same server we will try to activate them all. the
  assumption is that the user added all keys intentionally (e.g. a
  combo pbs+pve system) and would like to activate them all at once.
  since this only makes the api more permissive this shouldn't be a
  breaking change.
- if the `offline-key` sub-command encounters multiple subscription
  keys with the same product and server id, it only activates the one
  with the due date furthest in the future. this makes sense in a
  scenario where a user simply adds new subscription keys to their
  key medium without removing older ones (perhaps older subscriptions
  haven't even expired just yet).
- the interactive `setup` sub-command now only offers keys that have a
  matching server id. it also orders them in such a way that the top
  most key for a given product has the next due date furthest in the
  future.

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
 src/bin/proxmox-offline-mirror-helper.rs | 97 ++++++++++++++----------
 1 file changed, 57 insertions(+), 40 deletions(-)

diff --git a/src/bin/proxmox-offline-mirror-helper.rs b/src/bin/proxmox-offline-mirror-helper.rs
index f1b4bd7..def10ad 100644
--- a/src/bin/proxmox-offline-mirror-helper.rs
+++ b/src/bin/proxmox-offline-mirror-helper.rs
@@ -24,7 +24,7 @@ use proxmox_offline_mirror::helpers::tty::{
 use proxmox_offline_mirror::medium::{self, generate_repo_snippet, MediumState};
 
 fn set_subscription_key(
-    product: ProductType,
+    product: &ProductType,
     subscription: &SubscriptionInfo,
 ) -> Result<String, Error> {
     let data = base64::encode(serde_json::to_vec(subscription)?);
@@ -218,32 +218,34 @@ async fn setup(_param: Value) -> Result<(), Error> {
             }
             Action::UpdateOfflineSubscription => {
                 let server_id = proxmox_subscription::get_hardware_address()?;
-                let subscriptions: Vec<(&SubscriptionInfo, &str)> = state
+                let mut subscriptions: Vec<((ProductType, &SubscriptionInfo), &str)> = state
                     .subscriptions
                     .iter()
-                    .filter_map(|s| {
-                        if let Some(key) = s.key.as_ref() {
-                            if let Ok(product) = key[..3].parse::<ProductType>() {
-                                if product == ProductType::Pom {
-                                    return None;
-                                } else {
-                                    return Some((s, key.as_str()));
-                                }
-                            }
-                        }
-                        None
+                    .filter(|sub| sub.serverid.as_ref() == Some(&server_id))
+                    .filter_map(|sub| {
+                        sub.get_product_type()
+                            .ok()
+                            .and_then(|prod| Some(((prod, sub), sub.key.as_ref()?.as_str())))
                     })
+                    .filter(|((found_product, _), _)| found_product != &ProductType::Pom)
                     .collect();
 
+                subscriptions.sort_by(|((prod_a, sub_a), _), ((prod_b, sub_b), _)| {
+                    if prod_a == prod_b {
+                        return sub_b
+                            .get_next_due_date()
+                            .ok()
+                            .cmp(&sub_a.get_next_due_date().ok());
+                    }
+
+                    prod_a.cmp(prod_b)
+                });
+
                 if subscriptions.is_empty() {
-                    println!(
-                        "No matching subscription key found for server ID '{}'",
-                        server_id
-                    );
+                    println!("No matching subscription key found for server ID '{server_id}'");
                 } else {
-                    let info = read_selection_from_tty("Select key", &subscriptions, None)?;
-                    // safe unwrap, checked above!
-                    let product: ProductType = info.key.as_ref().unwrap()[..3].parse()?;
+                    let (product, info) =
+                        read_selection_from_tty("Select key", &subscriptions, None)?;
                     set_subscription_key(product, info)?;
                 }
             }
@@ -265,6 +267,7 @@ async fn setup(_param: Value) -> Result<(), Error> {
             },
             product: {
                 type: ProductType,
+                optional: true,
             },
         },
     },
@@ -272,10 +275,10 @@ async fn setup(_param: Value) -> Result<(), Error> {
 /// Configures and offline subscription key
 async fn setup_offline_key(
     mountpoint: String,
-    product: ProductType,
+    product: Option<ProductType>,
     _param: Value,
 ) -> Result<(), Error> {
-    if product == ProductType::Pom {
+    if product == Some(ProductType::Pom) {
         param_bail!(
             "product",
             format_err!("Proxmox Offline Mirror does not support offline operations.")
@@ -299,27 +302,41 @@ async fn setup_offline_key(
     );
 
     let server_id = proxmox_subscription::get_hardware_address()?;
-    let subscription = state.subscriptions.iter().find(|s| {
-        if let Some(key) = s.key.as_ref() {
-            if let Ok(found_product) = key[..3].parse::<ProductType>() {
-                return product == found_product;
-            }
-        }
-        false
-    });
-
-    match subscription {
-        Some(subscription) => {
-            eprintln!("Setting offline subscription key for {product}..");
-            match set_subscription_key(product, subscription) {
-                Ok(output) if !output.is_empty() => eprintln!("success: {output}"),
-                Ok(_) => eprintln!("success."),
-                Err(err) => eprintln!("error: {err}"),
+
+    let subscriptions: HashMap<ProductType, &SubscriptionInfo> = state
+        .subscriptions
+        .iter()
+        .filter(|sub| sub.serverid.as_ref() == Some(&server_id))
+        .filter_map(|sub| sub.get_product_type().ok().map(|prod| (prod, sub)))
+        .filter(|(found_product, _)| {
+            (product.is_none() || Some(found_product) == product.as_ref())
+                && found_product != &ProductType::Pom
+        })
+        .fold(HashMap::new(), |mut map, (found_product, sub)| {
+            if let Some(existing) = map.get(&found_product) {
+                if existing.get_next_due_date().ok() < sub.get_next_due_date().ok() {
+                    map.insert(found_product, sub);
+                }
+            } else {
+                map.insert(found_product, sub);
             }
-            Ok(())
+            map
+        });
+
+    if subscriptions.is_empty() {
+        bail!("No matching subscription key found for server ID '{server_id}'");
+    }
+
+    for (product, subscription) in subscriptions {
+        eprintln!("Setting offline subscription key for {product}...");
+        match set_subscription_key(&product, subscription) {
+            Ok(output) if !output.is_empty() => eprintln!("success: {output}"),
+            Ok(_) => eprintln!("success."),
+            Err(err) => eprintln!("error: {err}"),
         }
-        None => bail!("No matching subscription key found for product '{product}' and server ID '{server_id}'"),
     }
+
+    Ok(())
 }
 
 #[api(
-- 
2.39.2

[pbs-devel] [PATCH proxmox-offline-mirror v2 3/3] offline mirror binary: rustfmt clean up

[Stefan Sterz]

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
 src/bin/proxmox-offline-mirror.rs | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/src/bin/proxmox-offline-mirror.rs b/src/bin/proxmox-offline-mirror.rs
index a6d6c49..c645c2d 100644
--- a/src/bin/proxmox-offline-mirror.rs
+++ b/src/bin/proxmox-offline-mirror.rs
@@ -179,8 +179,8 @@ fn derive_debian_repo(
         (Release::Bookworm, DebianVariant::Security) => {
             "/usr/share/keyrings/debian-archive-bookworm-security-automatic.gpg"
         }
-        (Release::Bookworm, DebianVariant::Updates) |
-        (Release::Bookworm, DebianVariant::Backports) => {
+        (Release::Bookworm, DebianVariant::Updates)
+        | (Release::Bookworm, DebianVariant::Backports) => {
             "/usr/share/keyrings/debian-archive-bookworm-automatic.gpg"
         }
         (Release::Bookworm, _) => "/usr/share/keyrings/debian-archive-bookworm-stable.gpg",
@@ -239,13 +239,11 @@ fn action_add_mirror(config: &SectionConfigData) -> Result<Vec<MirrorConfig>, Er
 
                 let default_components = match release {
                     Release::Bookworm => "main contrib non-free non-free-firmware",
-                    _ => "main contrib non-free"
+                    _ => "main contrib non-free",
                 };
 
-                let components = read_string_from_tty(
-                    "Enter repository components",
-                    Some(default_components),
-                )?;
+                let components =
+                    read_string_from_tty("Enter repository components", Some(default_components))?;
 
                 derive_debian_repo(release, variant, &components)?
             }
-- 
2.39.2

[pbs-devel] applied: [PATCH proxmox-offline-mirror v2 0/3] improve pom multi-key handling

[Fabian Grünbichler]

FWIW, this got applied back in December by Thomas :)

On November 29, 2023 3:51 pm, Stefan Sterz wrote:
> this patch series intends to improve the user experience when a user
> adds multiple keys to one mirror and wants to use one medium to activate
> multiple products. the first commit removes the `ProductType` trait that
> has been moved to `proxmox-subscription` from pom. the second commit
> handles media with muliple keys for muliple products more gracefully in
> proxmox-offline-mirror-helper. it only tries to activate subscriptions
> that have a matching server id. the finally commit just adds some
> rustfmt clean ups.
> 
> changes since v1 @ (thanks to Fabian Grünbichler):
> 
> - moved to a hash map based approach when automatically selecting all
>   applicable keys for the current system
> 
> 
> Stefan Sterz (3):
>   type: move `ProductType` enum to `proxmox-subscription`
>   helper: improve handling of multiple keys when activating them
>   offline mirror binary: rustfmt clean up
> 
>  src/bin/proxmox-offline-mirror-helper.rs      | 101 ++++++++++--------
>  src/bin/proxmox-offline-mirror.rs             |  15 ++-
>  src/bin/proxmox_offline_mirror_cmds/medium.rs |   4 +-
>  .../subscription.rs                           |   4 +-
>  src/config.rs                                 |   4 +-
>  src/subscription.rs                           |   4 +-
>  src/types.rs                                  |  44 +-------
>  7 files changed, 75 insertions(+), 101 deletions(-)
> 
> --
> 2.39.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>