[pdh] Make the admin share configurable (#5485)

* Add the option to provide a different administrative share * Add admin_share config option * Bump check base dependency * Add tests for new config option * Fix typo * call initialize_pdh_tests * Allow for empty admin share * Inline rstrip * Update conf.yaml.example * Apply suggestions from code review Co-Authored-By: FlorianVeaux <[email protected]> * Update conf.yaml.example * Update pdh_check/setup.py Co-Authored-By: Florimond Manca <[email protected]> * Remove pin on unreleased base version Co-authored-by: FlorianVeaux <[email protected]> Co-authored-by: Florimond Manca <[email protected]>
DataDog · Jan 21, 2020 · 1abcf9a · 1abcf9a
1 parent 73eda08
commit 1abcf9a
Show file tree

Hide file tree

Showing 3 changed files with 89 additions and 14 deletions.
diff --git a/datadog_checks_base/datadog_checks/base/checks/win/winpdh_base.py b/datadog_checks_base/datadog_checks/base/checks/win/winpdh_base.py
@@ -13,6 +13,9 @@
     from .winpdh_stub import WinPDHCounter, DATA_TYPE_INT, DATA_TYPE_DOUBLE
 
 
+RESOURCETYPE_ANY = 0
+DEFAULT_SHARE = 'c$'
+
 int_types = ["int", "long", "uint"]
 
 double_types = ["double", "float"]
@@ -52,10 +55,7 @@ def __init__(self, name, init_config, agentConfig, instances, counter_list):
 
                         username = instance.get('username')
                         password = instance.get('password')
-                        nr = win32wnet.NETRESOURCE()
-                        nr.lpRemoteName = r"\\%s\c$" % remote_machine
-                        nr.dwType = 0
-                        nr.lpLocalName = None
+                        nr = self._get_netresource(remote_machine)
                         win32wnet.WNetAddConnection2(nr, password, username, 0)
 
                     except Exception as e:
@@ -105,6 +105,42 @@ def __init__(self, name, init_config, agentConfig, instances, counter_list):
         if key is None or not self._metrics.get(key):
             raise AttributeError('No valid counters to collect')
 
+    def _get_netresource(self, remote_machine):
+        # To connect you have to use the name of the server followed by an optional administrative share.
+        # Administrative shares are hidden network shares created that allow system administrators to have remote access
+        # to every disk volume on a network-connected system.
+        # These shares may not be permanently deleted but may be disabled.
+        # Administrative shares cannot be accessed by users without administrative privileges.
+        #
+        # This page explains how to enable them: https://www.wintips.org/how-to-enable-admin-shares-windows-7/
+        #
+        # The administrative share can be:
+        # * A disk volume like c$
+        # * admin$: The folder in which Windows is installed
+        # * fax$: The folder in which faxed pages and cover pages are cached
+        # * ipc$: Area used for interprocess communication and is not part of the file system.
+        # * print$: Virtual folder that contains a representation of the installed printers
+        # * Domain controller shares: Windows creates two domain controller specific shares called sysvol and netlogon
+        #   which do not have $ appended to their names.
+        # * Empty string: No admin share specified
+        administrative_share = self.instance.get('admin_share', DEFAULT_SHARE)
+
+        nr = win32wnet.NETRESOURCE()
+
+        # Specifies the network resource to connect to.
+        nr.lpRemoteName = r"\\{}\{}".format(remote_machine, administrative_share).rstrip('\\')
+
+        # The type of network resource to connect to.
+        #
+        # Although this member is required, its information may be ignored by the network service provider.
+        nr.dwType = RESOURCETYPE_ANY
+
+        # Specifies the name of a local device to redirect, such as "F:" or "LPT1".
+        # If the string is empty, NULL, it connects to the network resource without redirecting a local device.
+        nr.lpLocalName = None
+
+        return nr
+
     def check(self, instance):
         self.log.debug("PDHBaseCheck: check()")
         key = hash_mutable(instance)

diff --git a/datadog_checks_base/tests/test_pdhbasecheck.py b/datadog_checks_base/tests/test_pdhbasecheck.py
@@ -2,6 +2,8 @@
 # All rights reserved
 # Licensed under a 3-clause BSD style license (see LICENSE)
 
+import copy
+
 import pytest
 
 from .utils import requires_windows
@@ -26,6 +28,16 @@
     ["Processor", "1", "% Processor Time", "test.processor_time_1", "gauge"],
 ]
 
+PARTIAL_COUNTER_LIST = [
+    ["NTDS", None, "LDAP Client Sessions", "active_directory.ldap.client_sessions", "gauge"],
+    ["NTDS", None, "LDAP Bind Time", "active_directory.ldap.bind_time", "gauge"],
+    ["NTDS", None, "LDAP Successful Binds/sec", "active_directory.ldap.successful_binds_persec", "gauge"],
+    ["NTDS", None, "LDAP Searches/sec", "active_directory.ldap.searches_persec", "gauge"],
+    # these two don't exist
+    ["NTDS", None, "Kerberos Authentications/sec", "active_directory.kerberos.auths_persec", "gauge"],
+    ["NTDS", None, "NTLM Authentications/sec", "active_directory.ntlm.auths_persec", "gauge"],
+]
+
 
 @requires_windows
 def test_single_instance_counter(aggregator, pdh_mocks_fixture):  # noqa F811
@@ -69,22 +81,41 @@ def test_multi_instance_counter_specific_instances(aggregator, pdh_mocks_fixture
 
 @requires_windows
 def test_returns_partial_metrics(aggregator, pdh_mocks_fixture):  # noqa F811
-    COUNTER_LIST = [
-        ["NTDS", None, "LDAP Client Sessions", "active_directory.ldap.client_sessions", "gauge"],
-        ["NTDS", None, "LDAP Bind Time", "active_directory.ldap.bind_time", "gauge"],
-        ["NTDS", None, "LDAP Successful Binds/sec", "active_directory.ldap.successful_binds_persec", "gauge"],
-        ["NTDS", None, "LDAP Searches/sec", "active_directory.ldap.searches_persec", "gauge"],
-        # these two don't exist
-        ["NTDS", None, "Kerberos Authentications/sec", "active_directory.kerberos.auths_persec", "gauge"],
-        ["NTDS", None, "NTLM Authentications/sec", "active_directory.ntlm.auths_persec", "gauge"],
-    ]
     initialize_pdh_tests()
     instance = DEFAULT_INSTANCE
-    c = PDHBaseCheck("testcheck", {}, {}, [instance], COUNTER_LIST)
+    c = PDHBaseCheck("testcheck", {}, {}, [instance], PARTIAL_COUNTER_LIST)
     c.check(instance)
 
     aggregator.assert_metric("active_directory.ldap.client_sessions", tags=None, count=1)
     aggregator.assert_metric("active_directory.ldap.bind_time", tags=None, count=1)
     aggregator.assert_metric("active_directory.ldap.successful_binds_persec", tags=None, count=1)
     aggregator.assert_metric("active_directory.ldap.searches_persec", tags=None, count=1)
     assert aggregator.metrics_asserted_pct == 100.0
+
+
+@requires_windows
+def test_default_admin_share():
+    initialize_pdh_tests()
+    c = PDHBaseCheck("testcheck", {}, {}, [DEFAULT_INSTANCE], SINGLE_INSTANCE_COUNTER)
+    nr = c._get_netresource('1.1.1.1')
+    assert nr.lpRemoteName == '\\\\1.1.1.1\\c$'
+
+
+@requires_windows
+def test_custom_admin_share():
+    initialize_pdh_tests()
+    instance = copy.deepcopy(DEFAULT_INSTANCE)
+    instance['admin_share'] = 'ipc$'
+    c = PDHBaseCheck("testcheck", {}, {}, [instance], SINGLE_INSTANCE_COUNTER)
+    nr = c._get_netresource('1.2.3.4')
+    assert nr.lpRemoteName == '\\\\1.2.3.4\\ipc$'
+
+
+@requires_windows
+def test_no_admin_share():
+    initialize_pdh_tests()
+    instance = copy.deepcopy(DEFAULT_INSTANCE)
+    instance['admin_share'] = ''
+    c = PDHBaseCheck("testcheck", {}, {}, [instance], SINGLE_INSTANCE_COUNTER)
+    nr = c._get_netresource('1.2.3.4')
+    assert nr.lpRemoteName == '\\\\1.2.3.4'
diff --git a/pdh_check/datadog_checks/pdh_check/data/conf.yaml.example b/pdh_check/datadog_checks/pdh_check/data/conf.yaml.example
@@ -18,6 +18,14 @@ instances:
     #
     # password: <PASSWORD>
 
+    ## @param admin_share - string - optional - default: c$
+    ## The administrative share to connect to. Can be a drive or the `ipc$` share if available.
+    ## Note that to be able to connect to remote hosts the administrative share needs to be enabled and the
+    ## user needs network administrator permissions
+    ## If the remote machine doesn't expose any, set this to the empty string `""` to connect without an admin share. 
+    #
+    # admin_share: c$
+
     ## The following example fetches the number of processes and users:
     ##
     ## - countersetname: Processor