[pbs-devel] [PATCH] tape: forbid operations on a s3 datastore

* [pbs-devel] [PATCH] tape: forbid operations on a s3 datastore
@ 2025-07-23 14:31 Dominik Csapak
  2025-07-23 18:37 ` [pbs-devel] applied: " Thomas Lamprecht
  2025-07-23 19:51 ` [pbs-devel] " Thomas Lamprecht
  0 siblings, 2 replies; 7+ messages in thread
From: Dominik Csapak @ 2025-07-23 14:31 UTC (permalink / raw)
  To: pbs-devel

namely:
* backup to tape from s3 (including a configuring such a job)
* restore to s3 from tape

It does not work currently, but it probably does not make sense to allow
that at all for several reasons:
* both are designed to be 'off-site', so copying data from one off-site
  location to another directly does not make sense most of the time
* (modern) tape operations can reach relatively high speeds (> 300MB/s)
  and up/downloading to an (most likely remote) s3 storage will slow
  down the tape

Note that we could make the check in the restore case more efficient
(since we already have the parsed DataStore struct), but this to be done
only once for each tape restore operation and most of the time there
aren't that many datastores involved, so the extra runtime cost is
probably not that bad vs having multiple code paths for the error.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 pbs-config/src/datastore.rs        | 13 +++++++++++++
 src/api2/config/tape_backup_job.rs |  7 +++++++
 src/api2/tape/backup.rs            |  6 +++++-
 src/api2/tape/restore.rs           |  6 +++++-
 src/tape/mod.rs                    | 12 +++++++++++-
 5 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/pbs-config/src/datastore.rs b/pbs-config/src/datastore.rs
index 396dcb37..5a5553db 100644
--- a/pbs-config/src/datastore.rs
+++ b/pbs-config/src/datastore.rs
@@ -113,3 +113,16 @@ pub fn complete_calendar_event(_arg: &str, _param: &HashMap<String, String>) ->
         .map(|s| String::from(*s))
         .collect()
 }
+
+/// Returns the datastore backend type from it's name
+pub fn datastore_backend_type(store: &str) -> Result<pbs_api_types::DatastoreBackendType, Error> {
+    let (config, _) = config()?;
+    let store_config: DataStoreConfig = config.lookup("datastore", &store)?;
+
+    let backend_config: pbs_api_types::DatastoreBackendConfig = serde_json::from_value(
+        pbs_api_types::DatastoreBackendConfig::API_SCHEMA
+            .parse_property_string(store_config.backend.as_deref().unwrap_or(""))?,
+    )?;
+
+    Ok(backend_config.ty.unwrap_or_default())
+}
diff --git a/src/api2/config/tape_backup_job.rs b/src/api2/config/tape_backup_job.rs
index f2abf652..786acde0 100644
--- a/src/api2/config/tape_backup_job.rs
+++ b/src/api2/config/tape_backup_job.rs
@@ -13,6 +13,8 @@ use pbs_api_types::{
 
 use pbs_config::CachedUserInfo;
 
+use crate::tape::assert_datastore_type;
+
 #[api(
     input: {
         properties: {},
@@ -71,6 +73,8 @@ pub fn create_tape_backup_job(
     job: TapeBackupJobConfig,
     _rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<(), Error> {
+    assert_datastore_type(&job.setup.store)?;
+
     let _lock = pbs_config::tape_job::lock()?;
 
     let (mut config, _digest) = pbs_config::tape_job::config()?;
@@ -180,6 +184,9 @@ pub fn update_tape_backup_job(
     delete: Option<Vec<DeletableProperty>>,
     digest: Option<String>,
 ) -> Result<(), Error> {
+    if let Some(store) = &update.setup.store {
+        assert_datastore_type(&store)?;
+    }
     let _lock = pbs_config::tape_job::lock()?;
 
     let (mut config, expected_digest) = pbs_config::tape_job::config()?;
diff --git a/src/api2/tape/backup.rs b/src/api2/tape/backup.rs
index 31293a9a..16a26b83 100644
--- a/src/api2/tape/backup.rs
+++ b/src/api2/tape/backup.rs
@@ -20,7 +20,7 @@ use pbs_config::CachedUserInfo;
 use pbs_datastore::backup_info::{BackupDir, BackupInfo};
 use pbs_datastore::{DataStore, StoreProgress};
 
-use crate::tape::TapeNotificationMode;
+use crate::tape::{assert_datastore_type, TapeNotificationMode};
 use crate::{
     server::{
         jobstate::{compute_schedule_status, Job, JobState},
@@ -140,6 +140,8 @@ pub fn do_tape_backup_job(
     schedule: Option<String>,
     to_stdout: bool,
 ) -> Result<String, Error> {
+    assert_datastore_type(&setup.store)?;
+
     let job_id = format!(
         "{}:{}:{}:{}",
         setup.store,
@@ -302,6 +304,8 @@ pub fn backup(
     force_media_set: bool,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<Value, Error> {
+    assert_datastore_type(&setup.store)?;
+
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
 
     check_backup_permission(&auth_id, &setup.store, &setup.pool, &setup.drive)?;
diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
index 2cc1baab..9e55cae2 100644
--- a/src/api2/tape/restore.rs
+++ b/src/api2/tape/restore.rs
@@ -37,7 +37,7 @@ use pbs_tape::{
 };
 
 use crate::backup::check_ns_modification_privs;
-use crate::tape::TapeNotificationMode;
+use crate::tape::{assert_datastore_type, TapeNotificationMode};
 use crate::{
     tape::{
         drive::{lock_tape_device, request_and_load_media, set_tape_device_state, TapeDriver},
@@ -354,6 +354,10 @@ pub fn restore(
         bail!("no datastores given");
     }
 
+    for (_, (target, _)) in &used_datastores {
+        assert_datastore_type(target.name())?;
+    }
+
     for (target, namespaces) in used_datastores.values() {
         check_datastore_privs(
             &user_info,
diff --git a/src/tape/mod.rs b/src/tape/mod.rs
index f276f948..69fc373b 100644
--- a/src/tape/mod.rs
+++ b/src/tape/mod.rs
@@ -1,6 +1,6 @@
 //! Magnetic tape backup
 
-use anyhow::{format_err, Error};
+use anyhow::{bail, format_err, Error};
 use proxmox_auth_api::types::Userid;
 
 use proxmox_sys::fs::{create_path, CreateOptions};
@@ -155,3 +155,13 @@ impl From<(Option<Userid>, Option<NotificationMode>)> for TapeNotificationMode {
         }
     }
 }
+
+/// Asserts that the datastore is on a supported backend type
+pub fn assert_datastore_type(store: &str) -> Result<(), Error> {
+    match pbs_config::datastore::datastore_backend_type(store)? {
+        pbs_api_types::DatastoreBackendType::Filesystem => Ok(()),
+        pbs_api_types::DatastoreBackendType::S3 => {
+            bail!("direct s3/tape operations are not supported")
+        }
+    }
+}
-- 
2.39.5



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 7+ messages in thread