Skip to content

Commit b9e94e3

Browse files
authored
[SDBM-2104] Updates Postgres config handling around dbname when autodiscovery used (#21884)
* Updates config handling around dbname when autodiscovery used * Add changelog * Update logic to properly handle expected cases * Fix regex and additional cases
1 parent 81066a1 commit b9e94e3

File tree

3 files changed

+198
-4
lines changed

3 files changed

+198
-4
lines changed

postgres/changelog.d/21884.fixed

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Fixed a bug where excluding "postgres" database when configuring autodiscovery would still fallback to "postgres" if `dbname` wasn't also configured

postgres/datadog_checks/postgres/config.py

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,15 @@ def build_config(check: PostgreSql) -> Tuple[InstanceConfig, ValidationResult]:
102102
if f in instance:
103103
args[f] = instance[f]
104104

105+
# Build database_autodiscovery config first, as we may need it for dbname defaulting
106+
database_autodiscovery_config = {
107+
**dict_defaults.instance_database_autodiscovery().model_dump(),
108+
**(instance.get('database_autodiscovery', {})),
109+
}
110+
111+
if 'dbname' not in instance and database_autodiscovery_config.get('enabled'):
112+
args['dbname'] = database_autodiscovery_config.get('global_view_db')
113+
105114
# Set values for args that have deprecated fallbacks, are not supported by the spec model
106115
# or have other complexities
107116
# 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]:
119128
**dict_defaults.instance_database_identifier().model_dump(),
120129
**(instance.get('database_identifier', {})),
121130
},
122-
"database_autodiscovery": {
123-
**dict_defaults.instance_database_autodiscovery().model_dump(),
124-
**(instance.get('database_autodiscovery', {})),
125-
},
131+
"database_autodiscovery": database_autodiscovery_config,
126132
"query_metrics": {
127133
**dict_defaults.instance_query_metrics().model_dump(),
128134
**{
@@ -386,6 +392,28 @@ def validate_config(config: InstanceConfig, instance: dict, validation_result: V
386392
'Use the `exclude_hostname` option instead.'
387393
)
388394

395+
# Validate dbname is not excluded when using autodiscovery
396+
# Only validate when dbname was NOT explicitly set by the user (auto-defaulted)
397+
# If user explicitly set dbname, they may intentionally want to connect to an excluded database
398+
# for global operations while excluding it from per-database metric collection
399+
if config.database_autodiscovery.enabled and config.dbname and 'dbname' not in instance:
400+
import re
401+
402+
# Guard against None - exclude could be explicitly set to null to remove default exclusions
403+
for exclude_pattern in config.database_autodiscovery.exclude or ():
404+
try:
405+
# Note: Match is case-sensitive to match the behavior of the base Discovery Filter class
406+
if re.search(exclude_pattern, config.dbname):
407+
# Auto-defaulted dbname conflicts - suggest setting global_view_db
408+
validation_result.add_error(
409+
f'The default dbname "{config.dbname}" is excluded by autodiscovery pattern '
410+
f'"{exclude_pattern}". Set database_autodiscovery.global_view_db to a '
411+
f'non-excluded database.'
412+
)
413+
break
414+
except re.error:
415+
validation_result.add_warning(f'Invalid regex pattern in autodiscovery exclude: {exclude_pattern}')
416+
389417
# If the user provided config explicitly enables these features, we add a warning if dbm is not enabled
390418
dbm_required = ['query_activity', 'query_samples', 'query_metrics', 'collect_settings', 'collect_schemas']
391419
for feature in dbm_required:

postgres/tests/test_config.py

Lines changed: 165 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -381,3 +381,168 @@ def test_relations_validation_fails_if_schemas_is_wrong_type():
381381
def test_relations_validation_fails_if_relkind_is_wrong_type():
382382
with pytest.raises(ConfigurationError):
383383
RelationsManager.validate_relations_config([{"relation_name": "person", "relkind": "foo"}])
384+
385+
386+
def test_autodiscovery_dbname_defaults_to_global_view_db(mock_check):
387+
# When autodiscovery is enabled and dbname is not explicitly set,
388+
# dbname should default to global_view_db
389+
instance = {
390+
'host': 'localhost',
391+
'port': 5432,
392+
'username': 'testuser',
393+
'password': 'testpass',
394+
'database_autodiscovery': {
395+
'enabled': True,
396+
'global_view_db': 'main',
397+
},
398+
}
399+
mock_check.instance = instance
400+
mock_check.init_config = {}
401+
config, result = build_config(check=mock_check)
402+
assert result.valid
403+
assert config.dbname == 'main'
404+
405+
406+
def test_autodiscovery_dbname_respects_explicit_value(mock_check):
407+
# When autodiscovery is enabled and dbname IS explicitly set,
408+
# the explicit value should be used, even if it's in the exclude list
409+
# This allows users to connect to a database for global operations
410+
# while excluding it from per-database metric collection
411+
instance = {
412+
'host': 'localhost',
413+
'port': 5432,
414+
'username': 'testuser',
415+
'password': 'testpass',
416+
'dbname': 'postgres',
417+
'database_autodiscovery': {
418+
'enabled': True,
419+
'global_view_db': 'postgres',
420+
'exclude': ['postgres'],
421+
},
422+
}
423+
mock_check.instance = instance
424+
mock_check.init_config = {}
425+
config, result = build_config(check=mock_check)
426+
# Should be valid - user explicitly set dbname, so they know what they're doing
427+
assert result.valid
428+
assert config.dbname == 'postgres'
429+
430+
431+
def test_autodiscovery_excluded_default_dbname_fails(mock_check):
432+
# When autodiscovery is enabled, default dbname postgres is used,
433+
# and postgres is in the exclude list, should fail validation
434+
instance = {
435+
'host': 'localhost',
436+
'port': 5432,
437+
'username': 'testuser',
438+
'password': 'testpass',
439+
'database_autodiscovery': {
440+
'enabled': True,
441+
'exclude': ['postgres'],
442+
},
443+
}
444+
mock_check.instance = instance
445+
mock_check.init_config = {}
446+
config, result = build_config(check=mock_check)
447+
assert not result.valid
448+
assert any('is excluded by autodiscovery pattern' in str(e) for e in result.errors)
449+
assert any('global_view_db' in str(e) for e in result.errors)
450+
451+
452+
def test_autodiscovery_with_regex_exclude_pattern(mock_check):
453+
# Test that regex patterns work correctly in exclude validation
454+
instance = {
455+
'host': 'localhost',
456+
'port': 5432,
457+
'username': 'testuser',
458+
'password': 'testpass',
459+
'database_autodiscovery': {
460+
'enabled': True,
461+
'global_view_db': 'test_main',
462+
'exclude': ['test_.*'],
463+
},
464+
}
465+
mock_check.instance = instance
466+
mock_check.init_config = {}
467+
config, result = build_config(check=mock_check)
468+
assert not result.valid
469+
assert any('is excluded by autodiscovery pattern' in str(e) for e in result.errors)
470+
471+
472+
def test_autodiscovery_with_non_excluded_global_view_db_succeeds(mock_check):
473+
# When autodiscovery is enabled with exclude patterns,
474+
# but global_view_db doesn't match any of them, should succeed
475+
instance = {
476+
'host': 'localhost',
477+
'port': 5432,
478+
'username': 'testuser',
479+
'password': 'testpass',
480+
'database_autodiscovery': {
481+
'enabled': True,
482+
'global_view_db': 'production',
483+
'exclude': ['postgres', 'test_.*'],
484+
},
485+
}
486+
mock_check.instance = instance
487+
mock_check.init_config = {}
488+
config, result = build_config(check=mock_check)
489+
assert result.valid
490+
assert config.dbname == 'production'
491+
492+
493+
def test_autodiscovery_case_sensitive_exclude_matching(mock_check):
494+
instance = {
495+
'host': 'localhost',
496+
'port': 5432,
497+
'username': 'testuser',
498+
'password': 'testpass',
499+
'database_autodiscovery': {
500+
'enabled': True,
501+
'global_view_db': 'POSTGRES',
502+
'exclude': ['postgres'],
503+
},
504+
}
505+
mock_check.instance = instance
506+
mock_check.init_config = {}
507+
config, result = build_config(check=mock_check)
508+
# Should be valid - case-sensitive matching means POSTGRES != postgres
509+
assert result.valid
510+
assert config.dbname == 'POSTGRES'
511+
512+
513+
def test_autodiscovery_invalid_regex_pattern_warns(mock_check):
514+
# Invalid regex patterns should generate warnings
515+
instance = {
516+
'host': 'localhost',
517+
'port': 5432,
518+
'username': 'testuser',
519+
'password': 'testpass',
520+
'database_autodiscovery': {
521+
'enabled': True,
522+
'global_view_db': 'main',
523+
'exclude': ['[invalid'], # Invalid regex
524+
},
525+
}
526+
mock_check.instance = instance
527+
mock_check.init_config = {}
528+
config, result = build_config(check=mock_check)
529+
# Should have a warning about invalid regex
530+
assert any('Invalid regex pattern' in w for w in result.warnings)
531+
532+
533+
def test_autodiscovery_exclude_none_does_not_error(mock_check):
534+
instance = {
535+
'host': 'localhost',
536+
'port': 5432,
537+
'username': 'testuser',
538+
'password': 'testpass',
539+
'database_autodiscovery': {
540+
'enabled': True,
541+
'global_view_db': 'main',
542+
},
543+
}
544+
mock_check.instance = instance
545+
mock_check.init_config = {}
546+
config, result = build_config(check=mock_check)
547+
assert result.valid
548+
assert config.dbname == 'main'

0 commit comments

Comments
 (0)