SSH interface with PTY by InfJoker · Pull Request #48902 · ClickHouse/ClickHouse

InfJoker · 2023-04-18T14:28:18Z

Changelog category (leave one):

New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

SSH interface has been added. It is now possible to connect to the ClickHouse server using any SSH client and execute queries in an interactive fashion using PTY with an embedded clickhouse-client.

Documentation entry for user-facing changes

Documentation is written (mandatory for new features)

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

CI Settings (Only check the boxes if you know what you are doing)

All builds in Builds_1 and Builds_2 stages are always mandatory
and will run independently of the checks below:

Allow: All Required Checks
Allow: Stateless tests
Allow: Stateful tests
Allow: Integration Tests
Allow: Performance tests
Allow: All Builds
Allow: batch 1, 2 for multi-batch jobs
Allow: batch 3, 4, 5, 6 for multi-batch jobs

Run only fuzzers related jobs (libFuzzer fuzzers, AST fuzzers, etc.)
Exclude: AST fuzzers

robot-ch-test-poll · 2023-05-07T15:50:36Z

This is an automated comment for commit 32d1c06 with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

Check name	Description	Status
Integration tests	The integration tests report. In parenthesis the package type is given, and in square brackets are the optional part/total tests	❌ failure
Stateless tests	Runs stateless functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	❌ failure

Successful checks

Check name	Description	Status
AST fuzzer	Runs randomly generated queries to catch program errors. The build type is optionally given in parenthesis. If it fails, ask a maintainer for help	✅ success
Builds	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
BuzzHouse (asan)	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
BuzzHouse (debug)	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
BuzzHouse (msan)	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
BuzzHouse (tsan)	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
BuzzHouse (ubsan)	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
ClickBench	Runs ClickBench with instant-attach table	✅ success
Compatibility check	Checks that clickhouse binary runs on distributions with old libc versions. If it fails, ask a maintainer for help	✅ success
Docker keeper image	The check to build and optionally push the mentioned image to docker hub	✅ success
Docker server image	The check to build and optionally push the mentioned image to docker hub	✅ success
Docs check	Builds and tests the documentation	✅ success
Fast test	Normally this is the first check that is ran for a PR. It builds ClickHouse and runs most of stateless functional tests, omitting some. If it fails, further checks are not started until it is fixed. Look at the report to see which tests fail, then reproduce the failure locally as described here	✅ success
Flaky tests	Checks if new added or modified tests are flaky by running them repeatedly, in parallel, with more randomization. Functional tests are run 100 times with address sanitizer, and additional randomization of thread scheduling. Integration tests are run up to 10 times. If at least once a new test has failed, or was too long, this check will be red. We don't allow flaky tests, read the doc	✅ success
Install packages	Checks that the built packages are installable in a clear environment	✅ success
Performance Comparison	Measure changes in query performance. The performance test report is described in detail here. In square brackets are the optional part/total tests	✅ success
Stateful tests	Runs stateful functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	✅ success
Stress test	Runs stateless functional tests concurrently from several clients to detect concurrency-related errors	✅ success
Style check	Runs a set of checks to keep the code style clean. If some of tests failed, see the related log from the report	✅ success
Unit tests	Runs the unit tests for different release types	✅ success
Upgrade check	Runs stress tests on server version from last release and then tries to upgrade it to the version from the PR. It checks if the new server can successfully startup without any errors, crashes or sanitizer asserts	✅ success

antonio2368

A lot of changes!
there are a lot of comments but the majority of them are just code-style comments and general C++ rules for our repo.

Please go through https://clickhouse.com/docs/en/development/style and apply fixes where necessary.

programs/client/Client.cpp

programs/local/LocalServer.cpp

programs/server/Server.cpp

src/Server/SSH/SSHBind.cpp

src/Server/SSH/SSHBind.h

src/Server/SSH/SSHChannel.h

src/Server/SSH/SSHPtyHandler.cpp

InfJoker · 2024-04-20T14:59:17Z

src/Client/ReplxxLineReader.cpp

+    int out_fd_,
+    int err_fd_)
+    : LineReader(history_file_path_, multiline_, std::move(extenders_), std::move(delimiters_), input_stream_, output_stream_, in_fd_)
+    , rx(input_stream_, output_stream_, in_fd_, out_fd_, err_fd_)


this will not work untill replxx is patched

nikitamikhaylov · 2025-01-07T13:53:51Z

I see that 01700_deltasum is broken in master along with test_memory_limit/test.py::test_multiple_queries.

The TSAN failure looks related. Looking.

nikitamikhaylov · 2025-01-08T10:50:19Z

Stateless tests (asan) - #74287

nikitamikhaylov · 2025-01-08T20:58:43Z

So, another iteration:

test_async_insert_adaptive_busy_timeout/test.py::test_compare_parallel_inserts_durations_for_adaptive_and_fixed_async_timeouts is super flaky in recent days.
02871_peak_threads_usage failed in another PR in a recent week.

Given that these changes tend to conflict much, I will override the Mergable Check. As an excuse I will take a look at all the flaky tests later.

InfJoker · 2025-02-02T04:04:51Z

src/Common/SSHWrapper.cpp

    return ssh_key_type_to_char(ssh_key_type(key));
 }

+void SSHKey::setNeedsDeallocation(bool needs_deallocation_)


SSH key is a duplicate of src/SSH/SSHPublicKey. Further refactoring should remove this

InfJoker · 2025-02-02T04:24:19Z

src/Server/SSH/SSHPtyHandlerFactory.h

+        LOG_TRACE(log, "TCP Request. Address: {}", socket.peerAddress().toString());
+        ::ssh::libsshLogger::initialize();
+        ::ssh::SSHSession session;
+        session.disableSocketOwning();


for this I remember writing a monkey test that will run session and abrupt session 100 times - then double free happens. It might have been deleted. This is important since when I was making a poc libssh owns socket and will free it out without this patch InfJoker/libssh-clickhouse-patch@11289d6

InfJoker · 2025-02-02T04:31:27Z

src/Server/ClientEmbedded/ClientEmbedded.h

+    /// NOP. The embedded client runs inside the server process which has its own signal handlers.
+    /// Thus we cannot override it in any way.
+    void setupSignalHandler() override {}


@nikitamikhaylov for this ClientBase has a method

ClickHouse/src/Client/ClientBase.h

Line 102 in e9c7f51

bool tryStopQuery() { return query_interrupt_handler.tryStop(); }

which is called by ClientApplicationBase on signal interrupt

ClickHouse/src/Client/ClientApplicationBase.cpp

Line 54 in e9c7f51

if (base->tryStopQuery())

.

But since we can't setup signal handlers in a single process with database -> we should process charecters in SSHPtyHandler and if Ctrl-C is intercepted -> stop current query by calling mentioned method in ClientBase.

Or the other way around it is to spawn client in different process and figure out a way to talk with db back and forth

robot-ch-test-poll4 added pr-feature Pull request with new product feature submodule changed At least one submodule changed in this PR. labels Apr 18, 2023

InfJoker force-pushed the ssh-protocol-with-pty branch from c02f767 to 572c17b Compare April 18, 2023 14:55

Avogar added the can be tested Allows running workflows for external contributors label Apr 18, 2023

InfJoker force-pushed the ssh-protocol-with-pty branch from 572c17b to 0af463d Compare May 7, 2023 15:49

InfJoker force-pushed the ssh-protocol-with-pty branch 4 times, most recently from e3c09d2 to 2d7ff44 Compare May 12, 2023 08:52

InfJoker force-pushed the ssh-protocol-with-pty branch 9 times, most recently from 888573b to d431854 Compare May 15, 2023 15:47

InfJoker changed the title ~~[WIP] Ssh protocol with pty~~ Ssh protocol with pty May 15, 2023

InfJoker marked this pull request as ready for review May 15, 2023 17:50

InfJoker force-pushed the ssh-protocol-with-pty branch 2 times, most recently from 4026e62 to 4f028f4 Compare May 31, 2023 09:06

This was referenced May 31, 2023

Requesting pty for a subsystem kovidgoyal/kitty#6318

Closed

Feature: Requesting pty for a subsystem cyd01/KiTTY#493

Open

antonio2368 self-assigned this Jun 10, 2023

InfJoker force-pushed the ssh-protocol-with-pty branch from 4f028f4 to 019f5f5 Compare June 12, 2023 20:37

antonio2368 reviewed Jun 14, 2023

View reviewed changes

InfJoker force-pushed the ssh-protocol-with-pty branch 2 times, most recently from 6ef2e11 to a675f2b Compare June 17, 2023 20:55

InfJoker commented Apr 20, 2024

View reviewed changes

nikitamikhaylov mentioned this pull request Jun 25, 2024

Miscellaneous and insignificant changes around Client/ClientBase. #65669

Merged

nikitamikhaylov mentioned this pull request Jul 15, 2024

Uninteresting change: introducing ClientApplicationBase #66549

Merged

19 tasks

nikitamikhaylov mentioned this pull request Aug 22, 2024

Bump Replxx to support custom descriptors #68730

Merged

21 tasks

alexey-milovidov mentioned this pull request Dec 31, 2024

Roadmap 2025 #74046

Closed

76 tasks

nikitamikhaylov approved these changes Jan 7, 2025

View reviewed changes

InfJoker and others added 19 commits January 8, 2025 11:53

SSH Authentication

dce8775

Build fixes

9296e71

Adjustments

ce86490

Fix style

ac9d03b

Fix file descriptors

e2b51df

Make SSH available only for Linux

f9d5bb5

Exclude openpty from the stylecheck

56bb8f3

Pass options in a generic way

b7c1104

Fix tests

52f92d8

Fix style

c8d95fc

Fixed settings and options propagation, added a test

8dd9cd5

Fix black

3fa565b

Added documentation

0c1a1b3

Added pseudoterminal to the dictionary

dc34586

Fix clang-tidy

a4f0ca0

Fix build and cleanup

e41b53f

More cleanup

4f170b3

Fix datarace

83ff368

Fix build

32d1c06

nikitamikhaylov mentioned this pull request Jan 8, 2025

[Umbrella] Move SSH protocol for server to Beta stage #74340

Closed

3 tasks

InfJoker commented Feb 2, 2025

View reviewed changes

Conversation

InfJoker commented Apr 18, 2023 • edited by nikitamikhaylov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Documentation entry for user-facing changes

CI Settings (Only check the boxes if you know what you are doing)

Uh oh!

robot-ch-test-poll commented May 7, 2023 • edited by robot-ch-test-poll4 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antonio2368 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

InfJoker Apr 20, 2024

Choose a reason for hiding this comment

Uh oh!

nikitamikhaylov commented Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikitamikhaylov commented Jan 8, 2025

Uh oh!

nikitamikhaylov commented Jan 8, 2025

Uh oh!

InfJoker Feb 2, 2025

Choose a reason for hiding this comment

Uh oh!

InfJoker Feb 2, 2025

Choose a reason for hiding this comment

Uh oh!

InfJoker Feb 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

InfJoker commented Apr 18, 2023 •

edited by nikitamikhaylov

Loading

robot-ch-test-poll commented May 7, 2023 •

edited by robot-ch-test-poll4

Loading

nikitamikhaylov commented Jan 7, 2025 •

edited

Loading

InfJoker Feb 2, 2025 •

edited

Loading