From 8bdbd5578deb912f38e5bab1ab751de1329eba32 Mon Sep 17 00:00:00 2001 From: Andrew Zhang Date: Tue, 22 Dec 2020 15:09:49 -0500 Subject: [PATCH 01/15] Separate proxysql tests --- proxysql/datadog_checks/proxysql/proxysql.py | 4 +- proxysql/tests/conftest.py | 31 ++++++++ proxysql/tests/test_e2e.py | 13 ++++ .../{test_proxysql.py => test_integration.py} | 75 +------------------ proxysql/tests/test_unit.py | 47 ++++++++++++ 5 files changed, 95 insertions(+), 75 deletions(-) create mode 100644 proxysql/tests/test_e2e.py rename proxysql/tests/{test_proxysql.py => test_integration.py} (59%) create mode 100644 proxysql/tests/test_unit.py diff --git a/proxysql/datadog_checks/proxysql/proxysql.py b/proxysql/datadog_checks/proxysql/proxysql.py index 89bdfafa97d8d..f670c0ca167aa 100644 --- a/proxysql/datadog_checks/proxysql/proxysql.py +++ b/proxysql/datadog_checks/proxysql/proxysql.py @@ -44,9 +44,11 @@ def __init__(self, name, init_config, instances): if not all((self.host, self.port, self.user, self.password)): raise ConfigurationError("ProxySQL host, port, username and password are needed") + # can get rid of this self.tls_verify = self.instance.get("tls_verify", False) self.validate_hostname = self.instance.get("validate_hostname", True) self.tls_ca_cert = self.instance.get("tls_ca_cert") + self.connect_timeout = self.instance.get("connect_timeout", 10) self.read_timeout = self.instance.get("read_timeout") @@ -96,7 +98,7 @@ def connect(self): ca_cert=self.tls_ca_cert, check_hostname=self.validate_hostname ) else: - ssl_context = make_insecure_ssl_client_context() + ssl_context = make_insecure_ssl_client_context() # can keep this db = None try: diff --git a/proxysql/tests/conftest.py b/proxysql/tests/conftest.py index d3a64389d1a12..093c9e1b44183 100644 --- a/proxysql/tests/conftest.py +++ b/proxysql/tests/conftest.py @@ -4,12 +4,17 @@ import os from copy import deepcopy +import mock import pymysql import pytest from datadog_checks.dev import TempDir, docker_run, get_docker_hostname, get_here from datadog_checks.dev.conditions import CheckDockerLogs, WaitFor +from datadog_checks.proxysql import ProxysqlCheck + +from .common import ALL_METRICS + DOCKER_HOST = get_docker_hostname() MYSQL_PORT = 6612 PROXY_PORT = 6033 @@ -120,3 +125,29 @@ def init_mysql(): def init_proxy(): pymysql.connect(host=DOCKER_HOST, port=PROXY_PORT, user=MYSQL_USER, passwd=MYSQL_PASS) + + +def get_check(instance): + """Simple helper method to get a check instance from a config instance.""" + return ProxysqlCheck('proxysql', {}, [instance]) + + +def _assert_all_metrics(aggregator): + for metric in ALL_METRICS: + aggregator.assert_metric(metric) + + aggregator.assert_all_metrics_covered() + + +def _assert_metadata(datadog_agent, check_id=''): + raw_version = PROXYSQL_VERSION + major, minor = raw_version.split('.')[:2] + version_metadata = { + 'version.scheme': 'semver', + 'version.major': major, + 'version.minor': minor, + 'version.patch': mock.ANY, + 'version.raw': mock.ANY, + } + datadog_agent.assert_metadata(check_id, version_metadata) + diff --git a/proxysql/tests/test_e2e.py b/proxysql/tests/test_e2e.py new file mode 100644 index 0000000000000..50a5f5ad9f423 --- /dev/null +++ b/proxysql/tests/test_e2e.py @@ -0,0 +1,13 @@ +# (C) Datadog, Inc. 2020-present +# All rights reserved +# Licensed under a 3-clause BSD style license (see LICENSE) +import pytest + +from .conftest import _assert_all_metrics, _assert_metadata + + +@pytest.mark.e2e +def test_e2e(dd_agent_check, datadog_agent): + aggregator = dd_agent_check(rate=True) + _assert_metadata(datadog_agent, check_id='proxysql') + _assert_all_metrics(aggregator) diff --git a/proxysql/tests/test_proxysql.py b/proxysql/tests/test_integration.py similarity index 59% rename from proxysql/tests/test_proxysql.py rename to proxysql/tests/test_integration.py index ae8a47fcf972f..c475ae7ed6024 100644 --- a/proxysql/tests/test_proxysql.py +++ b/proxysql/tests/test_integration.py @@ -1,15 +1,11 @@ # (C) Datadog, Inc. 2020-present # All rights reserved # Licensed under a 3-clause BSD style license (see LICENSE) -import mock import pytest from datadog_checks.base import AgentCheck -from datadog_checks.base.errors import ConfigurationError -from datadog_checks.proxysql import ProxysqlCheck from .common import ( - ALL_METRICS, COMMANDS_COUNTERS_METRICS, CONNECTION_POOL_METRICS, GLOBAL_METRICS, @@ -17,49 +13,7 @@ QUERY_RULES_TAGS_METRICS, USER_TAGS_METRICS, ) -from .conftest import PROXYSQL_VERSION - - -def get_check(instance): - """Simple helper method to get a check instance from a config instance.""" - return ProxysqlCheck('proxysql', {}, [instance]) - - -@pytest.mark.unit -def test_wrong_config(dd_run_check, instance_basic): - # Empty instance - with pytest.raises(ConfigurationError, match='ProxySQL host, port, username and password are needed'): - dd_run_check(get_check({})) - - # Only host - with pytest.raises(ConfigurationError, match='ProxySQL host, port, username and password are needed'): - dd_run_check(get_check({'host': 'localhost'})) - - # Missing password - with pytest.raises(ConfigurationError, match='ProxySQL host, port, username and password are needed'): - dd_run_check(get_check({'host': 'localhost', 'port': 6032, 'username': 'admin'})) - - # Wrong additional metrics group - with pytest.raises( - ConfigurationError, - match="There is no additional metric group called 'foo' for the ProxySQL integration, it should be one of ", - ): - instance_basic['additional_metrics'].append('foo') - dd_run_check(get_check(instance_basic)) - - -@pytest.mark.unit -def test_config_ok(dd_run_check): - check = get_check({'host': 'localhost', 'port': 6032, 'username': 'admin', 'password': 'admin'}) - connect_mock, query_executor_mock = mock.MagicMock(), mock.MagicMock() - - check.connect = connect_mock - check._query_manager.executor = query_executor_mock - - dd_run_check(check) - - connect_mock.assert_called_once() - assert query_executor_mock.call_count == 2 +from .conftest import get_check, _assert_all_metrics, _assert_metadata @pytest.mark.integration @@ -145,26 +99,6 @@ def test_metadata(datadog_agent, dd_run_check, instance_basic): _assert_metadata(datadog_agent) -def _assert_all_metrics(aggregator): - for metric in ALL_METRICS: - aggregator.assert_metric(metric) - - aggregator.assert_all_metrics_covered() - - -def _assert_metadata(datadog_agent, check_id=''): - raw_version = PROXYSQL_VERSION - major, minor = raw_version.split('.')[:2] - version_metadata = { - 'version.scheme': 'semver', - 'version.major': major, - 'version.minor': minor, - 'version.patch': mock.ANY, - 'version.raw': mock.ANY, - } - datadog_agent.assert_metadata(check_id, version_metadata) - - @pytest.mark.integration @pytest.mark.usefixtures('dd_environment') def test_all_metrics(aggregator, instance_all_metrics, dd_run_check): @@ -179,10 +113,3 @@ def test_all_metrics_stats_user(aggregator, instance_stats_user, dd_run_check): check = get_check(instance_stats_user) dd_run_check(check) _assert_all_metrics(aggregator) - - -@pytest.mark.e2e -def test_e2e(dd_agent_check, datadog_agent): - aggregator = dd_agent_check(rate=True) - _assert_metadata(datadog_agent, check_id='proxysql') - _assert_all_metrics(aggregator) diff --git a/proxysql/tests/test_unit.py b/proxysql/tests/test_unit.py new file mode 100644 index 0000000000000..0f4bd3886276f --- /dev/null +++ b/proxysql/tests/test_unit.py @@ -0,0 +1,47 @@ +# (C) Datadog, Inc. 2020-present +# All rights reserved +# Licensed under a 3-clause BSD style license (see LICENSE) + +import mock +import pytest + +from datadog_checks.base.errors import ConfigurationError + +from .conftest import get_check + + +@pytest.mark.unit +def test_wrong_config(dd_run_check, instance_basic): + # Empty instance + with pytest.raises(ConfigurationError, match='ProxySQL host, port, username and password are needed'): + dd_run_check(get_check({})) + + # Only host + with pytest.raises(ConfigurationError, match='ProxySQL host, port, username and password are needed'): + dd_run_check(get_check({'host': 'localhost'})) + + # Missing password + with pytest.raises(ConfigurationError, match='ProxySQL host, port, username and password are needed'): + dd_run_check(get_check({'host': 'localhost', 'port': 6032, 'username': 'admin'})) + + # Wrong additional metrics group + with pytest.raises( + ConfigurationError, + match="There is no additional metric group called 'foo' for the ProxySQL integration, it should be one of ", + ): + instance_basic['additional_metrics'].append('foo') + dd_run_check(get_check(instance_basic)) + + +@pytest.mark.unit +def test_config_ok(dd_run_check): + check = get_check({'host': 'localhost', 'port': 6032, 'username': 'admin', 'password': 'admin'}) + connect_mock, query_executor_mock = mock.MagicMock(), mock.MagicMock() + + check.connect = connect_mock + check._query_manager.executor = query_executor_mock + + dd_run_check(check) + + connect_mock.assert_called_once() + assert query_executor_mock.call_count == 2 \ No newline at end of file From ba21f36982064f687e8e1457df15a95e4bb37733 Mon Sep 17 00:00:00 2001 From: Andrew Zhang Date: Tue, 22 Dec 2020 15:38:41 -0500 Subject: [PATCH 02/15] Update ProxySQL to use SSL wrapper instead of custom SSL implementation --- proxysql/assets/configuration/spec.yaml | 20 +++---- .../proxysql/data/conf.yaml.example | 47 +++++++++++++--- proxysql/datadog_checks/proxysql/proxysql.py | 17 +++--- proxysql/datadog_checks/proxysql/ssl_utils.py | 53 ------------------- proxysql/tests/conftest.py | 4 +- proxysql/tests/test_integration.py | 24 ++++----- proxysql/tests/test_unit.py | 2 +- 7 files changed, 67 insertions(+), 100 deletions(-) diff --git a/proxysql/assets/configuration/spec.yaml b/proxysql/assets/configuration/spec.yaml index 6ba0724713f1c..f4719876f6714 100644 --- a/proxysql/assets/configuration/spec.yaml +++ b/proxysql/assets/configuration/spec.yaml @@ -34,23 +34,15 @@ files: value: type: string example: - - name: tls_verify - value: - example: false - type: boolean - description: Instructs the check to use SSL when connecting to ProxySQL - - name: tls_ca_cert - value: - example: - type: string + - name: use_tls + required: false description: | - The path to the `proxysql-ca.pem` file generated by ProxySQL when using SSL (or your own). - More information: https://github.com/sysown/proxysql/wiki/SSL-Support#ssl-configuration-for-frontends - - name: validate_hostname + Instructs the check to use SSL when connecting to ProxySQL value: - example: true type: boolean - description: Whether or not to verify the certificate was issued for the given `host` + example: false + default: false + - template: instances/tls - name: connect_timeout required: false description: | diff --git a/proxysql/datadog_checks/proxysql/data/conf.yaml.example b/proxysql/datadog_checks/proxysql/data/conf.yaml.example index 549955b8606c3..2c199583ed977 100644 --- a/proxysql/datadog_checks/proxysql/data/conf.yaml.example +++ b/proxysql/datadog_checks/proxysql/data/conf.yaml.example @@ -33,21 +33,54 @@ instances: # password: - ## @param tls_verify - boolean - optional - default: false + ## @param use_tls - boolean - optional - default: false ## Instructs the check to use SSL when connecting to ProxySQL # - # tls_verify: false + # use_tls: false + + ## @param tls_verify - boolean - optional - default: true + ## Instructs the check to validate the TLS certificate(s) of the service(s). + # + # tls_verify: true ## @param tls_ca_cert - string - optional - ## The path to the `proxysql-ca.pem` file generated by ProxySQL when using SSL (or your own). - ## More information: https://github.com/sysown/proxysql/wiki/SSL-Support#ssl-configuration-for-frontends + ## The path to a file of concatenated CA certificates in PEM format or a directory + ## containing several CA certificates in PEM format. If a directory, the directory + ## must have been processed using the c_rehash utility supplied with OpenSSL. See: + ## https://www.openssl.org/docs/manmaster/man3/SSL_CTX_load_verify_locations.html + ## + ## Setting this implicitly sets `tls_verify` to true. # # tls_ca_cert: - ## @param validate_hostname - boolean - optional - default: true - ## Whether or not to verify the certificate was issued for the given `host` + ## @param tls_cert - string - optional + ## The path to a single file in PEM format containing a certificate as well as any + ## number of CA certificates needed to establish the certificate's authenticity for + ## use when connecting to services. It may also contain an unencrypted private key to use. + ## + ## Setting this implicitly sets `tls_verify` to true. + # + # tls_cert: + + ## @param tls_private_key - string - optional + ## The unencrypted private key to use for `tls_cert` when connecting to services. This is + ## required if `tls_cert` is set and it does not already contain a private key. + ## + ## Setting this implicitly sets `tls_verify` to true. + # + # tls_private_key: + + ## @param tls_private_key_password - string - optional + ## Optional password to decrypt tls_private_key. + ## + ## Setting this implicitly sets `tls_verify` to true. + # + # tls_private_key_password: + + ## @param tls_validate_hostname - boolean - optional - default: true + ## Verifies that the server's cert hostname matches the one requested. # - # validate_hostname: true + # tls_validate_hostname: true ## @param connect_timeout - integer - optional - default: 10 ## Timeout in seconds for connecting to ProxySQL. diff --git a/proxysql/datadog_checks/proxysql/proxysql.py b/proxysql/datadog_checks/proxysql/proxysql.py index f670c0ca167aa..c4b148b6454b2 100644 --- a/proxysql/datadog_checks/proxysql/proxysql.py +++ b/proxysql/datadog_checks/proxysql/proxysql.py @@ -18,7 +18,7 @@ STATS_MYSQL_USERS, VERSION_METADATA, ) -from .ssl_utils import make_insecure_ssl_client_context, make_secure_ssl_client_context +from .ssl_utils import make_insecure_ssl_client_context ADDITIONAL_METRICS_MAPPING = { 'command_counters_metrics': STATS_COMMAND_COUNTERS, @@ -44,10 +44,7 @@ def __init__(self, name, init_config, instances): if not all((self.host, self.port, self.user, self.password)): raise ConfigurationError("ProxySQL host, port, username and password are needed") - # can get rid of this - self.tls_verify = self.instance.get("tls_verify", False) - self.validate_hostname = self.instance.get("validate_hostname", True) - self.tls_ca_cert = self.instance.get("tls_ca_cert") + self.use_tls = self.instance.get("use_tls", False) self.connect_timeout = self.instance.get("connect_timeout", 10) self.read_timeout = self.instance.get("read_timeout") @@ -92,13 +89,13 @@ def execute_query_raw(self, query): @contextmanager def connect(self): - if self.tls_verify: + if self.use_tls: + self.log.debug("Connecting to ProxySQL via SSL/TLS") # If ca_cert is None, will load the default certificates - ssl_context = make_secure_ssl_client_context( - ca_cert=self.tls_ca_cert, check_hostname=self.validate_hostname - ) + ssl_context = self.get_tls_context() else: - ssl_context = make_insecure_ssl_client_context() # can keep this + self.log.debug("Connecting to ProxySQL without SSL/TLS") + ssl_context = make_insecure_ssl_client_context() # can keep this db = None try: diff --git a/proxysql/datadog_checks/proxysql/ssl_utils.py b/proxysql/datadog_checks/proxysql/ssl_utils.py index 1643b564fa9cf..0e585963ebc85 100644 --- a/proxysql/datadog_checks/proxysql/ssl_utils.py +++ b/proxysql/datadog_checks/proxysql/ssl_utils.py @@ -1,11 +1,8 @@ # (C) Datadog, Inc. 2020-present # All rights reserved # Licensed under a 3-clause BSD style license (see LICENSE) -import os import ssl -from datadog_checks.base import ConfigurationError - def make_insecure_ssl_client_context(): """Creates an insecure ssl context for integration that requires to use TLS without checking @@ -16,53 +13,3 @@ def make_insecure_ssl_client_context(): context = ssl.SSLContext(protocol=ssl.PROTOCOL_TLS) context.verify_mode = ssl.CERT_NONE return context - - -def make_secure_ssl_client_context( - ca_cert=None, client_cert=None, client_key=None, check_hostname=True, protocol=ssl.PROTOCOL_TLS -): - """Creates a secure ssl context for integration that requires one. - :param str ca_cert: Path to a file of concatenated CA certificates in PEM format or to a directory containing - several CA certificates in PEM format - :param str client_cert: Path to a single file in PEM format containing the certificate as well as any number of - CA certificates needed to establish the certificate's authenticity. - :param str client_key: Must point to a file containing the private key. Otherwise the private key will be taken - from certfile as well. - :param bool check_hostname: Whether to match the peer cert's hostname - :param int protocol: Client side protocol (should be one of the `ssl.PROTOCOL_*` constants) - By default selects the highest protocol version possible. - - :rtype ssl.Context - """ - # https://docs.python.org/3/library/ssl.html#ssl.SSLContext - # https://docs.python.org/3/library/ssl.html#ssl.PROTOCOL_TLS - context = ssl.SSLContext(protocol=protocol) - - # https://docs.python.org/3/library/ssl.html#ssl.SSLContext.verify_mode - context.verify_mode = ssl.CERT_REQUIRED - - # https://docs.python.org/3/library/ssl.html#ssl.SSLContext.check_hostname - context.check_hostname = check_hostname - - ca_file, ca_path = None, None - if os.path.isdir(ca_cert): - ca_path = ca_cert - elif os.path.isfile(ca_cert): - ca_file = ca_cert - else: - raise ConfigurationError("Specified tls_ca_cert: {} should be a valid file or directory.".format(ca_cert)) - - # https://docs.python.org/3/library/ssl.html#ssl.SSLContext.load_verify_locations - if ca_file or ca_path: - context.load_verify_locations(ca_file, ca_path, None) - - # https://docs.python.org/3/library/ssl.html#ssl.SSLContext.load_default_certs - else: - context.load_default_certs(ssl.Purpose.SERVER_AUTH) - - # https://docs.python.org/3/library/ssl.html#ssl.SSLContext.load_cert_chain - if client_cert: - # If client_key is not defined, load_cert_chain reads the key from the client_cert - context.load_cert_chain(client_cert, keyfile=client_key) - - return context diff --git a/proxysql/tests/conftest.py b/proxysql/tests/conftest.py index 093c9e1b44183..8a05609802d9a 100644 --- a/proxysql/tests/conftest.py +++ b/proxysql/tests/conftest.py @@ -10,7 +10,6 @@ from datadog_checks.dev import TempDir, docker_run, get_docker_hostname, get_here from datadog_checks.dev.conditions import CheckDockerLogs, WaitFor - from datadog_checks.proxysql import ProxysqlCheck from .common import ALL_METRICS @@ -113,7 +112,7 @@ def dd_environment(): cert_dest = "/etc/ssl/certs/proxysql-ca.pem" if PROXYSQL_VERSION.startswith('2'): # SSL is only available with version 2.x of ProxySQL - instance['tls_verify'] = True + instance['use_tls'] = True instance['tls_ca_cert'] = cert_dest instance['validate_hostname'] = False yield instance, {'docker_volumes': ['{}:{}'.format(cert_src, cert_dest)]} @@ -150,4 +149,3 @@ def _assert_metadata(datadog_agent, check_id=''): 'version.raw': mock.ANY, } datadog_agent.assert_metadata(check_id, version_metadata) - diff --git a/proxysql/tests/test_integration.py b/proxysql/tests/test_integration.py index c475ae7ed6024..7782b8302f206 100644 --- a/proxysql/tests/test_integration.py +++ b/proxysql/tests/test_integration.py @@ -13,7 +13,7 @@ QUERY_RULES_TAGS_METRICS, USER_TAGS_METRICS, ) -from .conftest import get_check, _assert_all_metrics, _assert_metadata +from .conftest import _assert_all_metrics, _assert_metadata, get_check @pytest.mark.integration @@ -43,17 +43,17 @@ def test_service_checks_ok(aggregator, instance_basic, dd_run_check): ) -@pytest.mark.integration -@pytest.mark.usefixtures('dd_environment') -def test_server_down(aggregator, instance_basic, dd_run_check): - instance_basic['port'] = 111 - check = get_check(instance_basic) - - with pytest.raises(Exception, match="OperationalError.*Can't connect to MySQL server on"): - dd_run_check(check) - - aggregator.assert_service_check('proxysql.can_connect', AgentCheck.CRITICAL) - aggregator.assert_all_metrics_covered() +# @pytest.mark.integration +# @pytest.mark.usefixtures('dd_environment') +# def test_server_down(aggregator, instance_basic, dd_run_check): +# instance_basic['port'] = 111 +# check = get_check(instance_basic) +# +# with pytest.raises(Exception, match="OperationalError.*Can't connect to MySQL server on"): +# dd_run_check(check) +# +# aggregator.assert_service_check('proxysql.can_connect', AgentCheck.CRITICAL) +# aggregator.assert_all_metrics_covered() @pytest.mark.integration diff --git a/proxysql/tests/test_unit.py b/proxysql/tests/test_unit.py index 0f4bd3886276f..5fae1c957890b 100644 --- a/proxysql/tests/test_unit.py +++ b/proxysql/tests/test_unit.py @@ -44,4 +44,4 @@ def test_config_ok(dd_run_check): dd_run_check(check) connect_mock.assert_called_once() - assert query_executor_mock.call_count == 2 \ No newline at end of file + assert query_executor_mock.call_count == 2 From 615cab816d5b13a3c659a5dee5a59dfcbd5c2dba Mon Sep 17 00:00:00 2001 From: Andrew Zhang Date: Tue, 22 Dec 2020 15:42:17 -0500 Subject: [PATCH 03/15] Add README to explain that SSL is enabled --- proxysql/README.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/proxysql/README.md b/proxysql/README.md index 499f53b4788e7..9bb5023fd2847 100644 --- a/proxysql/README.md +++ b/proxysql/README.md @@ -14,6 +14,9 @@ The ProxySQL integration is included in the [Datadog Agent][3] package, so you d ### Configuration +#### Enabling SSL +Text here + From 3972150c4b7579c0be26b60dd1826f5ef9fc89b8 Mon Sep 17 00:00:00 2001 From: Andrew Zhang Date: Tue, 22 Dec 2020 15:55:25 -0500 Subject: [PATCH 04/15] Update formatting --- proxysql/README.md | 4 +++- proxysql/tests/test_integration.py | 22 +++++++++++----------- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/proxysql/README.md b/proxysql/README.md index 9bb5023fd2847..e9a88a0204229 100644 --- a/proxysql/README.md +++ b/proxysql/README.md @@ -15,7 +15,9 @@ The ProxySQL integration is included in the [Datadog Agent][3] package, so you d ### Configuration #### Enabling SSL -Text here +To connect to ProxySQL using SSL/TLS, enable the `use_tls` option in `conf.yaml`. + +Note: You should include any certificates and passwords needed to connect via SSL/TLS. diff --git a/proxysql/tests/test_integration.py b/proxysql/tests/test_integration.py index 7782b8302f206..1684631b4047e 100644 --- a/proxysql/tests/test_integration.py +++ b/proxysql/tests/test_integration.py @@ -43,17 +43,17 @@ def test_service_checks_ok(aggregator, instance_basic, dd_run_check): ) -# @pytest.mark.integration -# @pytest.mark.usefixtures('dd_environment') -# def test_server_down(aggregator, instance_basic, dd_run_check): -# instance_basic['port'] = 111 -# check = get_check(instance_basic) -# -# with pytest.raises(Exception, match="OperationalError.*Can't connect to MySQL server on"): -# dd_run_check(check) -# -# aggregator.assert_service_check('proxysql.can_connect', AgentCheck.CRITICAL) -# aggregator.assert_all_metrics_covered() +@pytest.mark.integration +@pytest.mark.usefixtures('dd_environment') +def test_server_down(aggregator, instance_basic, dd_run_check): + instance_basic['port'] = 111 + check = get_check(instance_basic) + + with pytest.raises(Exception, match="OperationalError.*Can't connect to MySQL server on"): + dd_run_check(check) + + aggregator.assert_service_check('proxysql.can_connect', AgentCheck.CRITICAL) + aggregator.assert_all_metrics_covered() @pytest.mark.integration From fe3cfeef020370d72511b8c83eae529ee1389702 Mon Sep 17 00:00:00 2001 From: Andrew Zhang Date: Tue, 22 Dec 2020 16:06:50 -0500 Subject: [PATCH 05/15] Update base req --- proxysql/setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proxysql/setup.py b/proxysql/setup.py index f5f96d2003614..a45e32078e40a 100644 --- a/proxysql/setup.py +++ b/proxysql/setup.py @@ -27,7 +27,7 @@ def get_dependencies(): return f.readlines() -CHECKS_BASE_REQ = ['datadog-checks-base>=11.3.1'] # Needs fix integrations-core/#6146 for the QueryManager +CHECKS_BASE_REQ = ['datadog-checks-base>=15.2.0'] setup( From 4a02f1dd40a2167bd530737937386f5e755d7f94 Mon Sep 17 00:00:00 2001 From: Andrew Zhang Date: Wed, 23 Dec 2020 15:18:02 -0500 Subject: [PATCH 06/15] Update README with code sample --- proxysql/README.md | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/proxysql/README.md b/proxysql/README.md index e9a88a0204229..05cd9dc20f203 100644 --- a/proxysql/README.md +++ b/proxysql/README.md @@ -15,9 +15,15 @@ The ProxySQL integration is included in the [Datadog Agent][3] package, so you d ### Configuration #### Enabling SSL -To connect to ProxySQL using SSL/TLS, enable the `use_tls` option in `conf.yaml`. - -Note: You should include any certificates and passwords needed to connect via SSL/TLS. +To connect to ProxySQL using SSL/TLS, enable the `use_tls` option in `conf.yaml`. Include certificates and passwords needed to connect via SSL/TLS. + +```yaml + use_tls: true + tls_cert: cert.pem + tls_ca_cert: ca_cert.pem + tls_private_key: private_key.pem + tls_private_key_password: Password123 +``` From 34d13d146d4ab8fe705ed948cc09b95f75a54413 Mon Sep 17 00:00:00 2001 From: yzhan289 Date: Wed, 23 Dec 2020 15:21:23 -0500 Subject: [PATCH 07/15] Fix spacing --- proxysql/README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/proxysql/README.md b/proxysql/README.md index 05cd9dc20f203..1bbdab82714fd 100644 --- a/proxysql/README.md +++ b/proxysql/README.md @@ -23,6 +23,7 @@ To connect to ProxySQL using SSL/TLS, enable the `use_tls` option in `conf.yaml` tls_ca_cert: ca_cert.pem tls_private_key: private_key.pem tls_private_key_password: Password123 + ``` From 76b1c6209589b408d533a87733ffc89e359c9aab Mon Sep 17 00:00:00 2001 From: yzhan289 Date: Wed, 23 Dec 2020 16:44:56 -0500 Subject: [PATCH 08/15] Expand large test into smaller tests --- proxysql/tests/test_unit.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/proxysql/tests/test_unit.py b/proxysql/tests/test_unit.py index 5fae1c957890b..93487c1ed9e75 100644 --- a/proxysql/tests/test_unit.py +++ b/proxysql/tests/test_unit.py @@ -11,20 +11,25 @@ @pytest.mark.unit -def test_wrong_config(dd_run_check, instance_basic): - # Empty instance +def test_empty_instance(dd_run_check): with pytest.raises(ConfigurationError, match='ProxySQL host, port, username and password are needed'): dd_run_check(get_check({})) - # Only host + +@pytest.mark.unit +def test_only_host(dd_run_check): with pytest.raises(ConfigurationError, match='ProxySQL host, port, username and password are needed'): dd_run_check(get_check({'host': 'localhost'})) - # Missing password + +@pytest.mark.unit +def test_missing_password(dd_run_check): with pytest.raises(ConfigurationError, match='ProxySQL host, port, username and password are needed'): dd_run_check(get_check({'host': 'localhost', 'port': 6032, 'username': 'admin'})) - # Wrong additional metrics group + +@pytest.mark.unit +def test_wrong_additional_metrics_group(dd_run_check, instance_basic): with pytest.raises( ConfigurationError, match="There is no additional metric group called 'foo' for the ProxySQL integration, it should be one of ", From d261b87b8adc5777a3e9da800b15b6c432ca3dc6 Mon Sep 17 00:00:00 2001 From: yzhan289 Date: Wed, 23 Dec 2020 17:30:31 -0500 Subject: [PATCH 09/15] Add new instance for TLS --- proxysql/datadog_checks/proxysql/proxysql.py | 15 ++++- proxysql/tests/conftest.py | 37 ++++++++++++ proxysql/tests/test_unit.py | 60 ++++++++++++++++++-- 3 files changed, 106 insertions(+), 6 deletions(-) diff --git a/proxysql/datadog_checks/proxysql/proxysql.py b/proxysql/datadog_checks/proxysql/proxysql.py index c4b148b6454b2..59733b3c232d9 100644 --- a/proxysql/datadog_checks/proxysql/proxysql.py +++ b/proxysql/datadog_checks/proxysql/proxysql.py @@ -6,7 +6,7 @@ import pymysql import pymysql.cursors -from datadog_checks.base import AgentCheck, ConfigurationError +from datadog_checks.base import AgentCheck, ConfigurationError, is_affirmative from datadog_checks.base.utils.db import QueryManager from .queries import ( @@ -34,6 +34,11 @@ class ProxysqlCheck(AgentCheck): SERVICE_CHECK_NAME = "can_connect" __NAMESPACE__ = "proxysql" + # This remapper is used to support legacy Proxysql integration config values + TLS_CONFIG_REMAPPER = { + 'validate_hostname': {'name': 'tls_validate_hostname'}, + } + def __init__(self, name, init_config, instances): super(ProxysqlCheck, self).__init__(name, init_config, instances) self.host = self.instance.get("host", "") @@ -44,7 +49,13 @@ def __init__(self, name, init_config, instances): if not all((self.host, self.port, self.user, self.password)): raise ConfigurationError("ProxySQL host, port, username and password are needed") - self.use_tls = self.instance.get("use_tls", False) + # If `tls_verify` is explicitly set to true, set `use_tls` to true (for legacy support) + # `tls_verify` used to do what `use_tls` does now + self.tls_verify = is_affirmative(self.instance.get('tls_verify')) + self.use_tls = is_affirmative(self.instance.get('use_tls', False)) + + if self.tls_verify and not self.use_tls: + self.use_tls = True self.connect_timeout = self.instance.get("connect_timeout", 10) self.read_timeout = self.instance.get("read_timeout") diff --git a/proxysql/tests/conftest.py b/proxysql/tests/conftest.py index 8a05609802d9a..268ad610728de 100644 --- a/proxysql/tests/conftest.py +++ b/proxysql/tests/conftest.py @@ -37,6 +37,33 @@ 'additional_metrics': [], } + +BASIC_INSTANCE_TLS = { + 'host': DOCKER_HOST, + 'port': PROXY_ADMIN_PORT, + 'username': PROXY_ADMIN_USER, + 'password': PROXY_ADMIN_PASS, + 'tags': ["application:test"], + 'additional_metrics': [], + 'use_tls': True, + 'tls_ca_cert': "/etc/ssl/certs/proxysql-ca.pem", + 'tls_validate_hostname': True +} + + +BASIC_INSTANCE_TLS_LEGACY = { + 'host': DOCKER_HOST, + 'port': PROXY_ADMIN_PORT, + 'username': PROXY_ADMIN_USER, + 'password': PROXY_ADMIN_PASS, + 'tags': ["application:test"], + 'additional_metrics': [], + 'tls_verify': True, # legacy version of tls_verify + 'tls_ca_cert': "/etc/ssl/certs/proxysql-ca.pem", + 'validate_hostname': True # legacy version of tls_validate_hostname +} + + INSTANCE_ALL_METRICS = { 'host': DOCKER_HOST, 'port': PROXY_ADMIN_PORT, @@ -74,6 +101,16 @@ def instance_basic(): return deepcopy(BASIC_INSTANCE) +@pytest.fixture +def instance_basic_tls(): + return deepcopy(BASIC_INSTANCE_TLS) + + +@pytest.fixture +def instance_basic_tls_legacy(): + return deepcopy(BASIC_INSTANCE_TLS_LEGACY) + + @pytest.fixture() def instance_all_metrics(instance_basic): return deepcopy(INSTANCE_ALL_METRICS) diff --git a/proxysql/tests/test_unit.py b/proxysql/tests/test_unit.py index 93487c1ed9e75..68c3015a73e1e 100644 --- a/proxysql/tests/test_unit.py +++ b/proxysql/tests/test_unit.py @@ -31,16 +31,16 @@ def test_missing_password(dd_run_check): @pytest.mark.unit def test_wrong_additional_metrics_group(dd_run_check, instance_basic): with pytest.raises( - ConfigurationError, - match="There is no additional metric group called 'foo' for the ProxySQL integration, it should be one of ", + ConfigurationError, + match="There is no additional metric group called 'foo' for the ProxySQL integration, it should be one of ", ): instance_basic['additional_metrics'].append('foo') dd_run_check(get_check(instance_basic)) @pytest.mark.unit -def test_config_ok(dd_run_check): - check = get_check({'host': 'localhost', 'port': 6032, 'username': 'admin', 'password': 'admin'}) +def test_config_ok(dd_run_check, instance_basic): + check = get_check(instance_basic) connect_mock, query_executor_mock = mock.MagicMock(), mock.MagicMock() check.connect = connect_mock @@ -50,3 +50,55 @@ def test_config_ok(dd_run_check): connect_mock.assert_called_once() assert query_executor_mock.call_count == 2 + + +@pytest.mark.unit +def test_tls_config_ok(dd_run_check, instance_basic_tls): + with mock.patch('datadog_checks.base.utils.tls.ssl') as ssl: + with mock.patch('datadog_checks.proxysql.proxysql.pymysql') as pymysql: + check = get_check(instance_basic_tls) + + # mock TLS context + tls_context = mock.MagicMock() + ssl.SSLContext.return_value = tls_context + + # mock query executor + query_executor_mock = mock.MagicMock() + check._query_manager.executor = query_executor_mock + + # mock behavior of db + mock_db = mock.MagicMock() + mock_db.close.return_value = True + pymysql.connect.return_value = mock_db + + dd_run_check(check) + + assert check.use_tls is True + assert tls_context.check_hostname is True + assert check._connection is not None + + +@pytest.mark.unit +def test_tls_legacy_config_ok(dd_run_check, instance_basic_tls_legacy): + with mock.patch('datadog_checks.base.utils.tls.ssl') as ssl: + with mock.patch('datadog_checks.proxysql.proxysql.pymysql') as pymysql: + check = get_check(instance_basic_tls_legacy) + + # mock TLS context + tls_context = mock.MagicMock() + ssl.SSLContext.return_value = tls_context + + # mock query executor + query_executor_mock = mock.MagicMock() + check._query_manager.executor = query_executor_mock + + # mock behavior of db + mock_db = mock.MagicMock() + mock_db.close.return_value = True + pymysql.connect.return_value = mock_db + + dd_run_check(check) + + assert check.use_tls is True + assert tls_context.check_hostname is True + assert check._connection is not None From 20d46f4557221fa65cd5cf2e314e51c286248322 Mon Sep 17 00:00:00 2001 From: yzhan289 Date: Wed, 23 Dec 2020 17:35:41 -0500 Subject: [PATCH 10/15] Move constants to common.py --- proxysql/tests/common.py | 85 ++++++++++++++++++++++++++++++ proxysql/tests/conftest.py | 100 ++++++------------------------------ proxysql/tests/test_unit.py | 4 +- 3 files changed, 104 insertions(+), 85 deletions(-) diff --git a/proxysql/tests/common.py b/proxysql/tests/common.py index 924a8aaf69b09..4e5e1ae31482d 100644 --- a/proxysql/tests/common.py +++ b/proxysql/tests/common.py @@ -1,6 +1,10 @@ # (C) Datadog, Inc. 2020-present # All rights reserved # Licensed under a 3-clause BSD style license (see LICENSE) +import os + +from datadog_checks.dev import get_docker_hostname + GLOBAL_METRICS = ( 'proxysql.active_transactions', 'proxysql.query_processor_time_pct', @@ -99,3 +103,84 @@ + USER_TAGS_METRICS + QUERY_RULES_TAGS_METRICS ) + +DOCKER_HOST = get_docker_hostname() +MYSQL_PORT = 6612 +PROXY_PORT = 6033 +PROXY_ADMIN_PORT = 6032 +MYSQL_USER = 'proxysql' +MYSQL_PASS = 'pass' +PROXY_ADMIN_USER = 'proxy' +PROXY_ADMIN_PASS = 'proxy' +PROXY_STATS_USER = 'proxystats' +PROXY_STATS_PASS = 'proxystats' +MYSQL_DATABASE = 'test' +PROXY_MAIN_DATABASE = 'main' +PROXYSQL_VERSION = os.environ['PROXYSQL_VERSION'] + +BASIC_INSTANCE = { + 'host': DOCKER_HOST, + 'port': PROXY_ADMIN_PORT, + 'username': PROXY_ADMIN_USER, + 'password': PROXY_ADMIN_PASS, + 'tags': ["application:test"], + 'additional_metrics': [], +} + + +BASIC_INSTANCE_TLS = { + 'host': DOCKER_HOST, + 'port': PROXY_ADMIN_PORT, + 'username': PROXY_ADMIN_USER, + 'password': PROXY_ADMIN_PASS, + 'tags': ["application:test"], + 'additional_metrics': [], + 'use_tls': True, + 'tls_ca_cert': "/etc/ssl/certs/proxysql-ca.pem", + 'tls_validate_hostname': True, +} + + +BASIC_INSTANCE_TLS_LEGACY = { + 'host': DOCKER_HOST, + 'port': PROXY_ADMIN_PORT, + 'username': PROXY_ADMIN_USER, + 'password': PROXY_ADMIN_PASS, + 'tags': ["application:test"], + 'additional_metrics': [], + 'tls_verify': True, # legacy version of tls_verify + 'tls_ca_cert': "/etc/ssl/certs/proxysql-ca.pem", + 'validate_hostname': True, # legacy version of tls_validate_hostname +} + + +INSTANCE_ALL_METRICS = { + 'host': DOCKER_HOST, + 'port': PROXY_ADMIN_PORT, + 'username': PROXY_ADMIN_USER, + 'password': PROXY_ADMIN_PASS, + 'tags': ["application:test"], + 'additional_metrics': [ + 'command_counters_metrics', + 'connection_pool_metrics', + 'users_metrics', + 'memory_metrics', + 'query_rules_metrics', + ], +} + +INSTANCE_ALL_METRICS_STATS = { + 'host': DOCKER_HOST, + 'port': PROXY_ADMIN_PORT, + 'username': PROXY_STATS_USER, + 'password': PROXY_STATS_PASS, + 'database_name': PROXY_MAIN_DATABASE, + 'tags': ["application:test"], + 'additional_metrics': [ + 'command_counters_metrics', + 'connection_pool_metrics', + 'users_metrics', + 'memory_metrics', + 'query_rules_metrics', + ], +} diff --git a/proxysql/tests/conftest.py b/proxysql/tests/conftest.py index 268ad610728de..3f7532ae1ea45 100644 --- a/proxysql/tests/conftest.py +++ b/proxysql/tests/conftest.py @@ -8,92 +8,26 @@ import pymysql import pytest -from datadog_checks.dev import TempDir, docker_run, get_docker_hostname, get_here +from datadog_checks.dev import TempDir, docker_run, get_here from datadog_checks.dev.conditions import CheckDockerLogs, WaitFor from datadog_checks.proxysql import ProxysqlCheck -from .common import ALL_METRICS - -DOCKER_HOST = get_docker_hostname() -MYSQL_PORT = 6612 -PROXY_PORT = 6033 -PROXY_ADMIN_PORT = 6032 -MYSQL_USER = 'proxysql' -MYSQL_PASS = 'pass' -PROXY_ADMIN_USER = 'proxy' -PROXY_ADMIN_PASS = 'proxy' -PROXY_STATS_USER = 'proxystats' -PROXY_STATS_PASS = 'proxystats' -MYSQL_DATABASE = 'test' -PROXY_MAIN_DATABASE = 'main' -PROXYSQL_VERSION = os.environ['PROXYSQL_VERSION'] - -BASIC_INSTANCE = { - 'host': DOCKER_HOST, - 'port': PROXY_ADMIN_PORT, - 'username': PROXY_ADMIN_USER, - 'password': PROXY_ADMIN_PASS, - 'tags': ["application:test"], - 'additional_metrics': [], -} - - -BASIC_INSTANCE_TLS = { - 'host': DOCKER_HOST, - 'port': PROXY_ADMIN_PORT, - 'username': PROXY_ADMIN_USER, - 'password': PROXY_ADMIN_PASS, - 'tags': ["application:test"], - 'additional_metrics': [], - 'use_tls': True, - 'tls_ca_cert': "/etc/ssl/certs/proxysql-ca.pem", - 'tls_validate_hostname': True -} - - -BASIC_INSTANCE_TLS_LEGACY = { - 'host': DOCKER_HOST, - 'port': PROXY_ADMIN_PORT, - 'username': PROXY_ADMIN_USER, - 'password': PROXY_ADMIN_PASS, - 'tags': ["application:test"], - 'additional_metrics': [], - 'tls_verify': True, # legacy version of tls_verify - 'tls_ca_cert': "/etc/ssl/certs/proxysql-ca.pem", - 'validate_hostname': True # legacy version of tls_validate_hostname -} - - -INSTANCE_ALL_METRICS = { - 'host': DOCKER_HOST, - 'port': PROXY_ADMIN_PORT, - 'username': PROXY_ADMIN_USER, - 'password': PROXY_ADMIN_PASS, - 'tags': ["application:test"], - 'additional_metrics': [ - 'command_counters_metrics', - 'connection_pool_metrics', - 'users_metrics', - 'memory_metrics', - 'query_rules_metrics', - ], -} - -INSTANCE_ALL_METRICS_STATS = { - 'host': DOCKER_HOST, - 'port': PROXY_ADMIN_PORT, - 'username': PROXY_STATS_USER, - 'password': PROXY_STATS_PASS, - 'database_name': PROXY_MAIN_DATABASE, - 'tags': ["application:test"], - 'additional_metrics': [ - 'command_counters_metrics', - 'connection_pool_metrics', - 'users_metrics', - 'memory_metrics', - 'query_rules_metrics', - ], -} +from .common import ( + ALL_METRICS, + BASIC_INSTANCE, + BASIC_INSTANCE_TLS, + BASIC_INSTANCE_TLS_LEGACY, + DOCKER_HOST, + INSTANCE_ALL_METRICS, + INSTANCE_ALL_METRICS_STATS, + MYSQL_DATABASE, + MYSQL_PASS, + MYSQL_PORT, + MYSQL_USER, + PROXY_ADMIN_PORT, + PROXY_PORT, + PROXYSQL_VERSION, +) @pytest.fixture diff --git a/proxysql/tests/test_unit.py b/proxysql/tests/test_unit.py index 68c3015a73e1e..61ae379e7aa91 100644 --- a/proxysql/tests/test_unit.py +++ b/proxysql/tests/test_unit.py @@ -31,8 +31,8 @@ def test_missing_password(dd_run_check): @pytest.mark.unit def test_wrong_additional_metrics_group(dd_run_check, instance_basic): with pytest.raises( - ConfigurationError, - match="There is no additional metric group called 'foo' for the ProxySQL integration, it should be one of ", + ConfigurationError, + match="There is no additional metric group called 'foo' for the ProxySQL integration, it should be one of ", ): instance_basic['additional_metrics'].append('foo') dd_run_check(get_check(instance_basic)) From 48e194fdc9ca6fcf9208fb48d7f0cc22a0b785a1 Mon Sep 17 00:00:00 2001 From: yzhan289 Date: Mon, 28 Dec 2020 13:47:41 -0500 Subject: [PATCH 11/15] Remove use_tls --- proxysql/README.md | 4 +-- proxysql/assets/configuration/spec.yaml | 11 ++------ .../proxysql/data/conf.yaml.example | 9 ++---- proxysql/datadog_checks/proxysql/proxysql.py | 10 ++----- proxysql/tests/common.py | 15 +--------- proxysql/tests/conftest.py | 8 +----- proxysql/tests/test_integration.py | 22 +++++++-------- proxysql/tests/test_unit.py | 28 +------------------ 8 files changed, 23 insertions(+), 84 deletions(-) diff --git a/proxysql/README.md b/proxysql/README.md index 1bbdab82714fd..dc8be221e84b2 100644 --- a/proxysql/README.md +++ b/proxysql/README.md @@ -15,10 +15,10 @@ The ProxySQL integration is included in the [Datadog Agent][3] package, so you d ### Configuration #### Enabling SSL -To connect to ProxySQL using SSL/TLS, enable the `use_tls` option in `conf.yaml`. Include certificates and passwords needed to connect via SSL/TLS. +To connect to ProxySQL using full SSL/TLS validation, enable the `tls_verify` option in `conf.yaml`. Include certificates and passwords needed to connect via SSL/TLS. ```yaml - use_tls: true + tls_verify: true tls_cert: cert.pem tls_ca_cert: ca_cert.pem tls_private_key: private_key.pem diff --git a/proxysql/assets/configuration/spec.yaml b/proxysql/assets/configuration/spec.yaml index f4719876f6714..57f91bab7a368 100644 --- a/proxysql/assets/configuration/spec.yaml +++ b/proxysql/assets/configuration/spec.yaml @@ -34,15 +34,10 @@ files: value: type: string example: - - name: use_tls - required: false - description: | - Instructs the check to use SSL when connecting to ProxySQL - value: - type: boolean - example: false - default: false - template: instances/tls + overrides: + tls_verify.default: false + tls_verify.value.example: false - name: connect_timeout required: false description: | diff --git a/proxysql/datadog_checks/proxysql/data/conf.yaml.example b/proxysql/datadog_checks/proxysql/data/conf.yaml.example index 2c199583ed977..8ecf7db751038 100644 --- a/proxysql/datadog_checks/proxysql/data/conf.yaml.example +++ b/proxysql/datadog_checks/proxysql/data/conf.yaml.example @@ -33,15 +33,10 @@ instances: # password: - ## @param use_tls - boolean - optional - default: false - ## Instructs the check to use SSL when connecting to ProxySQL - # - # use_tls: false - - ## @param tls_verify - boolean - optional - default: true + ## @param tls_verify - boolean - optional - default: false ## Instructs the check to validate the TLS certificate(s) of the service(s). # - # tls_verify: true + # tls_verify: false ## @param tls_ca_cert - string - optional ## The path to a file of concatenated CA certificates in PEM format or a directory diff --git a/proxysql/datadog_checks/proxysql/proxysql.py b/proxysql/datadog_checks/proxysql/proxysql.py index 59733b3c232d9..b0f447b2af5f4 100644 --- a/proxysql/datadog_checks/proxysql/proxysql.py +++ b/proxysql/datadog_checks/proxysql/proxysql.py @@ -49,13 +49,7 @@ def __init__(self, name, init_config, instances): if not all((self.host, self.port, self.user, self.password)): raise ConfigurationError("ProxySQL host, port, username and password are needed") - # If `tls_verify` is explicitly set to true, set `use_tls` to true (for legacy support) - # `tls_verify` used to do what `use_tls` does now - self.tls_verify = is_affirmative(self.instance.get('tls_verify')) - self.use_tls = is_affirmative(self.instance.get('use_tls', False)) - - if self.tls_verify and not self.use_tls: - self.use_tls = True + self.tls_verify = is_affirmative(self.instance.get('tls_verify', False)) self.connect_timeout = self.instance.get("connect_timeout", 10) self.read_timeout = self.instance.get("read_timeout") @@ -100,7 +94,7 @@ def execute_query_raw(self, query): @contextmanager def connect(self): - if self.use_tls: + if self.tls_verify: self.log.debug("Connecting to ProxySQL via SSL/TLS") # If ca_cert is None, will load the default certificates ssl_context = self.get_tls_context() diff --git a/proxysql/tests/common.py b/proxysql/tests/common.py index 4e5e1ae31482d..4398a03a21761 100644 --- a/proxysql/tests/common.py +++ b/proxysql/tests/common.py @@ -135,25 +135,12 @@ 'password': PROXY_ADMIN_PASS, 'tags': ["application:test"], 'additional_metrics': [], - 'use_tls': True, + 'tls_verify': True, 'tls_ca_cert': "/etc/ssl/certs/proxysql-ca.pem", 'tls_validate_hostname': True, } -BASIC_INSTANCE_TLS_LEGACY = { - 'host': DOCKER_HOST, - 'port': PROXY_ADMIN_PORT, - 'username': PROXY_ADMIN_USER, - 'password': PROXY_ADMIN_PASS, - 'tags': ["application:test"], - 'additional_metrics': [], - 'tls_verify': True, # legacy version of tls_verify - 'tls_ca_cert': "/etc/ssl/certs/proxysql-ca.pem", - 'validate_hostname': True, # legacy version of tls_validate_hostname -} - - INSTANCE_ALL_METRICS = { 'host': DOCKER_HOST, 'port': PROXY_ADMIN_PORT, diff --git a/proxysql/tests/conftest.py b/proxysql/tests/conftest.py index 3f7532ae1ea45..2d8bb455ef4b5 100644 --- a/proxysql/tests/conftest.py +++ b/proxysql/tests/conftest.py @@ -16,7 +16,6 @@ ALL_METRICS, BASIC_INSTANCE, BASIC_INSTANCE_TLS, - BASIC_INSTANCE_TLS_LEGACY, DOCKER_HOST, INSTANCE_ALL_METRICS, INSTANCE_ALL_METRICS_STATS, @@ -40,11 +39,6 @@ def instance_basic_tls(): return deepcopy(BASIC_INSTANCE_TLS) -@pytest.fixture -def instance_basic_tls_legacy(): - return deepcopy(BASIC_INSTANCE_TLS_LEGACY) - - @pytest.fixture() def instance_all_metrics(instance_basic): return deepcopy(INSTANCE_ALL_METRICS) @@ -83,7 +77,7 @@ def dd_environment(): cert_dest = "/etc/ssl/certs/proxysql-ca.pem" if PROXYSQL_VERSION.startswith('2'): # SSL is only available with version 2.x of ProxySQL - instance['use_tls'] = True + instance['tls_verify'] = True instance['tls_ca_cert'] = cert_dest instance['validate_hostname'] = False yield instance, {'docker_volumes': ['{}:{}'.format(cert_src, cert_dest)]} diff --git a/proxysql/tests/test_integration.py b/proxysql/tests/test_integration.py index 1684631b4047e..7782b8302f206 100644 --- a/proxysql/tests/test_integration.py +++ b/proxysql/tests/test_integration.py @@ -43,17 +43,17 @@ def test_service_checks_ok(aggregator, instance_basic, dd_run_check): ) -@pytest.mark.integration -@pytest.mark.usefixtures('dd_environment') -def test_server_down(aggregator, instance_basic, dd_run_check): - instance_basic['port'] = 111 - check = get_check(instance_basic) - - with pytest.raises(Exception, match="OperationalError.*Can't connect to MySQL server on"): - dd_run_check(check) - - aggregator.assert_service_check('proxysql.can_connect', AgentCheck.CRITICAL) - aggregator.assert_all_metrics_covered() +# @pytest.mark.integration +# @pytest.mark.usefixtures('dd_environment') +# def test_server_down(aggregator, instance_basic, dd_run_check): +# instance_basic['port'] = 111 +# check = get_check(instance_basic) +# +# with pytest.raises(Exception, match="OperationalError.*Can't connect to MySQL server on"): +# dd_run_check(check) +# +# aggregator.assert_service_check('proxysql.can_connect', AgentCheck.CRITICAL) +# aggregator.assert_all_metrics_covered() @pytest.mark.integration diff --git a/proxysql/tests/test_unit.py b/proxysql/tests/test_unit.py index 61ae379e7aa91..5062fb27253ac 100644 --- a/proxysql/tests/test_unit.py +++ b/proxysql/tests/test_unit.py @@ -73,32 +73,6 @@ def test_tls_config_ok(dd_run_check, instance_basic_tls): dd_run_check(check) - assert check.use_tls is True - assert tls_context.check_hostname is True - assert check._connection is not None - - -@pytest.mark.unit -def test_tls_legacy_config_ok(dd_run_check, instance_basic_tls_legacy): - with mock.patch('datadog_checks.base.utils.tls.ssl') as ssl: - with mock.patch('datadog_checks.proxysql.proxysql.pymysql') as pymysql: - check = get_check(instance_basic_tls_legacy) - - # mock TLS context - tls_context = mock.MagicMock() - ssl.SSLContext.return_value = tls_context - - # mock query executor - query_executor_mock = mock.MagicMock() - check._query_manager.executor = query_executor_mock - - # mock behavior of db - mock_db = mock.MagicMock() - mock_db.close.return_value = True - pymysql.connect.return_value = mock_db - - dd_run_check(check) - - assert check.use_tls is True + assert check.tls_verify is True assert tls_context.check_hostname is True assert check._connection is not None From 80f0190a6f259a8113999460b75133cf97ddea6e Mon Sep 17 00:00:00 2001 From: yzhan289 Date: Mon, 28 Dec 2020 13:48:42 -0500 Subject: [PATCH 12/15] Remove comment --- proxysql/datadog_checks/proxysql/proxysql.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proxysql/datadog_checks/proxysql/proxysql.py b/proxysql/datadog_checks/proxysql/proxysql.py index b0f447b2af5f4..afb4a0ddcc12a 100644 --- a/proxysql/datadog_checks/proxysql/proxysql.py +++ b/proxysql/datadog_checks/proxysql/proxysql.py @@ -100,7 +100,7 @@ def connect(self): ssl_context = self.get_tls_context() else: self.log.debug("Connecting to ProxySQL without SSL/TLS") - ssl_context = make_insecure_ssl_client_context() # can keep this + ssl_context = make_insecure_ssl_client_context() db = None try: From 7cca171dd609f673d44b4d48e57ae6b939171437 Mon Sep 17 00:00:00 2001 From: yzhan289 Date: Wed, 30 Dec 2020 15:07:29 -0500 Subject: [PATCH 13/15] Remove client validation options --- proxysql/README.md | 4 ---- proxysql/assets/configuration/spec.yaml | 3 +++ .../proxysql/data/conf.yaml.example | 24 ------------------- 3 files changed, 3 insertions(+), 28 deletions(-) diff --git a/proxysql/README.md b/proxysql/README.md index dc8be221e84b2..90b02245f40f2 100644 --- a/proxysql/README.md +++ b/proxysql/README.md @@ -19,11 +19,7 @@ To connect to ProxySQL using full SSL/TLS validation, enable the `tls_verify` op ```yaml tls_verify: true - tls_cert: cert.pem tls_ca_cert: ca_cert.pem - tls_private_key: private_key.pem - tls_private_key_password: Password123 - ``` diff --git a/proxysql/assets/configuration/spec.yaml b/proxysql/assets/configuration/spec.yaml index 57f91bab7a368..4ec4d284dd4fa 100644 --- a/proxysql/assets/configuration/spec.yaml +++ b/proxysql/assets/configuration/spec.yaml @@ -38,6 +38,9 @@ files: overrides: tls_verify.default: false tls_verify.value.example: false + tls_cert.hidden: true + tls_private_key.hidden: true + tls_private_key_password.hidden: true - name: connect_timeout required: false description: | diff --git a/proxysql/datadog_checks/proxysql/data/conf.yaml.example b/proxysql/datadog_checks/proxysql/data/conf.yaml.example index 8ecf7db751038..2e5b1b55fa213 100644 --- a/proxysql/datadog_checks/proxysql/data/conf.yaml.example +++ b/proxysql/datadog_checks/proxysql/data/conf.yaml.example @@ -48,30 +48,6 @@ instances: # # tls_ca_cert: - ## @param tls_cert - string - optional - ## The path to a single file in PEM format containing a certificate as well as any - ## number of CA certificates needed to establish the certificate's authenticity for - ## use when connecting to services. It may also contain an unencrypted private key to use. - ## - ## Setting this implicitly sets `tls_verify` to true. - # - # tls_cert: - - ## @param tls_private_key - string - optional - ## The unencrypted private key to use for `tls_cert` when connecting to services. This is - ## required if `tls_cert` is set and it does not already contain a private key. - ## - ## Setting this implicitly sets `tls_verify` to true. - # - # tls_private_key: - - ## @param tls_private_key_password - string - optional - ## Optional password to decrypt tls_private_key. - ## - ## Setting this implicitly sets `tls_verify` to true. - # - # tls_private_key_password: - ## @param tls_validate_hostname - boolean - optional - default: true ## Verifies that the server's cert hostname matches the one requested. # From b3413f93a6ecdc946ca131e8dee4187d90192a7c Mon Sep 17 00:00:00 2001 From: yzhan289 Date: Mon, 4 Jan 2021 09:17:26 -0500 Subject: [PATCH 14/15] Uncomment test --- proxysql/tests/test_integration.py | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/proxysql/tests/test_integration.py b/proxysql/tests/test_integration.py index 7782b8302f206..1684631b4047e 100644 --- a/proxysql/tests/test_integration.py +++ b/proxysql/tests/test_integration.py @@ -43,17 +43,17 @@ def test_service_checks_ok(aggregator, instance_basic, dd_run_check): ) -# @pytest.mark.integration -# @pytest.mark.usefixtures('dd_environment') -# def test_server_down(aggregator, instance_basic, dd_run_check): -# instance_basic['port'] = 111 -# check = get_check(instance_basic) -# -# with pytest.raises(Exception, match="OperationalError.*Can't connect to MySQL server on"): -# dd_run_check(check) -# -# aggregator.assert_service_check('proxysql.can_connect', AgentCheck.CRITICAL) -# aggregator.assert_all_metrics_covered() +@pytest.mark.integration +@pytest.mark.usefixtures('dd_environment') +def test_server_down(aggregator, instance_basic, dd_run_check): + instance_basic['port'] = 111 + check = get_check(instance_basic) + + with pytest.raises(Exception, match="OperationalError.*Can't connect to MySQL server on"): + dd_run_check(check) + + aggregator.assert_service_check('proxysql.can_connect', AgentCheck.CRITICAL) + aggregator.assert_all_metrics_covered() @pytest.mark.integration From 9652e0bde9d1018361f05e4341c77791c70f7aff Mon Sep 17 00:00:00 2001 From: yzhan289 Date: Mon, 4 Jan 2021 09:37:33 -0500 Subject: [PATCH 15/15] Add legacy config remapper testing --- proxysql/tests/test_unit.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/proxysql/tests/test_unit.py b/proxysql/tests/test_unit.py index 5062fb27253ac..b6ab69dacf2de 100644 --- a/proxysql/tests/test_unit.py +++ b/proxysql/tests/test_unit.py @@ -6,6 +6,7 @@ import pytest from datadog_checks.base.errors import ConfigurationError +from datadog_checks.proxysql import ProxysqlCheck from .conftest import get_check @@ -76,3 +77,20 @@ def test_tls_config_ok(dd_run_check, instance_basic_tls): assert check.tls_verify is True assert tls_context.check_hostname is True assert check._connection is not None + + +@pytest.mark.parametrize( + 'extra_config, expected_http_kwargs', + [ + pytest.param( + {'validate_hostname': False}, {'tls_validate_hostname': False}, id='legacy validate_hostname param' + ), + ], +) +def test_tls_config_legacy(extra_config, expected_http_kwargs, instance_all_metrics): + instance = instance_all_metrics + instance.update(extra_config) + c = ProxysqlCheck('proxysql', {}, [instance]) + c.get_tls_context() # need to call this for config values to be saved by _tls_context_wrapper + actual_options = {k: v for k, v in c._tls_context_wrapper.config.items() if k in expected_http_kwargs} + assert expected_http_kwargs == actual_options