diff --git a/io-engine/src/grpc/v1/lvm/pool.rs b/io-engine/src/grpc/v1/lvm/pool.rs index b70e9cab8..0cb7bad51 100644 --- a/io-engine/src/grpc/v1/lvm/pool.rs +++ b/io-engine/src/grpc/v1/lvm/pool.rs @@ -3,12 +3,12 @@ use crate::{ grpc::{ acquire_subsystem_lock, lvm_enabled, - v1::pool::{PoolGrpc, PoolProbe}, + v1::pool::{PoolGrpc, PoolIdProbe, PoolSvcRpc}, GrpcResult, }, lvm::{CmnQueryArgs, Error as LvmError, VolumeGroup}, lvs::Lvs, - pool_backend::PoolArgs, + pool_backend::{PoolArgs, PoolBackend}, }; use io_engine_api::v1::pool::*; use std::{convert::TryFrom, fmt::Debug}; @@ -28,16 +28,16 @@ impl PoolService { } /// Probe the LVM Pool service for a pool. pub(crate) async fn probe( - probe: &PoolProbe, + probe: &PoolIdProbe, ) -> Result { if !MayastorFeatures::get_features().lvm() { return Ok(false); } let query = match probe { - PoolProbe::Uuid(uuid) => CmnQueryArgs::ours().uuid(uuid), - PoolProbe::UuidOrName(uuid) => CmnQueryArgs::ours().uuid(uuid), - PoolProbe::NameUuid { + PoolIdProbe::Uuid(uuid) => CmnQueryArgs::ours().uuid(uuid), + PoolIdProbe::UuidOrName(uuid) => CmnQueryArgs::ours().uuid(uuid), + PoolIdProbe::NameUuid { name, uuid, } => CmnQueryArgs::ours().named(name).uuid_opt(uuid), @@ -101,6 +101,13 @@ async fn find_pool( Ok(pool) } +#[async_trait::async_trait] +impl PoolSvcRpc for PoolService { + fn kind(&self) -> PoolBackend { + PoolBackend::Lvm + } +} + #[tonic::async_trait] impl PoolRpc for PoolService { async fn create_pool( diff --git a/io-engine/src/grpc/v1/lvs/pool.rs b/io-engine/src/grpc/v1/lvs/pool.rs index 0a4a87a72..690fc6ec2 100644 --- a/io-engine/src/grpc/v1/lvs/pool.rs +++ b/io-engine/src/grpc/v1/lvs/pool.rs @@ -4,11 +4,11 @@ use crate::{ acquire_subsystem_lock, rpc_submit, rpc_submit_ext, - v1::pool::{PoolGrpc, PoolProbe}, + v1::pool::{PoolGrpc, PoolIdProbe, PoolSvcRpc}, GrpcResult, }, lvs::{BsError, Lvs, LvsError}, - pool_backend::PoolArgs, + pool_backend::{PoolArgs, PoolBackend}, }; use io_engine_api::v1::pool::*; use std::{convert::TryFrom, fmt::Debug}; @@ -27,15 +27,17 @@ impl PoolService { Self {} } /// Probe the LVS Pool service for a pool. - pub(crate) async fn probe(probe: PoolProbe) -> Result { + pub(crate) async fn probe( + probe: PoolIdProbe, + ) -> Result { let rx = rpc_submit_ext(async move { match probe { - PoolProbe::Uuid(uuid) => Lvs::lookup_by_uuid(&uuid).is_some(), - PoolProbe::UuidOrName(id) => { + PoolIdProbe::Uuid(uuid) => Lvs::lookup_by_uuid(&uuid).is_some(), + PoolIdProbe::UuidOrName(id) => { Lvs::lookup_by_uuid(&id).is_some() || Lvs::lookup(&id).is_some() } - PoolProbe::NameUuid { + PoolIdProbe::NameUuid { name, uuid, } => match uuid { @@ -88,7 +90,7 @@ async fn find_pool( let Some(pool) = Lvs::lookup(name) else { return Err(LvsError::PoolNotFound { source: BsError::LvsNotFound {}, - msg: format!("Destroy failed as pool {name} was not found"), + msg: format!("Pool {name} was not found"), } .into()); }; @@ -105,6 +107,13 @@ async fn find_pool( Ok(pool) } +#[async_trait::async_trait] +impl PoolSvcRpc for PoolService { + fn kind(&self) -> PoolBackend { + PoolBackend::Lvs + } +} + #[tonic::async_trait] impl PoolRpc for PoolService { async fn create_pool( diff --git a/io-engine/src/grpc/v1/lvs/replica.rs b/io-engine/src/grpc/v1/lvs/replica.rs index b6850cd11..7ad7b3c34 100644 --- a/io-engine/src/grpc/v1/lvs/replica.rs +++ b/io-engine/src/grpc/v1/lvs/replica.rs @@ -12,7 +12,7 @@ use crate::{ v1::{pool::PoolGrpc, replica::ReplicaGrpc}, GrpcResult, }, - lvs::{LvsError, BsError, Lvol, Lvs, LvsLvol}, + lvs::{BsError, Lvol, Lvs, LvsError, LvsLvol}, }; use io_engine_api::v1::{pool::PoolType, replica::*}; use std::convert::TryFrom; diff --git a/io-engine/src/grpc/v1/pool.rs b/io-engine/src/grpc/v1/pool.rs index 0a023bb6a..279956520 100644 --- a/io-engine/src/grpc/v1/pool.rs +++ b/io-engine/src/grpc/v1/pool.rs @@ -33,11 +33,37 @@ struct UnixStream(tokio::net::UnixStream); /// Probe for pools using this criteria. #[derive(Debug, Clone)] -pub enum PoolProbe { +pub enum PoolIdProbe { Uuid(String), UuidOrName(String), NameUuid { name: String, uuid: Option }, } +impl PoolIdProbe { + fn name_uuid(name: &str, uuid: &Option) -> Self { + Self::NameUuid { + name: name.to_owned(), + uuid: uuid.to_owned(), + } + } +} + +/// The different types of probing. +pub(crate) enum ProbeType { + /// Probing the specific pool type directly. + ByKind(PoolBackend), + /// Probing using identifiers. + ById(PoolIdProbe), +} +impl From for ProbeType { + fn from(value: PoolIdProbe) -> Self { + Self::ById(value) + } +} +impl From for ProbeType { + fn from(value: PoolBackend) -> Self { + Self::ByKind(value) + } +} /// RPC service for mayastor pool operations #[derive(Debug, Clone)] @@ -219,6 +245,11 @@ impl Default for PoolService { } } +#[async_trait::async_trait] +pub trait PoolSvcRpc: PoolRpc { + fn kind(&self) -> PoolBackend; +} + pub(crate) struct PoolGrpc { pool: P, } @@ -296,46 +327,32 @@ impl PoolService { client_context: std::sync::Arc::new(tokio::sync::RwLock::new(None)), } } - /// Return a backend for the given type. - fn backend( - &self, - pooltype: i32, - ) -> Result, tonic::Status> { - Ok(match PoolBackend::try_from(pooltype)? { - PoolBackend::Lvs => Box::new(LvsSvc::new()), - PoolBackend::Lvm => Box::new(LvmSvc::new()), - }) - } + /// Probe backends for the given name and/or uuid and return the right one. - async fn probe_backend( + pub(crate) async fn probe>( &self, - name: &str, - uuid: &Option, - ) -> Result, tonic::Status> { - let probe = PoolProbe::NameUuid { - name: name.to_owned(), - uuid: uuid.to_owned(), + probe: I, + ) -> Result, tonic::Status> { + let probe = probe.into(); + let kind = match probe { + ProbeType::ByKind(kind) => kind, + ProbeType::ById(probe) => match ( + LvmSvc::probe(&probe).await, + LvsSvc::probe(probe.clone()).await, + ) { + (Ok(true), _) => Ok(PoolBackend::Lvm), + (_, Ok(true)) => Ok(PoolBackend::Lvs), + (Err(error), _) | (_, Err(error)) => Err(error), + _ => { + Err(Status::not_found(format!("Pool {probe:?} not found"))) + } + }?, }; - Ok(match self.probe_backend_kind(probe).await? { - PoolBackend::Lvm => Box::new(LvmSvc::new()), + Ok(match kind { PoolBackend::Lvs => Box::new(LvsSvc::new()), + PoolBackend::Lvm => Box::new(LvmSvc::new()), }) } - - pub(crate) async fn probe_backend_kind( - &self, - probe: PoolProbe, - ) -> Result { - match ( - LvmSvc::probe(&probe).await, - LvsSvc::probe(probe.clone()).await, - ) { - (Ok(true), _) => Ok(PoolBackend::Lvm), - (_, Ok(true)) => Ok(PoolBackend::Lvs), - (Err(error), _) | (_, Err(error)) => Err(error), - _ => Err(Status::not_found(format!("Pool {probe:?} not found"))), - } - } } #[tonic::async_trait] @@ -345,13 +362,15 @@ impl PoolRpc for PoolService { &self, request: Request, ) -> GrpcResult { - let backend = self.backend(request.get_ref().pooltype)?; + let backend = self + .probe(PoolBackend::try_from(request.get_ref().pooltype)?) + .await; self.locked( GrpcClientContext::new(&request, function_name!()), async move { info!("{:?}", request.get_ref()); - backend.create_pool(request).await + backend?.create_pool(request).await }, ) .await @@ -363,14 +382,17 @@ impl PoolRpc for PoolService { request: Request, ) -> GrpcResult<()> { let backend = self - .probe_backend(&request.get_ref().name, &request.get_ref().uuid) - .await?; + .probe(PoolIdProbe::name_uuid( + &request.get_ref().name, + &request.get_ref().uuid, + )) + .await; self.locked( GrpcClientContext::new(&request, function_name!()), async move { info!("{:?}", request.get_ref()); - backend.destroy_pool(request).await + backend?.destroy_pool(request).await }, ) .await @@ -382,14 +404,17 @@ impl PoolRpc for PoolService { request: Request, ) -> GrpcResult<()> { let backend = self - .probe_backend(&request.get_ref().name, &request.get_ref().uuid) - .await?; + .probe(PoolIdProbe::name_uuid( + &request.get_ref().name, + &request.get_ref().uuid, + )) + .await; self.locked( GrpcClientContext::new(&request, function_name!()), async move { info!("{:?}", request.get_ref()); - backend.export_pool(request).await + backend?.export_pool(request).await }, ) .await @@ -400,13 +425,15 @@ impl PoolRpc for PoolService { &self, request: Request, ) -> GrpcResult { - let backend = self.backend(request.get_ref().pooltype)?; + let backend = self + .probe(PoolBackend::try_from(request.get_ref().pooltype)?) + .await; self.locked( GrpcClientContext::new(&request, function_name!()), async move { info!("{:?}", request.get_ref()); - backend.import_pool(request).await + backend?.import_pool(request).await }, ) .await diff --git a/io-engine/src/grpc/v1/replica.rs b/io-engine/src/grpc/v1/replica.rs index f9d27d051..ccdf26e77 100644 --- a/io-engine/src/grpc/v1/replica.rs +++ b/io-engine/src/grpc/v1/replica.rs @@ -10,7 +10,7 @@ use crate::{ }, grpc::{ acquire_subsystem_lock, - v1::pool::PoolProbe, + v1::pool::PoolIdProbe, GrpcClientContext, GrpcResult, RWLock, @@ -108,6 +108,22 @@ impl RWLock for ReplicaService { } } +enum Probe<'a> { + Replica(&'a str), + Pool(PoolIdProbe), + ReplicaPool(&'a str, Option), +} +impl<'a> From<&'a String> for Probe<'a> { + fn from(value: &'a String) -> Self { + Self::Replica(value.as_str()) + } +} +impl From for Probe<'_> { + fn from(value: PoolIdProbe) -> Self { + Self::Pool(value) + } +} + impl ReplicaService { pub fn new(pool_svc: super::pool::PoolService) -> Self { Self { @@ -118,34 +134,47 @@ impl ReplicaService { } /// Probe backends for the given replica uuid and return the right one. - async fn probe_backend( - &self, - replica_uuid: &str, - ) -> Result, tonic::Status> { - Ok(match self.probe_backend_kind(replica_uuid).await? { - PoolBackend::Lvs => Box::new(LvsSvc::new()), - PoolBackend::Lvm => Box::new(LvmSvc::new()), - }) - } - /// Probe backends for the given pool uuid and return the right one. - pub async fn probe_backend_pool( + async fn probe_backend<'a, P: Into>>( &self, - probe: PoolProbe, + probe: P, ) -> Result, tonic::Status> { - Ok(match self.pool_svc.probe_backend_kind(probe).await? { - PoolBackend::Lvs => Box::new(LvsSvc::new()), - PoolBackend::Lvm => Box::new(LvmSvc::new()), - }) + let kind = match probe.into() { + Probe::Replica(replica_uuid) + | Probe::ReplicaPool(replica_uuid, None) => match ( + LvsSvc::probe(replica_uuid).await, + LvmSvc::probe(replica_uuid).await, + ) { + (Ok(true), _) => Ok(PoolBackend::Lvs), + (_, Ok(true)) => Ok(PoolBackend::Lvm), + (Err(error), _) | (_, Err(error)) => Err(error), + _ => Err(Status::not_found(format!( + "Replica {replica_uuid} not found" + ))), + }?, + Probe::Pool(probe) => self.pool_svc.probe(probe).await?.kind(), + Probe::ReplicaPool(_, Some(pool)) => { + match self.pool_svc.probe(pool).await { + Err(status) if status.code() == tonic::Code::NotFound => { + Err(Status::failed_precondition(status.to_string())) + } + _else => _else, + }? + .kind() + } + }; + match kind { + PoolBackend::Lvs => Ok(Box::new(LvsSvc::new())), + PoolBackend::Lvm => Ok(Box::new(LvmSvc::new())), + } } - async fn probe_backend_kind( - &self, - uuid: &str, - ) -> Result { - match (LvsSvc::probe(uuid).await, LvmSvc::probe(uuid).await) { - (Ok(true), _) => Ok(PoolBackend::Lvs), - (_, Ok(true)) => Ok(PoolBackend::Lvm), - (Err(error), _) | (_, Err(error)) => Err(error), - _ => Err(Status::not_found(format!("Replica {uuid} not found"))), +} +impl From for PoolIdProbe { + fn from(value: destroy_replica_request::Pool) -> Self { + match value { + destroy_replica_request::Pool::PoolName(name) => { + Self::UuidOrName(name) + } + destroy_replica_request::Pool::PoolUuid(uuid) => Self::Uuid(uuid), } } } @@ -269,8 +298,8 @@ impl ReplicaRpc for ReplicaService { request: Request, ) -> GrpcResult { let probe = - PoolProbe::UuidOrName(request.get_ref().pooluuid.to_owned()); - let backend = self.probe_backend_pool(probe).await?; + PoolIdProbe::UuidOrName(request.get_ref().pooluuid.to_owned()); + let backend = self.probe_backend(probe).await; self.locked( GrpcClientContext::new(&request, function_name!()), async move { @@ -286,7 +315,7 @@ impl ReplicaRpc for ReplicaService { ))); } - backend.create_replica(request).await + backend?.create_replica(request).await }, ) .await @@ -297,13 +326,17 @@ impl ReplicaRpc for ReplicaService { &self, request: Request, ) -> GrpcResult<()> { - let backend = self.probe_backend(&request.get_ref().uuid).await?; + let probe = Probe::ReplicaPool( + &request.get_ref().uuid, + request.get_ref().pool.clone().map(Into::into), + ); + let backend = self.probe_backend(probe).await; self.locked( GrpcClientContext::new(&request, function_name!()), async move { info!("{:?}", request.get_ref()); - backend.destroy_replica(request).await + backend?.destroy_replica(request).await }, ) .await @@ -352,13 +385,13 @@ impl ReplicaRpc for ReplicaService { &self, request: Request, ) -> GrpcResult { - let backend = self.probe_backend(&request.get_ref().uuid).await?; + let backend = self.probe_backend(&request.get_ref().uuid).await; self.locked( GrpcClientContext::new(&request, function_name!()), async move { info!("{:?}", request.get_ref()); - backend.share_replica(request).await + backend?.share_replica(request).await }, ) .await @@ -369,13 +402,13 @@ impl ReplicaRpc for ReplicaService { &self, request: Request, ) -> GrpcResult { - let backend = self.probe_backend(&request.get_ref().uuid).await?; + let backend = self.probe_backend(&request.get_ref().uuid).await; self.locked( GrpcClientContext::new(&request, function_name!()), async move { info!("{:?}", request.get_ref()); - backend.unshare_replica(request).await + backend?.unshare_replica(request).await }, ) .await @@ -386,13 +419,13 @@ impl ReplicaRpc for ReplicaService { &self, request: Request, ) -> GrpcResult { - let backend = self.probe_backend(&request.get_ref().uuid).await?; + let backend = self.probe_backend(&request.get_ref().uuid).await; self.locked( GrpcClientContext::new(&request, function_name!()), async move { info!("{:?}", request.get_ref()); - backend.resize_replica(request).await + backend?.resize_replica(request).await }, ) .await @@ -403,13 +436,13 @@ impl ReplicaRpc for ReplicaService { &self, request: Request, ) -> GrpcResult { - let backend = self.probe_backend(&request.get_ref().uuid).await?; + let backend = self.probe_backend(&request.get_ref().uuid).await; self.locked( GrpcClientContext::new(&request, function_name!()), async move { info!("{:?}", request.get_ref()); - backend.set_replica_entity_id(request).await + backend?.set_replica_entity_id(request).await }, ) .await diff --git a/io-engine/src/lvm/lv_replica.rs b/io-engine/src/lvm/lv_replica.rs index 1b12a0b11..772d63891 100644 --- a/io-engine/src/lvm/lv_replica.rs +++ b/io-engine/src/lvm/lv_replica.rs @@ -53,25 +53,29 @@ impl QueryArgs { } /// Get a comma-separated list of query selection args. /// todo: should be Display trait? - pub(super) fn query(&self) -> String { - let mut select = self.vg.query(); + pub(super) fn query(&self) -> Result { + let mut select = self.vg.query()?; let args = &self.lv; if self.regular_lv { if let Some(lv_name) = &args.name { + super::is_alphanumeric("lv_name", lv_name)?; select.push_str(&format!("lv_name={lv_name},")); } if let Some(lv_uuid) = &args.uuid { + super::is_alphanumeric("lv_uuid", lv_uuid)?; select.push_str(&format!("lv_uuid={lv_uuid},")); } } else { if let Some(name) = &args.name { + super::is_alphanumeric("name", name)?; select.push_str(&format!( "lv_tags={},", Property::LvName(name.to_string()).tag() )); } if let Some(lv_name) = &args.uuid { + super::is_alphanumeric("lv_name", lv_name)?; select.push_str(&format!("lv_name={lv_name},")); } } @@ -79,7 +83,7 @@ impl QueryArgs { if let Some(lv_tag) = &args.tag { select.push_str(&format!("lv_tags={lv_tag},")); } - select + Ok(select) } } @@ -220,7 +224,7 @@ impl LogicalVolume { pub(crate) async fn lookup(args: &QueryArgs) -> Result { let vgs = Self::list(args).await?; vgs.into_iter().next().ok_or(Error::LvNotFound { - query: args.query(), + query: args.query().unwrap_or_else(|e| e.to_string()), }) } @@ -257,7 +261,7 @@ impl LogicalVolume { "--nosuffix", "-q", ]; - let select = opts.query(); + let select = opts.query()?; let select_query = format!("--select={select}"); if !select.is_empty() { args.push(select_query.trim_end_matches(',')); diff --git a/io-engine/src/lvm/mod.rs b/io-engine/src/lvm/mod.rs index e80e212e1..7765ea303 100644 --- a/io-engine/src/lvm/mod.rs +++ b/io-engine/src/lvm/mod.rs @@ -53,6 +53,19 @@ use crate::{ }; use futures::channel::oneshot::Receiver; +pub(super) fn is_alphanumeric(name: &str, value: &str) -> Result<(), Error> { + if value.chars().any(|c| { + !(c.is_ascii_alphanumeric() || matches!(c, '_' | '-' | '.' | '+')) + }) { + return Err(Error::NotFound { + query: format!( + "{name}('{value}') invalid: must be [a-zA-Z0-9.-_+]" + ), + }); + } + Ok(()) +} + /// The LVM code currently uses an async executor which is not runnable within /// the spdk reactor, and as such we need a trampoline in order to use spdk /// functionality within the LVM code. diff --git a/io-engine/src/lvm/vg_pool.rs b/io-engine/src/lvm/vg_pool.rs index 0c9ad911b..02344357e 100644 --- a/io-engine/src/lvm/vg_pool.rs +++ b/io-engine/src/lvm/vg_pool.rs @@ -21,22 +21,26 @@ pub(crate) struct QueryArgs(CmnQueryArgs); impl QueryArgs { /// Get a comma-separated list of query selection args. /// todo: should be Display trait? - pub(super) fn query(&self) -> String { + pub(super) fn query(&self) -> Result { Self::query_args(&self.0) } /// Get a comma-separated list of query selection args. - pub(super) fn query_args(args: &CmnQueryArgs) -> String { + pub(super) fn query_args(args: &CmnQueryArgs) -> Result { let mut select = String::new(); if let Some(vg_name) = &args.name { + super::is_alphanumeric("vg_name", vg_name)?; select.push_str(&format!("vg_name={vg_name},")); } if let Some(vg_uuid) = &args.uuid { + super::is_alphanumeric("vg_uuid", vg_uuid)?; + // todo: validate more... select.push_str(&format!("vg_uuid={vg_uuid},")); } if let Some(vg_tag) = &args.tag { + super::is_alphanumeric("vg_tag", vg_tag)?; select.push_str(&format!("vg_tags={vg_tag},")); } - select + Ok(select) } } impl From for QueryArgs { @@ -96,7 +100,7 @@ impl VolumeGroup { pub(crate) async fn lookup(args: CmnQueryArgs) -> Result { let vgs = Self::list(&args).await?; vgs.into_iter().next().ok_or(Error::NotFound { - query: QueryArgs(args).query(), + query: QueryArgs(args).query().unwrap_or_else(|e| e.to_string()), }) } @@ -111,7 +115,7 @@ impl VolumeGroup { "--options=vg_name,vg_uuid,vg_size,vg_free,vg_tags,pv_name", "--report-format=json", ]; - let select = QueryArgs::query_args(opts); + let select = QueryArgs::query_args(opts)?; let select_query = format!("--select={select}"); if !select.is_empty() { args.push(select_query.trim_end_matches(',')); diff --git a/test/python/v1/replica/features/replica.feature b/test/python/v1/replica/features/replica.feature index 37ce1f444..d70da154a 100644 --- a/test/python/v1/replica/features/replica.feature +++ b/test/python/v1/replica/features/replica.feature @@ -95,6 +95,10 @@ Feature: Mayastor replica management When the user destroys a replica that does not exist Then the replica destroy command should fail + Scenario: destroying a replica that does not exist in the pool + When the user destroys a replica that does not exist in the pool + Then the replica destroy command should fail + Scenario: writing to a shared replica Given a replica shared over "nvmf" When the user writes to the replica diff --git a/test/python/v1/replica/test_bdd_replica.py b/test/python/v1/replica/test_bdd_replica.py index 2d4564e8f..5d9f161da 100644 --- a/test/python/v1/replica/test_bdd_replica.py +++ b/test/python/v1/replica/test_bdd_replica.py @@ -54,6 +54,13 @@ def test_fail_destroying_a_replica_that_does_not_exist(): """Destroying a replica that does not exist.""" +@scenario( + "features/replica.feature", "destroying a replica that does not exist in the pool" +) +def test_fail_destroying_a_replica_that_does_not_exist_in_the_pool(): + """Destroying a replica that does not exist in the pool.""" + + @scenario("features/replica.feature", "listing replicas") def test_listing_replicas(): """Listing replicas.""" @@ -317,6 +324,23 @@ def the_user_destroys_a_replica_that_does_not_exist( assert error.value.code() == grpc.StatusCode.NOT_FOUND +@when( + "the user destroys a replica that does not exist in the pool", + target_fixture="the_user_destroys_a_replica_that_does_not_exist_in_the_pool", +) +def the_user_destroys_a_replica_that_does_not_exist( + mayastor_instance, find_replica, replica_name, replica_uuid +): + assert find_replica(replica_name, replica_uuid) == None + with pytest.raises(grpc.RpcError) as error: + mayastor_instance.replica_rpc.DestroyReplica( + replica_pb.DestroyReplicaRequest( + uuid=replica_uuid, pool_name="does not exist" + ) + ) + assert error.value.code() == grpc.StatusCode.FAILED_PRECONDITION + + @when("the user destroys the replica") def the_user_destroys_the_replica(mayastor_instance, current_replicas, replica_uuid): replica = current_replicas[replica_uuid]