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

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

Open
patsevanton opened this issue Oct 27, 2024 · 0 comments
Open

Comments

@patsevanton
Copy link
Contributor

patsevanton commented Oct 27, 2024

Title: Error Connecting to ClickHouse with SSL Port: Unexpected Packet from Server

Description:

When attempting to connect to ClickHouse using the SSL port (e.g., 9440), the Snuba project throws an error indicating an unexpected packet from the server. This issue occurs during the migration process when Snuba tries to execute a query to check the ClickHouse connection.

Steps to Reproduce:

  1. Set up Snuba with a ClickHouse instance configured to use SSL on port 9440.
  2. Run the migration command:
    snuba migrations migrate --force
  3. Observe the following error:
    snuba.clickhouse.errors.ClickhouseError: Unexpected packet from server clickhouse-server:9440 (expected Hello or Exception, got Unknown packet)
    

Expected Behavior:

Snuba should successfully connect to ClickHouse using the SSL port and proceed with the migration process.

Actual Behavior:

The following error is thrown:

snuba.clickhouse.errors.ClickhouseError: Unexpected packet from server clickhouse-server:9440 (expected Hello or Exception, got Unknown packet)

Suggested Fix:

It appears that Snuba might not be handling the SSL connection properly, leading to the unexpected packet error. A potential fix could involve ensuring that the SSL configuration is correctly passed to the ClickHouse client and that the client is properly initialized for SSL connections.

Related PR:

Logs:

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/usr/local/bin/snuba", line 33, in <module>
    sys.exit(load_entry_point('snuba', 'console_scripts', 'snuba')())
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
         ^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.11/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/src/snuba/snuba/cli/migrations.py", line 107, in migrate
    check_clickhouse_connections(clusters_to_check)
  File "/usr/src/snuba/snuba/migrations/connect.py", line 90, in check_clickhouse_connections
    check_clickhouse(clickhouse)
  File "/usr/src/snuba/snuba/migrations/connect.py", line 112, in check_clickhouse
    ver = clickhouse.execute("SELECT version()").results[0][0]
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/src/snuba/snuba/clickhouse/native.py", line 285, in execute
    raise ClickhouseError(e.message, code=e.code) from e
snuba.clickhouse.errors.ClickhouseError: Unexpected packet from server clickhouse-server:9440 (expected Hello or Exception, got Unknown packet)
Error in install/bootstrap-snuba.sh:4.
'$dcr snuba-api migrations migrate --force' exited with status 1
-> ./install.sh:main:35
--> install/bootstrap-snuba.sh:source:4

install/error-handling.sh: line 82: /usr/bin/docker: Argument list too long
@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Oct 27, 2024
untitaker added a commit that referenced this issue 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 <[email protected]>
Co-authored-by: Markus Unterwaditzer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Waiting for: Product Owner
Development

No branches or pull requests

1 participant