(#1163) Initial support for sslmode #1177
Conversation
BradleyBoutcher
changed the title
BradleyBoutcher
force-pushed
the
1163-disable-sslmode
branch
from
749ede6
to
7021c4d
Compare
BradleyBoutcher
force-pushed
the
1163-disable-sslmode
branch
from
7021c4d
to
d9dca62
Compare
There was a problem hiding this comment.
@BradleyBoutcher One tiny nit comment from me since it seem like @doodlesbykumbi already covered my concerns too in hs review.
BradleyBoutcher
force-pushed
the
1163-disable-sslmode
branch
from
d9dca62
to
fdd08b3
Compare
BradleyBoutcher
force-pushed
the
1163-disable-sslmode
branch
11 times, most recently
from
b6c132c
to
4c2636a
Compare
| if credentials["sslmode"] == nil { | ||
| connDetails.SSLMode = "disable" | ||
| } else { | ||
| connDetails.SSLMode = string(credentials["sslmode"]) |
There was a problem hiding this comment.
Maybe this is related to the test comments below, but don't we currently only support "disable"? This feels like a reasonable spot to throw an error if a different value is set here
There was a problem hiding this comment.
I'm still unclear as to how we'd want to throw errors here, as we only have infrastructure for writing fake mssql errors to the client, and we have avoided throwing secretless specific errors. We discussed adding error handling to this exact method in the past, and decided against it at the time. I'm not against it, I just think it's out of scope for this story.
For now, I set a switch that only accepts "disable", and any other value falls through to the default, which is "disable" currently as well.
BradleyBoutcher
force-pushed
the
1163-disable-sslmode
branch
4 times, most recently
from
159e334
to
4281adf
Compare
Adds sslmode as a param for user configuration and injecting it into the DSN
sslmode support added to test configs for secretless and the associated integration tests
BradleyBoutcher
force-pushed
the
1163-disable-sslmode
branch
from
4281adf
to
1762b37
Compare
Currently, we only support disabled. A simple check was added to propogateParams to confirm that the 'disabled' parameter has been properly set on the server.
BradleyBoutcher
force-pushed
the
1163-disable-sslmode
branch
from
5bc273d
to
d4bf355
Compare
There was a problem hiding this comment.
LGTM! Great work.
I'd also like to highlight (for others) that we spoke briefly about
- In the next set of PRs in this epic we'll be cleaning up
propagateParamsfor the integration tests - Kicking off a discussion about when errors about configuration parameters like SSL should be reported to. And if the credentials section really is the best place for for these sort of static parameters.
What does this PR do (include background context, if relevant)?
Adds sslmode as a param for user configuration and injecting it
into the DSN
What ticket does this PR close?
Connected to #1163
Where should the reviewer start?
internal/plugin/connectors/tcp/mssql/connector.go#dataSourceName