fix(udb): dont configure ssl forpostgresif ssl config not set

[MasterPtato]

No description provided.

[MasterPtato]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• fix(udb): dont configure ssl forpostgresif ssl config not set #3949 👈 (View in Graphite)
• fix(gas, pb, api): make .removed events noop if dont match, add check for dead wfs to actor/runner errors, make api error types unknown #3948
• feat: add actor and kv metrics #3928
• fix: day 1 patch, add dashboards #3927
• main

---

How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[pkg-pr-new]

More templates

• actor-actions
• example-ai-agent
• example-ai-and-user-generated-actors-freestyle
• chat-room
• example-cloudflare-workers
• example-cloudflare-workers-hono
• example-cloudflare-workers-inline-client
• cross-actor-actions
• example-cursors
• example-cursors-raw-websocket
• custom-serverless
• example-sqlite
• example-elysia
• example-experimental-durable-streams-ai-agent
• hello-world
• example-hono
• hono-react
• example-kitchen-sink
• multi-region
• native-websockets
• next-js
• example-raw-fetch-handler
• example-raw-websocket-handler
• example-raw-websocket-handler-proxy
• example-react
• scheduling
• state
• example-stream
• example-trpc

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@3949

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@3949

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@3949

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@3949

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@3949

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@3949

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@3949

@rivetkit/virtual-websocket

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/virtual-websocket@3949

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3949

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3949

commit: 056407b

[claude]

Pull Request Review

Summary

This PR fixes an issue where SSL/TLS configuration was always being applied to Postgres connections, even when no SSL configuration was provided. The change makes SSL optional by conditionally creating either a TLS or NoTLS connection pool based on whether SSL configuration exists.

---

Code Quality & Best Practices

✅ Positive Changes

1. Better separation of concerns: The new PostgresSslConfig struct cleanly separates SSL-specific configuration from the main PostgresConfig, improving the API design at engine/packages/universaldb/src/driver/postgres/database.rs:36-41.

2. Cleaner conditional logic: Using Option<PostgresSslConfig> is more idiomatic Rust than having three separate Option<PathBuf> fields at the top level.

3. Correct handling of NoTls case: The code now properly creates a pool with tokio_postgres::NoTls when no SSL config is provided (engine/packages/universaldb/src/driver/postgres/database.rs:95), which is the correct approach for non-SSL connections.

4. Consistent struct construction: The mapping from config SSL to PostgresSslConfig at engine/packages/pools/src/db/udb.rs:26-32 is clean and follows Rust patterns.

⚠️ Issues Found

1. Commit message has typos: The commit message "dont configure ssl forpostgresif ssl config not set" has spacing issues:

   • "forpostgresif" should be "for postgres if"
   • "dont" should be "don't"
   • Suggested: fix(udb): don't configure ssl for postgres if ssl config not set

2. Missing context in change: While the code works, the PR description is empty. It would be helpful to explain:

   • What problem this fixes (e.g., connection failures when SSL is not configured?)
   • Whether this was causing actual bugs or just cleanup
   • Any related issues or user reports

---

Potential Bugs or Issues

⚠️ TLS Configuration Behavior Change

The previous code always called build_tls_config() which has this behavior (from engine/packages/postgres-util/src/lib.rs:40-43):

} else {
    // Use default webpki root certificates
    root_store.extend(webpki_roots::TLS_SERVER_ROOTS.iter().cloned());
}

Before this PR: Even with no SSL config provided, connections would use TLS with webpki root certificates by default.

After this PR: With no SSL config, connections use NoTls explicitly.

This is a behavior change that could affect deployments:

• ✅ Beneficial if the connection string uses sslmode=disable (no SSL intended)
• ⚠️ Potentially breaking if users were relying on the default TLS behavior without explicit config

Recommendation: Document this behavior change and verify it doesn't break existing deployments that expect TLS by default.

---

Performance Considerations

✅ No performance concerns: The changes are configuration-time operations only and don't affect runtime performance.

---

Security Concerns

⚠️ SSL/TLS Mode Handling

1. Connection string sslmode parameter: The Postgres config documentation mentions sslmode in the connection string (engine/packages/config/src/config/db/postgres.rs:38-42), but this PR's logic only looks at the ssl_config field. There could be a mismatch:

   • If connection string has sslmode=require but ssl_config is None, the code will use NoTls
   • This might cause connection failures or unexpected behavior

2. Fallback to less secure connections: The change makes it easier to accidentally connect without TLS when TLS was expected. Consider:

   • Adding a warning log when NoTls is used
   • Validating consistency between connection string sslmode and ssl_config presence

Suggested improvement:

let pool = if let Some(config) = &config.ssl_config {
    // ... existing TLS code ...
} else {
    tracing::warn!(
        "creating postgres connection without TLS (NoTls mode). \
        ensure connection string does not require SSL"
    );
    let tls = tokio_postgres::NoTls;
    // ... rest of code ...
};

---

Test Coverage

❌ Missing Tests

1. No tests for SSL config variations: The existing tests only use PostgresConfig::new(connection_string) which doesn't exercise the SSL configuration paths.

2. Recommended test cases:

   • Connection with ssl_config: None (NoTls path)
   • Connection with ssl_config: Some(...) with all cert paths
   • Connection with ssl_config: Some(...) with only root cert
   • Verify connection failures with mismatched sslmode/ssl_config

---

Additional Observations

1. Export is correct: The public API change in engine/packages/universaldb/src/driver/postgres/mod.rs:5 correctly exports PostgresSslConfig.

2. Consistent with Rust patterns: Using Option for optional configuration is idiomatic.

3. No breaking changes to existing code: The PostgresConfig::new() constructor still works, defaulting ssl_config to None.

---

Recommendations Summary

Must Fix:

• Fix commit message typos

Should Consider:

• Add PR description explaining the problem being solved
• Document the TLS behavior change (default TLS → explicit NoTls)
• Add warning log when using NoTls mode
• Validate consistency between connection string sslmode and ssl_config
• Add test coverage for SSL configuration paths

Nice to Have:

• Add integration tests for both TLS and NoTls connection modes

---

Overall, this is a reasonable refactoring that improves the API design. The main concerns are around the behavior change from implicit TLS to explicit NoTls, and the need for better test coverage and documentation.

[graphite-app]

Merge activity

• Jan 17, 12:07 AM UTC: MasterPtato added this pull request to the Graphite merge queue.
• Jan 17, 12:08 AM UTC: CI is running for this pull request on a draft pull request (#3951) due to your merge queue CI optimization settings.
• Jan 17, 12:09 AM UTC: Merged by the Graphite merge queue via draft PR: #3951.