[pbs-devel] [PATCH proxmox-backup 1/4] www: don't default to hourly sync job schedule

[pbs-devel] [PATCH proxmox-backup 1/4] www: don't default to hourly sync job schedule

[Fabian Grünbichler]

this breaks editing disabled sync jobs, and is also not consistent with
the backend defaults.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 www/window/SyncJobEdit.js | 1 -
 1 file changed, 1 deletion(-)

diff --git a/www/window/SyncJobEdit.js b/www/window/SyncJobEdit.js
index c89b95d2..43338cab 100644
--- a/www/window/SyncJobEdit.js
+++ b/www/window/SyncJobEdit.js
@@ -84,7 +84,6 @@ Ext.define('PBS.window.SyncJobEdit', {
 		fieldLabel: gettext('Schedule'),
 		xtype: 'pbsCalendarEvent',
 		name: 'schedule',
-		value: 'hourly',
 		emptyText: gettext('none (disabled)'),
 		cbind: {
 		    deleteEmpty: '{!isCreate}',
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 2/4] types: extract DataStoreListItem

[Fabian Grünbichler]

for reuse in remote scan API call

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/api2/admin/datastore.rs | 23 ++++++++---------------
 src/api2/types/mod.rs       | 19 +++++++++++++++++++
 2 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index a5d3e979..b051e8dd 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -902,15 +902,7 @@ pub fn garbage_collection_status(
         type: Array,
         items: {
             description: "Datastore name and description.",
-            properties: {
-                store: {
-                    schema: DATASTORE_SCHEMA,
-                },
-                comment: {
-                    optional: true,
-                    schema: SINGLE_LINE_COMMENT_SCHEMA,
-                },
-            },
+            type: DataStoreListItem,
         },
     },
     access: {
@@ -922,7 +914,7 @@ fn get_datastore_list(
     _param: Value,
     _info: &ApiMethod,
     rpcenv: &mut dyn RpcEnvironment,
-) -> Result<Value, Error> {
+) -> Result<Vec<DataStoreListItem>, Error> {
 
     let (config, _digest) = datastore::config()?;
 
@@ -935,11 +927,12 @@ fn get_datastore_list(
         let user_privs = user_info.lookup_privs(&auth_id, &["datastore", &store]);
         let allowed = (user_privs & (PRIV_DATASTORE_AUDIT| PRIV_DATASTORE_BACKUP)) != 0;
         if allowed {
-            let mut entry = json!({ "store": store });
-            if let Some(comment) = data["comment"].as_str() {
-                entry["comment"] = comment.into();
-            }
-            list.push(entry);
+            list.push(
+                DataStoreListItem {
+                    store: store.clone(),
+                    comment: data["comment"].as_str().map(String::from),
+                }
+            );
         }
     }
 
diff --git a/src/api2/types/mod.rs b/src/api2/types/mod.rs
index 7ee89f57..31fd89d2 100644
--- a/src/api2/types/mod.rs
+++ b/src/api2/types/mod.rs
@@ -370,6 +370,25 @@ pub const BLOCKDEVICE_NAME_SCHEMA: Schema = StringSchema::new("Block device name
 
 // Complex type definitions
 
+#[api(
+    properties: {
+        store: {
+            schema: DATASTORE_SCHEMA,
+        },
+        comment: {
+            optional: true,
+            schema: SINGLE_LINE_COMMENT_SCHEMA,
+        },
+    },
+)]
+#[derive(Serialize, Deserialize)]
+#[serde(rename_all="kebab-case")]
+/// Basic information about a datastore.
+pub struct DataStoreListItem {
+    pub store: String,
+    pub comment: Option<String>,
+}
+
 #[api(
     properties: {
         "backup-type": {
-- 
2.20.1

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

[Fabian Grünbichler]

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

 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,
+        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(())
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 4/4] www: add remote store selector

[Fabian Grünbichler]

(hopefully) improved upon NFS export selection in PVE

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    does not display loading errors nicely (the remote store picker needs to be
    opened before the remote changes, otherwise the elements needed to display the
    error don't exist yet)

 www/window/SyncJobEdit.js | 97 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 96 insertions(+), 1 deletion(-)

diff --git a/www/window/SyncJobEdit.js b/www/window/SyncJobEdit.js
index 43338cab..c71821d0 100644
--- a/www/window/SyncJobEdit.js
+++ b/www/window/SyncJobEdit.js
@@ -1,3 +1,90 @@
+Ext.define('PBS.form.RemoteStoreSelector', {
+    extend: 'Proxmox.form.ComboGrid',
+    alias: 'widget.pbsRemoteStoreSelector',
+
+    queryMode: 'local',
+
+    valueField: 'store',
+    displayField: 'store',
+    notFoundIsValid: true,
+
+    matchFieldWidth: false,
+    listConfig: {
+	loadingText: gettext('Scanning...'),
+	width: 350,
+	columns: [
+	    {
+		header: gettext('Datastore'),
+		sortable: true,
+		dataIndex: 'store',
+		renderer: Ext.String.htmlEncode,
+		flex: 1,
+	    },
+	    {
+		header: gettext('Comment'),
+		dataIndex: 'comment',
+		renderer: Ext.String.htmlEncode,
+		flex: 1,
+	    },
+	],
+    },
+
+    doRawQuery: function() {
+	// do nothing.
+    },
+
+    setRemote: function(remote) {
+	let me = this;
+
+	if (me.remote === remote) {
+	    return;
+	}
+
+	me.remote = remote;
+
+	let store = me.store;
+	store.removeAll();
+
+	if (me.remote) {
+	    me.setDisabled(false);
+	    if (me.actualChange) {
+		me.clearValue();
+	    }
+
+	    store.proxy.url = '/api2/json/config/remote/' + encodeURIComponent(me.remote) + '/scan';
+	    store.load();
+
+	    me.actualChange = true;
+	} else {
+	    me.setDisabled(true);
+	    me.clearValue();
+	}
+    },
+
+    initComponent: function() {
+	let me = this;
+
+	let store = Ext.create('Ext.data.Store', {
+	    fields: ['store', 'comment'],
+	    proxy: {
+		type: 'proxmox',
+		url: '/api2/json/config/remote/' + encodeURIComponent(me.remote) + '/scan',
+	    },
+	});
+
+	store.sort('store', 'ASC');
+
+	Ext.apply(me, {
+	    store: store,
+	});
+
+	me.callParent();
+
+	Proxmox.Utils.monStoreErrors(me.getPicker(), me.store);
+    },
+});
+
+
 Ext.define('PBS.window.SyncJobEdit', {
     extend: 'Proxmox.window.Edit',
     alias: 'widget.pbsSyncJobEdit',
@@ -52,12 +139,20 @@ Ext.define('PBS.window.SyncJobEdit', {
 		xtype: 'pbsRemoteSelector',
 		allowBlank: false,
 		name: 'remote',
+		listeners: {
+		    change: function(f, value) {
+			let me = this;
+			let remoteStoreField = me.up('pbsSyncJobEdit').down('field[name=remote-store]');
+			remoteStoreField.setRemote(value);
+		    },
+		},
 	    },
 	    {
 		fieldLabel: gettext('Source Datastore'),
-		xtype: 'proxmoxtextfield',
+		xtype: 'pbsRemoteStoreSelector',
 		allowBlank: false,
 		name: 'remote-store',
+		disabled: true,
 	    },
 	],
 
-- 
2.20.1

Re: [pbs-devel] [PATCH proxmox-backup 1/4] www: don't default to hourly sync job schedule

[Thomas Lamprecht]

(resend, as I forgot to keep the mailing list as recipient)

On 04.11.20 14:10, Fabian Grünbichler wrote:
> this breaks editing disabled sync jobs, and is also not consistent with

just means that we should only set it onCreate..

> the backend defaults.

which is? FWICT, there's no default in the backend - I want one here,
this can be easily edited and it's just better UX to avoid more decisions
which at this point may not be clear.

>
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
>  www/window/SyncJobEdit.js | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/www/window/SyncJobEdit.js b/www/window/SyncJobEdit.js
> index c89b95d2..43338cab 100644
> --- a/www/window/SyncJobEdit.js
> +++ b/www/window/SyncJobEdit.js
> @@ -84,7 +84,6 @@ Ext.define('PBS.window.SyncJobEdit', {
>  		fieldLabel: gettext('Schedule'),
>  		xtype: 'pbsCalendarEvent',
>  		name: 'schedule',
> -		value: 'hourly',
>  		emptyText: gettext('none (disabled)'),
>  		cbind: {
>  		    deleteEmpty: '{!isCreate}',
>

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

[Thomas Lamprecht]

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(())
> 

Re: [pbs-devel] [PATCH proxmox-backup 1/4] www: don't default to hourly sync job schedule

[Thomas Lamprecht]

On 04.11.20 14:46, Fabian Grünbichler wrote:
> On November 4, 2020 2:38 pm, Thomas Lamprecht wrote:
>> On 04.11.20 14:10, Fabian Grünbichler wrote:
>>> this breaks editing disabled sync jobs, and is also not consistent with
>>
>> just means that we should only set it onCreate..
> 
> also fine for me.
> 
>>> the backend defaults.
>>
>> which is? FWICT, there's no default in the backend - I want one here,
>> this can be easily edited and it's just better UX to avoid more decisions
>> which at this point may not be clear.
> 
> the backend's default is None/no schedule. for such simply values I 
> don't see much value in having a different default in the GUI, but I 
> don't care much either way.

A default for the backend would be a bit subtle and hard to change, if we'd like
to do so in the future. In the UI it's clearly presented to the user, they can
easily change it and we're pretty free to adapt it there if feedback shows that
would be better.

One should IMO not see the UI as plain "HTML form" like extension of an API, if it
would be that easy we could just auto generate it :)

> 
> for users that don't know much about sync jobs it might not be that 
> obvious how to have no schedule or even that that is an option when 
> creating a new one (you manually have to delete the value in the 
> picker/combobox).
> 

creating a new job without schedule seldom makes sense, I'd argue. Also, that it can
be temporarily be paused by deleting the schedule is subtle anyway, and IMO bad UX.
Not setting a default does not helps to suggest that is the case either, if we
want to make this clear, an explicit enable/disable checkbox which enables/disables
also the schedule field could be added.

But, an user also can just delete the job entry, if they just need to disable it and
cannot relate to the "clear schedule == disable", there's no relevant state full
information bound to a job. So, I do not see the need for it.


Again, to much words and pedantry for such a simple thing, sorry about that :-)

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

[Thomas Lamprecht]

we probably also do not want to bubble up the 401 error our server gets
from the remote server, as I then get logged out by the gui which thinks
the 401 are from our server...

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

[Fabian Grünbichler]

On November 4, 2020 5:57 pm, Thomas Lamprecht wrote:
> 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?

because the only thing that a remote datastore can currently be used for 
is syncing ;) but I am fine with AUDIT as well, I just wanted to mention 
it.

> 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

I think (as we discussed directly) this was an artifact of version 
mismatch?

> 
>> 
>>  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..

the field in the config is called userid, it contains an Authid 
(renaming would require postinst fixup, but if you want I can send a 
patch for switching it over).

> 
>> +        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(())
>> 
> 
> 
> 

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

[Fabian Grünbichler]

On November 4, 2020 6:12 pm, Thomas Lamprecht wrote:
> we probably also do not want to bubble up the 401 error our server gets
> from the remote server, as I then get logged out by the gui which thinks
> the 401 are from our server...

that's a good gotcha. should we just map errors to a generic 400 
prefixed with 'scanning remote '{}' failed' ?

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

[Thomas Lamprecht]

On 05.11.20 08:43, Fabian Grünbichler wrote:
> On November 4, 2020 6:12 pm, Thomas Lamprecht wrote:
>> we probably also do not want to bubble up the 401 error our server gets
>> from the remote server, as I then get logged out by the gui which thinks
>> the 401 are from our server...
> 
> that's a good gotcha. should we just map errors to a generic 400 
> prefixed with 'scanning remote '{}' failed' ?

or even a 5xx, as the server run into this error? but no hard feelings here.

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

[Thomas Lamprecht]

On 05.11.20 08:42, Fabian Grünbichler wrote:
> On November 4, 2020 5:57 pm, Thomas Lamprecht wrote:
>> 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?
> because the only thing that a remote datastore can currently be used for 
> is syncing ;) but I am fine with AUDIT as well, I just wanted to mention 
> it.

yes, but just because it will be used for that now, it still has not anything
to do with that directly - so I'd see it just as it's own thing, datastore
scanner - with no opinion on what the user wants to do with that.

> 
>> 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
> I think (as we discussed directly) this was an artifact of version 
> mismatch?
> 
>>>  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..
> the field in the config is called userid, it contains an Authid 
> (renaming would require postinst fixup, but if you want I can send a 
> patch for switching it over).
> 

it's a bit confusing, but that's it, was probably more confused in the light
of the outdated server at the other end.. So sorry for the noise :)