From 6dce30db73141673c6b6f3be906f09188d4dee88 Mon Sep 17 00:00:00 2001 From: Eric Weaver Date: Fri, 14 Nov 2025 16:01:26 -0500 Subject: [PATCH 1/4] Updates config handling around dbname when autodiscovery used --- postgres/datadog_checks/postgres/config.py | 39 ++++- postgres/tests/test_config.py | 165 +++++++++++++++++++++ 2 files changed, 200 insertions(+), 4 deletions(-) diff --git a/postgres/datadog_checks/postgres/config.py b/postgres/datadog_checks/postgres/config.py index 0eb90f528ac3f..a688acf4f67f5 100644 --- a/postgres/datadog_checks/postgres/config.py +++ b/postgres/datadog_checks/postgres/config.py @@ -102,6 +102,15 @@ def build_config(check: PostgreSql) -> Tuple[InstanceConfig, ValidationResult]: if f in instance: args[f] = instance[f] + # Build database_autodiscovery config first, as we may need it for dbname defaulting + database_autodiscovery_config = { + **dict_defaults.instance_database_autodiscovery().model_dump(), + **(instance.get('database_autodiscovery', {})), + } + + if 'dbname' not in instance and database_autodiscovery_config.get('enabled'): + args['dbname'] = database_autodiscovery_config.get('global_view_db') + # Set values for args that have deprecated fallbacks, are not supported by the spec model # or have other complexities # If you change a literal value here, make sure to update spec.yaml @@ -119,10 +128,7 @@ def build_config(check: PostgreSql) -> Tuple[InstanceConfig, ValidationResult]: **dict_defaults.instance_database_identifier().model_dump(), **(instance.get('database_identifier', {})), }, - "database_autodiscovery": { - **dict_defaults.instance_database_autodiscovery().model_dump(), - **(instance.get('database_autodiscovery', {})), - }, + "database_autodiscovery": database_autodiscovery_config, "query_metrics": { **dict_defaults.instance_query_metrics().model_dump(), **{ @@ -386,6 +392,31 @@ def validate_config(config: InstanceConfig, instance: dict, validation_result: V 'Use the `exclude_hostname` option instead.' ) + # Validate dbname is not excluded when using autodiscovery + if config.database_autodiscovery.enabled and config.dbname: + import re + + for exclude_pattern in config.database_autodiscovery.exclude: + try: + if re.search(exclude_pattern, config.dbname, re.IGNORECASE): + # Check if user explicitly set dbname + if 'dbname' in instance: + validation_result.add_error( + f'The configured dbname "{config.dbname}" matches the autodiscovery ' + f'exclude pattern "{exclude_pattern}". Either remove dbname from ' + f'configuration or adjust the exclude patterns.' + ) + else: + # Auto-defaulted dbname conflicts - suggest setting global_view_db + validation_result.add_error( + f'The default dbname "{config.dbname}" is excluded by autodiscovery pattern ' + f'"{exclude_pattern}". Set database_autodiscovery.global_view_db to a ' + f'non-excluded database.' + ) + break + except re.error: + validation_result.add_warning(f'Invalid regex pattern in autodiscovery exclude: {exclude_pattern}') + # If the user provided config explicitly enables these features, we add a warning if dbm is not enabled dbm_required = ['query_activity', 'query_samples', 'query_metrics', 'collect_settings', 'collect_schemas'] for feature in dbm_required: diff --git a/postgres/tests/test_config.py b/postgres/tests/test_config.py index a30c2ca789c0b..036e800f4e774 100644 --- a/postgres/tests/test_config.py +++ b/postgres/tests/test_config.py @@ -381,3 +381,168 @@ def test_relations_validation_fails_if_schemas_is_wrong_type(): def test_relations_validation_fails_if_relkind_is_wrong_type(): with pytest.raises(ConfigurationError): RelationsManager.validate_relations_config([{"relation_name": "person", "relkind": "foo"}]) + + +def test_autodiscovery_dbname_defaults_to_global_view_db(mock_check): + # When autodiscovery is enabled and dbname is not explicitly set, + # dbname should default to global_view_db + instance = { + 'host': 'localhost', + 'port': 5432, + 'username': 'testuser', + 'password': 'testpass', + 'database_autodiscovery': { + 'enabled': True, + 'global_view_db': 'main', + }, + } + mock_check.instance = instance + mock_check.init_config = {} + config, result = build_config(check=mock_check) + assert result.valid + assert config.dbname == 'main' + + +def test_autodiscovery_dbname_respects_explicit_value(mock_check): + # When autodiscovery is enabled and dbname IS explicitly set, + # the explicit value should be used + instance = { + 'host': 'localhost', + 'port': 5432, + 'username': 'testuser', + 'password': 'testpass', + 'dbname': 'mydb', + 'database_autodiscovery': { + 'enabled': True, + 'global_view_db': 'main', + }, + } + mock_check.instance = instance + mock_check.init_config = {} + config, result = build_config(check=mock_check) + # Should be valid if mydb is not excluded + assert config.dbname == 'mydb' + + +def test_autodiscovery_excluded_default_dbname_fails(mock_check): + # When autodiscovery is enabled, default dbname postgres is used, + # and postgres is in the exclude list, should fail validation + instance = { + 'host': 'localhost', + 'port': 5432, + 'username': 'testuser', + 'password': 'testpass', + 'database_autodiscovery': { + 'enabled': True, + 'exclude': ['postgres'], + }, + } + mock_check.instance = instance + mock_check.init_config = {} + config, result = build_config(check=mock_check) + assert not result.valid + assert any('is excluded by autodiscovery pattern' in str(e) for e in result.errors) + assert any('global_view_db' in str(e) for e in result.errors) + + +def test_autodiscovery_explicit_dbname_excluded_fails(mock_check): + # When autodiscovery is enabled and user explicitly sets dbname + # to a value that's in the exclude list, should fail validation + instance = { + 'host': 'localhost', + 'port': 5432, + 'username': 'testuser', + 'password': 'testpass', + 'dbname': 'testdb', + 'database_autodiscovery': { + 'enabled': True, + 'exclude': ['testdb'], + }, + } + mock_check.instance = instance + mock_check.init_config = {} + config, result = build_config(check=mock_check) + assert not result.valid + assert any('configured dbname "testdb" matches the autodiscovery' in str(e) for e in result.errors) + assert any('remove dbname from configuration' in str(e) for e in result.errors) + + +def test_autodiscovery_with_regex_exclude_pattern(mock_check): + # Test that regex patterns work correctly in exclude validation + instance = { + 'host': 'localhost', + 'port': 5432, + 'username': 'testuser', + 'password': 'testpass', + 'database_autodiscovery': { + 'enabled': True, + 'global_view_db': 'test_main', + 'exclude': ['test_.*'], + }, + } + mock_check.instance = instance + mock_check.init_config = {} + config, result = build_config(check=mock_check) + assert not result.valid + assert any('is excluded by autodiscovery pattern' in str(e) for e in result.errors) + + +def test_autodiscovery_with_non_excluded_global_view_db_succeeds(mock_check): + # When autodiscovery is enabled with exclude patterns, + # but global_view_db doesn't match any of them, should succeed + instance = { + 'host': 'localhost', + 'port': 5432, + 'username': 'testuser', + 'password': 'testpass', + 'database_autodiscovery': { + 'enabled': True, + 'global_view_db': 'production', + 'exclude': ['postgres', 'test_.*'], + }, + } + mock_check.instance = instance + mock_check.init_config = {} + config, result = build_config(check=mock_check) + assert result.valid + assert config.dbname == 'production' + + +def test_autodiscovery_case_insensitive_exclude_matching(mock_check): + # Exclude patterns should be case-insensitive + instance = { + 'host': 'localhost', + 'port': 5432, + 'username': 'testuser', + 'password': 'testpass', + 'database_autodiscovery': { + 'enabled': True, + 'global_view_db': 'POSTGRES', + 'exclude': ['postgres'], + }, + } + mock_check.instance = instance + mock_check.init_config = {} + config, result = build_config(check=mock_check) + assert not result.valid + assert any('is excluded by autodiscovery pattern' in str(e) for e in result.errors) + + +def test_autodiscovery_invalid_regex_pattern_warns(mock_check): + # Invalid regex patterns should generate warnings + instance = { + 'host': 'localhost', + 'port': 5432, + 'username': 'testuser', + 'password': 'testpass', + 'database_autodiscovery': { + 'enabled': True, + 'global_view_db': 'main', + 'exclude': ['[invalid'], # Invalid regex + }, + } + mock_check.instance = instance + mock_check.init_config = {} + config, result = build_config(check=mock_check) + # Should have a warning about invalid regex + assert any('Invalid regex pattern' in w for w in result.warnings) From 3efd3206aa235b5cc48983e39f6c3448f95a61ab Mon Sep 17 00:00:00 2001 From: Eric Weaver Date: Fri, 14 Nov 2025 16:16:06 -0500 Subject: [PATCH 2/4] Add changelog --- postgres/changelog.d/21884.fixed | 1 + 1 file changed, 1 insertion(+) create mode 100644 postgres/changelog.d/21884.fixed diff --git a/postgres/changelog.d/21884.fixed b/postgres/changelog.d/21884.fixed new file mode 100644 index 0000000000000..2b5794146a04f --- /dev/null +++ b/postgres/changelog.d/21884.fixed @@ -0,0 +1 @@ +Fixed a bug where excluding "postgres" database when configuring autodiscovery would still fallback to "postgres" if `dbname` wasn't also configured From dfcdbcf073140efab3852a8ab35789bb3ba962be Mon Sep 17 00:00:00 2001 From: Eric Weaver Date: Fri, 14 Nov 2025 16:44:01 -0500 Subject: [PATCH 3/4] Update logic to properly handle expected cases --- postgres/datadog_checks/postgres/config.py | 25 ++++++--------- postgres/tests/test_config.py | 36 ++++++---------------- 2 files changed, 19 insertions(+), 42 deletions(-) diff --git a/postgres/datadog_checks/postgres/config.py b/postgres/datadog_checks/postgres/config.py index a688acf4f67f5..ae09f5a154dec 100644 --- a/postgres/datadog_checks/postgres/config.py +++ b/postgres/datadog_checks/postgres/config.py @@ -393,26 +393,21 @@ def validate_config(config: InstanceConfig, instance: dict, validation_result: V ) # Validate dbname is not excluded when using autodiscovery - if config.database_autodiscovery.enabled and config.dbname: + # Only validate when dbname was NOT explicitly set by the user (auto-defaulted) + # If user explicitly set dbname, they may intentionally want to connect to an excluded database + # for global operations while excluding it from per-database metric collection + if config.database_autodiscovery.enabled and config.dbname and 'dbname' not in instance: import re for exclude_pattern in config.database_autodiscovery.exclude: try: if re.search(exclude_pattern, config.dbname, re.IGNORECASE): - # Check if user explicitly set dbname - if 'dbname' in instance: - validation_result.add_error( - f'The configured dbname "{config.dbname}" matches the autodiscovery ' - f'exclude pattern "{exclude_pattern}". Either remove dbname from ' - f'configuration or adjust the exclude patterns.' - ) - else: - # Auto-defaulted dbname conflicts - suggest setting global_view_db - validation_result.add_error( - f'The default dbname "{config.dbname}" is excluded by autodiscovery pattern ' - f'"{exclude_pattern}". Set database_autodiscovery.global_view_db to a ' - f'non-excluded database.' - ) + # Auto-defaulted dbname conflicts - suggest setting global_view_db + validation_result.add_error( + f'The default dbname "{config.dbname}" is excluded by autodiscovery pattern ' + f'"{exclude_pattern}". Set database_autodiscovery.global_view_db to a ' + f'non-excluded database.' + ) break except re.error: validation_result.add_warning(f'Invalid regex pattern in autodiscovery exclude: {exclude_pattern}') diff --git a/postgres/tests/test_config.py b/postgres/tests/test_config.py index 036e800f4e774..1b78fb9fb14a8 100644 --- a/postgres/tests/test_config.py +++ b/postgres/tests/test_config.py @@ -405,23 +405,27 @@ def test_autodiscovery_dbname_defaults_to_global_view_db(mock_check): def test_autodiscovery_dbname_respects_explicit_value(mock_check): # When autodiscovery is enabled and dbname IS explicitly set, - # the explicit value should be used + # the explicit value should be used, even if it's in the exclude list + # This allows users to connect to a database for global operations + # while excluding it from per-database metric collection instance = { 'host': 'localhost', 'port': 5432, 'username': 'testuser', 'password': 'testpass', - 'dbname': 'mydb', + 'dbname': 'postgres', 'database_autodiscovery': { 'enabled': True, - 'global_view_db': 'main', + 'global_view_db': 'postgres', + 'exclude': ['postgres'], }, } mock_check.instance = instance mock_check.init_config = {} config, result = build_config(check=mock_check) - # Should be valid if mydb is not excluded - assert config.dbname == 'mydb' + # Should be valid - user explicitly set dbname, so they know what they're doing + assert result.valid + assert config.dbname == 'postgres' def test_autodiscovery_excluded_default_dbname_fails(mock_check): @@ -445,28 +449,6 @@ def test_autodiscovery_excluded_default_dbname_fails(mock_check): assert any('global_view_db' in str(e) for e in result.errors) -def test_autodiscovery_explicit_dbname_excluded_fails(mock_check): - # When autodiscovery is enabled and user explicitly sets dbname - # to a value that's in the exclude list, should fail validation - instance = { - 'host': 'localhost', - 'port': 5432, - 'username': 'testuser', - 'password': 'testpass', - 'dbname': 'testdb', - 'database_autodiscovery': { - 'enabled': True, - 'exclude': ['testdb'], - }, - } - mock_check.instance = instance - mock_check.init_config = {} - config, result = build_config(check=mock_check) - assert not result.valid - assert any('configured dbname "testdb" matches the autodiscovery' in str(e) for e in result.errors) - assert any('remove dbname from configuration' in str(e) for e in result.errors) - - def test_autodiscovery_with_regex_exclude_pattern(mock_check): # Test that regex patterns work correctly in exclude validation instance = { From 17a44c4315f571de2b62e0d122e7ac8d9bb55a87 Mon Sep 17 00:00:00 2001 From: Eric Weaver Date: Fri, 14 Nov 2025 17:02:51 -0500 Subject: [PATCH 4/4] Fix regex and additional cases --- postgres/datadog_checks/postgres/config.py | 6 +++-- postgres/tests/test_config.py | 26 ++++++++++++++++++---- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/postgres/datadog_checks/postgres/config.py b/postgres/datadog_checks/postgres/config.py index ae09f5a154dec..24a955f3f2685 100644 --- a/postgres/datadog_checks/postgres/config.py +++ b/postgres/datadog_checks/postgres/config.py @@ -399,9 +399,11 @@ def validate_config(config: InstanceConfig, instance: dict, validation_result: V if config.database_autodiscovery.enabled and config.dbname and 'dbname' not in instance: import re - for exclude_pattern in config.database_autodiscovery.exclude: + # Guard against None - exclude could be explicitly set to null to remove default exclusions + for exclude_pattern in config.database_autodiscovery.exclude or (): try: - if re.search(exclude_pattern, config.dbname, re.IGNORECASE): + # Note: Match is case-sensitive to match the behavior of the base Discovery Filter class + if re.search(exclude_pattern, config.dbname): # Auto-defaulted dbname conflicts - suggest setting global_view_db validation_result.add_error( f'The default dbname "{config.dbname}" is excluded by autodiscovery pattern ' diff --git a/postgres/tests/test_config.py b/postgres/tests/test_config.py index 1b78fb9fb14a8..ead2c207c0be2 100644 --- a/postgres/tests/test_config.py +++ b/postgres/tests/test_config.py @@ -490,8 +490,7 @@ def test_autodiscovery_with_non_excluded_global_view_db_succeeds(mock_check): assert config.dbname == 'production' -def test_autodiscovery_case_insensitive_exclude_matching(mock_check): - # Exclude patterns should be case-insensitive +def test_autodiscovery_case_sensitive_exclude_matching(mock_check): instance = { 'host': 'localhost', 'port': 5432, @@ -506,8 +505,9 @@ def test_autodiscovery_case_insensitive_exclude_matching(mock_check): mock_check.instance = instance mock_check.init_config = {} config, result = build_config(check=mock_check) - assert not result.valid - assert any('is excluded by autodiscovery pattern' in str(e) for e in result.errors) + # Should be valid - case-sensitive matching means POSTGRES != postgres + assert result.valid + assert config.dbname == 'POSTGRES' def test_autodiscovery_invalid_regex_pattern_warns(mock_check): @@ -528,3 +528,21 @@ def test_autodiscovery_invalid_regex_pattern_warns(mock_check): config, result = build_config(check=mock_check) # Should have a warning about invalid regex assert any('Invalid regex pattern' in w for w in result.warnings) + + +def test_autodiscovery_exclude_none_does_not_error(mock_check): + instance = { + 'host': 'localhost', + 'port': 5432, + 'username': 'testuser', + 'password': 'testpass', + 'database_autodiscovery': { + 'enabled': True, + 'global_view_db': 'main', + }, + } + mock_check.instance = instance + mock_check.init_config = {} + config, result = build_config(check=mock_check) + assert result.valid + assert config.dbname == 'main'