Skip to content

feat: add --sslmode flag and PGSSLMODE env var support#353

Merged
tianzhou merged 5 commits intomainfrom
fix/issue-351-sslmode
Mar 9, 2026
Merged

feat: add --sslmode flag and PGSSLMODE env var support#353
tianzhou merged 5 commits intomainfrom
fix/issue-351-sslmode

Conversation

@tianzhou
Copy link
Contributor

@tianzhou tianzhou commented Mar 9, 2026

Summary

  • Add --sslmode CLI flag (default: prefer) to dump, plan, and apply commands with PGSSLMODE env var fallback
  • Add --plan-sslmode flag for external plan database connections with PGSCHEMA_PLAN_SSLMODE env var fallback
  • Thread sslmode through all connection paths (GetIRFromDatabase, ExternalDatabaseConfig, DetectPostgresVersionFromDB)

Closes #351

Test plan

  • go build ./... passes
  • go vet ./... passes
  • Diff tests pass (TestDiffFromFiles)
  • Dump tests pass (TestDumpCommand_Employee)
  • Manual test: pgschema dump --sslmode disable --host localhost --db mydb --user postgres
  • Manual test: PGSSLMODE=require pgschema dump --host localhost --db mydb --user postgres

🤖 Generated with Claude Code

…onnections (#351)

Allow users to configure SSL mode for database connections via --sslmode
CLI flag (default: "prefer") or PGSSLMODE environment variable. Also adds
--plan-sslmode / PGSCHEMA_PLAN_SSLMODE for external plan database connections.

Closes #351

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 9, 2026 07:46
@greptile-apps
Copy link

greptile-apps bot commented Mar 9, 2026

Greptile Summary

This PR adds --sslmode and --plan-sslmode CLI flags (with PGSSLMODE / PGSCHEMA_PLAN_SSLMODE env-var fallbacks) to the dump, plan, and apply commands, replacing the previous hardcoded "prefer" throughout all connection paths. The change is well-structured: each command independently handles the env-var override pattern, GetIRFromDatabase and DetectPostgresVersionFromDB receive the sslmode explicitly, ExternalDatabaseConfig gains a first-class SSLMode field with a sslModeOrDefault() helper, and ResetFlags is updated for test isolation.

Key concerns:

  • Embedded postgres SSL mismatch – in GeneratePlan, providerSSLMode is set from config.PlanDBSSLMode for all provider types including embedded postgres. Because embedded postgres is started with SSLMode: "disable", passing --plan-sslmode require (or verify-ca / verify-full) without --plan-host will fail with a confusing SSL error at the second GetIRFromDatabase call. The embedded postgres path should unconditionally use "disable".
  • No sslmode value validation – invalid values (typos, unsupported modes) are passed directly to the DSN, producing a low-quality libpq error instead of a clear CLI message. Adding an allow-list check early in each command's RunE would significantly improve UX.

Confidence Score: 3/5

  • Safe to merge for typical use cases, but contains a latent failure path when --plan-sslmode is set to a strict mode without an external plan database.
  • The SSL threading is correct for the common case (target DB and external plan DB). The main risk is the embedded postgres path in GeneratePlan, which will fail at runtime if the user passes --plan-sslmode require/verify-ca/verify-full without --plan-host, since embedded postgres does not support SSL. This is not a regression from the previous behaviour (the flag didn't exist before), but it is a new footgun introduced by the PR. No sslmode input validation also means bad values produce unhelpful errors.
  • cmd/plan/plan.go — the providerSSLMode block (GeneratePlan) incorrectly forwards PlanDBSSLMode to the embedded postgres connection path.

Important Files Changed

Filename Overview
cmd/plan/plan.go Adds planSSLMode/planDBSSLMode flags and threads them through CreateDesiredStateProvider and GeneratePlan; however, the providerSSLMode block incorrectly applies PlanDBSSLMode for the embedded postgres path, which will fail if set to require or verify-*.
cmd/apply/apply.go Correctly propagates applySSLMode and applyPlanDBSSLMode through ApplyConfig and the internal PlanConfig, and env-var override logic (PGSSLMODE) is applied consistently before building the config.
cmd/dump/dump.go Cleanly adds --sslmode flag and PGSSLMODE env-var fallback; threads SSLMode into DumpConfig and passes it to GetIRFromDatabase. No issues found.
cmd/util/connection.go Signature change to GetIRFromDatabase correctly forwards sslmode into ConnectionConfig; no validation of the sslmode string is performed before passing it to the DSN.
cmd/util/env.go Extends ApplyPlanDBEnvVars signature with sslmodePtr and applies PGSCHEMA_PLAN_SSLMODE consistently, mirroring the handling of other plan-DB parameters.
internal/postgres/embedded.go Adds sslmode parameter to DetectPostgresVersionFromDB with a safe "prefer" default when empty; the embedded postgres instance itself continues to use "disable" for its own connection.
internal/postgres/external.go Adds SSLMode field to ExternalDatabaseConfig and introduces sslModeOrDefault() helper that defaults to "prefer"; cleanly wired into NewExternalDatabase.

Sequence Diagram

sequenceDiagram
    participant User
    participant CLI as dump/plan/apply (cmd)
    participant Env as PGSSLMODE / PGSCHEMA_PLAN_SSLMODE
    participant Util as cmd/util
    participant EmbPG as embedded.go
    participant ExtPG as external.go
    participant DB as Target PostgreSQL

    User->>CLI: --sslmode <mode>
    alt flag not explicitly set
        CLI->>Env: os.Getenv("PGSSLMODE")
        Env-->>CLI: finalSSLMode
    end

    CLI->>EmbPG: DetectPostgresVersionFromDB(..., finalSSLMode)
    EmbPG->>DB: connect (sslmode=finalSSLMode)
    DB-->>EmbPG: pgVersion
    EmbPG-->>CLI: pgVersion

    alt --plan-host provided
        CLI->>ExtPG: NewExternalDatabase(config{SSLMode: planDBSSLMode})
        ExtPG->>DB: connect (sslmode=planDBSSLMode)
    else embedded postgres
        CLI->>EmbPG: StartEmbeddedPostgres (sslmode=disable internally)
    end

    CLI->>Util: GetIRFromDatabase(..., finalSSLMode, ...)
    Util->>DB: connect (sslmode=finalSSLMode)
    DB-->>Util: schema IR
    Util-->>CLI: IR

    note over CLI,Util: providerSSLMode uses PlanDBSSLMode<br/>even for embedded postgres path (⚠️)
Loading

Last reviewed commit: c5117cf

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds configurable PostgreSQL sslmode across CLI commands and connection helpers so users can control TLS behavior via flags and environment variables (addressing #351).

Changes:

  • Add --sslmode flag (with PGSSLMODE fallback) to dump, plan, and apply.
  • Add --plan-sslmode flag (with PGSCHEMA_PLAN_SSLMODE fallback) for external plan DB connections.
  • Thread sslmode through connection paths (GetIRFromDatabase, ExternalDatabaseConfig, DetectPostgresVersionFromDB) and document env vars in .env.example.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
internal/postgres/external.go Add SSLMode to external plan DB config and defaulting behavior.
internal/postgres/embedded.go Thread sslmode into version detection connection (defaulting to prefer).
cmd/util/env.go Extend plan DB env var loader to support PGSCHEMA_PLAN_SSLMODE.
cmd/util/connection.go Add sslmode parameter to GetIRFromDatabase and pass through to DSN builder.
cmd/plan/plan.go Add --sslmode/--plan-sslmode, env fallbacks, and plumb into plan generation paths.
cmd/plan/external_db_integration_test.go Update version-detection call for new sslmode parameter.
cmd/dump/dump.go Add --sslmode and PGSSLMODE fallback; plumb into GetIRFromDatabase.
cmd/apply/apply.go Add --sslmode/--plan-sslmode, env fallbacks, and plumb into apply + fingerprint validation.
.env.example Document PGSSLMODE and PGSCHEMA_PLAN_SSLMODE.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tianzhou and others added 2 commits March 9, 2026 01:07
Tests call runDump(nil, nil) which panics on cmd.Flags().Changed().
Add nil guard for cmd in all three commands.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix embedded postgres SSL mismatch: use "disable" for embedded path,
  only use PlanDBSSLMode for external plan databases
- Add ValidateSSLMode() with allowlist of 6 valid PostgreSQL SSL modes
- Default empty sslmode to "prefer" in GetIRFromDatabase for safety
- Add unit tests for ValidateSSLMode

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

ApplyMigration builds a PlanConfig for GeneratePlan but was missing
PlanDBHost/PlanDBSSLMode, causing GeneratePlan to always treat the
provider as embedded postgres and force sslmode="disable" even when
an external plan database requiring SSL is in use.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add sslmode to connection options in dump, plan, and apply docs.
Add PGSSLMODE to dotenv docs. Add --plan-sslmode to plan-db docs.
Add --sslmode to coding-agent connection options reference.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@tianzhou tianzhou merged commit 53df33d into main Mar 9, 2026
5 checks passed
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.

No ability tu set sslmode for connections

2 participants