Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fully parse secure and skip_verify DSN params #862

Merged
merged 1 commit into from
Dec 23, 2022
Merged

Conversation

n-oden
Copy link
Contributor

@n-oden n-oden commented Dec 22, 2022

The current code will interpret a DSN with ?secure=false in its options as a directive to turn TSL on!

Obviously re-setting what is the default value is redundant, but deployment systems that template the DSN from e.g. environment variables may need to do this. (Also, the v1 clickhouse-go library supported this pattern, so this is technically a regression.)

The previous behavior (?secure without a value set turns secure on) is preserved to minimize surprise.

Signed-off-by: Nathan J. Mehl n@oden.io

The current code will interpret a DSN with `?secure=false` in its
options as a directive to turn TSL _on!_

Obviously re-setting what is the default value is redundant, but
deployment systems that template the DSN from e.g. environment variables
may need to do this.

The previous behavior (`?secure` without a value set turns secure on) is
preserved to minimize surprise.

Signed-off-by: Nathan J. Mehl <n@oden.io>
@n-oden
Copy link
Contributor Author

n-oden commented Dec 23, 2022

AFAICT the integration test failures are unrelated to this PR?

@jkaflik
Copy link
Contributor

jkaflik commented Dec 23, 2022

AFAICT the integration test failures are unrelated to this PR?

@n-oden yes. your fork is not able to access secrets. it's fine.

Thanks for this contribution. Really appreciated. I'm going to include it in an upcoming release.

@jkaflik jkaflik merged commit 12d1c31 into ClickHouse:main Dec 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants