Prevent hostname evaluating to None in sqlserver check (#18237)

* Prevent hostname evaluating to None in sqlserver check

* always reference resolved_hostname property

* add changelog

* fix tests

* reload hostname when engine edition static info expires

---------

Co-authored-by: Zhengda Lu <[email protected]>

Author: jmeunier28

sqlserver/changelog.d/18237.fixed

@@ -0,0 +1 @@
+Prevent hostname evaluating to None in sqlserver check

sqlserver/datadog_checks/sqlserver/connection.py

@@ -152,8 +152,7 @@ class Connection(object):
 
     VALID_ADOPROVIDERS = ['SQLOLEDB', 'MSOLEDBSQL', 'MSOLEDBSQL19', 'SQLNCLI11']
 
-    def __init__(self, host, init_config, instance_config, service_check_handler):
-        self.host = host
+    def __init__(self, init_config, instance_config, service_check_handler):
         self.instance = instance_config
         self.service_check_handler = service_check_handler
         self.log = get_check_logger()

sqlserver/datadog_checks/sqlserver/sqlserver.py

@@ -154,7 +154,7 @@ def __init__(self, name, init_config, instances):
         # go through the agent internal metrics submission processing those tags
         self.non_internal_tags = copy.deepcopy(self.tags)
         self.check_initializations.append(self.initialize_connection)
-        self.check_initializations.append(self.set_resolved_hostname)
+        self.check_initializations.append(self.load_static_information)
         self.check_initializations.append(self.set_resolved_hostname_metadata)
         self.check_initializations.append(self.config_checks)
         self.check_initializations.append(self.make_metric_list_to_collect)
@@ -244,7 +244,6 @@ def set_resource_tags(self):
 
     def set_resolved_hostname(self):
         # load static information cache
-        self.load_static_information()
         if self._resolved_hostname is None:
             if self._config.reported_hostname:
                 self._resolved_hostname = self._config.reported_hostname
@@ -280,6 +279,7 @@ def resolved_hostname(self):
         return self._resolved_hostname
 
     def load_static_information(self):
+        engine_edition_reloaded = False
         expected_keys = {STATIC_INFO_VERSION, STATIC_INFO_MAJOR_VERSION, STATIC_INFO_ENGINE_EDITION, STATIC_INFO_RDS}
         missing_keys = expected_keys - set(self.static_info_cache.keys())
         if missing_keys:
@@ -309,6 +309,7 @@ def load_static_information(self):
                         result = cursor.fetchone()
                         if result:
                             self.static_info_cache[STATIC_INFO_ENGINE_EDITION] = result[0]
+                            engine_edition_reloaded = True
                         else:
                             self.log.warning("failed to load version static information due to empty results")
                     if STATIC_INFO_RDS not in self.static_info_cache:
@@ -318,9 +319,11 @@ def load_static_information(self):
                             self.static_info_cache[STATIC_INFO_RDS] = True
                         else:
                             self.static_info_cache[STATIC_INFO_RDS] = False
-            # re-initialize resolved_hostname to ensure we take into consideration the static information
+            # re-initialize resolved_hostname to ensure we take into consideration the egine edition
             # after it's loaded
-            self._resolved_hostname = None
+            if engine_edition_reloaded:
+                self._resolved_hostname = None
+            self.set_resolved_hostname()
 
     def debug_tags(self):
         return self.tags + ["agent_hostname:{}".format(self.agent_hostname)]
@@ -342,7 +345,6 @@ def agent_hostname(self):
 
     def initialize_connection(self):
         self.connection = Connection(
-            host=self.resolved_hostname,
             init_config=self.init_config,
             instance_config=self.instance,
             service_check_handler=self.handle_service_check,
@@ -709,6 +711,7 @@ def _check_database_conns(self):
 
     def check(self, _):
         if self.do_check:
+            self.load_static_information()
             # configure custom queries for the check
             if self._query_manager is None:
                 # use QueryManager to process custom queries

sqlserver/tests/test_connection.py

@@ -74,8 +74,7 @@ def test_warn_trusted_connection_username_pass(instance_minimal_defaults, cs, us
     instance_minimal_defaults["connection_string"] = cs
     instance_minimal_defaults["username"] = username
     instance_minimal_defaults["password"] = password
-    check = SQLServer(CHECK_NAME, {}, [instance_minimal_defaults])
-    connection = Connection(check.resolved_hostname, {}, instance_minimal_defaults, None)
+    connection = Connection({}, instance_minimal_defaults, None)
     connection.log = mock.MagicMock()
     connection._connection_options_validation('somekey', 'somedb')
     if expect_warning:
@@ -97,8 +96,7 @@ def test_warn_trusted_connection_username_pass(instance_minimal_defaults, cs, us
 )
 def test_will_warn_parameters_for_the_wrong_connection(instance_minimal_defaults, connector, param):
     instance_minimal_defaults.update({'connector': connector, param: 'foo'})
-    check = SQLServer(CHECK_NAME, {}, [instance_minimal_defaults])
-    connection = Connection(check.resolved_hostname, {}, instance_minimal_defaults, None)
+    connection = Connection({}, instance_minimal_defaults, None)
     connection.log = mock.MagicMock()
     connection._connection_options_validation('somekey', 'somedb')
     connection.log.warning.assert_called_once_with(
@@ -130,8 +128,7 @@ def test_will_warn_parameters_for_the_wrong_connection(instance_minimal_defaults
 )
 def test_will_fail_for_duplicate_parameters(instance_minimal_defaults, connector, cs, param, should_fail):
     instance_minimal_defaults.update({'connector': connector, param: 'foo', 'connection_string': cs + "=foo"})
-    check = SQLServer(CHECK_NAME, {}, [instance_minimal_defaults])
-    connection = Connection(check.resolved_hostname, {}, instance_minimal_defaults, None)
+    connection = Connection({}, instance_minimal_defaults, None)
     if should_fail:
         match = (
             "%s has been provided both in the connection string and as a configuration option (%s), "
@@ -162,8 +159,7 @@ def test_will_fail_for_duplicate_parameters(instance_minimal_defaults, connector
 def test_will_fail_for_wrong_parameters_in_the_connection_string(instance_minimal_defaults, connector, cs):
     instance_minimal_defaults.update({'connector': connector, 'connection_string': cs + '=foo'})
     other_connector = 'odbc' if connector != 'odbc' else 'adodbapi'
-    check = SQLServer(CHECK_NAME, {}, [instance_minimal_defaults])
-    connection = Connection(check.resolved_hostname, {}, instance_minimal_defaults, None)
+    connection = Connection({}, instance_minimal_defaults, None)
     match = (
         "%s has been provided in the connection string. "
         "This option is only available for %s connections, however %s has been selected"
@@ -226,8 +222,7 @@ def test_managed_auth_config_valid(instance_minimal_defaults, name, managed_iden
         for k, v in managed_identity_config.items():
             instance_minimal_defaults[k] = v
     instance_minimal_defaults.update({'connector': 'odbc'})
-    check = SQLServer(CHECK_NAME, {}, [instance_minimal_defaults])
-    connection = Connection(check.resolved_hostname, {}, instance_minimal_defaults, None)
+    connection = Connection({}, instance_minimal_defaults, None)
     if should_fail:
         with pytest.raises(ConfigurationError, match=re.escape(expected_err)):
             connection._connection_options_validation('somekey', 'somedb')
@@ -287,8 +282,7 @@ def test_managed_auth_config_valid(instance_minimal_defaults, name, managed_iden
 def test_config_with_and_without_port(instance_minimal_defaults, host, port, expected_host):
     instance_minimal_defaults["host"] = host
     instance_minimal_defaults["port"] = port
-    check = SQLServer(CHECK_NAME, {}, [instance_minimal_defaults])
-    connection = Connection(check.resolved_hostname, {}, instance_minimal_defaults, None)
+    connection = Connection({}, instance_minimal_defaults, None)
     _, result_host, _, _, _, _ = connection._get_access_info('somekey', 'somedb')
     assert result_host == expected_host
 
@@ -369,9 +363,7 @@ def test_connection_failure(aggregator, dd_run_check, instance_docker):
 
     try:
         # Break the connection
-        check.connection = Connection(
-            check.resolved_hostname, {}, {'host': '', 'username': '', 'password': ''}, check.handle_service_check
-        )
+        check.connection = Connection({}, {'host': '', 'username': '', 'password': ''}, check.handle_service_check)
         dd_run_check(check)
     except Exception:
         aggregator.assert_service_check(
@@ -495,7 +487,7 @@ def test_connection_error_reporting(
     expected_error_pattern = matching_patterns[0]
 
     check = SQLServer(CHECK_NAME, {}, [instance_docker])
-    connection = Connection(check.resolved_hostname, check.init_config, check.instance, check.handle_service_check)
+    connection = Connection(check.init_config, check.instance, check.handle_service_check)
     with pytest.raises(SQLConnectionError) as excinfo:
         with connection.open_managed_default_connection():
             pytest.fail("connection should not have succeeded")

sqlserver/tests/test_integration.py

@@ -873,3 +873,27 @@ def test_propagate_agent_tags(
                 status=SQLServer.OK,
                 tags=expected_tags,
             )
+
+
+@pytest.mark.integration
+@pytest.mark.usefixtures('dd_environment')
+def test_check_static_information_expire(aggregator, dd_run_check, init_config, instance_docker):
+    sqlserver_check = SQLServer(CHECK_NAME, init_config, [instance_docker])
+    dd_run_check(sqlserver_check)
+    assert sqlserver_check.static_info_cache is not None
+    assert len(sqlserver_check.static_info_cache.keys()) == 4
+    assert sqlserver_check.resolved_hostname == 'stubbed.hostname'
+
+    # manually clear static information cache
+    sqlserver_check.static_info_cache.clear()
+    dd_run_check(sqlserver_check)
+    assert sqlserver_check.static_info_cache is not None
+    assert len(sqlserver_check.static_info_cache.keys()) == 4
+    assert sqlserver_check.resolved_hostname == 'stubbed.hostname'
+
+    # manually pop STATIC_INFO_ENGINE_EDITION to make sure it is reloaded
+    sqlserver_check.static_info_cache.pop(STATIC_INFO_ENGINE_EDITION)
+    dd_run_check(sqlserver_check)
+    assert sqlserver_check.static_info_cache is not None
+    assert len(sqlserver_check.static_info_cache.keys()) == 4
+    assert sqlserver_check.resolved_hostname == 'stubbed.hostname'