Re: [pbs-devel] [PATCH proxmox-backup v2 1/2] proxmox-backup-api/proxy: Add proxmox-product-config as dependency

From: Dominik Csapak <d.csapak@proxmox.com>
To: "Proxmox Backup Server development discussion"
	<pbs-devel@lists.proxmox.com>,
	"Laurențiu Leahu-Vlăducu" <l.leahu-vladucu@proxmox.com>
Subject: Re: [pbs-devel] [PATCH proxmox-backup v2 1/2] proxmox-backup-api/proxy: Add proxmox-product-config as dependency
Date: Mon, 17 Feb 2025 12:49:16 +0100	[thread overview]
Message-ID: <683d6ab3-2d45-4b4b-ac0a-35980e00d4c8@proxmox.com> (raw)
In-Reply-To: <20250206122833.171965-3-l.leahu-vladucu@proxmox.com>

Hi,

On 2/6/25 13:28, Laurențiu Leahu-Vlăducu wrote:
> The proxmox-product-config crate has now been added as a dependency
> to allow easily serializing and deserializing config or state files.
> While this functionality was already supported by pbs-config, the
> previous code was always writing config files as the root user.

I find that sentence a bit confusing. what is the 'previous code'?
Also it sounds like the behavior changes from always writing as root
to not-always writing as root?

IMHO such a change would benefit from more explanation

> 
> In order to serialize/deserialize further config or state files
> without duplicating existing code unnecessarily, future code will be
> able to use the proxmox-product-config crate.

While it can be OK (IMO) to not immediately change all calls to the
new one, it would be nice to add a 'TODO' comment or similar
to the existing code paths. Otherwise we'll overlook them when we want to do it.

Otherwise we could switch them all over now, or is there any argument against that?

> 
> If the backup or root user could not be found, it is unsafe
> to continue, so the initialization will fail.>
> Signed-off-by: Laurențiu Leahu-Vlăducu <l.leahu-vladucu@proxmox.com>
> ---
>   Cargo.toml                      | 2 ++
>   src/bin/proxmox-backup-api.rs   | 2 ++
>   src/bin/proxmox-backup-proxy.rs | 2 ++
>   src/tools/mod.rs                | 9 +++++++++
>   4 files changed, 15 insertions(+)
> 
> diff --git a/Cargo.toml b/Cargo.toml
> index bc1e9ed2..55a80dcd 100644
> --- a/Cargo.toml
> +++ b/Cargo.toml
> @@ -71,6 +71,7 @@ proxmox-ldap = "0.2.1"
>   proxmox-metrics = "0.3.1"
>   proxmox-notify = "0.5.1"
>   proxmox-openid = "0.10.0"
> +proxmox-product-config = "0.2.2"
>   proxmox-rest-server = { version = "0.8.5", features = [ "templates" ] }
>   # some use "cli", some use "cli" and "server", pbs-config uses nothing
>   proxmox-router = { version = "3.0.0", default-features = false }
> @@ -218,6 +219,7 @@ proxmox-ldap.workspace = true
>   proxmox-metrics.workspace = true
>   proxmox-notify = { workspace = true, features = [ "pbs-context" ] }
>   proxmox-openid.workspace = true
> +proxmox-product-config.workspace = true
>   proxmox-rest-server = { workspace = true, features = [ "rate-limited-stream" ] }
>   proxmox-router = { workspace = true, features = [ "cli", "server"] }
>   proxmox-schema = { workspace = true, features = [ "api-macro" ] }
> diff --git a/src/bin/proxmox-backup-api.rs b/src/bin/proxmox-backup-api.rs
> index 829974d2..0b5863f3 100644
> --- a/src/bin/proxmox-backup-api.rs
> +++ b/src/bin/proxmox-backup-api.rs
> @@ -43,6 +43,8 @@ fn get_index() -> Pin<Box<dyn Future<Output = Response<Body>> + Send>> {
>   async fn run() -> Result<(), Error> {
>       init_logger("PBS_LOG", LevelFilter::INFO)?;
>   
> +    proxmox_backup::tools::init_product_config()?;
> +
>       config::create_configdir()?;
>   
>       config::update_self_signed_cert(false)?;
> diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs"
> index e9870532..edd0a4cc 100644
> --- a/src/bin/proxmox-backup-proxy.rs
> +++ b/src/bin/proxmox-backup-proxy.rs
> @@ -181,6 +181,8 @@ async fn get_index_future(env: RestEnvironment, parts: Parts) -> Response<Body>
>   async fn run() -> Result<(), Error> {
>       init_logger("PBS_LOG", LevelFilter::INFO)?;
>   
> +    proxmox_backup::tools::init_product_config()?;
> +
>       proxmox_backup::auth_helpers::setup_auth_context(false);
>       proxmox_backup::server::notifications::init()?;
>       metric_collection::init()?;
> diff --git a/src/tools/mod.rs b/src/tools/mod.rs
> index 322894dd..8887df6e 100644
> --- a/src/tools/mod.rs
> +++ b/src/tools/mod.rs
> @@ -61,3 +61,12 @@ pub fn setup_safe_path_env() {
>           std::env::remove_var(name);
>       }
>   }
> +
> +/// Initializes the global product configuration for the proxmox_product_config crate.
> +pub fn init_product_config() -> Result<(), Error> {
> +    let backup_user = pbs_config::backup_user()?;
> +    let root_user = nix::unistd::User::from_uid(nix::unistd::ROOT)?
> +        .ok_or(anyhow::format_err!("Could not find root user"))?;
> +    proxmox_product_config::init(backup_user, root_user);
> +    Ok(())
> +}

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