Skip to content

Don't try to authenticate twice when using ssh#93547

Merged
alexey-milovidov merged 11 commits intoClickHouse:masterfrom
spinojara:ssh-twice
Jan 19, 2026
Merged

Don't try to authenticate twice when using ssh#93547
alexey-milovidov merged 11 commits intoClickHouse:masterfrom
spinojara:ssh-twice

Conversation

@spinojara
Copy link
Copy Markdown
Contributor

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in an official stable release)

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

Fix a bug where clickhouse-client would ask for password twice when connecting using ssh.

@alexey-milovidov
Copy link
Copy Markdown
Member

Thanks! Could you please sign a CLA by the link above? Also, we need a test - it's possible to do with the .expect test type in tests/queries/0_stateless.

@alexey-milovidov alexey-milovidov self-assigned this Jan 7, 2026
@spinojara
Copy link
Copy Markdown
Contributor Author

I have already signed under a corporate contributor license agreement.

@alexey-milovidov alexey-milovidov added the can be tested Allows running workflows for external contributors label Jan 8, 2026
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Jan 8, 2026

Workflow [PR], commit [6136678]

Summary:

job_name test_name status info comment
Bugfix validation (functional tests) failure
03780_failed_ssh FAIL cidb
Check errors failure
Stateless tests (arm_binary, parallel) failure
03622_explain_indexes_distributed_index_analysis_in FAIL cidb

@clickhouse-gh clickhouse-gh bot added the pr-bugfix Pull request with bugfix, not backported by default label Jan 8, 2026
@ClickHouse ClickHouse deleted a comment from CLAassistant Jan 8, 2026
@ClickHouse ClickHouse deleted a comment from CLAassistant Jan 9, 2026
@ClickHouse ClickHouse deleted a comment from CLAassistant Jan 12, 2026
@ClickHouse ClickHouse deleted a comment from CLAassistant Jan 12, 2026
@ClickHouse ClickHouse deleted a comment from CLAassistant Jan 12, 2026
@ClickHouse ClickHouse deleted a comment from CLAassistant Jan 13, 2026
0lgWuG96Yy/MxT62bvAAAAE2lzYWtlbEBwYzY0MTAxLTI1MzYBAgMEBQYH
-----END OPENSSH PRIVATE KEY-----' > id_rsa_03780_failed_ssh"

spawn bash -c "source $basedir/../shell_config.sh ; \$CLICKHOUSE_CURL -sS \$CLICKHOUSE_URL -d 'CREATE USER IF NOT EXISTS user_03780_failed_ssh'"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"Flaky check" runs the same test many times and in parallel with itself. To make it succeed, let's use the unique name of the user.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do you mean a new unique name for each test? Is there another way to do this than to append a random string?

@ClickHouse ClickHouse deleted a comment from CLAassistant Jan 15, 2026
@alexey-milovidov
Copy link
Copy Markdown
Member

Now it says:

There is no user user_03780__test_u0vidgvy in user directories

-----END OPENSSH PRIVATE KEY-----' > id_rsa_03780_failed_ssh"

spawn bash -c "source $basedir/../shell_config.sh ; \$CLICKHOUSE_CURL -sS \$CLICKHOUSE_URL -d 'CREATE USER IF NOT EXISTS user_03780_\$CLICKHOUSE_TEST_UNIQUE_NAME'"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should wait for completion here.

@ClickHouse ClickHouse deleted a comment from CLAassistant Jan 16, 2026
@alexey-milovidov
Copy link
Copy Markdown
Member

The test didn't pass.

IPac9E4ZR36UA2lWK/gr0x8PvxzlonzWqBcYFmCAQv9lPvk1GJK8/EnCkkD5pTvKbHAAAA
QQDbVchcKjXrhsYklDvJQ4xyGkUH6+xMRVINwzjP3JQ1akUF/MJAYg5vr8j4gibWysVwWV
0lgWuG96Yy/MxT62bvAAAAE2lzYWtlbEBwYzY0MTAxLTI1MzYBAgMEBQYH
-----END OPENSSH PRIVATE KEY-----' > id_rsa_03780_failed_ssh"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This file may be overwritten during concurrent runs. Let's make it unique by using the CLICKHOUSE_TMP directory.

@clickhouse-gh clickhouse-gh bot added the manual approve Manual approve required to run CI label Jan 19, 2026
The test was failing because:
1. `CLICKHOUSE_TEST_UNIQUE_NAME` doesn't work in `bash -c` context
   since it's computed from `BASH_SOURCE[1]` which is empty
2. Single quotes in `-d '...'` prevented variable expansion

Fixed by using `CLICKHOUSE_DATABASE` (properly inherited env var)
and double quotes for the curl `-d` argument. Also made the SSH
key filename unique and added cleanup.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@ClickHouse ClickHouse deleted a comment from CLAassistant Jan 19, 2026
@alexey-milovidov alexey-milovidov removed the manual approve Manual approve required to run CI label Jan 19, 2026
@alexey-milovidov
Copy link
Copy Markdown
Member

Don't look at the "Bugfix validation (functional tests)" for now - it is broken, CC @maxknv

@alexey-milovidov
Copy link
Copy Markdown
Member

Now the test runs well in parallel.

@alexey-milovidov alexey-milovidov merged commit e0c47ab into ClickHouse:master Jan 19, 2026
128 of 132 checks passed
@robot-ch-test-poll1 robot-ch-test-poll1 added the pr-synced-to-cloud The PR is synced to the cloud repo label Jan 19, 2026
@Algunenano
Copy link
Copy Markdown
Member

Please note that this PR has been reverted as it introduced a flaky test in the CI

@alexey-milovidov
Copy link
Copy Markdown
Member

@spinojara, please resubmit this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

can be tested Allows running workflows for external contributors pr-bugfix Pull request with bugfix, not backported by default pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants