From 92b1055e418b51a699fd7c81e374a69c81a0c2e0 Mon Sep 17 00:00:00 2001 From: Dom Dwyer Date: Tue, 18 Apr 2023 14:46:22 +0200 Subject: [PATCH 1/3] refactor: error message should not end in . Change the "bad character" namespace name error to not include a trailing "." --- data_types/src/namespace_name.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/data_types/src/namespace_name.rs b/data_types/src/namespace_name.rs index ffa0b451ec..a89230e9b5 100644 --- a/data_types/src/namespace_name.rs +++ b/data_types/src/namespace_name.rs @@ -41,7 +41,7 @@ pub enum NamespaceNameError { /// The provided namespace name contains an unacceptable character. #[error( "namespace name '{}' contains invalid character, character number {} \ - is a control which is not allowed.", + is a control which is not allowed", name, bad_char_offset )] @@ -256,25 +256,25 @@ mod tests { #[test] fn test_bad_chars_null() { let got = NamespaceName::new("example\x00").unwrap_err(); - assert_eq!(got.to_string() , "namespace name 'example\x00' contains invalid character, character number 7 is a control which is not allowed."); + assert_eq!(got.to_string() , "namespace name 'example\x00' contains invalid character, character number 7 is a control which is not allowed"); } #[test] fn test_bad_chars_high_control() { let got = NamespaceName::new("\u{007f}example").unwrap_err(); - assert_eq!(got.to_string() , "namespace name '\u{007f}example' contains invalid character, character number 0 is a control which is not allowed."); + assert_eq!(got.to_string() , "namespace name '\u{007f}example' contains invalid character, character number 0 is a control which is not allowed"); } #[test] fn test_bad_chars_tab() { let got = NamespaceName::new("example\tdb").unwrap_err(); - assert_eq!(got.to_string() , "namespace name 'example\tdb' contains invalid character, character number 7 is a control which is not allowed."); + assert_eq!(got.to_string() , "namespace name 'example\tdb' contains invalid character, character number 7 is a control which is not allowed"); } #[test] fn test_bad_chars_newline() { let got = NamespaceName::new("my_example\ndb").unwrap_err(); - assert_eq!(got.to_string() , "namespace name 'my_example\ndb' contains invalid character, character number 10 is a control which is not allowed."); + assert_eq!(got.to_string() , "namespace name 'my_example\ndb' contains invalid character, character number 10 is a control which is not allowed"); } #[test] From 54d4ca87294b21bcc629882865210fd4f96a8964 Mon Sep 17 00:00:00 2001 From: Dom Dwyer Date: Tue, 18 Apr 2023 14:48:44 +0200 Subject: [PATCH 2/3] test: invalid namespace name creation Adds tests that attempt to create namespaces via the gRPC NamespaceService with a variety of invalid names. NOTE: this test currently fails! --- Cargo.lock | 1 + service_grpc_namespace/Cargo.toml | 1 + service_grpc_namespace/src/lib.rs | 119 ++++++++++++++++++++++++++++++ 3 files changed, 121 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 39af73a204..d5b51f5010 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5111,6 +5111,7 @@ dependencies = [ "iox_tests", "metric", "observability_deps", + "paste", "tokio", "tonic 0.9.2", "workspace-hack", diff --git a/service_grpc_namespace/Cargo.toml b/service_grpc_namespace/Cargo.toml index e7683835b9..1d7488f45a 100644 --- a/service_grpc_namespace/Cargo.toml +++ b/service_grpc_namespace/Cargo.toml @@ -17,4 +17,5 @@ workspace-hack = { version = "0.1", path = "../workspace-hack" } assert_matches = "1.5.0" iox_tests = { path = "../iox_tests" } metric = { path = "../metric" } +paste = "1.0.12" tokio = { version = "1", features = ["rt-multi-thread", "macros"] } diff --git a/service_grpc_namespace/src/lib.rs b/service_grpc_namespace/src/lib.rs index 26754f77b2..86bf64b77d 100644 --- a/service_grpc_namespace/src/lib.rs +++ b/service_grpc_namespace/src/lib.rs @@ -552,4 +552,123 @@ mod tests { ); assert_eq!(status.code(), Code::InvalidArgument); } + + macro_rules! test_create_namespace_name { + ( + $test_name:ident, + name = $name:expr, // The input namespace name string + want = $($want:tt)+ // A pattern match against Result + // where the Ok variant contains the actual namespace + // name used on the server side (potentially encoded) + ) => { + paste::paste! { + #[tokio::test] + async fn []() { + let catalog: Arc = + Arc::new(MemCatalog::new(Arc::new(metric::Registry::default()))); + + let topic = catalog + .repositories() + .await + .topics() + .create_or_get("kafka-topic") + .await + .unwrap(); + let query_pool = catalog + .repositories() + .await + .query_pools() + .create_or_get("query-pool") + .await + .unwrap(); + + let handler = NamespaceService::new(catalog, Some(topic.id), Some(query_pool.id)); + + let req = CreateNamespaceRequest { + name: String::from($name), + retention_period_ns: Some(RETENTION), + }; + + let got = handler.create_namespace(Request::new(req)).await; + + // Map the result into just the name so it's easier for the + // test to assert the correct namespace name was used. + let actual_name_res = got.as_ref().map(|v| { + v.get_ref() + .namespace + .as_ref() + .expect("no namespace in response") + .name + .as_str() + }); + assert_matches!(actual_name_res, $($want)+); + + // Attempt to list the namespaces + let list = handler + .get_namespaces(Request::new(Default::default())) + .await + .expect("must return namespaces") + .into_inner() + .namespaces; + + // Validate the expected state - if the request succeeded, then the + // namespace MUST exist. + match got { + Ok(got) => { + assert_matches!(list.as_slice(), [ns] => { + assert_eq!(ns, got.get_ref().namespace.as_ref().unwrap()); + }) + } + Err(_) => assert!(list.is_empty()), + } + } + } + } + } + + test_create_namespace_name!(ok, name = "bananas", want = Ok("bananas")); + + test_create_namespace_name!(multi_byte, name = "🍌", want = Ok("🍌")); + + test_create_namespace_name!( + tab, + name = "it\tis\ttabtasitc", + want = Err(e) => { + assert_eq!(e.code(), Code::InvalidArgument); + assert_eq!(e.message(), "namespace name 'it\tis\ttabtasitc' contains invalid character, character number 2 is a control which is not allowed"); + } + ); + + test_create_namespace_name!( + null, + name = "bad \0 bananas", + want = Err(e) => { + assert_eq!(e.code(), Code::InvalidArgument); + assert_eq!(e.message(), "namespace name 'bad \0 bananas' contains invalid character, character number 4 is a control which is not allowed"); + } + ); + + test_create_namespace_name!( + length_lower, + name = "", + want = Err(e) => { + assert_eq!(e.code(), Code::InvalidArgument); + assert_eq!(e.message(), r#"namespace name length must be between 1 and 64 characters"#); + } + ); + + test_create_namespace_name!( + length_upper_inclusive, + name = "A".repeat(64), + want = Ok(v) if v == "A".repeat(64) + ); + + test_create_namespace_name!( + length_upper_exclusive, + name = "A".repeat(65), + want = Err(e) => { + assert_eq!(e.code(), Code::InvalidArgument); + assert_eq!(e.message(), r#"namespace name AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA length must be between 1 and 64 characters"#); + } + ); } From c4a802cea81edee304d42c55a9fc4178a2d6969e Mon Sep 17 00:00:00 2001 From: Dom Dwyer Date: Tue, 18 Apr 2023 14:50:51 +0200 Subject: [PATCH 3/3] fix: apply namespace name validation on create Prior to this commit, the gRPC NamespaceService would allow namespaces to be created with invalid names due to a missing call to NamespaceName that performs validation. --- service_grpc_namespace/src/lib.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/service_grpc_namespace/src/lib.rs b/service_grpc_namespace/src/lib.rs index 86bf64b77d..f624838367 100644 --- a/service_grpc_namespace/src/lib.rs +++ b/service_grpc_namespace/src/lib.rs @@ -1,7 +1,7 @@ //! Implementation of the namespace gRPC service use std::sync::Arc; -use data_types::{Namespace as CatalogNamespace, QueryPoolId, TopicId}; +use data_types::{Namespace as CatalogNamespace, NamespaceName, QueryPoolId, TopicId}; use generated_types::influxdata::iox::namespace::v1::{ update_namespace_service_protection_limit_request::LimitUpdate, *, }; @@ -69,6 +69,11 @@ impl namespace_service_server::NamespaceService for NamespaceService { retention_period_ns, } = request.into_inner(); + // Ensure the namespace name is consistently processed within IOx - this + // is handled by the NamespaceName type. + let namespace_name = NamespaceName::try_from(namespace_name) + .map_err(|v| Status::invalid_argument(v.to_string()))?; + let retention_period_ns = map_retention_period(retention_period_ns)?; debug!(%namespace_name, ?retention_period_ns, "Creating namespace"); @@ -88,7 +93,7 @@ impl namespace_service_server::NamespaceService for NamespaceService { })?; info!( - namespace_name, + %namespace_name, namespace_id = %namespace.id, "created namespace" );