microsoft / SQLServerPSModule

This repo is the home of SQL Server PowerShell Module development.
MIT License
45 stars 1 forks source link

Migrate from MDS4.x to MDS5.x - Invoke-SqlCmd should expose the new Encrypt option (Strict/Optional/Mandatory) #12

Closed Matteo-T closed 1 year ago

Matteo-T commented 1 year ago

We need to move forward and also support existing cmdlet (Read-SqlXEvent) which moved already to MDS4.x

Long term, we want to move to MDS 5.x - and bite the bullet around the new defaults for TrustServerCertificate and the new Encrypt options (Strict/Optional/Mandatory) to support TDS8.0 and such.

It would be great to have it by the time v22 gets out of Previews.

Matteo-T commented 1 year ago

Current thinking:

Some notes:

Referto https://learn.microsoft.com/en-us/sql/connect/ado-net/introduction-microsoft-data-sqlclient-namespace?view=sql-server-ver16#release-notes-for-microsoftdatasqlclient-50 for details.

erinstellato-ms commented 1 year ago

To do after discussion (Oct 28):

Matteo-T commented 1 year ago

@erinstellato-ms - I'll need the documentation (=description of the parameters and, possibly examples). Ideally, aligned to the same stuff that may become the same documentation around all these MDS5 stuff.

FYI, the module (as of last night) switched over to MDS 5.0.1 in the main branch; extensive testing will start probably this weekend, when I hope I can implement this work item.

Matteo-T commented 1 year ago

@erinstellato-ms - I changed my mind a little around the Make the -EncryptConnection switch a no-op part.

It is still deprecated, but making it a full no-op could break a scenario, i.e. where a user had specified -EncryptConnection:$false.

So, here's the new proposed mapping:

  1. -EncryptConnection or --EncryptConnection:$true ==> -Encrypt Mandatory
  2. -EncryptConnection:$false ==> '-Encrypt Optional'
  3. -EncryptConnection not passed at all ==> same as not passing -Encrypt
Matteo-T commented 1 year ago

@erinstellato-ms - I would like to discuss if we should have a default for -Encrypt, i.e. what to do when the user does not pass that parameter. Options:

  1. Make it the same as if the user passed -Encrypt Mandatory
  2. Do not set any value, thus accepting the default of the driver

1 has the advantage that things would be a little more predictable, should the driver decide to change its default behavior. 2 allows the driver to choose "what's best"

I'm currently leaning towards 2 right now.

@cheenamalhotra, FYI (and for your input)

erinstellato-ms commented 1 year ago

@erinstellato-ms - I changed my mind a little around the Make the -EncryptConnection switch a no-op part.

It is still deprecated, but making it a full no-op could break a scenario, i.e. where a user had specified -EncryptConnection:$false.

So, here's the new proposed mapping:

  1. -EncryptConnection or --EncryptConnection:$true ==> -Encrypt Mandatory
  2. -EncryptConnection:$false ==> '-Encrypt Optional'
  3. -EncryptConnection not passed at all ==> same as not passing -Encrypt

Understood the potential breaking scenario and agree with this change.

erinstellato-ms commented 1 year ago

@erinstellato-ms - I would like to discuss if we should have a default for -Encrypt, i.e. what to do when the user does not pass that parameter. Options:

  1. Make it the same as if the user passed -Encrypt Mandatory
  2. Do not set any value, thus accepting the default of the driver

1 has the advantage that things would be a little more predictable, should the driver decide to change its default behavior. 2 allows the driver to choose "what's best"

I'm currently leaning towards 2 right now.

@cheenamalhotra, FYI (and for your input)

I think it would be entirely unexpected for the default behavior to change, but I understand why you would prefer to make that the default behavior. I'm ok with that, pending input from @Cheenamalhot.

cheenamalhotra commented 1 year ago

I agree on keeping Encrypt default to driver's default (Mandatory or True).

Anyone specifying -EncryptConnection wouldn't need to anymore and stays unaffected, but the affected customers would be who don't specify it, so we certainly need a big announcement in Docs/Blog posts about changing the default behavior.

Matteo-T commented 1 year ago

This was checked in and is going to roll out with the next release of v22 (>22.0.30)