electric icon indicating copy to clipboard operation
electric copied to clipboard

fix(electric): Fix the order of case clauses when parsing database SSL config

Open alco opened this issue 9 months ago • 4 comments

@icehaunter spotted this in https://github.com/electric-sql/electric/pull/1249#discussion_r1595304797.

I have reordered the case clauses by specificity and added comments for readability.

alco avatar May 14 '24 12:05 alco

This is a breaking change because it switches the default from false to true on DATABASE_REQUIRE_SSL. This matches the docs though. This is specified in the issue VAX-1839. So coordinate with @samwillis and @balegas on this

icehaunter avatar May 14 '24 12:05 icehaunter

When the default value of DATABASE_REQUIRE_SSL was set to true in https://github.com/electric-sql/electric/pull/767 (2023-12-22), it was implemented correctly. The later change that accidentally flipped the default (https://github.com/electric-sql/electric/pull/848) was made on 2024-01-18.

I would argue this is a bug fix, more than a breaking change, but still worth mentioning in the Release Notes.

alco avatar May 14 '24 13:05 alco

Corrections:

  • the original enforcement of SSL connections by default was released in v0.9.0 on 2024-01-18.
  • the "fix" that added support for sslmode and accidentally flipped the default value was released in v0.9.1 on 2024-01-24.

alco avatar May 14 '24 14:05 alco

I agree it should be should to true.

samwillis avatar May 14 '24 14:05 samwillis

I have prepared a draft of release notes if we release this change as part of a patch release.

alco avatar May 24 '24 09:05 alco

Seems like everyone agrees on this change (I see Valter acked on Linear too) so I think we can get this merged? (I've had deploy issues/confusion as well because of this so making sure we get it merged!)

msfstef avatar May 30 '24 13:05 msfstef