Re: [pbs-devel] [PATCH proxmox-backup 08/12] api2/admin/datastore: add get/set_protection

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On September 6, 2021 12:57 pm, Dominik Csapak wrote:
> for gettin/setting the protected flag for snapshots (akin to notes)
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/api2/admin/datastore.rs | 101 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 101 insertions(+)
> 
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index f88fd105..572c65a9 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -1751,6 +1751,101 @@ pub fn set_notes(
>      Ok(())
>  }
>  
> +#[api(
> +    input: {
> +        properties: {
> +            store: {
> +                schema: DATASTORE_SCHEMA,
> +            },
> +            "backup-type": {
> +                schema: BACKUP_TYPE_SCHEMA,
> +            },
> +            "backup-id": {
> +                schema: BACKUP_ID_SCHEMA,
> +            },
> +            "backup-time": {
> +                schema: BACKUP_TIME_SCHEMA,
> +            },
> +        },
> +    },
> +    access: {
> +        permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_BACKUP, true),
> +    },
> +)]
> +/// Query protection for a specific backup
> +pub fn get_protection(
> +    store: String,
> +    backup_type: String,
> +    backup_id: String,
> +    backup_time: i64,
> +    rpcenv: &mut dyn RpcEnvironment,
> +) -> Result<bool, Error> {
> +    let datastore = DataStore::lookup_datastore(&store)?;
> +
> +    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> +    let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
> +
> +    check_priv_or_backup_owner(&datastore, backup_dir.group(), &auth_id, PRIV_DATASTORE_AUDIT)?;
> +
> +    let protected_path = backup_dir.protected_file(datastore.base_path());
> +
> +    Ok(protected_path.exists())
> +}
> +
> +#[api(
> +    input: {
> +        properties: {
> +            store: {
> +                schema: DATASTORE_SCHEMA,
> +            },
> +            "backup-type": {
> +                schema: BACKUP_TYPE_SCHEMA,
> +            },
> +            "backup-id": {
> +                schema: BACKUP_ID_SCHEMA,
> +            },
> +            "backup-time": {
> +                schema: BACKUP_TIME_SCHEMA,
> +            },
> +            protected: {
> +                description: "Enable/disable protection.",

protected is already part of the CLI schema and API path, maybe simply 
'value' or 'enabled'?

> +            },
> +        },
> +    },
> +    access: {
> +        permission: &Permission::Privilege(&["datastore", "{store}"],
> +                                           PRIV_DATASTORE_MODIFY | PRIV_DATASTORE_BACKUP,
> +                                           true),
> +    },
> +)]
> +/// En- or disable protection for a specific backup
> +pub fn set_protection(
> +    store: String,
> +    backup_type: String,
> +    backup_id: String,
> +    backup_time: i64,
> +    protected: bool,
> +    rpcenv: &mut dyn RpcEnvironment,
> +) -> Result<(), Error> {
> +    let datastore = DataStore::lookup_datastore(&store)?;
> +
> +    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> +    let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
> +
> +    check_priv_or_backup_owner(&datastore, backup_dir.group(), &auth_id, PRIV_DATASTORE_MODIFY)?;
> +
> +    let protected_path = backup_dir.protected_file(datastore.base_path());
> +    if protected {
> +        std::fs::File::create(protected_path)
> +            .map_err(|err| format_err!("could not create protection file: {}", err))?;
> +    } else {
> +        std::fs::remove_file(protected_path)
> +            .map_err(|err| format_err!("could not remove protection file: {}", err))?;
> +    }

this is modifying a file related to the snapdir, shouldn't this have 
some sort of locking? to protect against other modifications of the 
protection flag, but also other operations that might make decisions 
based on the flag? haven't fully thought it through, but it seems to be 
there might be something missing here..

e.g., setting/removing notes is guarded by the manifest update 
mechanism, which does the locking.

> +
> +    Ok(())
> +}
> +
>  #[api(
>      input: {
>          properties: {
> @@ -1899,6 +1994,12 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
>              .get(&API_METHOD_GET_NOTES)
>              .put(&API_METHOD_SET_NOTES)
>      ),
> +    (
> +        "protected",
> +        &Router::new()
> +            .get(&API_METHOD_GET_PROTECTION)
> +            .put(&API_METHOD_SET_PROTECTION)
> +    ),
>      (
>          "prune",
>          &Router::new()
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
>