Add option to connect to database without SSL (fix "SSL is not enabled on the server" error)

[ericyd]

Type

Bug

System

OS: Mac OSX 11.6.3
DB: Postgres 12.8

Details

Installation went smoothly, but when I tried to inspect my existing local DB schema, I got an error

> atlas schema inspect --dsn "postgres://eric@localhost:5432/my_db"
Error: postgres: scanning system variables: pq: SSL is not enabled on the server

A quick search led me to this SO answer.

It appears that Atlas is attempting to connect with the raw DSN, without specifying an SSL mode (it appears the SSL is enabled by default):

atlas/cmd/action/provider.go

Line 35 in ee1ec7a

db, err := sql.Open("postgres", url)

Perhaps this is necessary in some cases but for local DB inspection, probably not. The tests appear to get around this by explicitly disabling SSL:

atlas/internal/integration/postgres_test.go

Line 41 in ee1ec7a

db, err := sql.Open("postgres", fmt.Sprintf("host=localhost port=%d user=postgres dbname=test password=pass sslmode=disable", port))

I verified this fix by running Atlas with the ?sslmode=disable query param added to my DB url, and it worked

> atlas schema inspect --dsn "postgres://eric@localhost:5432/my_db?sslmode=disable"

🎉 prints a schema to terminal

I am not well versed in Go to propose a specific solution but I think the most flexible option would be to add flag option for SSL mode (and perhaps default to false for inspection, since I would imagine inspecting a local DB is the most common initial step)

[crossworth]

Hello, if you don't mind my opinion, I dont think atlas should have a flag for it.

The way it works right now is very simple and standardized on the GO comunity, each driver can have it own set of options and will parse it from the DSN.

By creating a flag would mean that atlas should know all the flags for each driver to be able to generate the correct DSN.

[a8m]

Hey @ericyd, and thanks for suggesting how to improve atlas.
I actually agree with @crossworth, and not sure why a flag is preferred over a query param?

btw @crossworth, ent community members are always welcome here 😃

[ericyd]

Hello 👋🏻

Thanks for the replies.

While this tool is built with Go, I'm not compelled by the argument that the CLI usage should be beholden to the standards in the Go community. Atlas is distributed as a cross-platform binary, not a library.

I have personally never had to specify ?sslmode=disable when connecting to a database - ever. This goes for ORMs as well as SQL clients like TablesPlus or even just the CLI psql. The default that I'm most familiar with is sslmode=preferred, not required. I respect and understand that this is not the standard in the underlying library used by Atlas, and I understand why required would be a safer default setting from a library perspective. But I think as Atlas grows in popularity it will be peculiar to have people always add a query param instead of a flag in the CLI.

This very much just my own opinion. Ultimately, the decision lies with the project maintainers and whatever you decide is good by me. I agree adding a query param is very low cost to the end user so it's really not a huge deal if this is not implemented.

If you decide not to implement this, I would recommend adding a note to the Getting Started documentation that the default SSL mode is required (though I realize that can also be a slippery slope to note all possible edge cases 😄)

Edit:

If you decide not to implement a flag, the docs should at least be updated. None of the example Postgres URLs have ?sslmode=disable, which makes the docs feel like something is broken (note that I opened this issue as a bug before I heard other opinions). Examples here and here

Here's an example demonstrating how the docs are misleading

Start Postgres container:

docker run --rm --name atlas-db -p 5432:5432 -e POSTGRES_PASSWORD=pass -e POSTGRES_DB=example -e POSTGRES_USER=postgres postgres:12.9-alpine

Connect to DB with psql

psql postgres://postgres:pass@localhost:5432/example
✅ success

Inspect schema with Atlas

atlas schema inspect -d postgres://postgres:pass@localhost:5432/example
❌ error

[rotemtam]

@ericyd I agree this should be better documented. Do you want to contribute a patch to the docs?

Otherwise, I'll do it this week.

[ericyd]

I'll give it a shot. It might be a couple days before I can get to this though -- if you get to it first that is OK by me

[ericyd]

I'm looking into this more and realizing I don't fully understand the issue.

Why is it that Postgres requires disabling SSL, but MariaDB and MySQL handle it just fine? I don't see anything indicating special SSL keys/certs for the MySQL/MariaDB connections. It's easy enough to say "add ?sslmode=disable" but I don't really like recommending that without understanding the reason that it's different only for Postgres

[ericyd]

I see now -- it is literally just a discrepancy in feature of the libpq implementation

https://github.com/lib/pq/blob/8446d16b8935fdf2b5c0fe333538ac395e3e1e4b/ssl.go#L19-L20

I'll submit a PR for some doc updates and more discussion can happen there if desired

[krilor]

I just now took atlas for a spin for the first time. Using postgres in a local docker container. This SSL error message was the only hiccup/bump in the road. +1 for adding it to the docs so that it is less likely that others experience the same.

Excited to see what this tool can do for my workflow :)

[rotemtam]

@krilor,

thanks for the feedback @krilor, and welcome!
you can find the maintainers and a bunch of people from the community hanging out in our Discord Server

@ericyd - yep you nailed it. I want to add a bit on the design choice to delegate the parsing of DSN to the drivers.
I'm a big fan of orthogonal design - that is - striving to build components in a decoupled way such that you can change one without the other. Keeping the cli oblivious to the minutiae of each driver makes it much safer and easier to change. The only contract between the CLI and the underlying sql.DB implementations is that a string is passed, and an error is returned if something is wrong.

I agree that from the user perspective it creates a "burden" to figure out the correct connection string per driver. However, I'd say that it generally would happen very little (perhaps a handful of time per team that's working with Atlas) and can be solved with better documentation as you've advised.

Finally, thanks for these discussions - your clear writing style and attention to detail is very valuable, these discussions say alive forever and tend to have great SEO - so many will read them in the future 🙏