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

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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

patsevanton
Copy link

@patsevanton patsevanton commented Oct 27, 2024

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

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.

@patsevanton patsevanton requested a review from a team as a code owner October 27, 2024 15:04
@patsevanton patsevanton marked this pull request as draft October 27, 2024 15:08
@patsevanton patsevanton marked this pull request as ready for review October 28, 2024 02:52
@patsevanton
Copy link
Author

patsevanton commented Oct 28, 2024

docker compose up

[+] Running 3/0
 ✔ Container getsentry-snuba-fork-redis-1       Created                                                                                                  0.0s 
 ✔ Container getsentry-snuba-fork-snuba-api-1   Created                                                                                                  0.0s 
 ✔ Container getsentry-snuba-fork-clickhouse-1  Created                                                                                                  0.0s 
Attaching to clickhouse-1, redis-1, snuba-api-1
clickhouse-1  | Processing configuration file '/etc/clickhouse-server/config.xml'.
clickhouse-1  | Merging configuration file '/etc/clickhouse-server/config.d/docker_related_config.xml'.
clickhouse-1  | Logging trace to /var/log/clickhouse-server/clickhouse-server.log
clickhouse-1  | Logging errors to /var/log/clickhouse-server/clickhouse-server.err.log
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.
redis-1       | 1:C 28 Oct 2024 04:40:40.091 # oO0OoO0OoO0Oo Redis is starting oO0OoO0OoO0Oo
redis-1       | 1:C 28 Oct 2024 04:40:40.091 # Redis version=5.0.14, bits=64, commit=00000000, modified=0, pid=1, just started
redis-1       | 1:C 28 Oct 2024 04:40:40.091 # Warning: no config file specified, using the default config. In order to specify a config file use redis-server /path/to/redis.conf
redis-1       | 1:M 28 Oct 2024 04:40:40.092 * Running mode=standalone, port=6379.
redis-1       | 1:M 28 Oct 2024 04:40:40.092 # Server initialized
redis-1       | 1:M 28 Oct 2024 04:40:40.092 # WARNING overcommit_memory is set to 0! Background save may fail under low memory condition. To fix this issue add 'vm.overcommit_memory = 1' to /etc/sysctl.conf and then reboot or run the command 'sysctl vm.overcommit_memory=1' for this to take effect.
redis-1       | 1:M 28 Oct 2024 04:40:40.092 # WARNING you have Transparent Huge Pages (THP) support enabled in your kernel. This will create latency and memory usage issues with Redis. To fix this issue run the command 'echo never > /sys/kernel/mm/transparent_hugepage/enabled' as root, and add it to your /etc/rc.local in order to retain the setting after a reboot. Redis must be restarted after THP is disabled.
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.
redis-1       | 1:M 28 Oct 2024 04:40:40.092 * DB loaded from disk: 0.000 seconds
redis-1       | 1:M 28 Oct 2024 04:40:40.092 * Ready to accept connections
clickhouse-1  | Processing configuration file '/etc/clickhouse-server/config.xml'.
clickhouse-1  | Merging configuration file '/etc/clickhouse-server/config.d/docker_related_config.xml'.
clickhouse-1  | Saved preprocessed configuration to '/var/lib/clickhouse/preprocessed_configs/config.xml'.
clickhouse-1  | Processing configuration file '/etc/clickhouse-server/users.xml'.
clickhouse-1  | Saved preprocessed configuration to '/var/lib/clickhouse/preprocessed_configs/users.xml'.
snuba-api-1   | 2024-10-28 04:40:40,487 Initializing Snuba...
snuba-api-1   | 2024-10-28 04:40:41,834 Snuba initialization took 1.3474392070002068s
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.
snuba-api-1   | 2024-10-28 04:40:42,235 Initializing Snuba...
snuba-api-1   | 2024-10-28 04:40:43,194 Snuba initialization took 0.9595513919994119s
snuba-api-1   | Usage: snuba api [OPTIONS]
snuba-api-1   | Try 'snuba api --help' for help.
snuba-api-1   | 
snuba-api-1   | Error: No such option: --http Did you mean --help?
snuba-api-1 exited with code 2

@patsevanton
Copy link
Author

make test-distributed-migrations

SNUBA_IMAGE=snuba-test docker-compose --profile multi_node -f docker-compose.gcb.yml down
Define and run multi-container applications with Docker.

Usage:
  docker-compose [-f <arg>...] [options] [COMMAND] [ARGS...]
  docker-compose -h|--help

@untitaker
Copy link
Member

/gcbrun

@untitaker
Copy link
Member

@patsevanton hi, there are still some CI issues after trying to fix up formatting and typing

$ make backend-typing
mypy snuba tests scripts --strict --config-file mypy.ini --exclude 'tests/datasets|tests/query'
snuba/clusters/cluster.py:293: error: Too many arguments for "get_node_connection" of "ConnectionCache"  [call-arg]
tests/clusters/fake_cluster.py:82: error: Missing positional arguments "secure", "ca_certs", "verify" in call to "__init__" of "ClickhouseCluster"  [call-arg]
tests/replacer/test_load_balancer.py:99: error: Missing positional arguments "secure", "ca_certs", "verify" in call to "FakeClickhouseCluster"  [call-arg]
Found 3 errors in 3 files (checked 937 source files)
make: *** [backend-typing] Error 1

sorry for the delay on this.

@patsevanton
Copy link
Author

@untitaker fixed

@untitaker
Copy link
Member

well, that's dumb. apparently our CI workflow breaks on third-party contributions in a very hard to fix way. I'm gonna create another temp branch with your commits in a new PR

@untitaker
Copy link
Member

#6575

@untitaker
Copy link
Member

@patsevanton https://github.com/getsentry/snuba/actions/runs/11915367391/job/33206067046?pr=6575#step:7:2090

hope you have access to these logs: seems like launching the testsuite fails because clickhouse still can't connect

@patsevanton
Copy link
Author

@untitaker you can copy my latest commit 53a73be

@patsevanton
Copy link
Author

patsevanton commented Nov 20, 2024

@untitaker I see the failed tests. how can I fix them? (for example: self-hosted-end-to-end)

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