feat: better error message when adding DBs (apache#13601)

* WIP

* Adding tests

* Add unit tests

* Show error message

* Fix lint

* Fix after rebase

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.

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> = T[keyof T];

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();
 }

superset/databases/api.py

@@ -64,6 +64,7 @@
     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 @@ def test_connection(  # pylint: disable=too-many-return-statements
             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("/<int:pk>/related_objects/", methods=["GET"])
     @protect()

superset/databases/commands/exceptions.py

@@ -25,6 +25,7 @@
     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

superset/databases/commands/test_connection.py

@@ -20,19 +20,23 @@
 
 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
 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 @@ def __init__(self, user: User, data: Dict[str, Any]):
         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 @@ def run(self) -> None:
                 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 @@ def run(self) -> None:
                 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")

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 @@ class SupersetErrorType(str, Enum):
             ),
         },
     ],
+    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."
+            ),
+        },
+    ],
 }
 
 

superset/utils/log.py

@@ -34,6 +34,9 @@
 
 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 @@ def log_with_context(  # pylint: disable=too-many-locals
     ) -> 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
 

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