From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <d.csapak@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits))
 (No client certificate requested)
 by lists.proxmox.com (Postfix) with UTF8SMTPS id BA8F362CD8
 for <pbs-devel@lists.proxmox.com>; Thu,  1 Oct 2020 12:40:41 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
 by firstgate.proxmox.com (Proxmox) with UTF8SMTP id B0605204D3
 for <pbs-devel@lists.proxmox.com>; Thu,  1 Oct 2020 12:40:11 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
 [212.186.127.180])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits))
 (No client certificate requested)
 by firstgate.proxmox.com (Proxmox) with UTF8SMTPS id 840BB204C6
 for <pbs-devel@lists.proxmox.com>; Thu,  1 Oct 2020 12:40:10 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
 by proxmox-new.maurer-it.com (Proxmox) with UTF8SMTP id 427AB45B7E
 for <pbs-devel@lists.proxmox.com>; Thu,  1 Oct 2020 12:40:10 +0200 (CEST)
To: pbs-devel@lists.proxmox.com
References: <20200925084330.75484-1-h.laimer@proxmox.com>
 <20200925084330.75484-4-h.laimer@proxmox.com>
From: Dominik Csapak <d.csapak@proxmox.com>
Message-ID: <b9a6436b-7991-47d6-496a-f46f18ebf479@proxmox.com>
Date: Thu, 1 Oct 2020 12:40:08 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:82.0) Gecko/20100101
 Thunderbird/82.0
MIME-Version: 1.0
In-Reply-To: <20200925084330.75484-4-h.laimer@proxmox.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
X-SPAM-LEVEL: Spam detection results:  0
 AWL 0.545 Adjusted score from AWL reputation of From: address
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 NICE_REPLY_A           -0.001 Looks like a legit reply (A)
 RCVD_IN_DNSWL_MED        -2.3 Sender listed at https://www.dnswl.org/,
 medium trust
 SPF_HELO_NONE           0.001 SPF: HELO does not publish an SPF Record
 SPF_PASS               -0.001 SPF: sender matches SPF record
 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
 http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
 information. [verify.rs, data.store, config.rs]
Subject: Re: [pbs-devel] [PATCH v1 proxmox-backup 03/14] api2: add verify
 job config endpoint
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
 <pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>, 
 <mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>, 
 <mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Thu, 01 Oct 2020 10:40:41 -0000

comments inline

On 9/25/20 10:43 AM, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>   src/api2/config.rs        |   2 +
>   src/api2/config/verify.rs | 272 ++++++++++++++++++++++++++++++++++++++
>   2 files changed, 274 insertions(+)
>   create mode 100644 src/api2/config/verify.rs
> 
> diff --git a/src/api2/config.rs b/src/api2/config.rs
> index be7397c8..7a5129c7 100644
> --- a/src/api2/config.rs
> +++ b/src/api2/config.rs
> @@ -4,11 +4,13 @@ use proxmox::list_subdirs_api_method;
>   pub mod datastore;
>   pub mod remote;
>   pub mod sync;
> +pub mod verify;
>   
>   const SUBDIRS: SubdirMap = &[
>       ("datastore", &datastore::ROUTER),
>       ("remote", &remote::ROUTER),
>       ("sync", &sync::ROUTER),
> +    ("verify", &verify::ROUTER)
>   ];
>   
>   pub const ROUTER: Router = Router::new()
> diff --git a/src/api2/config/verify.rs b/src/api2/config/verify.rs
> new file mode 100644
> index 00000000..5e0db40f
> --- /dev/null
> +++ b/src/api2/config/verify.rs
> @@ -0,0 +1,272 @@
> +use anyhow::{bail, Error};
> +use serde_json::Value;
> +use ::serde::{Deserialize, Serialize};
> +
> +use proxmox::api::{api, Router, RpcEnvironment};
> +use proxmox::tools::fs::open_file_locked;
> +
> +use crate::api2::types::*;
> +use crate::config::verify::{self, VerifyJobConfig};
> +
> +#[api(
> +    input: {
> +        properties: {},
> +    },
> +    returns: {
> +        description: "List configured jobs.",
> +        type: Array,
> +        items: { type: verify::VerifyJobConfig },
> +    },
> +)]
> +/// List all verify jobs
> +pub fn list_verify_jobs(
> +    _param: Value,
> +    mut rpcenv: &mut dyn RpcEnvironment,
> +) -> Result<Vec<VerifyJobConfig>, Error> {
> +
> +    let (config, digest) = verify::config()?;
> +
> +    let list = config.convert_to_typed_array("verify")?;
> +
> +    rpcenv["digest"] = proxmox::tools::digest_to_hex(&digest).into();
> +
> +    Ok(list)
> +}
> +
> +
> +#[api(
> +    protected: true,
> +    input: {
> +        properties: {
> +            id: {
> +                schema: JOB_ID_SCHEMA,
> +            },
> +            store: {
> +                schema: DATASTORE_SCHEMA,
> +            },
> +            "ignore-verified": {
> +                schema: IGNORE_VERIFIED_BACKUPS_SCHEMA,

i would find it more sensible to have this also optional, as is
already has default anyway

also i would probably make it an Option in the struct also,
since there is a documented default and a not serialized option
takes less space in the config

> +            },
> +            "outdated-after": {
> +                optional: true,
> +                schema: VERIFICATION_OUTDATED_AFTER_SCHEMA,
> +            },
> +            comment: {
> +                optional: true,
> +                schema: SINGLE_LINE_COMMENT_SCHEMA,
> +            },
> +            schedule: {
> +                optional: true,
> +                schema: VERIFY_SCHEDULE_SCHEMA,
> +            },
> +        }
> +    }
> +)]
> +/// Create a new verify job.
> +pub fn create_verify_job(param: Value) -> Result<(), Error> {
> +
> +    let _lock = open_file_locked(verify::VERIFY_CFG_LOCKFILE, std::time::Duration::new(10, 0))?;
> +
> +    let verify_job: verify::VerifyJobConfig = serde_json::from_value(param.clone())?;
> +
> +    let (mut config, _digest) = verify::config()?;
> +
> +    if let Some(_) = config.sections.get(&verify_job.id) {
> +        bail!("job '{}' already exists.", verify_job.id);
> +    }
> +
> +    config.set_data(&verify_job.id, "verify", &verify_job)?;
> +
> +    verify::save_config(&config)?;
> +
> +    crate::config::jobstate::create_state_file("verifyjob", &verify_job.id)?;
> +
> +    Ok(())
> +}
> +
> +#[api(
> +   input: {
> +        properties: {
> +            id: {
> +                schema: JOB_ID_SCHEMA,
> +            },
> +        },
> +    },
> +    returns: {
> +        description: "The verify job configuration.",
> +        type: verify::VerifyJobConfig,
> +    },
> +)]
> +/// Read a verify job configuration.
> +pub fn read_verify_job(
> +    id: String,
> +    mut rpcenv: &mut dyn RpcEnvironment,
> +) -> Result<VerifyJobConfig, Error> {
> +    let (config, digest) = verify::config()?;
> +
> +    let verify_job = config.lookup("verify", &id)?;
> +    rpcenv["digest"] = proxmox::tools::digest_to_hex(&digest).into();
> +
> +    Ok(verify_job)
> +}
> +
> +#[api()]
> +#[derive(Serialize, Deserialize)]
> +#[serde(rename_all="kebab-case")]
> +#[allow(non_camel_case_types)]
> +/// Deletable property name
> +pub enum DeletableProperty {
> +    /// Delete the comment property.
> +    comment,
> +    /// Delete the job schedule.
> +    schedule,
> +    /// Delete outdated_after.
> +    outdated_after > +}
> +
> +#[api(
> +    protected: true,
> +    input: {
> +        properties: {
> +            id: {
> +                schema: JOB_ID_SCHEMA,
> +            },
> +            store: {
> +                schema: DATASTORE_SCHEMA,
> +                optional: true,
> +            },
> +            "ignore-verified": {
> +                schema: IGNORE_VERIFIED_BACKUPS_SCHEMA,
> +                optional: true,
> +            },
> +            "outdated-after": {
> +                optional: true,
> +                schema: VERIFICATION_OUTDATED_AFTER_SCHEMA,
> +            },
> +            comment: {
> +                optional: true,
> +                schema: SINGLE_LINE_COMMENT_SCHEMA,
> +            },
> +            schedule: {
> +                optional: true,
> +                schema: VERIFY_SCHEDULE_SCHEMA,
> +            },
> +            delete: {
> +                description: "List of properties to delete.",
> +                type: Array,
> +                optional: true,
> +                items: {
> +                    type: DeletableProperty,
> +                }
> +            },
> +            digest: {
> +                optional: true,
> +                schema: PROXMOX_CONFIG_DIGEST_SCHEMA,
> +            },
> +        },
> +    },
> +)]
> +/// Update verify job config.
> +pub fn update_verify_job(
> +    id: String,
> +    store: Option<String>,
> +    ignore_verified: Option<bool>,
> +    outdated_after: Option<i64>,
> +    comment: Option<String>,
> +    schedule: Option<String>,
> +    delete: Option<Vec<DeletableProperty>>,
> +    digest: Option<String>,
> +) -> Result<(), Error> {
> +
> +    let _lock = open_file_locked(verify::VERIFY_CFG_LOCKFILE, std::time::Duration::new(10, 0))?;
> +
> +    // pass/compare digest
> +    let (mut config, expected_digest) = verify::config()?;
> +
> +    if let Some(ref digest) = digest {
> +        let digest = proxmox::tools::hex_to_digest(digest)?;
> +        crate::tools::detect_modified_configuration_file(&digest, &expected_digest)?;
> +    }
> +
> +    let mut data: verify::VerifyJobConfig = config.lookup("verify", &id)?;
> +
> +     if let Some(delete) = delete {
> +        for delete_prop in delete {
> +            match delete_prop {
> +                DeletableProperty::outdated_after => { data.outdated_after = None; },
> +                DeletableProperty::comment => { data.comment = None; },
> +                DeletableProperty::schedule => { data.schedule = None; },
> +            }
> +        }
> +    }
> +
> +    if let Some(comment) = comment {
> +        let comment = comment.trim().to_string();
> +        if comment.is_empty() {
> +            data.comment = None;
> +        } else {
> +            data.comment = Some(comment);
> +        }
> +    }
> +
> +    if let Some(store) = store { data.store = store; }
> +    if let Some(ignore_verified) = ignore_verified {
> +        data.ignore_verified = ignore_verified;
> +    }
> +    if outdated_after.is_some() { data.outdated_after = outdated_after }
> +    if schedule.is_some() { data.schedule = schedule; }
> +
> +    config.set_data(&id, "verify", &data)?;
> +
> +    verify::save_config(&config)?;
> +
> +    Ok(())
> +}
> +
> +#[api(
> +    protected: true,
> +    input: {
> +        properties: {
> +            id: {
> +                schema: JOB_ID_SCHEMA,
> +            },
> +            digest: {
> +                optional: true,
> +                schema: PROXMOX_CONFIG_DIGEST_SCHEMA,
> +            },
> +        },
> +    },
> +)]
> +/// Remove a verify job configuration
> +pub fn delete_verify_job(id: String, digest: Option<String>) -> Result<(), Error> {
> +
> +    let _lock = open_file_locked(verify::VERIFY_CFG_LOCKFILE, std::time::Duration::new(10, 0))?;
> +
> +    let (mut config, expected_digest) = verify::config()?;
> +
> +    if let Some(ref digest) = digest {
> +        let digest = proxmox::tools::hex_to_digest(digest)?;
> +        crate::tools::detect_modified_configuration_file(&digest, &expected_digest)?;
> +    }
> +
> +    match config.sections.get(&id) {
> +        Some(_) => { config.sections.remove(&id); },
> +        None => bail!("job '{}' does not exist.", id),
> +    }
> +
> +    verify::save_config(&config)?;
> +
> +    crate::config::jobstate::remove_state_file("verifyjob", &id)?;
> +
> +    Ok(())
> +}
> +
> +const ITEM_ROUTER: Router = Router::new()
> +    .get(&API_METHOD_READ_VERIFY_JOB)
> +    .put(&API_METHOD_UPDATE_VERIFY_JOB)
> +    .delete(&API_METHOD_DELETE_VERIFY_JOB);
> +
> +pub const ROUTER: Router = Router::new()
> +    .get(&API_METHOD_LIST_VERIFY_JOBS)
> +    .post(&API_METHOD_CREATE_VERIFY_JOB)
> +    .match_all("id", &ITEM_ROUTER);
> \ No newline at end of file
>