Re: [pbs-devel] [PATCH proxmox-backup 3/4] api: refactor remote client and add remote scan

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 04.11.20 14:10, Fabian Grünbichler wrote:
> to allow on-demand scanning of remote datastores accessible for the
> configured remote user.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> 
> Notes:
>     not 100% sure about PRIV_REMOTE_AUDIT vs PRIV_REMOTE_READ.. the latter is required to use a datastore for syncing/pull purposes


you are not syncing here, so why should the permissions required for
that matter, when getting a general list of datastores of a remote?

If, that would be an extra filter param to set.

I setup a remote with a token, got ->
GET /api2/json/config/remote/tuxis/scan: 401 Unauthorized: [client [::ffff:192.168.16.38]:47544] authentication failed - invalid user name in user id

> 
>  src/api2/config/remote.rs         | 66 ++++++++++++++++++++++++++++++-
>  src/api2/pull.rs                  | 12 +-----
>  src/bin/proxmox-backup-manager.rs | 26 +++---------
>  3 files changed, 71 insertions(+), 33 deletions(-)
> 
> diff --git a/src/api2/config/remote.rs b/src/api2/config/remote.rs
> index ffbba1d2..b415f63d 100644
> --- a/src/api2/config/remote.rs
> +++ b/src/api2/config/remote.rs
> @@ -1,4 +1,4 @@
> -use anyhow::{bail, Error};
> +use anyhow::{bail, format_err, Error};
>  use serde_json::Value;
>  use ::serde::{Deserialize, Serialize};
>  
> @@ -6,6 +6,7 @@ use proxmox::api::{api, ApiMethod, Router, RpcEnvironment, Permission};
>  use proxmox::tools::fs::open_file_locked;
>  
>  use crate::api2::types::*;
> +use crate::client::{HttpClient, HttpClientOptions};
>  use crate::config::cached_user_info::CachedUserInfo;
>  use crate::config::remote;
>  use crate::config::acl::{PRIV_REMOTE_AUDIT, PRIV_REMOTE_MODIFY};
> @@ -301,10 +302,71 @@ pub fn delete_remote(name: String, digest: Option<String>) -> Result<(), Error>
>      Ok(())
>  }
>  
> +/// Helper to get client for remote.cfg entry
> +pub async fn remote_client(remote: remote::Remote) -> Result<HttpClient, Error> {
> +    let options = HttpClientOptions::new()
> +        .password(Some(remote.password.clone()))
> +        .fingerprint(remote.fingerprint.clone());
> +
> +    let client = HttpClient::new(
> +        &remote.host,
> +        remote.port.unwrap_or(8007),
> +        &remote.userid,

sure about userid, shouldn't this be authid or is that the same here?
At least would explain the error I get..

> +        options)?;
> +    let _auth_info = client.login() // make sure we can auth
> +        .await
> +        .map_err(|err| format_err!("remote connection to '{}' failed - {}", remote.host, err))?;
> +
> +    Ok(client)
> +}
> +
> +
> +#[api(
> +    input: {
> +        properties: {
> +            name: {
> +                schema: REMOTE_ID_SCHEMA,
> +            },
> +        },
> +    },
> +    access: {
> +        permission: &Permission::Privilege(&["remote", "{name}"], PRIV_REMOTE_AUDIT, false),
> +    },
> +    returns: {
> +        description: "List the accessible datastores.",
> +        type: Array,
> +        items: {
> +            description: "Datastore name and description.",
> +            type: DataStoreListItem,
> +        },
> +    },
> +)]
> +/// List datastores of a remote.cfg entry
> +pub async fn scan_remote_datastores(name: String) -> Result<Vec<DataStoreListItem>, Error> {
> +    let (remote_config, _digest) = remote::config()?;
> +    let remote: remote::Remote = remote_config.lookup("remote", &name)?;
> +
> +    let client = remote_client(remote).await?;
> +    let api_res = client.get("api2/json/admin/datastore", None).await?;
> +    let parse_res = match api_res.get("data") {
> +        Some(data) => serde_json::from_value::<Vec<DataStoreListItem>>(data.to_owned()),
> +        None => bail!("remote {} did not return any datastore list data", &name),
> +    };
> +
> +    match parse_res {
> +        Ok(parsed) => Ok(parsed),
> +        Err(_) => bail!("Failed to parse remote scan api result."),
> +    }
> +}
> +
> +const SCAN_ROUTER: Router = Router::new()
> +    .get(&API_METHOD_SCAN_REMOTE_DATASTORES);
> +
>  const ITEM_ROUTER: Router = Router::new()
>      .get(&API_METHOD_READ_REMOTE)
>      .put(&API_METHOD_UPDATE_REMOTE)
> -    .delete(&API_METHOD_DELETE_REMOTE);
> +    .delete(&API_METHOD_DELETE_REMOTE)
> +    .subdirs(&[("scan", &SCAN_ROUTER)]);
>  
>  pub const ROUTER: Router = Router::new()
>      .get(&API_METHOD_LIST_REMOTES)
> diff --git a/src/api2/pull.rs b/src/api2/pull.rs
> index d9e9d31d..87015c72 100644
> --- a/src/api2/pull.rs
> +++ b/src/api2/pull.rs
> @@ -9,7 +9,7 @@ use proxmox::api::{ApiMethod, Router, RpcEnvironment, Permission};
>  
>  use crate::server::{WorkerTask, jobstate::Job};
>  use crate::backup::DataStore;
> -use crate::client::{HttpClient, HttpClientOptions, BackupRepository, pull::pull_store};
> +use crate::client::{HttpClient, BackupRepository, pull::pull_store};
>  use crate::api2::types::*;
>  use crate::config::{
>      remote,
> @@ -50,17 +50,9 @@ pub async fn get_pull_parameters(
>      let (remote_config, _digest) = remote::config()?;
>      let remote: remote::Remote = remote_config.lookup("remote", remote)?;
>  
> -    let options = HttpClientOptions::new()
> -        .password(Some(remote.password.clone()))
> -        .fingerprint(remote.fingerprint.clone());
> -
>      let src_repo = BackupRepository::new(Some(remote.userid.clone()), Some(remote.host.clone()), remote.port, remote_store.to_string());
>  
> -    let client = HttpClient::new(&src_repo.host(), src_repo.port(), &src_repo.auth_id(), options)?;
> -    let _auth_info = client.login() // make sure we can auth
> -        .await
> -        .map_err(|err| format_err!("remote connection to '{}' failed - {}", remote.host, err))?;
> -
> +    let client = crate::api2::config::remote::remote_client(remote).await?;
>  
>      Ok((client, src_repo, tgt_store))
>  }
> diff --git a/src/bin/proxmox-backup-manager.rs b/src/bin/proxmox-backup-manager.rs
> index 7499446b..e52c2f76 100644
> --- a/src/bin/proxmox-backup-manager.rs
> +++ b/src/bin/proxmox-backup-manager.rs
> @@ -413,29 +413,13 @@ pub fn complete_remote_datastore_name(_arg: &str, param: &HashMap<String, String
>  
>      let _ = proxmox::try_block!({
>          let remote = param.get("remote").ok_or_else(|| format_err!("no remote"))?;
> -        let (remote_config, _digest) = config::remote::config()?;
>  
> -        let remote: config::remote::Remote = remote_config.lookup("remote", &remote)?;
> +        let data = crate::tools::runtime::block_on(async move {
> +            crate::api2::config::remote::scan_remote_datastores(remote.clone()).await
> +        })?;
>  
> -        let options = HttpClientOptions::new()
> -            .password(Some(remote.password.clone()))
> -            .fingerprint(remote.fingerprint.clone());
> -
> -        let client = HttpClient::new(
> -            &remote.host,
> -            remote.port.unwrap_or(8007),
> -            &remote.userid,
> -            options,
> -        )?;
> -
> -        let result = crate::tools::runtime::block_on(client.get("api2/json/admin/datastore", None))?;
> -
> -        if let Some(data) = result["data"].as_array() {
> -            for item in data {
> -                if let Some(store) = item["store"].as_str() {
> -                    list.push(store.to_owned());
> -                }
> -            }
> +        for item in data {
> +            list.push(item.store);
>          }
>  
>          Ok(())
>