feat(admin): Allow copy-tables CREATE on any target host - #8411
Merged
Conversation
The Copy Tables target host is meant for applying schemas on a node that is not in the source cluster. Membership validation on that connection rejected those hosts. Keep source reads checked; skip topology checks only for the sudo-gated CREATE target.
phacops
marked this pull request as ready for review
August 28, 2026 15:53
Pre-commit mypy flags the import-untyped ignore on sql_metadata as unused once common.py is in the changed-file set.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
onewland
approved these changes
Aug 28, 2026
Unvalidated target hosts could receive source-cluster credentials. Require the host to be a known cluster node or listed in admin.copy_tables_allowed_target_hosts so bootstrap nodes can be added before they exist in Snuba config.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Annotate the sentry-options allowlist so mypy can infer its type. Treat a typed query-node hostname as a cluster member even when the default HTTP port is not the Envoy listen port.
list[str] is not a valid get_option type var. Cast the returned value, matching other array option readers.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6957930. Configure here.
| # Envoy port instead. Match the connect path, which uses _http_port_for_host. | ||
| topology_port = _http_port_for_host(host, cluster) | ||
| if topology_port != port and is_valid_node(host, topology_port, cluster, storage_name): | ||
| return |
There was a problem hiding this comment.
Port fallback skipped on topology errors
Low Severity
The new Envoy-port fallback shares a try with the first is_valid_node call. When that call raises InvalidNodeError because topology discovery failed, the fallback never runs, so a query-node hostname typed without a port is rejected even though static cluster config would accept it on the Envoy port.
Reviewed by Cursor Bugbot for commit 6957930. Configure here.
Allowlist and cluster membership are the trust boundary. Parsing only splits host or host:port.
Share host:port parsing, drop get_unvalidated_node_connection, and reuse get_clusterless_node_connection with validate_node=False for allowlisted CREATE targets.
phacops
enabled auto-merge (squash)
August 28, 2026 18:04
onewland
added a commit
that referenced
this pull request
Aug 28, 2026
Follows #8411. System Queries' manual host entry can now target a host that is not a member of the storage's cluster, provided it is listed in `admin.copy_tables_allowed_target_hosts` — the same option copy-tables CREATE targets already use. That lets a node which has been provisioned but not yet added to Snuba cluster config be inspected. The allowlist gates the EXPLAIN validation connection as well as the query connection: without that, `validate_query` rejects the query before the read ever reaches the node. Skipping validation also means the typed port is used verbatim instead of being rewritten to the cluster's Envoy/8123 port, which is what you want for a node that is not behind the cluster proxy yet. Non-allowlisted hosts still go through `_validate_node`, and node-picker (non-clusterless) queries are untouched — the relaxation is limited to manual host entry. ### Changes - `common.py`: host/port parsing and the allowlist lookup move out of `copy_tables.py` (`ADMIN_ALLOWED_HOSTS_OPTION`, `parse_host`, `host_is_allowlisted`) so both tools read one list. The option key value is unchanged, so no ops change is needed. - `common.py`: `get_ro_clusterless_node_connection` gains `validate_node`; the sudo variant already had it from #8411. - `system_queries.py`: `_validate_clusterless_node` gates `_run_sql_query_on_host` (sudo and read-only) and `_run_explain_on_host`. - Frontend: manual entry parses `host:port` (defaults to 8123), keeping the raw text in its own state so a half-typed port is not clobbered, plus help text naming the option. ### Review notes The `validate_node=False` path is the one to scrutinize. It is reached only after `host_is_allowlisted` returns true, and read-only clusterless queries still use the global readonly user, so a non-allowlisted host never receives credentials. Open question: the option key still reads `admin.copy_tables_allowed_target_hosts` while now governing two tools. Only the schema description was updated here; renaming would need a coordinated options change. ### Tests `tests/admin` passes (172 + 80). New coverage: allowlisted vs. non-allowlisted hosts for sudo and read-only, the EXPLAIN path, and that non-clusterless mode ignores the allowlist. Copy-tables tests updated for the moved helpers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Copy Tables can now run CREATE on a typed-in target host that is not in the source cluster. Source reads stay membership-checked; the CREATE connection is still sudo-gated and still uses the source storage's ClickHouse credentials.
This restores the original target-host workflow (stand up schemas on a new cluster) that node-validation had blocked. Review the
validate_node=Falsepath in_build_validated_pool— it is only used by copy-tables CREATE.