From db57f90a34b86b06cc07f4c8cde33fcdf3245e3e Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Wed, 17 Mar 2021 20:29:26 -0700 Subject: [PATCH] feat: better error message when adding DBs (#13601) * WIP * Adding tests * Add unit tests * Show error message * Fix lint * Fix after rebase --- .../pages/docs/Miscellaneous/issue_codes.mdx | 29 ++++++ .../src/components/ErrorMessage/types.ts | 4 + .../src/setup/setupErrorMessages.ts | 12 +++ superset/databases/api.py | 3 + superset/databases/commands/exceptions.py | 5 ++ .../databases/commands/test_connection.py | 55 +++++++++++- superset/errors.py | 21 +++++ superset/utils/log.py | 5 +- superset/utils/network.py | 71 +++++++++++++++ tests/databases/api_tests.py | 88 +++++++++++++++++++ 10 files changed, 291 insertions(+), 2 deletions(-) create mode 100644 superset/utils/network.py diff --git a/docs/src/pages/docs/Miscellaneous/issue_codes.mdx b/docs/src/pages/docs/Miscellaneous/issue_codes.mdx index abf4886ce..8b8e0c8e7 100644 --- a/docs/src/pages/docs/Miscellaneous/issue_codes.mdx +++ b/docs/src/pages/docs/Miscellaneous/issue_codes.mdx @@ -84,3 +84,32 @@ Your query was not submitted to the database because it's missing one or more parameters. You should define all the parameters referenced in the query in a valid JSON document. Check that the parameters are spelled correctly and that the document has a valid syntax. + +## Issue 1007 + +``` +The hostname provided can't be resolved. +``` + +The hostname provided when adding a new database is invalid and cannot be +resolved. Please check that there are no typos in the hostname. + +## Issue 1008 + +``` +The port is closed. +``` + +The port provided when adding a new database is not open. Please check that +the port number is correct, and that the database is running and listening on +that port. + +## Issue 1009 + +``` +The host might be down, and cannot be reached on the provided port. +``` + +The host provided when adding a new database doesn't seem to be up. +Additionally, it cannot be reached on the provided port. Please check that +there are no firewall rules preventing access to the host. diff --git a/superset-frontend/src/components/ErrorMessage/types.ts b/superset-frontend/src/components/ErrorMessage/types.ts index b64aaf163..30cb2658b 100644 --- a/superset-frontend/src/components/ErrorMessage/types.ts +++ b/superset-frontend/src/components/ErrorMessage/types.ts @@ -46,6 +46,10 @@ export const ErrorTypeEnum = { // Sqllab error MISSING_TEMPLATE_PARAMS_ERROR: 'MISSING_TEMPLATE_PARAMS_ERROR', + TEST_CONNECTION_INVALID_HOSTNAME_ERROR: + 'TEST_CONNECTION_INVALID_HOSTNAME_ERROR', + TEST_CONNECTION_PORT_CLOSED_ERROR: 'TEST_CONNECTION_PORT_CLOSED_ERROR', + TEST_CONNECTION_HOST_DOWN_ERROR: 'TEST_CONNECTION_HOST_DOWN_ERROR', } as const; type ValueOf = T[keyof T]; diff --git a/superset-frontend/src/setup/setupErrorMessages.ts b/superset-frontend/src/setup/setupErrorMessages.ts index 9c8541b2c..ef34c3b7d 100644 --- a/superset-frontend/src/setup/setupErrorMessages.ts +++ b/superset-frontend/src/setup/setupErrorMessages.ts @@ -51,5 +51,17 @@ export default function setupErrorMessages() { ErrorTypeEnum.MISSING_TEMPLATE_PARAMS_ERROR, ParameterErrorMessage, ); + errorMessageComponentRegistry.registerValue( + ErrorTypeEnum.TEST_CONNECTION_INVALID_HOSTNAME_ERROR, + DatabaseErrorMessage, + ); + errorMessageComponentRegistry.registerValue( + ErrorTypeEnum.TEST_CONNECTION_PORT_CLOSED_ERROR, + DatabaseErrorMessage, + ); + errorMessageComponentRegistry.registerValue( + ErrorTypeEnum.TEST_CONNECTION_HOST_DOWN_ERROR, + DatabaseErrorMessage, + ); setupErrorMessagesExtra(); } diff --git a/superset/databases/api.py b/superset/databases/api.py index 1c92ab757..d98d79744 100644 --- a/superset/databases/api.py +++ b/superset/databases/api.py @@ -64,6 +64,7 @@ from superset.databases.schemas import ( TableMetadataResponseSchema, ) from superset.databases.utils import get_table_metadata +from superset.exceptions import SupersetErrorException from superset.extensions import security_manager from superset.models.core import Database from superset.typing import FlaskResponse @@ -608,6 +609,8 @@ class DatabaseRestApi(BaseSupersetModelRestApi): return self.response(200, message="OK") except DatabaseTestConnectionFailedError as ex: return self.response_422(message=str(ex)) + except SupersetErrorException as ex: + return self.response(ex.status, message=ex.error.message) @expose("//related_objects/", methods=["GET"]) @protect() diff --git a/superset/databases/commands/exceptions.py b/superset/databases/commands/exceptions.py index 2bbc8faec..a191ac96f 100644 --- a/superset/databases/commands/exceptions.py +++ b/superset/databases/commands/exceptions.py @@ -25,6 +25,7 @@ from superset.commands.exceptions import ( ImportFailedError, UpdateFailedError, ) +from superset.exceptions import SupersetErrorException class DatabaseInvalidError(CommandInvalidError): @@ -134,3 +135,7 @@ class DatabaseTestConnectionUnexpectedError(DatabaseTestConnectionFailedError): class DatabaseImportError(ImportFailedError): message = _("Import database failed for an unknown reason") + + +class DatabaseTestConnectionNetworkError(SupersetErrorException): + status = 400 diff --git a/superset/databases/commands/test_connection.py b/superset/databases/commands/test_connection.py index 101b07220..b6171f107 100644 --- a/superset/databases/commands/test_connection.py +++ b/superset/databases/commands/test_connection.py @@ -20,6 +20,7 @@ from typing import Any, Dict, Optional from flask_appbuilder.security.sqla.models import User from flask_babel import gettext as _ +from sqlalchemy.engine.url import make_url from sqlalchemy.exc import DBAPIError, NoSuchModuleError from superset.commands.base import BaseCommand @@ -27,12 +28,15 @@ from superset.databases.commands.exceptions import ( DatabaseSecurityUnsafeError, DatabaseTestConnectionDriverError, DatabaseTestConnectionFailedError, + DatabaseTestConnectionNetworkError, DatabaseTestConnectionUnexpectedError, ) from superset.databases.dao import DatabaseDAO +from superset.errors import ErrorLevel, SupersetErrorType from superset.exceptions import SupersetSecurityException from superset.extensions import event_logger from superset.models.core import Database +from superset.utils.network import is_host_up, is_hostname_valid, is_port_open logger = logging.getLogger(__name__) @@ -43,6 +47,53 @@ class TestConnectionDatabaseCommand(BaseCommand): self._properties = data.copy() self._model: Optional[Database] = None + @staticmethod + def _diagnose(uri: str) -> None: + parsed_uri = make_url(uri) + if parsed_uri.host: + if not is_hostname_valid(parsed_uri.host): + raise DatabaseTestConnectionNetworkError( + error_type=SupersetErrorType.TEST_CONNECTION_INVALID_HOSTNAME_ERROR, + message=_( + 'Unable to resolve hostname "%(hostname)s".', + hostname=parsed_uri.host, + ), + level=ErrorLevel.ERROR, + extra={"hostname": parsed_uri.host}, + ) + + if parsed_uri.port: + if not is_port_open(parsed_uri.host, parsed_uri.port): + if is_host_up(parsed_uri.host): + raise DatabaseTestConnectionNetworkError( + error_type=( + SupersetErrorType.TEST_CONNECTION_PORT_CLOSED_ERROR + ), + message=_( + "The host %(host)s is up, but the port %(port)s is " + "closed.", + host=parsed_uri.host, + port=parsed_uri.port, + ), + level=ErrorLevel.ERROR, + extra={ + "hostname": parsed_uri.host, + "port": parsed_uri.port, + }, + ) + + raise DatabaseTestConnectionNetworkError( + error_type=SupersetErrorType.TEST_CONNECTION_HOST_DOWN_ERROR, + message=_( + "The host %(host)s might be down, ond can't be reached on " + "port %(port)s.", + host=parsed_uri.host, + port=parsed_uri.port, + ), + level=ErrorLevel.ERROR, + extra={"hostname": parsed_uri.host, "port": parsed_uri.port,}, + ) + def run(self) -> None: self.validate() uri = self._properties.get("sqlalchemy_uri", "") @@ -79,6 +130,8 @@ class TestConnectionDatabaseCommand(BaseCommand): action=f"test_connection_error.{ex.__class__.__name__}", engine=database.db_engine_spec.__name__, ) + # check if we have connectivity to the host, and if the port is open + self._diagnose(uri) raise DatabaseTestConnectionFailedError() except SupersetSecurityException as ex: event_logger.log_with_context( @@ -91,7 +144,7 @@ class TestConnectionDatabaseCommand(BaseCommand): action=f"test_connection_error.{ex.__class__.__name__}", engine=database.db_engine_spec.__name__, ) - raise DatabaseTestConnectionUnexpectedError() + raise DatabaseTestConnectionUnexpectedError(str(ex)) def validate(self) -> None: database_name = self._properties.get("database_name") diff --git a/superset/errors.py b/superset/errors.py index 24745e65e..248d75d27 100644 --- a/superset/errors.py +++ b/superset/errors.py @@ -56,6 +56,9 @@ class SupersetErrorType(str, Enum): # Sql Lab errors MISSING_TEMPLATE_PARAMS_ERROR = "MISSING_TEMPLATE_PARAMS_ERROR" + TEST_CONNECTION_INVALID_HOSTNAME_ERROR = "TEST_CONNECTION_INVALID_HOSTNAME_ERROR" + TEST_CONNECTION_PORT_CLOSED_ERROR = "TEST_CONNECTION_PORT_CLOSED_ERROR" + TEST_CONNECTION_HOST_DOWN_ERROR = "TEST_CONNECTION_HOST_DOWN_ERROR" ERROR_TYPES_TO_ISSUE_CODES_MAPPING = { @@ -114,6 +117,24 @@ ERROR_TYPES_TO_ISSUE_CODES_MAPPING = { ), }, ], + SupersetErrorType.TEST_CONNECTION_INVALID_HOSTNAME_ERROR: [ + { + "code": 1007, + "message": _("Issue 1007 - The hostname provided can't be resolved."), + }, + ], + SupersetErrorType.TEST_CONNECTION_PORT_CLOSED_ERROR: [ + {"code": 1008, "message": _("Issue 1008 - The port is closed."),}, + ], + SupersetErrorType.TEST_CONNECTION_HOST_DOWN_ERROR: [ + { + "code": 1009, + "message": _( + "Issue 1009 - The host might be down, and can't be reached on the " + "provided port." + ), + }, + ], } diff --git a/superset/utils/log.py b/superset/utils/log.py index 8482871c8..b48c47c01 100644 --- a/superset/utils/log.py +++ b/superset/utils/log.py @@ -34,6 +34,9 @@ from superset.stats_logger import BaseStatsLogger def collect_request_payload() -> Dict[str, Any]: """Collect log payload identifiable from request context""" + if not request: + return {} + payload: Dict[str, Any] = { "path": request.path, **request.form.to_dict(), @@ -111,7 +114,7 @@ class AbstractEventLogger(ABC): ) -> None: from superset.views.core import get_form_data - referrer = request.referrer[:1000] if request.referrer else None + referrer = request.referrer[:1000] if request and request.referrer else None duration_ms = int(duration.total_seconds() * 1000) if duration else None diff --git a/superset/utils/network.py b/superset/utils/network.py new file mode 100644 index 000000000..4a49c9a9f --- /dev/null +++ b/superset/utils/network.py @@ -0,0 +1,71 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +import platform +import socket +import subprocess + +PORT_TIMEOUT = 5 +PING_TIMEOUT = 5 + + +def is_port_open(host: str, port: int) -> bool: + """ + Test if a given port in a host is open. + """ + # pylint: disable=invalid-name + s = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + s.settimeout(PORT_TIMEOUT) + try: + s.connect((host, int(port))) + s.shutdown(socket.SHUT_RDWR) + return True + except socket.error: + return False + finally: + s.close() + + return False + + +def is_hostname_valid(host: str) -> bool: + """ + Test if a given hostname can be resolved. + """ + try: + socket.gethostbyname(host) + return True + except socket.gaierror: + return False + + return False + + +def is_host_up(host: str) -> bool: + """ + Ping a host to see if it's up. + + Note that if we don't get a response the host might still be up, + since many firewalls block ICMP packets. + """ + param = "-n" if platform.system().lower() == "windows" else "-c" + command = ["ping", param, "1", host] + try: + output = subprocess.call(command, timeout=PING_TIMEOUT) + except subprocess.TimeoutExpired: + return False + + return output == 0 diff --git a/tests/databases/api_tests.py b/tests/databases/api_tests.py index 36d2179ae..897edd0e1 100644 --- a/tests/databases/api_tests.py +++ b/tests/databases/api_tests.py @@ -26,6 +26,7 @@ import prison import pytest import yaml +from sqlalchemy.engine.url import make_url from sqlalchemy.sql import func from superset import db, security_manager @@ -866,6 +867,93 @@ class TestDatabaseApi(SupersetTestCase): app.config["PREVENT_UNSAFE_DB_CONNECTIONS"] = False + @mock.patch("superset.databases.commands.test_connection.is_hostname_valid",) + def test_test_connection_failed_invalid_hostname(self, mock_is_hostname_valid): + """ + Database API: Test test connection failed due to invalid hostname + """ + mock_is_hostname_valid.return_value = False + + self.login("admin") + data = { + "sqlalchemy_uri": "postgres://username:password@invalidhostname:12345/db", + "database_name": "examples", + "impersonate_user": False, + "server_cert": None, + } + url = "api/v1/database/test_connection" + rv = self.post_assert_metric(url, data, "test_connection") + + assert rv.status_code == 400 + assert rv.headers["Content-Type"] == "application/json; charset=utf-8" + response = json.loads(rv.data.decode("utf-8")) + expected_response = { + "message": 'Unable to resolve hostname "invalidhostname".', + } + assert response == expected_response + + @mock.patch("superset.databases.commands.test_connection.is_hostname_valid") + @mock.patch("superset.databases.commands.test_connection.is_port_open") + @mock.patch("superset.databases.commands.test_connection.is_host_up") + def test_test_connection_failed_closed_port( + self, mock_is_host_up, mock_is_port_open, mock_is_hostname_valid + ): + """ + Database API: Test test connection failed due to closed port. + """ + mock_is_hostname_valid.return_value = True + mock_is_port_open.return_value = False + mock_is_host_up.return_value = True + + self.login("admin") + data = { + "sqlalchemy_uri": "postgres://username:password@localhost:12345/db", + "database_name": "examples", + "impersonate_user": False, + "server_cert": None, + } + url = "api/v1/database/test_connection" + rv = self.post_assert_metric(url, data, "test_connection") + + assert rv.status_code == 400 + assert rv.headers["Content-Type"] == "application/json; charset=utf-8" + response = json.loads(rv.data.decode("utf-8")) + expected_response = { + "message": "The host localhost is up, but the port 12345 is closed.", + } + assert response == expected_response + + @mock.patch("superset.databases.commands.test_connection.is_hostname_valid") + @mock.patch("superset.databases.commands.test_connection.is_port_open") + @mock.patch("superset.databases.commands.test_connection.is_host_up") + def test_test_connection_failed_host_down( + self, mock_is_host_up, mock_is_port_open, mock_is_hostname_valid + ): + """ + Database API: Test test connection failed due to host being down. + """ + mock_is_hostname_valid.return_value = True + mock_is_port_open.return_value = False + mock_is_host_up.return_value = False + + self.login("admin") + data = { + "sqlalchemy_uri": "postgres://username:password@localhost:12345/db", + "database_name": "examples", + "impersonate_user": False, + "server_cert": None, + } + url = "api/v1/database/test_connection" + rv = self.post_assert_metric(url, data, "test_connection") + + assert rv.status_code == 400 + assert rv.headers["Content-Type"] == "application/json; charset=utf-8" + response = json.loads(rv.data.decode("utf-8")) + expected_response = { + "message": "The host localhost might be down, ond can't be reached on port 12345.", + } + assert response == expected_response + @pytest.mark.usefixtures( "load_unicode_dashboard_with_position", "load_energy_table_with_slice",