Re: [PATCH] client: support individual repository component parameters

[Maximiliano Sandoval <m.sandoval@proxmox.com>]

Thomas Lamprecht <t.lamprecht@proxmox.com> writes:

I will test this during the week. Having something akin to these
parameters has been in my TODO-list/wishlist since forever.

Some comments below.

> 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.
>
> 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.
>
> 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.
>
> 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.
>
> Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> ---
>
> 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.
>
>  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(-)
>
> 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 option, or
>  by setting the ``PBS_REPOSITORY`` environment variable.
>  
> +Alternatively, you can specify the repository components as separate
> +command-line options:
> +
> +``--server <host>``
> +  Backup server address (hostname or IP address). Defaults to ``localhost``.

I would mention here that this requires the datastore option, like the
docs a couple of chunks below do.

> +
> +``--port <number>``
> +  Backup server port. Defaults to ``8007``.
> +
> +``--datastore <name>``
> +  Name of the target datastore. Required when using component options instead
> +  of ``--repository``.
> +
> +``--auth-id <user@realm[!token]>``
> +  Authentication identity, either a user (``user@realm``) or an API token
> +  (``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 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:
> +
> +.. code-block:: console
> +
> +  # Using component options
> +  # proxmox-backup-client backup root.pxar:/ \
> +      --auth-id 'user@pbs!backup' --server pbs.example.com --datastore store1
> +
> +  # 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 the
> +   exclamation mark (``!``) is a special character in most shells

This makes me wonder if there should too be a dedicated --api-token
parameter too (that basically appends `!tokenname` to the user). I ran
into this issue a couple of times in the past, while having it in the
documentation is good, perhaps it is not the first place one would look
for this.

> +
>  The web interface provides copyable repository text in the datastore summary
>  with the `Show Connection Information` button.
>  
> @@ -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.

I am not sure about the use of "default", since one has to specify one
or the other. I would personally say something like:

Backup server address. Requires to be used in conjunction to --datastore
or PBS_DATASTORE. This option is not compatible with --repository or
PBS_REPOSITORY.

> +
> +``PBS_PORT``
> +  Default backup server port. Defaults to ``8007`` if unset.
> +
> [...]
>              "keyfile": {
>                  optional: true,

-- 
Maximiliano