feat: Secure clickhouse connections #6575

untitaker · 2024-11-19T14:39:07Z

this pr is a mirror of #6459 due to issues with CI

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

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.
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.
Configuration:
- Added SSL/TLS configuration options in settings and tests.
- Updated environment variables to support SSL/TLS settings.
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

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

Related Pull Requests:

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.

…too" This reverts commit bbc3962.

untitaker · 2024-12-04T22:35:47Z

@patsevanton still failing: https://github.com/getsentry/snuba/actions/runs/12053427269/job/33609672900?pr=6575#step:5:2161

ignore the CI failures on your PR

patsevanton · 2024-12-05T05:50:01Z

@untitaker secure-clickhouse-connections-2 is not my branch. Can i change code in secure-clickhouse-connections-2 ?

untitaker · 2024-12-05T14:15:26Z

@patsevanton I am manually forwarding your commits to your branch, because we have an issue with CI that prevents third-party contributions from ever passing the testsuite.

The test failure you linked is not because your code is wrong, it's because the CI runs on your branch lack a permission that we are not able to grant right now. It will take us a lot of time to fix this, so right now I'm working around it manually by maintaining that other PR.

The test failure I linked occurs on both branches though. I suggest setting up the devenlopment environment locally, or trying to deploy your own docker build . of your branch to see.

patsevanton · 2024-12-07T09:16:04Z

@untitaker the command docker build . was completed successfully

and the command docker compose up failed with an error

snuba-api-1   | ERROR: ld.so: object '/usr/src/snuba/libjemalloc.so.2' from LD_PRELOAD cannot be preloaded (cannot open shared object file): ignored.

how do I run the merge request check locally?

untitaker · 2024-12-08T15:23:08Z

after building the image, you have to run it. I assume you have a snuba installation already where that is possible? right now it definitely crashes on first start, see https://github.com/getsentry/snuba/actions/runs/12053427269/job/33609672900?pr=6575#step:5:2161

patsevanton · 2024-12-12T04:32:05Z

@untitaker try commit 53a73be

codecov · 2024-12-12T15:19:36Z

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
746	1	745	3

View the top 1 failed tests by shortest run time

tests.clusters.test_cluster::test_sliced_cluster

Stack Traces | 0.204s run time

Traceback (most recent call last):
  File ".../tests/clusters/test_cluster.py", line 273, in test_sliced_cluster
    res_cluster = cluster.get_cluster(StorageSetKey.GENERIC_METRICS_DISTRIBUTIONS, 1)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../snuba/clusters/cluster.py", line 559, in get_cluster
    part_storage_set_cluster_map = _get_sliced_storage_set_cluster_map()
                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../snuba/clusters/cluster.py", line 530, in _get_sliced_storage_set_cluster_map
    ] = _build_sliced_cluster(cluster)
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../snuba/clusters/cluster.py", line 501, in _build_sliced_cluster
    secure=cluster["secure"],
           ~~~~~~~^^^^^^^^^^
KeyError: 'secure'

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

untitaker · 2024-12-12T15:57:13Z

@patsevanton that commit was already included, the test failure is from a different line.

patsevanton · 2024-12-12T23:50:32Z

All checks have passed!

untitaker · 2024-12-12T23:51:59Z

@patsevanton can you please ensure that the resulting docker image works for your usecase (and test it) before we merge? i'll get this reviewed this week

patsevanton · 2024-12-13T00:09:37Z

I need time. can you add a description to the pull request?

I will write how I will test:

git clone https://github.com/getsentry/snuba.git
cd snuba
git checkout secure-clickhouse-connections-2
docker build -t antonpatsev/secure-clickhouse-connections-2:1 .
docker push antonpatsev/secure-clickhouse-connections-2:1

git clone https://github.com/getsentry/self-hosted.git
cd self-hosted

Modify this section docker-compose.yml

x-snuba-defaults: &snuba_defaults
............................
  environment:
    SNUBA_SETTINGS: self_hosted
    CLICKHOUSE_HOST: rc1a-xxx.mdb.cloud.net
    CLICKHOUSE_PORT: 9440
    CLICKHOUSE_USER: sentry
    CLICKHOUSE_PASSWORD: your_password_here
    CLICKHOUSE_DATABASE: sentry
    CLICKHOUSE_HTTP_PORT: 8443
    CLICKHOUSE_SECURE: True
    CLICKHOUSE_CA_CERTS: "/usr/local/share/ca-certificates/RootCA.crt"
    CLICKHOUSE_VERIFY: True
............................
  volumes:
    - "./certificates:/usr/local/share/ca-certificates:ro"
services:

Modify this section .env

SNUBA_IMAGE=antonpatsev/secure-clickhouse-connections-2:3 # change code "secure": os.environ.get("CLICKHOUSE_SECURE", "0") == "1",

Run sentry

./install.sh

@untitaker Please change code:

"secure": os.environ.get("CLICKHOUSE_SECURE", "0") == "1",

to

"secure": os.environ.get("CLICKHOUSE_SECURE", False),

here and here

untitaker · 2024-12-23T12:48:28Z

Please change code:

I don't think this is a good idea. Then export CLICKHOUSE_SECURE=0 will also enable secure mode.

patsevanton · 2024-12-23T13:43:59Z

@untitaker if use "secure": os.environ.get("CLICKHOUSE_SECURE", "0") == "1", then get error.
I can reproduce and show error.

untitaker · 2024-12-23T13:44:44Z

What is the error message?

patsevanton · 2024-12-23T13:45:59Z

@untitaker i don`t remember. I try reproduce later.

patsevanton · 2024-12-24T05:17:53Z

@untitaker error with "secure": os.environ.get("CLICKHOUSE_SECURE", "0") == "1",

tested by image antonpatsev/secure-clickhouse-connections-2:1
{"module": "snuba.migrations.connect", "event": "Connection to Clickhouse cluster rc1a-xxxx.mdb.cloud.net:9440 failed (attempt 0)", "severity": "error", "exception": "Traceback (most recent call last):\n File \"/usr/src/snuba/snuba/clickhouse/native.py\", line 206, in execute\n result_data = query_execute()\n ^^^^^^^^^^^^^^^\n File \"/usr/src/snuba/snuba/clickhouse/native.py\", line 189, in query_execute\n return conn.execute( # type: ignore\n ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n File \"/usr/local/lib/python3.11/site-packages/clickhouse_driver/client.py\", line 361, in execute\n with self.disconnect_on_error(query, settings):\n File \"/usr/local/lib/python3.11/contextlib.py\", line 137, in __enter__\n return next(self.gen)\n ^^^^^^^^^^^^^^\n File \"/usr/local/lib/python3.11/site-packages/clickhouse_driver/client.py\", line 305, in disconnect_on_error\n self.establish_connection(settings)\n File \"/usr/local/lib/python3.11/site-packages/clickhouse_driver/client.py\", line 292, in establish_connection\n self.connection.force_connect()\n File \"/usr/local/lib/python3.11/site-packages/clickhouse_driver/connection.py\", line 254, in force_connect\n self.connect()\n File \"/usr/local/lib/python3.11/site-packages/clickhouse_driver/connection.py\", line 395, in connect\n return self._init_connection(host, port)\n ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n File \"/usr/local/lib/python3.11/site-packages/clickhouse_driver/connection.py\", line 339, in _init_connection\n self.receive_hello()\n File \"/usr/local/lib/python3.11/site-packages/clickhouse_driver/connection.py\", line 519, in receive_hello\n raise errors.UnexpectedPacketFromServerError(message)\nclickhouse_driver.errors.UnexpectedPacketFromServerError: Code: 102. Unexpected packet from server rc1a-xxxx.mdb.cloud.net:9440 (expected Hello or Exception, got Unknown packet)\n\nThe above exception was the direct cause of the following exception:\n\nTraceback (most recent call last):\n File \"/usr/src/snuba/snuba/migrations/connect.py\", line 90, in check_clickhouse_connections\n check_clickhouse(clickhouse)\n File \"/usr/src/snuba/snuba/migrations/connect.py\", line 112, in check_clickhouse\n ver = clickhouse.execute(\"SELECT version()\").results[0][0]\n ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n File \"/usr/src/snuba/snuba/clickhouse/native.py\", line 291, in execute\n raise ClickhouseError(e.message, code=e.code) from e\nsnuba.clickhouse.errors.ClickhouseError: Unexpected packet from server rc1a-xxxx.mdb.cloud.net:9440 (expected Hello or Exception, got Unknown packet)", "timestamp": "2024-12-24T05:16:22.488584Z"}

untitaker · 2025-01-02T13:22:53Z

@patsevanton then you're not setting the variable correctly. the code you want me to change to simply enables secure mode unconditionally, with no way to turn it off.

patsevanton · 2025-01-02T14:55:12Z

@untitaker I think the code 'secure': os.environ.get('CLICKHOUSE_SECURE', False), sets the secure parameter to connect to ClickHouse using the value from the CLICKHOUSE_SECURE environment variable. If the environment variable is not set, the default value of False is used.

untitaker · 2025-01-02T14:57:43Z

The return value of os.environ.get is a string, and even if that is explicitly set to "False" or "0" it will evaluate to true: bool("False") == True. This is the behavior I'm objecting to. But you are right that this doesn't explain the errors you are seeing

zhenyatsk · 2025-01-02T17:19:39Z

os.environ.get("CLICKHOUSE_SECURE", "False").lower() in ("true", "1")

if you want to support CLICKHOUSE_SECURE=True and CLICKHOUSE_SECURE=1

% CLICKHOUSE_SECURE=False python3 -c 'import os; print(os.environ.get("CLICKHOUSE_SECURE", "False").lower() in ("true", "1"))'
False
% CLICKHOUSE_SECURE=True python3 -c 'import os; print(os.environ.get("CLICKHOUSE_SECURE", "False").lower() in ("true", "1"))'
True
% CLICKHOUSE_SECURE=1 python3 -c 'import os; print(os.environ.get("CLICKHOUSE_SECURE", "False").lower() in ("true", "1"))'
True
% CLICKHOUSE_SECURE=0 python3 -c 'import os; print(os.environ.get("CLICKHOUSE_SECURE", "False").lower() in ("true", "1"))'
False

% CLICKHOUSE_SECURE=0 python3 -c 'import os; print(os.environ.get("CLICKHOUSE_SECURE", "0") == "1")'                      
False
% CLICKHOUSE_SECURE=1 python3 -c 'import os; print(os.environ.get("CLICKHOUSE_SECURE", "0") == "1")'
True
% CLICKHOUSE_SECURE=True python3 -c 'import os; print(os.environ.get("CLICKHOUSE_SECURE", "0") == "1")'
False
% CLICKHOUSE_SECURE=False python3 -c 'import os; print(os.environ.get("CLICKHOUSE_SECURE", "0") == "1")'
False

patsevanton and others added 9 commits October 27, 2024 21:03

feat: Add SSL/TLS support for ClickHouse connections

13afe6f

Merge branch 'master' into secure-ClickHouse-connections

e4f92ee

run black on python files

bf02b9b

fix some typing issues

bf7849e

Merge branch 'master' into secure-ClickHouse-connections

9cda824

feat: Add SSL/TLS support for ClickHouse connections

851fc53

feat: Add SSL/TLS support for ClickHouse connections

9463928

attempt to push docker image cache for external contributors too

bbc3962

Revert "attempt to push docker image cache for external contributors …

6a4bba0

…too" This reverts commit bbc3962.

untitaker mentioned this pull request Nov 19, 2024

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

Open

patsevanton and others added 3 commits November 20, 2024 11:06

Merge branch 'master' into secure-ClickHouse-connections

edec438

feat: Add SSL/TLS support for ClickHouse connections

53a73be

Merge branch 'master' into secure-ClickHouse-connections

ab494ef

more fixes

9600071

more fixes

b99c6f1

untitaker marked this pull request as ready for review December 12, 2024 23:51

untitaker requested a review from a team as a code owner December 12, 2024 23:51

untitaker changed the title ~~secure clickhouse connections 2~~ feat: Secure clickhouse connections Dec 12, 2024

untitaker mentioned this pull request Jan 7, 2025

fix(ci): Enable third-party contributions #6721

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Secure clickhouse connections #6575

feat: Secure clickhouse connections #6575

untitaker commented Nov 19, 2024 •

edited

Loading

untitaker commented Dec 4, 2024

patsevanton commented Dec 5, 2024

untitaker commented Dec 5, 2024

patsevanton commented Dec 7, 2024 •

edited

Loading

untitaker commented Dec 8, 2024

patsevanton commented Dec 12, 2024

codecov bot commented Dec 12, 2024

untitaker commented Dec 12, 2024

patsevanton commented Dec 12, 2024

untitaker commented Dec 12, 2024

patsevanton commented Dec 13, 2024 •

edited

Loading

untitaker commented Dec 23, 2024

patsevanton commented Dec 23, 2024

untitaker commented Dec 23, 2024

patsevanton commented Dec 23, 2024

patsevanton commented Dec 24, 2024 •

edited

Loading

untitaker commented Jan 2, 2025

patsevanton commented Jan 2, 2025

untitaker commented Jan 2, 2025

zhenyatsk commented Jan 2, 2025 •

edited

Loading

feat: Secure clickhouse connections #6575

Are you sure you want to change the base?

feat: Secure clickhouse connections #6575

Conversation

untitaker commented Nov 19, 2024 • edited Loading

Description

Changes Overview

Detailed Changes

Related Issues

Related Pull Requests:

Additional Notes

Legal Boilerplate

untitaker commented Dec 4, 2024

patsevanton commented Dec 5, 2024

untitaker commented Dec 5, 2024

patsevanton commented Dec 7, 2024 • edited Loading

untitaker commented Dec 8, 2024

patsevanton commented Dec 12, 2024

codecov bot commented Dec 12, 2024

❌ 1 Tests Failed:

untitaker commented Dec 12, 2024

patsevanton commented Dec 12, 2024

untitaker commented Dec 12, 2024

patsevanton commented Dec 13, 2024 • edited Loading

untitaker commented Dec 23, 2024

patsevanton commented Dec 23, 2024

untitaker commented Dec 23, 2024

patsevanton commented Dec 23, 2024

patsevanton commented Dec 24, 2024 • edited Loading

untitaker commented Jan 2, 2025

patsevanton commented Jan 2, 2025

untitaker commented Jan 2, 2025

zhenyatsk commented Jan 2, 2025 • edited Loading

untitaker commented Nov 19, 2024 •

edited

Loading

patsevanton commented Dec 7, 2024 •

edited

Loading

patsevanton commented Dec 13, 2024 •

edited

Loading

patsevanton commented Dec 24, 2024 •

edited

Loading

zhenyatsk commented Jan 2, 2025 •

edited

Loading