From 38d3819c7840c0260a8540d307b701ee55a1ec79 Mon Sep 17 00:00:00 2001 From: forestmvey Date: Mon, 11 Jul 2022 16:18:54 -0700 Subject: [PATCH 1/5] Initial Implementation Signed-off-by: forestmvey --- .../sql/legacy/plugin/RestSQLQueryAction.java | 14 ++++++++++++++ .../sql/legacy/plugin/RestSqlAction.java | 10 ++++++---- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java index 51484feda7a..4bef7631765 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java @@ -6,9 +6,12 @@ package org.opensearch.sql.legacy.plugin; +import static org.opensearch.rest.RestStatus.BAD_REQUEST; import static org.opensearch.rest.RestStatus.INTERNAL_SERVER_ERROR; import static org.opensearch.rest.RestStatus.OK; +import static org.opensearch.rest.RestStatus.SERVICE_UNAVAILABLE; import static org.opensearch.sql.executor.ExecutionEngine.QueryResponse; +import static org.opensearch.sql.legacy.plugin.RestSqlAction.isClientError; import static org.opensearch.sql.protocol.response.format.JsonResponseFormatter.Style.PRETTY; import java.io.IOException; @@ -27,6 +30,7 @@ import org.opensearch.sql.common.response.ResponseListener; import org.opensearch.sql.common.setting.Settings; import org.opensearch.sql.executor.ExecutionEngine.ExplainResponse; +import org.opensearch.sql.legacy.executor.format.ErrorMessageFactory; import org.opensearch.sql.legacy.metrics.MetricName; import org.opensearch.sql.legacy.metrics.Metrics; import org.opensearch.sql.opensearch.security.SecurityAccess; @@ -60,6 +64,7 @@ public class RestSQLQueryAction extends BaseRestHandler { * Settings required by been initialization. */ private final Settings pluginSettings; + private String ErrorStr; /** * Constructor of RestSQLQueryAction. @@ -85,6 +90,14 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient nod throw new UnsupportedOperationException("New SQL handler is not ready yet"); } + public void setErrorStr(String error) { + ErrorStr = error; + } + + public String getErrorStr() { + return ErrorStr; + } + /** * Prepare REST channel consumer for a SQL query request. * @param request SQL request @@ -109,6 +122,7 @@ public RestChannelConsumer prepareRequest(SQLQueryRequest request, NodeClient no if (request.isExplainRequest()) { LOG.info("Request is falling back to old SQL engine due to: " + e.getMessage()); } + setErrorStr(ErrorMessageFactory.createErrorMessage(e, isClientError(e) ? BAD_REQUEST.getStatus() : SERVICE_UNAVAILABLE.getStatus()).toString()); return NOT_SUPPORTED_YET; } diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java index 10d9dab0fa0..83feda1f558 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java @@ -162,7 +162,7 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli return channel -> executeSqlRequest(request, queryAction, client, channel); } catch (Exception e) { logAndPublishMetrics(e); - return channel -> reportError(channel, e, isClientError(e) ? BAD_REQUEST : SERVICE_UNAVAILABLE); + return channel -> reportError(channel, e, isClientError(e) ? BAD_REQUEST : SERVICE_UNAVAILABLE, newSqlQueryHandler.getErrorStr()); } } @@ -234,7 +234,7 @@ private static boolean isExplainRequest(final RestRequest request) { return request.path().endsWith("/_explain"); } - private static boolean isClientError(Exception e) { + public static boolean isClientError(Exception e) { return e instanceof NullPointerException // NPE is hard to differentiate but more likely caused by bad query || e instanceof SqlParseException || e instanceof ParserException @@ -253,8 +253,10 @@ private void sendResponse(final RestChannel channel, final String message, final channel.sendResponse(new BytesRestResponse(status, message)); } - private void reportError(final RestChannel channel, final Exception e, final RestStatus status) { - sendResponse(channel, ErrorMessageFactory.createErrorMessage(e, status.getStatus()).toString(), status); + private void reportError(final RestChannel channel, final Exception e, final RestStatus status, String otherError) { + sendResponse(channel, ErrorMessageFactory.createErrorMessage(e, status.getStatus()).toString() + (otherError.isEmpty() + ? "" : "\nQuery failed both legacy and new SQL engines, see error message below for new SQL engine error.\n" + + otherError), status); } private boolean isSQLFeatureEnabled() { From d9bfd6c607738fa073152f6404aeb6b1e358268e Mon Sep 17 00:00:00 2001 From: forestmvey Date: Tue, 12 Jul 2022 08:42:57 -0700 Subject: [PATCH 2/5] Adding JAVADOC and code cleanup Signed-off-by: forestmvey --- .../sql/legacy/plugin/RestSQLQueryAction.java | 20 +++++++++++++++++++ .../sql/legacy/plugin/RestSqlAction.java | 13 +++++++++--- 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java index 4bef7631765..fc849ed9897 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java @@ -64,6 +64,13 @@ public class RestSQLQueryAction extends BaseRestHandler { * Settings required by been initialization. */ private final Settings pluginSettings; + + /** + * Captured error message to aggregate diagnostics + * for both legacy and new SQL engines. + * This member variable and it's usage can be deleted once the + * legacy SQL engine is deprecated. + */ private String ErrorStr; /** @@ -90,10 +97,18 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient nod throw new UnsupportedOperationException("New SQL handler is not ready yet"); } + /** + * Setter for ErrorStr member variable. + * @param error : String error value to set member variable. + */ public void setErrorStr(String error) { ErrorStr = error; } + /** + * Getter for ErrorStr member variable. + * @return : ErrorStr member variable. + */ public String getErrorStr() { return ErrorStr; } @@ -122,6 +137,11 @@ public RestChannelConsumer prepareRequest(SQLQueryRequest request, NodeClient no if (request.isExplainRequest()) { LOG.info("Request is falling back to old SQL engine due to: " + e.getMessage()); } + + /** + * Setting ErrorStr member variable is used to aggregate error messages when both legacy and new SQL engines fail. + * This implementation can be removed when the legacy SQL engine is deprecated. + */ setErrorStr(ErrorMessageFactory.createErrorMessage(e, isClientError(e) ? BAD_REQUEST.getStatus() : SERVICE_UNAVAILABLE.getStatus()).toString()); return NOT_SUPPORTED_YET; } diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java index 83feda1f558..996c572a128 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java @@ -253,10 +253,17 @@ private void sendResponse(final RestChannel channel, final String message, final channel.sendResponse(new BytesRestResponse(status, message)); } - private void reportError(final RestChannel channel, final Exception e, final RestStatus status, String otherError) { - sendResponse(channel, ErrorMessageFactory.createErrorMessage(e, status.getStatus()).toString() + (otherError.isEmpty() + /** + * Report Error message to user. + * @param channel : Rest channel to sent response through. + * @param e : Exception caught when attempting query. + * @param status : Status for rest request made. + * @param newSqlEngineError : Error message for new SQL engine. Can be removed when old SQL engine is deprecated. + */ + private void reportError(final RestChannel channel, final Exception e, final RestStatus status, String newSqlEngineError) { + sendResponse(channel, ErrorMessageFactory.createErrorMessage(e, status.getStatus()).toString() + (newSqlEngineError.isEmpty() ? "" : "\nQuery failed both legacy and new SQL engines, see error message below for new SQL engine error.\n" - + otherError), status); + + newSqlEngineError), status); } private boolean isSQLFeatureEnabled() { From 9866cce5f88c098d5e396806e36e37a672d812b4 Mon Sep 17 00:00:00 2001 From: forestmvey Date: Tue, 12 Jul 2022 14:03:55 -0700 Subject: [PATCH 3/5] Fixing output errors on tests, variable name formatting, minor revisions Signed-off-by: forestmvey --- docs/user/admin/settings.rst | 18 ++++++++++++++++++ .../sql/legacy/plugin/RestSQLQueryAction.java | 14 +++++++------- .../sql/legacy/plugin/RestSqlAction.java | 12 +++++++----- 3 files changed, 32 insertions(+), 12 deletions(-) diff --git a/docs/user/admin/settings.rst b/docs/user/admin/settings.rst index b5da4e28e22..453e299e93f 100644 --- a/docs/user/admin/settings.rst +++ b/docs/user/admin/settings.rst @@ -109,6 +109,15 @@ Result set:: }, "status" : 400 } + Query failed on both V1 and V2 SQL parser engines. V2 SQL parser error following: + { + "error": { + "reason": "Invalid SQL query", + "details": "Failed to parse query due to offending symbol [DELETE] at: 'DELETE' <--- HERE... More details: Expecting tokens in {, 'DESCRIBE', 'SELECT', 'SHOW', ';'}", + "type": "SyntaxCheckException" + }, + "status": 400 + } plugins.sql.slowlog ============================ @@ -310,4 +319,13 @@ SQL query:: }, "status": 400 } + Query failed on both V1 and V2 SQL parser engines. V2 SQL parser error following: + { + "error": { + "reason": "Invalid SQL query", + "details": "Failed to parse query due to offending symbol [DELETE] at: 'DELETE' <--- HERE... More details: Expecting tokens in {, 'DESCRIBE', 'SELECT', 'SHOW', ';'}", + "type": "SyntaxCheckException" + }, + "status": 400 + } diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java index fc849ed9897..562fb53d625 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java @@ -71,7 +71,7 @@ public class RestSQLQueryAction extends BaseRestHandler { * This member variable and it's usage can be deleted once the * legacy SQL engine is deprecated. */ - private String ErrorStr; + private String errorStr; /** * Constructor of RestSQLQueryAction. @@ -98,19 +98,19 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient nod } /** - * Setter for ErrorStr member variable. + * Setter for errorStr member variable. * @param error : String error value to set member variable. */ public void setErrorStr(String error) { - ErrorStr = error; + errorStr = error; } /** - * Getter for ErrorStr member variable. - * @return : ErrorStr member variable. + * Getter for errorStr member variable. + * @return : errorStr member variable. */ public String getErrorStr() { - return ErrorStr; + return errorStr; } /** @@ -139,7 +139,7 @@ public RestChannelConsumer prepareRequest(SQLQueryRequest request, NodeClient no } /** - * Setting ErrorStr member variable is used to aggregate error messages when both legacy and new SQL engines fail. + * Setting errorStr member variable is used to aggregate error messages when both legacy and new SQL engines fail. * This implementation can be removed when the legacy SQL engine is deprecated. */ setErrorStr(ErrorMessageFactory.createErrorMessage(e, isClientError(e) ? BAD_REQUEST.getStatus() : SERVICE_UNAVAILABLE.getStatus()).toString()); diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java index 996c572a128..8f2043f915b 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java @@ -258,12 +258,14 @@ private void sendResponse(final RestChannel channel, final String message, final * @param channel : Rest channel to sent response through. * @param e : Exception caught when attempting query. * @param status : Status for rest request made. - * @param newSqlEngineError : Error message for new SQL engine. Can be removed when old SQL engine is deprecated. + * @param v2SqlEngineError : Error message for new SQL engine. Can be removed when old SQL engine is deprecated. */ - private void reportError(final RestChannel channel, final Exception e, final RestStatus status, String newSqlEngineError) { - sendResponse(channel, ErrorMessageFactory.createErrorMessage(e, status.getStatus()).toString() + (newSqlEngineError.isEmpty() - ? "" : "\nQuery failed both legacy and new SQL engines, see error message below for new SQL engine error.\n" - + newSqlEngineError), status); + private void reportError(final RestChannel channel, final Exception e, final RestStatus status, String v2SqlEngineError) { + String errorMsg = ErrorMessageFactory.createErrorMessage(e, status.getStatus()).toString(); + errorMsg += v2SqlEngineError.isEmpty() ? "" : + "\nQuery failed on both V1 and V2 SQL parser engines. V2 SQL parser error following: \n" + + v2SqlEngineError; + sendResponse(channel, errorMsg, status); } private boolean isSQLFeatureEnabled() { From 05b4c9063b01a21cd1c2b75d9203b8965db5b269 Mon Sep 17 00:00:00 2001 From: forestmvey Date: Wed, 13 Jul 2022 13:52:42 -0700 Subject: [PATCH 4/5] Fixed so only successfully anonymized data is logged. Handled stack trace separately in log to avoid logging un-anonymized data. Added function comments Signed-off-by: forestmvey --- .../sql/legacy/plugin/RestSqlAction.java | 28 +++++++++++++++++-- .../sql/legacy/utils/QueryDataAnonymizer.java | 2 +- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java index 8f2043f915b..7e04299754c 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSqlAction.java @@ -12,6 +12,9 @@ import com.alibaba.druid.sql.parser.ParserException; import com.google.common.collect.ImmutableList; + +import java.io.PrintWriter; +import java.io.StringWriter; import java.sql.SQLFeatureNotSupportedException; import java.util.Arrays; import java.util.HashMap; @@ -118,6 +121,13 @@ public String getName() { return "sql_action"; } + /** + * Prepare and execute rest SQL request. In the event the V2 SQL engine fails, the V1 + * engine attempts the query. + * @param request : Rest request being made. + * @param client : Rest client for making the request. + * @return : Resulting values for request. + */ @Override protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) { Metrics.getInstance().getNumericalMetric(MetricName.REQ_TOTAL).increment(); @@ -161,6 +171,7 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli final QueryAction queryAction = explainRequest(client, sqlRequest, format); return channel -> executeSqlRequest(request, queryAction, client, channel); } catch (Exception e) { + LOG.error(LogUtils.getRequestId() + " V2 SQL error during query execution", QueryDataAnonymizer.anonymizeData(newSqlQueryHandler.getErrorStr())); logAndPublishMetrics(e); return channel -> reportError(channel, e, isClientError(e) ? BAD_REQUEST : SERVICE_UNAVAILABLE, newSqlQueryHandler.getErrorStr()); } @@ -180,14 +191,27 @@ private void handleCursorRequest(final RestRequest request, final String cursor, cursorRestExecutor.execute(client, request.params(), channel); } + /** + * Log error message for exception and increment failure statistics. + * @param e : Caught exception. + */ private static void logAndPublishMetrics(final Exception e) { + /** + * Use PrintWriter to copy the stack trace for logging. This is used to anonymize + * log messages, and can be reverted to the simpler implementation when + * the anonymizer is fixed. + */ + StringWriter sw = new StringWriter(); + e.printStackTrace(new PrintWriter(sw)); + String stackTrace = sw.toString(); if (isClientError(e)) { - LOG.error(LogUtils.getRequestId() + " Client side error during query execution", e); + LOG.error(LogUtils.getRequestId() + " Client side error during query execution", QueryDataAnonymizer.anonymizeData(e.getMessage())); Metrics.getInstance().getNumericalMetric(MetricName.FAILED_REQ_COUNT_CUS).increment(); } else { - LOG.error(LogUtils.getRequestId() + " Server side error during query execution", e); + LOG.error(LogUtils.getRequestId() + " Server side error during query execution", QueryDataAnonymizer.anonymizeData(e.getMessage())); Metrics.getInstance().getNumericalMetric(MetricName.FAILED_REQ_COUNT_SYS).increment(); } + LOG.error(stackTrace); } private static QueryAction explainRequest(final NodeClient client, final SqlRequest sqlRequest, Format format) diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/utils/QueryDataAnonymizer.java b/legacy/src/main/java/org/opensearch/sql/legacy/utils/QueryDataAnonymizer.java index 91406333aed..5eacc603a86 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/utils/QueryDataAnonymizer.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/utils/QueryDataAnonymizer.java @@ -39,7 +39,7 @@ public static String anonymizeData(String query) { .replaceAll("[\\n][\\t]+", " "); } catch (Exception e) { LOG.warn("Caught an exception when anonymizing sensitive data"); - resultQuery = query; + resultQuery = "Failed to anonymize data."; } return resultQuery; } From 0467d131f9015006222848200e0579ef05bb4b16 Mon Sep 17 00:00:00 2001 From: forestmvey Date: Wed, 13 Jul 2022 14:58:24 -0700 Subject: [PATCH 5/5] Fix whitespace and add comments on updates for anonymizing string function Signed-off-by: forestmvey --- docs/user/admin/settings.rst | 14 +++++++------- .../sql/legacy/utils/QueryDataAnonymizer.java | 6 ++++-- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/docs/user/admin/settings.rst b/docs/user/admin/settings.rst index 453e299e93f..8ee6df99446 100644 --- a/docs/user/admin/settings.rst +++ b/docs/user/admin/settings.rst @@ -111,13 +111,13 @@ Result set:: } Query failed on both V1 and V2 SQL parser engines. V2 SQL parser error following: { - "error": { - "reason": "Invalid SQL query", - "details": "Failed to parse query due to offending symbol [DELETE] at: 'DELETE' <--- HERE... More details: Expecting tokens in {, 'DESCRIBE', 'SELECT', 'SHOW', ';'}", - "type": "SyntaxCheckException" - }, - "status": 400 - } + "error": { + "reason": "Invalid SQL query", + "details": "Failed to parse query due to offending symbol [DELETE] at: 'DELETE' <--- HERE... More details: Expecting tokens in {, 'DESCRIBE', 'SELECT', 'SHOW', ';'}", + "type": "SyntaxCheckException" + }, + "status": 400 + } plugins.sql.slowlog ============================ diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/utils/QueryDataAnonymizer.java b/legacy/src/main/java/org/opensearch/sql/legacy/utils/QueryDataAnonymizer.java index 5eacc603a86..b58691c0221 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/utils/QueryDataAnonymizer.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/utils/QueryDataAnonymizer.java @@ -26,7 +26,8 @@ public class QueryDataAnonymizer { * Sensitive data includes index names, column names etc., * which in druid parser are parsed to SQLIdentifierExpr instances * @param query entire sql query string - * @return sql query string with all identifiers replaced with "***" + * @return sql query string with all identifiers replaced with "***" on success + * and failure string otherwise to ensure no non-anonymized data is logged in production. */ public static String anonymizeData(String query) { String resultQuery; @@ -38,7 +39,8 @@ public static String anonymizeData(String query) { .replaceAll("false", "boolean_literal") .replaceAll("[\\n][\\t]+", " "); } catch (Exception e) { - LOG.warn("Caught an exception when anonymizing sensitive data"); + LOG.warn("Caught an exception when anonymizing sensitive data."); + LOG.debug("String {} failed anonymization.", query); resultQuery = "Failed to anonymize data."; } return resultQuery;