From cd5068f5813901f79dafb8e5ff6d1bab2f520701 Mon Sep 17 00:00:00 2001 From: tison Date: Fri, 26 Apr 2024 03:50:32 +0800 Subject: [PATCH 1/2] refactor: log statement on error by Display Signed-off-by: tison --- src/frontend/src/instance.rs | 12 +++---- src/frontend/src/instance/prom_store.rs | 4 +-- src/servers/src/error.rs | 3 +- src/servers/src/query_handler/sql.rs | 13 ++------ src/sql/src/util.rs | 43 ------------------------- 5 files changed, 9 insertions(+), 66 deletions(-) diff --git a/src/frontend/src/instance.rs b/src/frontend/src/instance.rs index 5709c57d2931..dbf35e5603d0 100644 --- a/src/frontend/src/instance.rs +++ b/src/frontend/src/instance.rs @@ -315,20 +315,18 @@ impl SqlQueryHandler for Instance { break; } - match self.query_statement(stmt, query_ctx.clone()).await { + match self.query_statement(stmt.clone(), query_ctx.clone()).await { Ok(output) => { let output_result = query_interceptor.post_execute(output, query_ctx.clone()); results.push(output_result); } Err(e) => { - let redacted = sql::util::redact_sql_secrets(query.as_ref()); if e.status_code().should_log_error() { - error!(e; "Failed to execute query: {redacted}"); + error!(e; "Failed to execute query: {stmt}"); } else { - debug!("Failed to execute query: {redacted}, {e}"); + debug!("Failed to execute query: {stmt}, {e}"); } - results.push(Err(e)); break; } @@ -448,9 +446,7 @@ impl PrometheusHandler for Instance { .execute_stmt(stmt, query_ctx.clone()) .await .map_err(BoxedError::new) - .with_context(|_| ExecuteQuerySnafu { - query: format!("{query:?}"), - })?; + .with_context(|_| ExecuteQuerySnafu)?; Ok(interceptor.post_execute(output, query_ctx)?) } diff --git a/src/frontend/src/instance/prom_store.rs b/src/frontend/src/instance/prom_store.rs index 104573bf8621..e9ff182a0a33 100644 --- a/src/frontend/src/instance/prom_store.rs +++ b/src/frontend/src/instance/prom_store.rs @@ -149,9 +149,7 @@ impl Instance { .handle_remote_query(&ctx, catalog_name, schema_name, &table_name, query) .await .map_err(BoxedError::new) - .with_context(|_| error::ExecuteQuerySnafu { - query: format!("{query:#?}"), - })?; + .with_context(|_| error::ExecuteQuerySnafu)?; results.push((table_name, output)); } diff --git a/src/servers/src/error.rs b/src/servers/src/error.rs index e3172ae8f89c..2af6a38243d7 100644 --- a/src/servers/src/error.rs +++ b/src/servers/src/error.rs @@ -97,9 +97,8 @@ pub enum Error { error: Box, }, - #[snafu(display("Failed to execute query, query: {}", query))] + #[snafu(display("Failed to execute query"))] ExecuteQuery { - query: String, location: Location, source: BoxedError, }, diff --git a/src/servers/src/query_handler/sql.rs b/src/servers/src/query_handler/sql.rs index 79e63f86e7a7..6d4d70349bf8 100644 --- a/src/servers/src/query_handler/sql.rs +++ b/src/servers/src/query_handler/sql.rs @@ -84,10 +84,7 @@ where .do_query(query, query_ctx) .await .into_iter() - .map(|x| { - x.map_err(BoxedError::new) - .context(error::ExecuteQuerySnafu { query }) - }) + .map(|x| x.map_err(BoxedError::new).context(error::ExecuteQuerySnafu)) .collect() } @@ -109,12 +106,8 @@ where .await .into_iter() .map(|x| { - x.map_err(BoxedError::new).with_context(|_| { - let query_literal = format!("{query:?}"); - error::ExecuteQuerySnafu { - query: query_literal, - } - }) + x.map_err(BoxedError::new) + .with_context(|_| error::ExecuteQuerySnafu) }) .collect() } diff --git a/src/sql/src/util.rs b/src/sql/src/util.rs index 39efe2c3e411..f2d8a9aaab83 100644 --- a/src/sql/src/util.rs +++ b/src/sql/src/util.rs @@ -13,20 +13,11 @@ // limitations under the License. use std::fmt::{Display, Formatter}; -use std::sync::LazyLock; -use regex::Regex; use sqlparser::ast::{Expr, ObjectName, SqlOption, Value}; use crate::error::{InvalidTableOptionValueSnafu, Result}; -static SQL_SECRET_PATTERNS: LazyLock> = LazyLock::new(|| { - vec![ - Regex::new(r#"(?i)access_key_id=["']([^"']*)["'].*"#).unwrap(), - Regex::new(r#"(?i)secret_access_key=["']([^"']*)["'].*"#).unwrap(), - ] -}); - /// Format an [ObjectName] without any quote of its idents. pub fn format_raw_object_name(name: &ObjectName) -> String { struct Inner<'a> { @@ -59,37 +50,3 @@ pub fn parse_option_string(option: SqlOption) -> Result<(String, String)> { let k = key.value.to_lowercase(); Ok((k, v)) } - -/// Use regex to match and replace common seen secret values in SQL. -pub fn redact_sql_secrets(sql: &str) -> String { - let mut s = sql.to_string(); - for p in SQL_SECRET_PATTERNS.iter() { - if let Some(captures) = p.captures(&s) { - if let Some(m) = captures.get(1) { - s = s.replace(m.as_str(), "******"); - } - } - } - s -} - -#[cfg(test)] -mod test { - use super::*; - - #[test] - fn test_redact_sql_secrets() { - assert_eq!( - redact_sql_secrets( - r#"COPY 'my_table' FROM '/test.orc' WITH (FORMAT = 'orc') CONNECTION(ENDPOINT = 's3.storage.site', REGION = 'hz', ACCESS_KEY_ID='my_key_id', SECRET_ACCESS_KEY="my_access_key");"# - ), - r#"COPY 'my_table' FROM '/test.orc' WITH (FORMAT = 'orc') CONNECTION(ENDPOINT = 's3.storage.site', REGION = 'hz', ACCESS_KEY_ID='******', SECRET_ACCESS_KEY="******");"# - ); - assert_eq!( - redact_sql_secrets( - r#"COPY 'my_table' FROM '/test.orc' WITH (FORMAT = 'orc') CONNECTION(ENDPOINT = 's3.storage.site', REGION = 'hz', ACCESS_KEY_ID='@scoped/key_id', SECRET_ACCESS_KEY="@scoped/access_key");"# - ), - r#"COPY 'my_table' FROM '/test.orc' WITH (FORMAT = 'orc') CONNECTION(ENDPOINT = 's3.storage.site', REGION = 'hz', ACCESS_KEY_ID='******', SECRET_ACCESS_KEY="******");"# - ); - } -} From bc4932c043f57564b52a0933ac6863219ef7be95 Mon Sep 17 00:00:00 2001 From: tison Date: Fri, 26 Apr 2024 11:42:24 +0800 Subject: [PATCH 2/2] Apply suggestions from code review Co-authored-by: Yingwen Signed-off-by: tison --- src/frontend/src/instance.rs | 2 +- src/frontend/src/instance/prom_store.rs | 2 +- src/servers/src/query_handler/sql.rs | 5 +---- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/frontend/src/instance.rs b/src/frontend/src/instance.rs index dbf35e5603d0..8e45d37af02a 100644 --- a/src/frontend/src/instance.rs +++ b/src/frontend/src/instance.rs @@ -446,7 +446,7 @@ impl PrometheusHandler for Instance { .execute_stmt(stmt, query_ctx.clone()) .await .map_err(BoxedError::new) - .with_context(|_| ExecuteQuerySnafu)?; + .context(ExecuteQuerySnafu)?; Ok(interceptor.post_execute(output, query_ctx)?) } diff --git a/src/frontend/src/instance/prom_store.rs b/src/frontend/src/instance/prom_store.rs index e9ff182a0a33..8821f064f0bd 100644 --- a/src/frontend/src/instance/prom_store.rs +++ b/src/frontend/src/instance/prom_store.rs @@ -149,7 +149,7 @@ impl Instance { .handle_remote_query(&ctx, catalog_name, schema_name, &table_name, query) .await .map_err(BoxedError::new) - .with_context(|_| error::ExecuteQuerySnafu)?; + .context(error::ExecuteQuerySnafu)?; results.push((table_name, output)); } diff --git a/src/servers/src/query_handler/sql.rs b/src/servers/src/query_handler/sql.rs index 6d4d70349bf8..9b9148af881a 100644 --- a/src/servers/src/query_handler/sql.rs +++ b/src/servers/src/query_handler/sql.rs @@ -105,10 +105,7 @@ where .do_promql_query(query, query_ctx) .await .into_iter() - .map(|x| { - x.map_err(BoxedError::new) - .with_context(|_| error::ExecuteQuerySnafu) - }) + .map(|x| x.map_err(BoxedError::new).context(error::ExecuteQuerySnafu)) .collect() }