Do not replace `ProxyConfig::host` with a resolved address. Poco reuses it as
the TLS peer name whenever TLS terminates at the proxy rather than at the
target - an `https` proxy, or an `http` proxy with tunneling turned off - so an
address there breaks certificate verification against the proxy's host name.
`ClickHouse` only turns tunneling on for an `https` request over an `http`
proxy (`ProxyConfiguration::useTunneling`), so every other proxied
configuration takes the `setPeerHostName(getProxyHost())` branch of
`HTTPSClientSession::connect`. Routing the proxy host through the cache needs a
resolved-proxy-host field in the session, mirroring `setResolvedHost`; the
constraint is now recorded as a comment where the substitution used to be.
The two four-letter-word helpers now drop the host from the DNS cache when a
connection attempt fails, like `Connection` and `ZooKeeper::connect` already
do. It matters most in `clickhouse-keeper-client`: a client program has no
`DNSCacheUpdater`, so nothing would ever refresh the entry and one lookup would
pin the address for the whole session.
Server paths that are not latency critical on reconnect (the Graphite writer,
the distributed query exchange source, Redis) keep relying on
`DNSCacheUpdater` to refresh the cache within `dns_cache_update_period`, which
is the same contract `Cluster` and the HTTP connection pools have always had.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Poco::Net::SocketAddress(host, port)` resolves the host name itself, through
`Poco::Net::DNS::hostByName`. Every place that constructed one from a host name
therefore bypassed `DNSResolver`, so the lookup was not cached, not shared with
the rest of the server, not visible in `system.dns_cache`, not counted, and not
subject to `dns_allow_resolve_names_to_ipv4` and
`dns_allow_resolve_names_to_ipv6`.
All of those places resolve through `DNSResolver` now:
- the ZooKeeper and Keeper client, which resolved every configured node on
every connection attempt;
- `GraphiteWriter`, which resolved the Graphite host on every transmission;
- the four-letter-word command target of `system.zookeeper_info`;
- the Redis dictionary source and table function;
- the HTTP proxy host in `HTTPConnectionPool` - Poco resolves
`ProxyConfig::host` on its own, so the per-endpoint `HostResolver` never
covered it;
- the remote peer of a distributed query exchange stream, and the exchange
listener;
- the `is_local` check of the ClickHouse dictionary source;
- `listen_host` and `bind_host` when they are host names;
- `clickhouse-keeper-client`, `clickhouse-keeper-bench` and the BuzzHouse HTTP
helper.
A literal IP address in `listen_host` or `bind_host` is still used as is: there
is no name to look up, and `dns_allow_resolve_names_to_ipv4` and
`dns_allow_resolve_names_to_ipv6` restrict which addresses we resolve names of
remote hosts to, not which of our own addresses we may bind. Without this,
`<listen_host>::</listen_host>` would stop working on a server that has
`dns_allow_resolve_names_to_ipv6` disabled.
Detecting a literal cannot use `Poco::Net::IPAddress::tryParse` alone: it
reports failure for an address that parses to all zeroes, because it decides
success by comparing the parsed value against a default constructed one. So it
rejects `::`, the most common `listen_host` of all (the IPv4 wildcard is worked
around there with a string comparison against `0.0.0.0`). `inet_pton` is used
as a fallback; `tryParse` is still tried first because it also accepts a scope
suffix.
The ZooKeeper client drops a host from the cache whenever a connection attempt
to it fails, the same way `Connection` does, so a changed address is picked up
on the next attempt instead of waiting for `dns_cache_update_period`.
Clients from third-party libraries (MySQL, PostgreSQL, Kafka, RabbitMQ, NATS,
MongoDB, Hive Metastore over Thrift, gRPC, NuRaft) keep their own resolvers:
handing them a pre-resolved address would defeat their own multi-address
failover and TLS host name verification.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses the review of #118422.
`clickhouse-keeper-client` now finds `config.yaml` and `config.yml`, but its
diagnostics still named `config.xml`: the `--config-file` help text and the
`Found keeper node in the config.xml, will use it for connection` message. A
user connected through a YAML file was pointed at a file that does not exist.
The resolved path is now threaded through `connectToKeeper` and logged, and the
help text mentions all three names.
The utility documentation of `clickhouse-keeper-client` and `clickhouse-disks`
still described the XML-only default, and the `Configuration files` section of
the `clickhouse-local` page contradicted the new `Usage` section; all three are
updated.
Test `05182_keeper_client_config_file_yaml_in_current_directory` covers the
`clickhouse-keeper-client` loader, which is separate from the shared
`BaseDaemon` path exercised by the other two tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A ClickHouse configuration file can be written in XML or in YAML, but the
lookup of the *default* configuration file only knew about `.xml`, so a
`config.yaml` or `config.yml` in the current directory was silently
ignored. `clickhouse-server` was the worst case: it fell back to the
configuration embedded into the binary and started up with a
configuration the user did not write.
The other lookup paths already probe all three extensions, so
`~/.clickhouse-local/config.yaml` worked while `./config.yaml` did not.
Now the default configuration file is looked up as `config.xml`,
`config.yaml` and `config.yml`, in this order, and the first one that
exists is used. This covers `clickhouse-server`, `clickhouse-keeper`
(`keeper_config.*`), `clickhouse-local`, `clickhouse-keeper-client` and
`clickhouse-disks`. The extension probing is factored into
`tryGetConfigPath` and `getConfigPathForAnySupportedFormat`, which
`getLocalConfigPath` and `getClientConfigPath` now use as well.
A file requested explicitly with `--config-file` or `-C` is still used as
is: silently substituting a different file for the name the user typed
would be worse than reporting that it does not exist.
Closes: https://github.com/ClickHouse/ClickHouse/issues/118411
Add `static` to file-local functions and forward declarations
for cross-TU functions to satisfy `-Wmissing-prototypes`.
This ensures all functions have visible prototypes before their
definitions, improving code quality and catching accidental
linkage issues.
Reference: https://github.com/ClickHouse/ClickHouse/issues/76944
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
shell-like completion support in clickhouse keeper-client (handle
completions of arguments in quotes, i.e. 'foo ba', handle escaped
arguments, i.e. foo\ ba, make ls print quoted nodes, if they have
whitespaces)
Also remove on_complete_modify_callback hack, replace it with pure
getCompletions implementation
Co-Authored-By: Claude <noreply@anthropic.com>
Add support of `password` and `identity` fields to keeper-client config.
More over, those parameters could be passed via CLICKHOUSE_KEEPER_PASSWORD
and CLICKHOUSE_KEEPER_IDENTITY environment variables. The priority is
the following: CMD PARAMS > ENVIRONMENT VARIABLES > CONFIG FILE.
Fix command name completion which was broken because appending a closing
quote before tokenizing caused the tokenizer to misinterpret command
prefixes (e.g. 'ls') as path arguments. Split into two-phase approach:
probe the original prefix to decide command vs path, then append quote
only for path parsing.
Fix double closing quote when TAB is pressed with cursor before an
existing closing quote — strip the old quote so completion re-adds it
with the correct suffix (closing quote for leaves, / for directories).
Remove duplicate methods (askConfirmation, getAbsolutePath, loadCommands)
now provided by KeeperClientBase.
Add PTY-based autocomplete test covering command completion, path
completion, directory trailing slash, leaf closing quote, and drill-down.
Assisted-by: Claude Opus 4.6 via GitHub Copilot
The `execute_query` method in the Python test helper uses `epoll` to
read from both stdout and stderr of the keeper-client process. When
a command fails, the keeper-client writes the error to stderr and then
the separator to stdout. However, when both events arrive simultaneously
in `epoll.poll()`, the loop could process stdout first, find the
separator, and return successfully without ever reading the pending
stderr error data - causing the exception to be silently swallowed.
Fix both sides:
- C++ (`KeeperClient.cpp`): flush stderr before writing the stdout
separator, ensuring error data is in the pipe buffer first.
- Python (`keeper_utils.py`): process stderr events before stdout
events in the poll loop, so errors are always collected before
checking the separator.
Also fix test cleanup: add `try/finally` with `drop_node_if_exists`
to `test_rm_with_version` and `test_rm_without_version`, and give
`test_rm_without_version` its own unique node path instead of
reusing `/test_rm_with_version_node`.
https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=97554&sha=3f86a6142c6920239c1b3adedfe82ccb5fcd6515&name_0=PR&name_1=Integration%20tests%20%28amd_binary%2C%204%2F5%29
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract `ErrnoException` into `Common/ErrnoException.h` to reduce the
include footprint of `Exception.h`. This removes `<base/errnoToString.h>`
and `<cerrno>` from `Exception.h`, which is included by ~95% of all
translation units.
All files referencing `ErrnoException` now include the new header
directly. Files that used `errnoToString` without `ErrnoException`
got an explicit `<base/errnoToString.h>` include.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Alexey Milovidov