Revert "Add params for secure ClickHouse connections." #2033

evanh · 2021-07-29T21:20:36Z

Reverts #2018

This reverts commit 39cdb11.

codecov-commenter · 2021-07-29T21:24:12Z

Codecov Report

Merging #2033 (a700917) into master (39cdb11) will increase coverage by 0.16%.
The diff coverage is 100.00%.

❗ Current head a700917 differs from pull request most recent head 1169fa0. Consider uploading reports for the commit 1169fa0 to get more accurate results

@@            Coverage Diff             @@
##           master    #2033      +/-   ##
==========================================
+ Coverage   90.93%   91.10%   +0.16%     
==========================================
  Files         499      499              
  Lines       21576    21564      -12     
==========================================
+ Hits        19621    19646      +25     
+ Misses       1955     1918      -37

Impacted Files	Coverage Δ
snuba/cli/cleanup.py	`0.00% <ø> (ø)`
snuba/cli/migrations.py	`0.00% <ø> (ø)`
snuba/cli/optimize.py	`0.00% <ø> (ø)`
snuba/clickhouse/native.py	`73.04% <ø> (-0.47%)`	⬇️
snuba/clusters/cluster.py	`93.63% <ø> (-1.90%)`	⬇️
snuba/migrations/runner.py	`91.66% <ø> (ø)`
snuba/settings/__init__.py	`92.50% <ø> (ø)`
snuba/settings/settings_distributed.py	`0.00% <ø> (-100.00%)`	⬇️
tests/clusters/test_cluster.py	`100.00% <ø> (ø)`
tests/migrations/test_table_engines.py	`100.00% <ø> (ø)`
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39cdb11...1169fa0. Read the comment docs.

katsil · 2021-08-20T10:02:45Z

Hi! Can you tell me why this issue reverted, i guess cliclhouse secure connection is very important issue

katsil · 2021-08-20T10:02:59Z

@fpacifici @evanh

fpacifici · 2021-08-20T23:21:58Z

Hi, sorry this was reverted because of a push safety issue with the original PR that caused a production issue when deployed.
The original PR is still desirable and should be fixed and re-issued. @evanh did you have a chance to nail down the root cause ?

katsil · 2021-08-23T12:40:59Z

@evanh hi, im really waiting this feautre asap because we are trying to launch sentry 20 in production in destributed mode and DBaas databases (clickhouse) which supports only ssl/tls mode..

evanh · 2021-08-23T13:58:27Z

When we deployed this it caused Snuba to go into a crashloop and I haven't had time to spin up an environment to try and debug why. @katsil Did you test your changes outside of the tests included in Snuba?

katsil · 2021-08-23T17:13:45Z

@evanh i dont even tryed to run tests - just insert all changes inside snuba code, build docker image and push them to private registry, then got SSL CA CERT ERRORs..

Like:

2021-08-23 09:41:35,706 Failed to connect to my-ch-db-host:8443
Traceback (most recent call last):
  File "/usr/local/lib/python3.8/dist-packages/clickhouse_driver/connection.py", line 260, in connect
    return self._init_connection(host, port)
  File "/usr/local/lib/python3.8/dist-packages/clickhouse_driver/connection.py", line 226, in _init_connection
    self.socket = self._create_socket(host, port)
  File "/usr/local/lib/python3.8/dist-packages/clickhouse_driver/connection.py", line 221, in _create_socket
    raise err
  File "/usr/local/lib/python3.8/dist-packages/clickhouse_driver/connection.py", line 212, in _create_socket
    sock.connect(sa)
  File "/usr/lib/python3.8/ssl.py", line 1342, in connect
    self._real_connect(addr, False)
  File "/usr/lib/python3.8/ssl.py", line 1333, in _real_connect
    self.do_handshake()
  File "/usr/lib/python3.8/ssl.py", line 1309, in do_handshake
    self._sslobj.do_handshake()
ssl.SSLCertVerificationError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: self signed certificate in certificate chain (_ssl.c:1108)

clickhouse_driver.errors.NetworkError: Code: 210. [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: self signed certificate in certificate chain (_ssl.c:1108) (sas-wfwqv2yebnxbtfrx.db.yandex.net:8443)

Can you tell me on which port i should connect - clickhouse own native port (9000/9440 for tls) or http/https clickhouse port (8123 or 8443 for https) *in my setup only 8443/9440 ports are available

katsil · 2021-08-25T08:12:53Z

@evanh hi, maybe any news? or we can reopen issue, i guess supporting clickhouse ssl is very important feature for snuba

evanh · 2021-08-25T17:07:00Z

So looking at the error, it looks to be because the certificate is self signed and Clickhouse has to be configured correctly to support self signed certificates. https://clickhouse.tech/docs/en/operations/server-configuration-parameters/settings/#server_configuration_parameters-openssl

Can you connect directly to Clickhouse with your cert? Without going through Snuba?

As for the port, that is also configured explicitly in Clickhouse. https://clickhouse.tech/docs/en/operations/server-configuration-parameters/settings/#http-porthttps-port

If you can connect correctly to Clickhouse, then I would try adding your changes back to Snuba, and testing that you can connect and run queries in Snuba with HTTPS.

katsil · 2021-08-30T14:17:26Z

@evanh If i run multiple snuba instances with multiple crons gosu snuba snuba cleanup --storage transactions --dry-run False is there any native snuba locks (if multiple cronjobs will run simultaneously?)

evanh · 2021-09-01T18:50:19Z

@katsil There aren't any internal locks I'm aware of. I'm not familiar with running Snuba in this way but the only contention I could think of is that all of the instances will be listening for connections on the same port.

konstantin-popov · 2021-09-13T17:21:13Z

Hi @evanh, actually I successfully connected to ClickHouse using SSL. May be you can somehow debug the issue on your deploy?
@katsil, I guess your problem is in client-side cert validation error (disliking self-signed certificate), not server-side.

evanh · 2021-09-15T17:42:15Z

@konstantin-popov Sorry are you saying you tested your SSL change in Snuba and were able to successfully run tests with SSL enabled?

konstantin-popov · 2021-09-16T09:41:50Z

No, I didn't change testing environment to use SSL for ClickHouse connections (it would require some configuration effort to create certificates for CH and I thought it was not the thing worth testing).
What I meant is that I was able to start Snuba instances working with ClickHouse using SSL and it appears to function ok.
Is there a way I can help you in debugging the issue?

patsevanton · 2024-10-27T09:10:34Z

Has anyone had any luck connecting snuba to clickhouse over SSL? How did you do it (besides fixing the source code) ? Have you tried adding a certificate to the python certifi package ?

@konstantin-popov

### Description This pull request introduces SSL/TLS support for ClickHouse connections in the Snuba project. The changes include new CLI options for enabling secure connections, updates to the ClickhousePool and HTTPBatchWriter classes, and corresponding configuration options in settings and tests. ### Changes Overview 1. **CLI Options**: - Introduced new CLI options for enabling secure connections to ClickHouse: - `--clickhouse-secure`: If true, an encrypted connection will be used. - `--clickhouse-ca-certs`: An optional path to certificates directory. - `--clickhouse-verify`: Verify ClickHouse SSL cert. 2. **Class Updates**: - Modified `ClickhousePool`, `HTTPBatchWriter`, and other relevant classes to support SSL/TLS connections. - Updated constructors and methods to accept and handle SSL/TLS parameters. 3. **Configuration**: - Added SSL/TLS configuration options in settings and tests. - Updated environment variables to support SSL/TLS settings. 4. **Testing**: - Included SSL/TLS configuration in test cases. - Updated existing tests to ensure compatibility with SSL/TLS options. ### Detailed Changes - **snuba/cli/cleanup.py**: Added new CLI options for secure ClickHouse connections. - **snuba/cli/migrations.py**: Added new CLI options for secure ClickHouse connections. - **snuba/cli/optimize.py**: Added new CLI options for secure ClickHouse connections. - **snuba/clickhouse/http.py**: Modified `HTTPBatchWriter` to support SSL/TLS connections. - **snuba/clickhouse/native.py**: Updated `ClickhousePool` to handle SSL/TLS parameters. - **snuba/clusters/cluster.py**: Updated `ClickhouseCluster` to include SSL/TLS configuration. - **snuba/migrations/runner.py**: Added SSL/TLS parameters to migration runner. - **snuba/settings/__init__.py**: Added SSL/TLS configuration options. - **snuba/settings/settings_distributed.py**: Added SSL/TLS configuration options. - **tests/clusters/fake_cluster.py**: Updated `FakeClickhouseCluster` to include SSL/TLS parameters. - **tests/clusters/test_cluster.py**: Updated tests to include SSL/TLS configuration. - **tests/conftest.py**: Updated test setup to include SSL/TLS configuration. - **tests/migrations/test_connect.py**: Updated tests to include SSL/TLS configuration. - **tests/migrations/test_table_engines.py**: Updated tests to include SSL/TLS configuration. - **tests/replacer/test_cluster_replacements.py**: Updated tests to include SSL/TLS configuration. ## Related Issues - #6458 ## Related Pull Requests: - #2018 - #2033 ### Additional Notes - This change is backward compatible and does not require any additional setup for users who do not wish to enable SSL/TLS. - Please review the changes carefully to ensure that the SSL/TLS implementation is secure and efficient. FYI @konstantin-popov Thank you for reviewing this pull request! ### Legal Boilerplate Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms. --------- Co-authored-by: Markus Unterwaditzer <markus-honeypot@unterwaditzer.net> Co-authored-by: Markus Unterwaditzer <markus-tarpit+git@unterwaditzer.net>

Revert "Add params for secure ClickHouse connections (#2018)"

1169fa0

This reverts commit 39cdb11.

evanh requested a review from a team as a code owner July 29, 2021 21:20

fpacifici approved these changes Jul 29, 2021

View reviewed changes

evanh merged commit 3317622 into master Jul 29, 2021

evanh deleted the revert-2018-ch_secure_connection branch July 29, 2021 21:33

shilovmaksim mentioned this pull request Aug 9, 2022

Add support to connect to external Clickhouse database using secure connection #3035

Closed

williamdes mentioned this pull request Apr 4, 2024

Add support for new auth ENVs for ClickHouse auth #5722

Closed

This was referenced Oct 27, 2024

Connecting to ClickHouse with SSL Port: Unexpected Packet from Server #6458

Open

feat: Add SSL/TLS support for ClickHouse connections #6459

Merged

untitaker mentioned this pull request Dec 13, 2024

feat: Secure clickhouse connections #6575

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert "Add params for secure ClickHouse connections." #2033

Revert "Add params for secure ClickHouse connections." #2033

evanh commented Jul 29, 2021

codecov-commenter commented Jul 29, 2021

katsil commented Aug 20, 2021

katsil commented Aug 20, 2021

fpacifici commented Aug 20, 2021

katsil commented Aug 23, 2021

evanh commented Aug 23, 2021

katsil commented Aug 23, 2021

katsil commented Aug 25, 2021

evanh commented Aug 25, 2021

katsil commented Aug 30, 2021

evanh commented Sep 1, 2021 •

edited

Loading

konstantin-popov commented Sep 13, 2021

evanh commented Sep 15, 2021

konstantin-popov commented Sep 16, 2021

patsevanton commented Oct 27, 2024 •

edited

Loading

Revert "Add params for secure ClickHouse connections." #2033

Revert "Add params for secure ClickHouse connections." #2033

Conversation

evanh commented Jul 29, 2021

codecov-commenter commented Jul 29, 2021

Codecov Report

katsil commented Aug 20, 2021

katsil commented Aug 20, 2021

fpacifici commented Aug 20, 2021

katsil commented Aug 23, 2021

evanh commented Aug 23, 2021

katsil commented Aug 23, 2021

katsil commented Aug 25, 2021

evanh commented Aug 25, 2021

katsil commented Aug 30, 2021

evanh commented Sep 1, 2021 • edited Loading

konstantin-popov commented Sep 13, 2021

evanh commented Sep 15, 2021

konstantin-popov commented Sep 16, 2021

patsevanton commented Oct 27, 2024 • edited Loading

evanh commented Sep 1, 2021 •

edited

Loading

patsevanton commented Oct 27, 2024 •

edited

Loading