livebook-dev / kino_db

Database integrations for Livebook
Apache License 2.0
40 stars 16 forks source link

Add SSL support to connection cell #60

Closed msmithstubbs closed 1 year ago

msmithstubbs commented 1 year ago

This PR adds toggle to enable SSL connections for Postgres and MySQL

CleanShot 2023-08-29 at 15 53 33@2x

If the toggle is enabled ssl: true is passed to the database connection options.

Please provide any feedback, in particular on these areas:

josevalim commented 1 year ago

Thank you @msmithstubbs!

@wojtekmach, is this enough? Also, maybe myxql and postgrex should be the ones starting ssl?

wojtekmach commented 1 year ago

MyXQL adds :ssl app: https://github.com/elixir-ecto/myxql/blob/v0.6.3/mix.exs#L24

but Postgrex does not: https://github.com/elixir-ecto/postgrex/blob/v0.17.2/mix.exs#L24

I think we should add :ssl app to Postgrex which I'm happy to do tomorrow. Then we don't need to add ensure_all_started(:ssl) nor the user have to do anything. I believe if in Postgrex we set ssl: true but people don't have the app, we print a warning to help them out which we could just remove.

setting just ssl: true might not be enough for using hosted DBs like PlanetScale and possible some others. We have a long standing issue: https://github.com/elixir-ecto/myxql/issues/61#issuecomment-900131048. KinoDB as a low-code/no-code solution should just work for these. We might have to add :cacerts or require OTP 25 (if we aren't already) which has :public_key.cacerts_get(). This change may have to happen in both mysql and Postgrex. Or just in KinoDB. After we improve the ssl story for cloud services in my opinion we can ship 1.0 for both MyXQL and Postgrex.

josevalim commented 1 year ago

@msmithstubbs can you please remove the :ssl.start bit? We will ship a new Postgrex and then merge it. :)

wojtekmach commented 1 year ago

@msmithstubbs Postrgex 0.17.3 is out with the :ssl application already included. Please make this change:

- {:postgrex, "~> 0.16.3 or ~> 0.17", optional: true},
+ {:postgrex, "~> 0.16.3 or ~> 0.17.3 or ~> 0.18", optional: true},

and then we will no longer need to manually start :ssl here!

msmithstubbs commented 1 year ago

Thank you @wojtekmach, all done.

josevalim commented 1 year ago

:green_heart: :blue_heart: :purple_heart: :yellow_heart: :heart: