From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id E976B1FF13B for ; Wed, 25 Mar 2026 12:27:56 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 70A75150EE; Wed, 25 Mar 2026 12:28:17 +0100 (CET) Message-ID: <48da8840-9203-43e5-aa87-3559a04f310a@proxmox.com> Date: Wed, 25 Mar 2026 12:28:08 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH] client: support individual repository component parameters To: =?UTF-8?Q?Fabian_Gr=C3=BCnbichler?= , pbs-devel@lists.proxmox.com References: <20260323211400.2661765-1-t.lamprecht@proxmox.com> <1774358455.07xtrnyeyt.astroid@yuna.none> Content-Language: en-US From: Thomas Lamprecht In-Reply-To: <1774358455.07xtrnyeyt.astroid@yuna.none> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1774438041649 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.011 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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: 5HTSOCLUVTKGORNZISU35VS3BHP3O36N X-Message-ID-Hash: 5HTSOCLUVTKGORNZISU35VS3BHP3O36N X-MailFrom: t.lamprecht@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: Am 24.03.26 um 14:42 schrieb Fabian Grünbichler: > On March 23, 2026 10:11 pm, Thomas Lamprecht wrote: >> +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 with >> +``--repository`` and then running a backup with ``--server``/``--datastore`` >> +(or vice versa) reuses the same ticket, as long as the server address and >> +user match. >> + >> +The component options make it easy to change individual parts of the >> +connection, for example switching to a different datastore or server without >> +having to rewrite the entire repository string. They also simplify usage >> +with API tokens, which require escaping the ``@`` separator in the compact >> +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`.. yeah, that was more for inside perl use cases, but it's bogus; will drop. [...] >> @@ -70,6 +114,22 @@ Environment Variables >> ``PBS_REPOSITORY`` >> The default backup repository. >> >> +``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=5340 Yeah, that makes sense to incorporate in this series, thanks for the pointer. >> 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, shellword_split}; [...] >> +/// Remove repository-related keys from a JSON Value and return the parsed [`BackupRepository`]. >> +/// >> +/// This is used by commands that forward the remaining parameters to the server API after stripping >> +/// the repository fields. >> pub fn remove_repository_from_value(param: &mut Value) -> Result { >> - if let Some(url) = param >> + let map = 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 be a string)"))? >> - .parse(); >> + .ok_or_else(|| format_err!("unable to get repository (parameter is not an object)"))?; >> + >> + let to_string = |v: Value| v.as_str().map(String::from); >> + >> + let args = 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) => Ok(repo), >> + Err(_) => { >> + // Fall back to environment. >> + if let Some(url) = get_default_repository() { >> + return url.parse(); >> + } >> + let env_args = args_from_env(); >> + if env_args.has_atoms() { >> + return BackupRepository::try_from(env_args); >> + } >> + bail!("repository not passed via CLI options and unable to get (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() >> } >> >> +/// Extract a [`BackupRepository`] from CLI parameters. >> +/// >> +/// Resolution precedence: >> +/// 1. CLI `--repository` + CLI atoms → error (mutually exclusive) > > this doesn't work nicely in practice atm, e.g. when I run > > `proxmox-backup-client backup .. --repository "something" --auth-id "something"` : > > Error: unable to get (default) repository > > instead of a nice error point at the invalid combination of arguments.. ack >> +/// 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 = param["repository"] >> - .as_str() >> - .map(String::from) >> - .or_else(get_default_repository) >> - .ok_or_else(|| format_err!("unable to get (default) repository"))?; >> + let args = args_from_value(param); >> >> - let repo: BackupRepository = repo_url.parse()?; >> - >> - Ok(repo) >> + match BackupRepository::try_from(args) { >> + Ok(repo) => Ok(repo), >> + Err(_) => { > > probably this error here > >> + // Fall back to environment. >> + if let Some(url) = get_default_repository() { >> + return url.parse(); >> + } >> + let env_args = 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? I checked a few CLI tools and most allow mixing environment and CLI for different such atoms, e.g. restic or borg, but also others like mysql or psql, and rethinking this also what I would expect to happen. I'll recheck this and try to add some unit tests to check and encode the inteded behavior. >> +/// Extract a [`BackupRepository`] from a parameter map (used for shell >> +/// completion callbacks). >> pub fn extract_repository_from_map(param: &HashMap) -> Option { >> - param >> - .get("repository") >> - .map(String::from) >> - .or_else(get_default_repository) >> - .and_then(|repo_url| repo_url.parse::().ok()) >> + let args = 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) = BackupRepository::try_from(args) { >> + return Some(repo); >> + } >> + >> + // Fall back to environment: compound URL, then atoms. >> + if let Some(url) = get_default_repository() { >> + if let Ok(repo) = url.parse() { >> + return Some(repo); >> + } >> + } >> + >> + let env_args = 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 > Will recheck more closely, thanks for taking a look!