Skip to content

Conversation

@konstantin-popov
Copy link
Contributor

Support secure connections to ClickHouse (both native and http).

@konstantin-popov konstantin-popov requested a review from a team as a code owner July 21, 2021 12:46
@evanh
Copy link
Member

evanh commented Jul 21, 2021

/gcbrun

@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2021

Codecov Report

Merging #2018 (a0bb174) into master (24b75fb) will increase coverage by 0.04%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2018      +/-   ##
==========================================
+ Coverage   90.95%   90.99%   +0.04%     
==========================================
  Files         499      499              
  Lines       21560    21572      +12     
==========================================
+ Hits        19609    19629      +20     
+ Misses       1951     1943       -8     
Impacted Files Coverage Δ
snuba/cli/cleanup.py 0.00% <0.00%> (ø)
snuba/cli/migrations.py 0.00% <0.00%> (ø)
snuba/cli/optimize.py 0.00% <0.00%> (ø)
snuba/migrations/runner.py 91.66% <ø> (ø)
snuba/settings/__init__.py 92.50% <ø> (ø)
snuba/settings/settings_distributed.py 100.00% <ø> (ø)
tests/clusters/test_cluster.py 100.00% <ø> (ø)
tests/migrations/test_table_engines.py 100.00% <ø> (ø)
tests/test_split.py 97.26% <ø> (ø)
snuba/clickhouse/http.py 92.92% <75.00%> (-0.78%) ⬇️
... and 5 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 f554042...a0bb174. Read the comment docs.

@evanh
Copy link
Member

evanh commented Jul 21, 2021

/gcbrun

@evanh
Copy link
Member

evanh commented Jul 28, 2021

/gcbrun

@evanh
Copy link
Member

evanh commented Jul 28, 2021

/gcbrun

Copy link
Member

@evanh evanh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry this took so long to merge.

@evanh
Copy link
Member

evanh commented Jul 29, 2021

/gcbrun

@evanh evanh merged commit 39cdb11 into getsentry:master Jul 29, 2021
@konstantin-popov konstantin-popov deleted the ch_secure_connection branch July 29, 2021 19:40
evanh added a commit that referenced this pull request Jul 29, 2021
evanh added a commit that referenced this pull request Jul 29, 2021
untitaker added a commit that referenced this pull request Jan 14, 2025
### 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>
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.

3 participants