From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id A68511FF164 for ; Wed, 6 Nov 2024 12:58:30 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id C281AD6EF; Wed, 6 Nov 2024 12:58:41 +0100 (CET) MIME-Version: 1.0 In-Reply-To: <20241031121519.434337-13-c.ebner@proxmox.com> References: <20241031121519.434337-1-c.ebner@proxmox.com> <20241031121519.434337-13-c.ebner@proxmox.com> From: Fabian =?utf-8?q?Gr=C3=BCnbichler?= To: Christian Ebner , pbs-devel@lists.proxmox.com Date: Wed, 06 Nov 2024 12:57:59 +0100 Message-ID: <173089427968.79072.3773251895934605531@yuna.proxmox.com> User-Agent: alot/0.10 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.048 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH v6 proxmox-backup 12/29] api/api-types: refactor api endpoint version, add api types X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Cc: Thomas Lamprecht Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" @Thomas: since there's a few questions below that have long-term implications, I'd appreciate feedback.. Quoting Christian Ebner (2024-10-31 13:15:02) > Add a dedicated api type for the `version` api endpoint and helper > methods for supported feature comparison. > This will be used to detect api incompatibility of older hosts, not > supporting some features. > > Use the new api type to refactor the version endpoint and set it as > return type. > > Signed-off-by: Christian Ebner > --- > changes since version 5: > - add `features` vector to store supported feature strings > - drop `min_version_check`, introduce `supported_feature` check > > pbs-api-types/src/lib.rs | 3 + > pbs-api-types/src/version.rs | 109 +++++++++++++++++++++++++++++++++++ > src/api2/version.rs | 42 ++++++++------ > 3 files changed, 137 insertions(+), 17 deletions(-) > create mode 100644 pbs-api-types/src/version.rs > > diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs > index 460c7da7c..6bae4a52b 100644 > --- a/pbs-api-types/src/lib.rs > +++ b/pbs-api-types/src/lib.rs > @@ -155,6 +155,9 @@ pub use zfs::*; > mod metrics; > pub use metrics::*; > > +mod version; > +pub use version::*; > + > const_regex! { > // just a rough check - dummy acceptor is used before persisting > pub OPENSSL_CIPHERS_REGEX = r"^[0-9A-Za-z_:, +!\-@=.]+$"; > diff --git a/pbs-api-types/src/version.rs b/pbs-api-types/src/version.rs > new file mode 100644 > index 000000000..c7c91a53a > --- /dev/null > +++ b/pbs-api-types/src/version.rs > @@ -0,0 +1,109 @@ > +//! Defines the types for the api version info endpoint > +use std::convert::TryFrom; > + > +use anyhow::Context; > + > +use proxmox_schema::api; > + > +#[api( > + description: "Api version information", > + properties: { > + "version": { > + description: "Version 'major.minor'", > + type: String, > + }, > + "release": { > + description: "Version release", > + type: String, > + }, > + "repoid": { > + description: "Version repository id", > + type: String, > + }, > + "features": { > + description: "List of supported features", > + type: Array, > + items: { > + type: String, > + description: "Feature id", > + }, > + }, > + } > +)] > +#[derive(serde::Deserialize, serde::Serialize)] > +pub struct ApiVersionInfo { > + pub version: String, > + pub release: String, > + pub repoid: String, > + #[serde(default, skip_serializing_if = "Vec::is_empty")] > + pub features: Vec, > +} > + > +pub type ApiVersionMajor = u64; > +pub type ApiVersionMinor = u64; > +pub type ApiVersionRelease = u64; > + > +#[allow(dead_code)] > +pub struct ApiVersion { > + major: ApiVersionMajor, > + minor: ApiVersionMinor, > + release: ApiVersionRelease, > + features: Vec, > +} nit: if the fields were pub, this wouldn't be dead code and the new below could be dropped.. but, I am not sure if we even need this now, we could also just implement helpers on ApiVersionInfo that give us the major, minor, release versions as u64? especially if we do "does the server support XX" via explicit named features, and don't even have a use case (yet) for accessing the version parts? the big question here is - do we want to expose this kind of thing? so far, we've used the approach of making things opt-in or backwards compatible, or failing hard if a newer client tries to use a feature that is not supported by an older server (e.g., if a client tries to use namespaces with a server that doesn't support them, it will just error out on whichever request it makes). there are two ways to handle explicit versioning between client and server: 1.) client retrieves the version, and has a list of "feature A is supported since version X.Y.Z" 2.) client retrieves a list of supported features from the server (this patch (series)) variant 1 has the advantage that we don't have to keep an ever-growing list of features around (or worry about "naming" and organizing them). variant 2 has the advantage that the server can explicitly tell what it supports without needing the client to adapt its version <-> feature mapping (i.e., if we or somebody else backports a feature). it also has the advantage that there is no risk of the version mapping being wrong (e.g., because there was unexpected delay in applying a patch series, or somebody made a mistake in the contained version number). variant 1 was what I actually had in mind when I originally proposed this, but I do like variant 2 as well! > +impl TryFrom for ApiVersion { > + type Error = anyhow::Error; > + > + fn try_from(value: ApiVersionInfo) -> Result { > + let mut parts = value.version.split('.'); > + let major: ApiVersionMajor = if let Some(val) = parts.next() { > + val.parse() > + .with_context(|| "failed to parse major version")? > + } else { > + ApiVersionMajor::default() > + }; > + let minor: ApiVersionMinor = if let Some(val) = parts.next() { > + val.parse() > + .with_context(|| "failed to parse minor version")? > + } else { > + ApiVersionMinor::default() > + }; > + > + let release: ApiVersionMinor = value > + .release > + .parse() > + .with_context(|| "failed to parse release version")?; > + > + Ok(Self { > + major, > + minor, > + release, > + features: value.features.to_vec(), > + }) > + } > +} > + > +impl ApiVersion { > + pub fn new( > + major: ApiVersionMajor, > + minor: ApiVersionMinor, > + release: ApiVersionRelease, > + features: Vec, > + ) -> Self { > + Self { > + major, > + minor, > + release, > + features, > + } > + } > + > + pub fn supports_feature(&self, feature: &str) -> bool { this is just ver.features.iter().any(|f| f == feature) which isn't really that longer than ver.supports_feature(feature) that being said, if we expect to do more complicated things here in the feature, an explicit helper might be nice anyway.. but then the body can just be that single line for now ;) > + for supported_feature in &self.features { > + if *supported_feature == feature { > + return true; > + } > + } > + false > + } > +} > diff --git a/src/api2/version.rs b/src/api2/version.rs > index 0e91688b5..a6cec5216 100644 > --- a/src/api2/version.rs > +++ b/src/api2/version.rs > @@ -1,27 +1,35 @@ > //! Version information > > use anyhow::Error; > -use serde_json::{json, Value}; > +use serde_json::Value; > > -use proxmox_router::{ApiHandler, ApiMethod, Permission, Router, RpcEnvironment}; > -use proxmox_schema::ObjectSchema; > +use proxmox_router::{ApiMethod, Permission, Router, RpcEnvironment}; > +use proxmox_schema::api; > > -fn get_version( > +use pbs_api_types::ApiVersionInfo; > + > +const FEATURES: &'static [&'static str] = &[]; > + > +#[api( > + returns: { > + type: ApiVersionInfo, > + }, > + access: { > + permission: &Permission::Anybody, > + } > +)] > +///Proxmox Backup Server API version. > +fn version( > _param: Value, > _info: &ApiMethod, > _rpcenv: &mut dyn RpcEnvironment, > -) -> Result { > - Ok(json!({ > - "version": pbs_buildcfg::PROXMOX_PKG_VERSION, > - "release": pbs_buildcfg::PROXMOX_PKG_RELEASE, > - "repoid": pbs_buildcfg::PROXMOX_PKG_REPOID > - })) > +) -> Result { > + Ok(ApiVersionInfo { > + version: pbs_buildcfg::PROXMOX_PKG_VERSION.to_string(), > + release: pbs_buildcfg::PROXMOX_PKG_RELEASE.to_string(), > + repoid: pbs_buildcfg::PROXMOX_PKG_REPOID.to_string(), > + features: FEATURES.iter().map(|feature| feature.to_string()).collect(), > + }) > } > > -pub const ROUTER: Router = Router::new().get( > - &ApiMethod::new( > - &ApiHandler::Sync(&get_version), > - &ObjectSchema::new("Proxmox Backup Server API version.", &[]), > - ) > - .access(None, &Permission::Anybody), > -); > +pub const ROUTER: Router = Router::new().get(&API_METHOD_VERSION); > -- > 2.39.5 > > > > _______________________________________________ > pbs-devel mailing list > pbs-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel > > _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel