Re: [PATCH datacenter-manager v2 3/8] subscription: add key pool data model and config layer

["Lukas Wagner" <l.wagner@proxmox.com>]

Looking good mostly, some notes inline.

Inline comments are mostly cosmetic, so in any way:

Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>

On Thu May 7, 2026 at 10:26 AM CEST, Thomas Lamprecht wrote:
[...]
> +#[api(
> +    properties: {
> +        "key": { schema: SUBSCRIPTION_KEY_SCHEMA },
> +        "level": { optional: true },
> +        "status": { optional: true },
> +        "source": { optional: true },
> +    },
> +)]
> +#[derive(Clone, Debug, Default, Deserialize, Serialize, PartialEq)]
> +#[serde(rename_all = "kebab-case")]
> +/// A subscription key entry in the PDM key pool.
> +pub struct SubscriptionKeyEntry {
> +    /// The subscription key (for example, pve4b-1234567890).
> +    pub key: String,
> +
> +    /// Product type derived from the key prefix.
> +    #[serde(rename = "product-type")]
> +    pub product_type: ProductType,
> +
> +    /// Subscription level, derived from the key suffix.
> +    #[serde(default)]
> +    pub level: SubscriptionLevel,

the subscription level is serialized uppercase, e.g. "Community" -- is
this intentional? Not a big deal IMO, just wanted to make sure if this was
on purpose or not.

> +
> +    /// Where the key entry came from. Defaults to manual entry.
> +    #[serde(default)]
> +    pub source: SubscriptionKeySource,
> +
> +    /// Remote this key is assigned to (if any).
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    pub remote: Option<String>,
> +
> +    /// Node within the remote this key is assigned to (if any).
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    pub node: Option<String>,
> +
> +    /// Server ID this key is bound to (from signed info, if available).
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    pub serverid: Option<String>,

Same note as at (1) with regards to hyphenation, but can also stay as is
to have consistency with the API in PVE and PBS...

> +
> +    /// Subscription status from last check.
> +    #[serde(default)]
> +    pub status: SubscriptionStatus,
> +
> +    /// Next due date.
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    pub nextduedate: Option<String>,

(1)

Personally i'm not a big fan of these word amalgamations, I think I'd
prefer 'next_due_date' (serialized as 'next-due-date'). That being said,
it might still be fine to keep as is, since that is the exact spelling
used in the PVE/PBS API ... so no hard feelings from my side.

> +
> +    /// Product name.
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    pub productname: Option<String>,

Same as (1). Additionally, we have `product-type` above, so I think
`product-name` would make the config make self-consistent.

> +
> +    /// Epoch of last import or refresh of this key's data.
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    pub checktime: Option<i64>,
> +}
> +
> +impl ApiSectionDataEntry for SubscriptionKeyEntry {
> +    const INTERNALLY_TAGGED: Option<&'static str> = Some("product-type");
> +    const SECION_CONFIG_USES_TYPE_KEY: bool = true;
> +
> +    fn section_config() -> &'static SectionConfig {
> +        static CONFIG: OnceLock<SectionConfig> = OnceLock::new();
> +
> +        CONFIG.get_or_init(|| {
> +            let mut this =
> +                SectionConfig::new(&SUBSCRIPTION_KEY_SCHEMA).with_type_key("product-type");
> +            for ty in [
> +                ProductType::Pve,
> +                ProductType::Pbs,
> +                ProductType::Pmg,
> +                ProductType::Pom,
> +            ] {
> +                this.register_plugin(SectionConfigPlugin::new(
> +                    ty.as_section_type().to_string(),
> +                    Some("key".to_string()),
> +                    SubscriptionKeyEntry::API_SCHEMA.unwrap_object_schema(),
> +                ));
> +            }
> +            this
> +        })
> +    }
> +
> +    fn section_type(&self) -> &'static str {
> +        self.product_type.as_section_type()
> +    }
> +}
> +
> +#[api(
> +    properties: {
> +        "key": { schema: SUBSCRIPTION_KEY_SCHEMA },
> +    },
> +)]
> +#[derive(Clone, Debug, Default, Deserialize, Serialize)]
> +#[serde(rename_all = "kebab-case")]
> +/// Shadow entry storing the signed subscription info blob for a key.
> +///
> +/// Currently only populated by the future shop-bundle import flow; manually-added keys leave
> +/// this table empty. The data layer is in place so that adding the import path later does not
> +/// require reshaping the on-disk config.
> +pub struct SubscriptionKeyShadow {
> +    /// The subscription key.
> +    pub key: String,
> +
> +    /// Product type (section type marker).
> +    #[serde(rename = "product-type")]
> +    pub product_type: ProductType,
> +
> +    /// Base64-encoded signed SubscriptionInfo JSON.
> +    #[serde(default)]
> +    pub info: String,
> +}
> +
> +impl ApiSectionDataEntry for SubscriptionKeyShadow {
> +    const INTERNALLY_TAGGED: Option<&'static str> = Some("product-type");
> +    const SECION_CONFIG_USES_TYPE_KEY: bool = true;
> +
> +    fn section_config() -> &'static SectionConfig {
> +        static CONFIG: OnceLock<SectionConfig> = OnceLock::new();
> +
> +        CONFIG.get_or_init(|| {
> +            let mut this =
> +                SectionConfig::new(&SUBSCRIPTION_KEY_SCHEMA).with_type_key("product-type");
> +            for ty in [
> +                ProductType::Pve,
> +                ProductType::Pbs,
> +                ProductType::Pmg,
> +                ProductType::Pom,
> +            ] {
> +                this.register_plugin(SectionConfigPlugin::new(
> +                    ty.as_section_type().to_string(),
> +                    Some("key".to_string()),
> +                    SubscriptionKeyShadow::API_SCHEMA.unwrap_object_schema(),
> +                ));
> +            }
> +            this
> +        })
> +    }
> +
> +    fn section_type(&self) -> &'static str {
> +        self.product_type.as_section_type()
> +    }
> +}
> +
> +/// Decode a base64-encoded `SubscriptionInfo` JSON blob from the shadow file.
> +///
> +/// Forward-compat helper for the future shop-bundle import path. Returns the parsed
> +/// `SubscriptionInfo`; the caller is responsible for verifying the signature against the shop's
> +/// signing key.
> +pub fn parse_signed_info_blob(b64: &str) -> Result<SubscriptionInfo, Error> {
> +    let bytes = proxmox_base64::decode(b64)?;
> +    let info = serde_json::from_slice(&bytes)?;
> +    Ok(info)
> +}
> +
> +/// Cross-check the `serverid` of a shadowed entry against what the remote reports.
> +///
> +/// Forward-compat helper for the future bundle-import and push flow: when the shadow has a
> +/// signed serverid binding, the operator should be warned if the remote it is being pushed to
> +/// has a different hardware id. Returns Ok(None) when there is nothing to compare.
> +pub fn verify_serverid(
> +    entry: &SubscriptionKeyEntry,
> +    remote_info: &SubscriptionInfo,
> +) -> Result<Option<ServeridMismatch>, Error> {
> +    let Some(expected) = entry.serverid.as_deref() else {
> +        return Ok(None);
> +    };
> +    let Some(actual) = remote_info.serverid.as_deref() else {
> +        return Ok(None);
> +    };
> +    if expected == actual {
> +        Ok(None)
> +    } else {
> +        Ok(Some(ServeridMismatch {
> +            key: entry.key.clone(),
> +            expected: expected.to_string(),
> +            actual: actual.to_string(),
> +        }))
> +    }
> +}
> +
> +/// Result of [`verify_serverid`] when the bound and observed server-ids disagree.
> +#[derive(Clone, Debug, PartialEq, Eq)]
> +pub struct ServeridMismatch {
> +    pub key: String,
> +    pub expected: String,
> +    pub actual: String,
> +}

This could use some docstrings for the struct members, as to make it
clearer what `expected` and `actual` acutally refer to. Alternatively or
additionally, it could be {actual,expected}_server_id

> +
> +#[api]
> +#[derive(Clone, Debug, Default, Serialize, Deserialize, PartialEq)]
> +#[serde(rename_all = "kebab-case")]
> +/// Subscription status of a single remote node, combining remote query data with key pool
> +/// assignment information.
> +pub struct RemoteNodeStatus {
> +    /// Remote name.
> +    pub remote: String,
> +    /// Remote type (pve or pbs).
> +    #[serde(rename = "type")]
> +    pub ty: RemoteType,
> +    /// Node name.
> +    pub node: String,
> +    /// Number of CPU sockets (PVE only).
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    pub sockets: Option<i64>,
> +    /// Current subscription status.
> +    #[serde(default)]
> +    pub status: SubscriptionStatus,
> +    /// Subscription level.
> +    #[serde(default)]
> +    pub level: SubscriptionLevel,
> +    /// Currently assigned key from the pool (if any).
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    pub assigned_key: Option<String>,
> +    /// Current key on the node (from remote query).
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    pub current_key: Option<String>,
> +}
> +
> +#[api]
> +#[derive(Clone, Debug, Default, Serialize, Deserialize, PartialEq, Eq)]
> +#[serde(rename_all = "kebab-case")]
> +/// A proposed key-to-node assignment from the auto-assign algorithm.
> +pub struct ProposedAssignment {
> +    /// The subscription key to assign.
> +    pub key: String,
> +    /// Target remote.
> +    pub remote: String,
> +    /// Target node.
> +    pub node: String,
> +    /// Socket count of the key (PVE only).
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    pub key_sockets: Option<u32>,
> +    /// Socket count of the node (PVE only).
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    pub node_sockets: Option<i64>,
> +}

[...]


> diff --git a/lib/pdm-config/src/lib.rs b/lib/pdm-config/src/lib.rs
> index 5b9bcca..46ad1a2 100644
> --- a/lib/pdm-config/src/lib.rs
> +++ b/lib/pdm-config/src/lib.rs
> @@ -8,6 +8,7 @@ pub mod domains;
>  pub mod node;
>  pub mod remotes;
>  pub mod setup;
> +pub mod subscriptions;
>  pub mod views;
>  
>  mod config_version_cache;
> diff --git a/lib/pdm-config/src/subscriptions.rs b/lib/pdm-config/src/subscriptions.rs

Doc-Strings for `pub` fn's in this file are missing. No big deal here
since the usage is clear enough from the signature alone, but still
mentioning it for completeness' sake.

> new file mode 100644
> index 0000000..7e930ba
> --- /dev/null
> +++ b/lib/pdm-config/src/subscriptions.rs
> @@ -0,0 +1,102 @@
> +//! Read/write subscription key pool configuration.
> +//!
> +//! Call [`init`] to inject a concrete `SubscriptionKeyConfig` instance before using the
> +//! module-level functions.
> +//!
> +//! The shadow-config functions stash signed `SubscriptionInfo` blobs alongside the plain key
> +//! entries, which is intended as future proofing for a more automated (shop) import without having
> +//! to adapt the data layer.
> +
> +use std::sync::OnceLock;
> +
> +use anyhow::Error;
> +
> +use proxmox_config_digest::ConfigDigest;
> +use proxmox_product_config::{open_api_lockfile, replace_config, ApiLockGuard};
> +use proxmox_section_config::typed::{ApiSectionDataEntry, SectionConfigData};
> +
> +use pdm_api_types::subscription::{SubscriptionKeyEntry, SubscriptionKeyShadow};
> +use pdm_buildcfg::configdir;
> +
> +pub const SUBSCRIPTIONS_CFG_FILENAME: &str = configdir!("/subscriptions.cfg");
> +const SUBSCRIPTIONS_SHADOW_FILENAME: &str = configdir!("/subscriptions.shadow");
> +pub const SUBSCRIPTIONS_CFG_LOCKFILE: &str = configdir!("/.subscriptions.lock");
> +
> +static INSTANCE: OnceLock<Box<dyn SubscriptionKeyConfig + Send + Sync>> = OnceLock::new();
> +
> +fn instance() -> &'static (dyn SubscriptionKeyConfig + Send + Sync) {
> +    INSTANCE
> +        .get()
> +        .expect("subscription key config not initialized")
> +        .as_ref()
> +}
> +
> +pub fn lock_config() -> Result<ApiLockGuard, Error> {
> +    instance().lock_config()
> +}
> +
> +pub fn config() -> Result<(SectionConfigData<SubscriptionKeyEntry>, ConfigDigest), Error> {
> +    instance().config()
> +}
> +
> +pub fn shadow_config() -> Result<SectionConfigData<SubscriptionKeyShadow>, Error> {
> +    instance().shadow_config()
> +}
> +
> +pub fn save_config(config: &SectionConfigData<SubscriptionKeyEntry>) -> Result<(), Error> {
> +    instance().save_config(config)
> +}
> +
> +pub fn save_shadow(shadow: &SectionConfigData<SubscriptionKeyShadow>) -> Result<(), Error> {
> +    instance().save_shadow(shadow)
> +}
> +
> +pub trait SubscriptionKeyConfig {
> +    fn config(&self) -> Result<(SectionConfigData<SubscriptionKeyEntry>, ConfigDigest), Error>;
> +    fn shadow_config(&self) -> Result<SectionConfigData<SubscriptionKeyShadow>, Error>;
> +    fn lock_config(&self) -> Result<ApiLockGuard, Error>;
> +    fn save_config(&self, config: &SectionConfigData<SubscriptionKeyEntry>) -> Result<(), Error>;
> +    fn save_shadow(&self, shadow: &SectionConfigData<SubscriptionKeyShadow>) -> Result<(), Error>;
> +}
> +
> +pub struct DefaultSubscriptionKeyConfig;
> +
> +impl SubscriptionKeyConfig for DefaultSubscriptionKeyConfig {
> +    fn lock_config(&self) -> Result<ApiLockGuard, Error> {
> +        open_api_lockfile(SUBSCRIPTIONS_CFG_LOCKFILE, None, true)
> +    }
> +
> +    fn config(&self) -> Result<(SectionConfigData<SubscriptionKeyEntry>, ConfigDigest), Error> {
> +        let content = proxmox_sys::fs::file_read_optional_string(SUBSCRIPTIONS_CFG_FILENAME)?
> +            .unwrap_or_default();
> +
> +        let digest = openssl::sha::sha256(content.as_bytes());
> +        let data =
> +            SubscriptionKeyEntry::parse_section_config(SUBSCRIPTIONS_CFG_FILENAME, &content)?;
> +
> +        Ok((data, digest.into()))
> +    }
> +
> +    fn shadow_config(&self) -> Result<SectionConfigData<SubscriptionKeyShadow>, Error> {
> +        let content = proxmox_sys::fs::file_read_optional_string(SUBSCRIPTIONS_SHADOW_FILENAME)?
> +            .unwrap_or_default();
> +        SubscriptionKeyShadow::parse_section_config(SUBSCRIPTIONS_SHADOW_FILENAME, &content)
> +    }
> +
> +    fn save_config(&self, config: &SectionConfigData<SubscriptionKeyEntry>) -> Result<(), Error> {
> +        let raw = SubscriptionKeyEntry::write_section_config(SUBSCRIPTIONS_CFG_FILENAME, config)?;
> +        replace_config(SUBSCRIPTIONS_CFG_FILENAME, raw.as_bytes())
> +    }
> +
> +    fn save_shadow(&self, shadow: &SectionConfigData<SubscriptionKeyShadow>) -> Result<(), Error> {
> +        let raw =
> +            SubscriptionKeyShadow::write_section_config(SUBSCRIPTIONS_SHADOW_FILENAME, shadow)?;
> +        replace_config(SUBSCRIPTIONS_SHADOW_FILENAME, raw.as_bytes())
> +    }
> +}
> +
> +pub fn init(instance: Box<dyn SubscriptionKeyConfig + Send + Sync>) {
> +    if INSTANCE.set(instance).is_err() {
> +        panic!("subscription key config instance already set");
> +    }
> +}