Fix autodiscovery over UNIX domain sockets

[Arusekk]

Fixes #1316
Fixes #476

Compiles fine, but not tested any further. I hope this is trivial enough to be safe.

[sysadmind]

Can you help me understand what this fixes and how? Based on my understanding, the existing .String call on the URI should properly serialize and URI information.

[Arusekk]

Sure. As I explained in the issue, is that URI is updated from postgres:///dbname?host=/run/postgresql (scheme postgres, empty host, path /dbname, query host=/run/postgresql) to have path otherdbname. Then, .String unparses the URL to postgresql://otherdbname?host=/run/postgresql&sslmode=disable (scheme postgres, host otherdbname, empty path, query host=/run/postgresql). The third slash is gone.

As a result, for each discovered otherdbname, postgres_exporter tries to resolve otherdbname via DNS and connect to it. Thankfully most database names do not resolve to any malicious servers.

The problem is not visible when using remote connections - only local ones are affected. For remote ones, this happens to work - by chance - because the URLs have a non-empty host.

Why the difference? I wonder, too. I tried reporting it to golang, as I believed this is a golang problem (after all, why should URL objects be so inconsistent), but they said that since no actual URL parses to this exact structure, the resulting .String return value is left undefined. They resort to correct guesses for a non-empty host, but prefer to guess wrong when the host is empty. See golang/go#5927 (comment) (maybe they can be convinced when presented with evidence of downstream impact, but my arguments alone proved insufficient).

[n3ph]

Still looking forward for this. ATM auto-discovery through UNIX domain sockets with alloy is an architectural blocker.

@Arusekk Thank you for being on it 🙇🏼

[n3ph]

• fixes PG_EXPORTER_AUTO_DISCOVER_DATABASES can not be used with unix socket host #476

[ArthurSens]

Hey, sorry for the delay, @Arusekk!

Although the fix looks correct, it would be awesome if we could add a test to make sure we don't accidentally reintroduce this bug. Maybe we could have a table-driven test covering:

• postgres:///postgres?host=/run/postgresql
• postgresql://user@/postgres?host=/run/postgresql
• A normal TCP URL to prevent regressions
• Preservation of credentials and query parameters

[ArthurSens]

/workflow-approve

[Arusekk]

I tired my best, but GitHub Actions has beaten me. Maybe someone can take over the testing part? It works perfectly fine on several machines.

[ArthurSens]

/workflow-approve

[ArthurSens]

Hey, I'm not sure if I was clear enough. The tests I mentioned above aren't integration tests; they are just unit tests to confirm we're parsing the string correctly.

The changes made to the integration tests are unnecessary 🤔