From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id C45D71FF144 for ; Tue, 24 Mar 2026 14:42:41 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D5F09124CA; Tue, 24 Mar 2026 14:43:01 +0100 (CET) Date: Tue, 24 Mar 2026 14:42:52 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= Subject: Re: [PATCH] client: support individual repository component parameters To: pbs-devel@lists.proxmox.com, Thomas Lamprecht References: <20260323211400.2661765-1-t.lamprecht@proxmox.com> In-Reply-To: <20260323211400.2661765-1-t.lamprecht@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.17.0 (https://github.com/astroidmail/astroid) Message-Id: <1774358455.07xtrnyeyt.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1774359728559 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.054 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: N27KGJ2VG7JBJCSW7CTVHYRCYDDVBGGY X-Message-ID-Hash: N27KGJ2VG7JBJCSW7CTVHYRCYDDVBGGY X-MailFrom: f.gruenbichler@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On March 23, 2026 10:11 pm, Thomas Lamprecht wrote: > The compact repository URL format ([[auth-id@]server[:port]:]datastore) > can be cumbersome to remember the exact syntax and work with. > It also makes it awkward to change a single aspect of the connection, > like switching to a different datastore, without rewriting the whole > string. >=20 > So add the "expanded" --server, --port, --datastore, and --auth-id as > separate CLI parameters, mutually exclusive with --repository. Both > forms resolve to the same BackupRepository internally. >=20 > Add a BackupRepositoryArgs that holds both the atoms and the combined > URL and replace the existing "repository" parameter in all client > tools, i.e. proxmox-backup-client, proxmox-file-restore, and > proxmox-backup-debug. This struct gets flattened into the cli (api) > schemas such that it's fully backward compatible. >=20 > Also add recognition for corresponding environment variables > (PBS_SERVER, PBS_PORT, PBS_DATASTORE, PBS_AUTH_ID) serve as fallback > when neither --repository/PBS_REPOSITORY nor CLI component options are > given. >=20 > Signed-off-by: Thomas Lamprecht > --- >=20 > Disclaimer: only lightly tested! but as this is somewhat of a pain point > I run myself into when adding a new Debian host to backup to a PBS via > the client, I finally wanted to have this improved and polished the POC > I have lying around for some time now already. >=20 > docs/backup-client.rst | 60 ++++++++++ > pbs-client/src/backup_repo.rs | 117 +++++++++++++++++- > pbs-client/src/tools/mod.rs | 159 +++++++++++++++++++------ > proxmox-backup-client/src/benchmark.rs | 8 +- > proxmox-backup-client/src/catalog.rs | 16 +-- > proxmox-backup-client/src/group.rs | 8 +- > proxmox-backup-client/src/main.rs | 68 +++++------ > proxmox-backup-client/src/mount.rs | 13 +- > proxmox-backup-client/src/namespace.rs | 20 ++-- > proxmox-backup-client/src/snapshot.rs | 52 ++++---- > proxmox-backup-client/src/task.rs | 20 ++-- > proxmox-file-restore/src/main.rs | 15 ++- > src/bin/proxmox_backup_debug/diff.rs | 9 +- > 13 files changed, 418 insertions(+), 147 deletions(-) >=20 > diff --git a/docs/backup-client.rst b/docs/backup-client.rst > index 40962f0e2..4d0467eb1 100644 > --- a/docs/backup-client.rst > +++ b/docs/backup-client.rst > @@ -28,6 +28,50 @@ brackets (for example, `[fe80::01]`). > You can pass the repository with the ``--repository`` command-line optio= n, or > by setting the ``PBS_REPOSITORY`` environment variable. > =20 > +Alternatively, you can specify the repository components as separate > +command-line options: > + > +``--server `` > + Backup server address (hostname or IP address). Defaults to ``localhos= t``. > + > +``--port `` > + Backup server port. Defaults to ``8007``. > + > +``--datastore `` > + Name of the target datastore. Required when using component options in= stead > + of ``--repository``. > + > +``--auth-id `` > + Authentication identity, either a user (``user@realm``) or an API toke= n > + (``user@realm!tokenname``). Defaults to ``root@pam``. > + > +These options are mutually exclusive with ``--repository``. Both forms > +resolve to the same internal representation, so cached login tickets and > +other session state are shared between them. For example, logging in wit= h > +``--repository`` and then running a backup with ``--server``/``--datasto= re`` > +(or vice versa) reuses the same ticket, as long as the server address an= d > +user match. > + > +The component options make it easy to change individual parts of the > +connection, for example switching to a different datastore or server wit= hout > +having to rewrite the entire repository string. They also simplify usage > +with API tokens, which require escaping the ``@`` separator in the compa= ct > +form: that last sentence I don't understand, the `@` is part of both a user and an API token authid, it's the `!` that is only there for tokens? I've never had to escape the `@` in `--repository`.. > + > +.. code-block:: console > + > + # Using component options > + # proxmox-backup-client backup root.pxar:/ \ > + --auth-id 'user@pbs!backup' --server pbs.example.com --datastore s= tore1 > + > + # Equivalent compact form (the \@ disambiguates the user@realm > + # separator from the user-to-host separator in the URL format) > + # proxmox-backup-client backup root.pxar:/ \ > + --repository 'user\@pbs!backup@pbs.example.com:store1' > + > +.. Note:: Remember to quote API token identifiers on the shell, since th= e > + exclamation mark (``!``) is a special character in most shells. > + > The web interface provides copyable repository text in the datastore sum= mary > with the `Show Connection Information` button. > =20 > @@ -70,6 +114,22 @@ Environment Variables > ``PBS_REPOSITORY`` > The default backup repository. > =20 > +``PBS_SERVER`` > + Default backup server address. Used as a fallback when neither > + ``--repository`` / ``PBS_REPOSITORY`` nor ``--server`` is given. > + Requires ``PBS_DATASTORE`` to be set as well. > + > +``PBS_PORT`` > + Default backup server port. Defaults to ``8007`` if unset. > + > +``PBS_DATASTORE`` > + Default datastore name. Used as a fallback when neither ``--repository= `` / > + ``PBS_REPOSITORY`` nor ``--datastore`` is given. > + > +``PBS_AUTH_ID`` > + Default authentication identity (``user@realm`` or > + ``user@realm!tokenname``). Defaults to ``root@pam`` if unset. > + we also have a request open for PBS_NAMESPACE, with similar rationale as your changes here: https://bugzilla.proxmox.com/show_bug.cgi?id=3D5340 > ``PBS_PASSWORD`` > When set, this value is used as the password for the backup server. > You can also set this to an API token secret. > diff --git a/pbs-client/src/backup_repo.rs b/pbs-client/src/backup_repo.r= s > index 458f89dc2..8d2d356f4 100644 > --- a/pbs-client/src/backup_repo.rs > +++ b/pbs-client/src/backup_repo.rs > @@ -1,8 +1,121 @@ > use std::fmt; > =20 > -use anyhow::{format_err, Error}; > +use anyhow::{bail, format_err, Error}; > +use serde::{Deserialize, Serialize}; > =20 > -use pbs_api_types::{Authid, Userid, BACKUP_REPO_URL_REGEX, IP_V6_REGEX}; > +use proxmox_schema::api; > +use proxmox_schema::*; > + > +use pbs_api_types::{ > + Authid, Userid, BACKUP_REPO_URL, BACKUP_REPO_URL_REGEX, DATASTORE_SC= HEMA, IP_V6_REGEX, > +}; > + > +pub const REPO_URL_SCHEMA: Schema =3D > + StringSchema::new("Repository URL: [[auth-id@]server[:port]:]datasto= re") > + .format(&BACKUP_REPO_URL) > + .max_length(256) > + .schema(); > + > +pub const BACKUP_REPO_SERVER_SCHEMA: Schema =3D > + StringSchema::new("Backup server address (hostname or IP). Default: = localhost") > + .format(&api_types::DNS_NAME_OR_IP_FORMAT) > + .max_length(256) > + .schema(); > + > +pub const BACKUP_REPO_PORT_SCHEMA: Schema =3D IntegerSchema::new("Backup= server port. Default: 8007") > + .minimum(1) > + .maximum(65535) > + .default(8007) > + .schema(); > + > +#[api( > + properties: { > + repository: { > + schema: REPO_URL_SCHEMA, > + optional: true, > + }, > + server: { > + schema: BACKUP_REPO_SERVER_SCHEMA, > + optional: true, > + }, > + port: { > + schema: BACKUP_REPO_PORT_SCHEMA, > + optional: true, > + }, > + datastore: { > + schema: DATASTORE_SCHEMA, > + optional: true, > + }, > + "auth-id": { > + type: Authid, > + optional: true, > + }, > + }, > +)] > +#[derive(Default, Serialize, Deserialize)] > +#[serde(rename_all =3D "kebab-case")] > +/// Backup repository location, specified either as a repository URL or = as individual components > +/// (server, port, datastore, auth-id). > +pub struct BackupRepositoryArgs { > + #[serde(skip_serializing_if =3D "Option::is_none")] > + pub repository: Option, > + #[serde(skip_serializing_if =3D "Option::is_none")] > + pub server: Option, > + #[serde(skip_serializing_if =3D "Option::is_none")] > + pub port: Option, > + #[serde(skip_serializing_if =3D "Option::is_none")] > + pub datastore: Option, > + #[serde(skip_serializing_if =3D "Option::is_none")] > + pub auth_id: Option, > +} > + > +impl BackupRepositoryArgs { > + /// Returns `true` if any atom parameter (server, port, datastore, o= r auth-id) is set. > + pub fn has_atoms(&self) -> bool { > + self.server.is_some() > + || self.port.is_some() > + || self.datastore.is_some() > + || self.auth_id.is_some() > + } > +} > + > +impl TryFrom for BackupRepository { > + type Error =3D anyhow::Error; > + > + /// Convert explicit CLI arguments into a [`BackupRepository`]. > + /// > + /// * If `repository` and any atom are both set, returns an error. > + /// * If atoms are present, builds the repository from them (require= s `datastore`). > + /// * If only `repository` is set, parses the repo URL. > + /// * If nothing is set, returns an error - callers must fall back t= o environment variables / > + /// credentials themselves. > + fn try_from(args: BackupRepositoryArgs) -> Result= { > + let has_url =3D args.repository.is_some(); > + let has_atoms =3D args.has_atoms(); > + > + if has_url && has_atoms { > + bail!("--repository and --server/--port/--datastore/--auth-i= d are mutually exclusive"); > + } > + > + if has_atoms { > + let store =3D args.datastore.ok_or_else(|| { > + format_err!("--datastore is required when not using --re= pository") > + })?; > + return Ok(BackupRepository::new( > + args.auth_id, > + args.server, > + args.port, > + store, > + )); > + } > + > + if let Some(url) =3D args.repository { > + return url.parse(); > + } > + > + anyhow::bail!("no repository specified") > + } > +} > =20 > /// Reference remote backup locations > /// > diff --git a/pbs-client/src/tools/mod.rs b/pbs-client/src/tools/mod.rs > index 7a496d14c..ad83d8cab 100644 > --- a/pbs-client/src/tools/mod.rs > +++ b/pbs-client/src/tools/mod.rs > @@ -17,12 +17,14 @@ use proxmox_router::cli::{complete_file_name, shellwo= rd_split}; > use proxmox_schema::*; > use proxmox_sys::fs::file_get_json; > =20 > -use pbs_api_types::{ > - Authid, BackupArchiveName, BackupNamespace, RateLimitConfig, UserWit= hTokens, BACKUP_REPO_URL, > -}; > +use pbs_api_types::{Authid, BackupArchiveName, BackupNamespace, RateLimi= tConfig, UserWithTokens}; > use pbs_datastore::BackupManifest; > =20 > -use crate::{BackupRepository, HttpClient, HttpClientOptions}; > +use crate::{BackupRepository, BackupRepositoryArgs, HttpClient, HttpClie= ntOptions}; > + > +// Re-export for backward compatibility; the canonical definition is now= in > +// backup_repo.rs alongside BackupRepositoryArgs. > +pub use crate::REPO_URL_SCHEMA; > =20 > pub mod key_source; > =20 > @@ -30,6 +32,10 @@ const ENV_VAR_PBS_FINGERPRINT: &str =3D "PBS_FINGERPRI= NT"; > const ENV_VAR_PBS_PASSWORD: &str =3D "PBS_PASSWORD"; > const ENV_VAR_PBS_ENCRYPTION_PASSWORD: &str =3D "PBS_ENCRYPTION_PASSWORD= "; > const ENV_VAR_PBS_REPOSITORY: &str =3D "PBS_REPOSITORY"; > +const ENV_VAR_PBS_SERVER: &str =3D "PBS_SERVER"; > +const ENV_VAR_PBS_PORT: &str =3D "PBS_PORT"; > +const ENV_VAR_PBS_DATASTORE: &str =3D "PBS_DATASTORE"; > +const ENV_VAR_PBS_AUTH_ID: &str =3D "PBS_AUTH_ID"; > =20 > /// Directory with system [credential]s. See systemd-creds(1). > /// > @@ -44,11 +50,6 @@ const CRED_PBS_REPOSITORY: &str =3D "proxmox-backup-cl= ient.repository"; > /// Credential name of the the fingerprint. > const CRED_PBS_FINGERPRINT: &str =3D "proxmox-backup-client.fingerprint"= ; > =20 > -pub const REPO_URL_SCHEMA: Schema =3D StringSchema::new("Repository URL.= ") > - .format(&BACKUP_REPO_URL) > - .max_length(256) > - .schema(); > - > pub const CHUNK_SIZE_SCHEMA: Schema =3D IntegerSchema::new("Chunk size i= n KB. Must be a power of 2.") > .minimum(64) > .maximum(4096) > @@ -233,41 +234,131 @@ pub fn get_fingerprint() -> Option { > .unwrap_or_default() > } > =20 > +/// Build [`BackupRepositoryArgs`] from the fields in a JSON Value. > +fn args_from_value(param: &Value) -> BackupRepositoryArgs { > + BackupRepositoryArgs { > + repository: param["repository"].as_str().map(String::from), > + server: param["server"].as_str().map(String::from), > + port: param["port"].as_u64().map(|p| p as u16), > + datastore: param["datastore"].as_str().map(String::from), > + auth_id: param["auth-id"] > + .as_str() > + .and_then(|s| s.parse::().ok()), > + } > +} > + > +/// Build [`BackupRepositoryArgs`] from `PBS_*` environment variables. > +fn args_from_env() -> BackupRepositoryArgs { > + BackupRepositoryArgs { > + repository: None, > + server: std::env::var(ENV_VAR_PBS_SERVER).ok(), > + port: std::env::var(ENV_VAR_PBS_PORT) > + .ok() > + .and_then(|p| p.parse::().ok()), > + datastore: std::env::var(ENV_VAR_PBS_DATASTORE).ok(), > + auth_id: std::env::var(ENV_VAR_PBS_AUTH_ID) > + .ok() > + .and_then(|s| s.parse::().ok()), > + } > +} > + > +/// Remove repository-related keys from a JSON Value and return the pars= ed [`BackupRepository`]. > +/// > +/// This is used by commands that forward the remaining parameters to th= e server API after stripping > +/// the repository fields. > pub fn remove_repository_from_value(param: &mut Value) -> Result { > - if let Some(url) =3D param > + let map =3D param > .as_object_mut() > - .ok_or_else(|| format_err!("unable to get repository (parameter = is not an object)"))? > - .remove("repository") > - { > - return url > - .as_str() > - .ok_or_else(|| format_err!("invalid repository value (must b= e a string)"))? > - .parse(); > + .ok_or_else(|| format_err!("unable to get repository (parameter = is not an object)"))?; > + > + let to_string =3D |v: Value| v.as_str().map(String::from); > + > + let args =3D BackupRepositoryArgs { > + repository: map.remove("repository").and_then(to_string), > + server: map.remove("server").and_then(to_string), > + port: map > + .remove("port") > + .and_then(|v| v.as_u64()) > + .map(|p| p as u16), > + datastore: map.remove("datastore").and_then(to_string), > + auth_id: map > + .remove("auth-id") > + .and_then(to_string) > + .map(|s| s.parse::()) > + .transpose()?, > + }; > + > + match BackupRepository::try_from(args) { > + Ok(repo) =3D> Ok(repo), > + Err(_) =3D> { > + // Fall back to environment. > + if let Some(url) =3D get_default_repository() { > + return url.parse(); > + } > + let env_args =3D args_from_env(); > + if env_args.has_atoms() { > + return BackupRepository::try_from(env_args); > + } > + bail!("repository not passed via CLI options and unable to g= et (default) repository from environment"); a similar issue like below also exists here I think, the code path below was just how I triggered it.. > + } > } > - > - get_default_repository() > - .ok_or_else(|| format_err!("unable to get default repository"))? > - .parse() > } > =20 > +/// Extract a [`BackupRepository`] from CLI parameters. > +/// > +/// Resolution precedence: > +/// 1. CLI `--repository` + CLI atoms =E2=86=92 error (mutually exclusiv= e) this doesn't work nicely in practice atm, e.g. when I run `proxmox-backup-client backup .. --repository "something" --auth-id "someth= ing"` : Error: unable to get (default) repository instead of a nice error point at the invalid combination of arguments.. > +/// 2. CLI atoms (at least `--datastore`) > +/// 3. CLI `--repository` > +/// 4. `PBS_REPOSITORY` environment variable / systemd credential > +/// 5. `PBS_SERVER`/`PBS_PORT`/`PBS_DATASTORE`/`PBS_AUTH_ID` environment= variables > pub fn extract_repository_from_value(param: &Value) -> Result { > - let repo_url =3D param["repository"] > - .as_str() > - .map(String::from) > - .or_else(get_default_repository) > - .ok_or_else(|| format_err!("unable to get (default) repository")= )?; > + let args =3D args_from_value(param); > =20 > - let repo: BackupRepository =3D repo_url.parse()?; > - > - Ok(repo) > + match BackupRepository::try_from(args) { > + Ok(repo) =3D> Ok(repo), > + Err(_) =3D> { probably this error here > + // Fall back to environment. > + if let Some(url) =3D get_default_repository() { > + return url.parse(); > + } > + let env_args =3D args_from_env(); > + if env_args.has_atoms() { > + return BackupRepository::try_from(env_args); > + } > + bail!("unable to get (default) repository"); should be printed here? or we should not fall back to the environment if at least one atom or the URL is given as argument? what about mix and match, e.g. setting the server via the env, but the datastore and namespace via arguments? > + } > + } > } > =20 > +/// Extract a [`BackupRepository`] from a parameter map (used for shell > +/// completion callbacks). > pub fn extract_repository_from_map(param: &HashMap) -> O= ption { > - param > - .get("repository") > - .map(String::from) > - .or_else(get_default_repository) > - .and_then(|repo_url| repo_url.parse::().ok()) > + let args =3D BackupRepositoryArgs { > + repository: param.get("repository").cloned(), > + server: param.get("server").cloned(), > + port: param.get("port").and_then(|p| p.parse().ok()), > + datastore: param.get("datastore").cloned(), > + auth_id: param.get("auth-id").and_then(|s| s.parse().ok()), > + }; > + > + if let Ok(repo) =3D BackupRepository::try_from(args) { > + return Some(repo); > + } > + > + // Fall back to environment: compound URL, then atoms. > + if let Some(url) =3D get_default_repository() { > + if let Ok(repo) =3D url.parse() { > + return Some(repo); > + } > + } > + > + let env_args =3D args_from_env(); > + if env_args.has_atoms() { > + return BackupRepository::try_from(env_args).ok(); > + } > + > + None > } this helper probably requires similar attention for consistency's sake > =20 > pub fn connect(repo: &BackupRepository) -> Result { > [..]