Skip to content

fix(pg-pg): better tls wiring for pgdump command#4540

Merged
Amogh-Bharadwaj merged 5 commits into
mainfrom
pgdump-fix-tls
Jul 9, 2026
Merged

fix(pg-pg): better tls wiring for pgdump command#4540
Amogh-Bharadwaj merged 5 commits into
mainfrom
pgdump-fix-tls

Conversation

@Amogh-Bharadwaj

@Amogh-Bharadwaj Amogh-Bharadwaj commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

When a root CA certificate was provided in the peer configuration but TLS wasn't explicitly required, pg_dump and psql would attempt to connect without TLS — even though the regular connector correctly used TLS in this scenario. This caused schema migration to fail with a connection error.

Now pg_dump and psql enable TLS under the same conditions as the regular connector. Added unit tests covering argument building and all TLS configuration combinations.

Also adds TLS host wiring for pgdump

@Amogh-Bharadwaj Amogh-Bharadwaj requested a review from a team as a code owner July 6, 2026 14:41
@claude

claude Bot commented Jul 6, 2026

Copy link
Copy Markdown

Code review: Found 1 issue - see review below.

@claude

claude Bot commented Jul 6, 2026

Copy link
Copy Markdown

Bug in appendTLSEnv (pgdump_schema.go line 302): The if hasRootCA block runs unconditionally after the switch statement. When SkipCertVerification=true and a root CA is provided, the switch correctly sets PGSSLMODE=require, but the if hasRootCA block still writes the root CA to a temp file and sets PGSSLROOTCERT. This contradicts the test case 'skip cert verification with root CA uses require' which expects wantRootCertSet: false. The test will fail at line 511-512 because PGSSLROOTCERT is non-empty but the assertion expects it to be empty. Suggested fix: change line 302 from if hasRootCA to if hasRootCA && !config.SkipCertVerification.

@claude

claude Bot commented Jul 6, 2026

Copy link
Copy Markdown

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

❌ 2 Tests Failed:

Tests completed Failed Passed Skipped
2964 2 2962 344
View the top 2 failed test(s) by shortest run time
github.com/PeerDB-io/peerdb/flow/connectors/postgres::TestAppendTLSEnv
Stack Traces | 0s run time
=== RUN   TestAppendTLSEnv
--- FAIL: TestAppendTLSEnv (0.00s)
github.com/PeerDB-io/peerdb/flow/connectors/postgres::TestAppendTLSEnv/skip_cert_verification_with_root_CA_uses_require
Stack Traces | 0s run time
=== RUN   TestAppendTLSEnv/skip_cert_verification_with_root_CA_uses_require
    pgdump_schema_test.go:512: expected PGSSLROOTCERT to be empty, got "/tmp/peerdb-root-ca-2991070322.pem"
--- FAIL: TestAppendTLSEnv/skip_cert_verification_with_root_CA_uses_require (0.00s)

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

❌ Test Failure

Analysis: A deterministic (0.00s) assertion failure in TestAppendTLSEnv/pgdump_schema_test.go — expected PGSSLROOTCERT empty but got a cert path — which is exactly the pgdump TLS wiring logic this PR modifies, so it's a real bug in the change, not a flaky/timeout/race/network failure.
Confidence: 0.96

⚠️ This appears to be a real bug - manual intervention needed

View workflow run

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

🔄 Flaky Test Detected

Analysis: All 13 failures are SSH-tunnel tests rejected by the test SSH bastion with "ssh: rejected: administratively prohibited (open failed)"—an infra issue that hit only 1 of 3 matrix jobs while the identical tests passed on the other two, on a TLS-focused PR unrelated to SSH.
Confidence: 0.95

✅ Automatically retrying the workflow

View workflow run

@Amogh-Bharadwaj Amogh-Bharadwaj enabled auto-merge (squash) July 9, 2026 13:02
@Amogh-Bharadwaj Amogh-Bharadwaj merged commit fa9c3c0 into main Jul 9, 2026
19 of 20 checks passed
@Amogh-Bharadwaj Amogh-Bharadwaj deleted the pgdump-fix-tls branch July 9, 2026 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants