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 diff --git a/postgres/datadog_checks/postgres/config.py b/postgres/datadog_checks/postgres/config.py index 0eb90f528ac3f..24a955f3f2685 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,28 @@ def validate_config(config: InstanceConfig, instance: dict, validation_result: V 'Use the `exclude_hostname` option instead.' ) + # Validate dbname is not excluded when using autodiscovery + # 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 + + # Guard against None - exclude could be explicitly set to null to remove default exclusions + for exclude_pattern in config.database_autodiscovery.exclude or (): + try: + # 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 ' + 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..ead2c207c0be2 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, 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': 'postgres', + '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) + # 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): + # 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_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_sensitive_exclude_matching(mock_check): + 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) + # 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): + # 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) + + +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'