feat(admin): Allow system queries on allowlisted hosts - #8414
Conversation
Manual host entry in System Queries can now target a host that is not a member of the storage's cluster, as long as it is listed in admin.copy_tables_allowed_target_hosts — the same option copy-tables CREATE targets already use. This lets a node that has been provisioned but not yet added to Snuba cluster config be inspected. The allowlist check gates the EXPLAIN validation connection as well as the query connection, otherwise validation rejects the query before the read reaches the node. Non-allowlisted hosts still go through _validate_node, and node-picker (non-clusterless) queries are untouched. The host/port parsing and allowlist lookup move from copy_tables.py to common.py so both tools read one list. Also restores the `# type: ignore[import-untyped]` on the sql_metadata import that #8411 dropped; mypy fails on master without it.
The previous commit re-added `# type: ignore[import-untyped]` because mypy flagged the import locally. That was a stale local venv holding sql_metadata 2.11.0; the lock pins 3.0.1, which ships py.typed, so CI correctly reports the ignore as unused.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 17db68b. Configure here.
| predefinedQueryOptions: Array<PredefinedQuery>; | ||
| }) { | ||
| const [nodeData, setNodeData] = useState<ClickhouseNodeData[]>([]); | ||
| const [manualHostInput, setManualHostInput] = useState<string>(""); |
There was a problem hiding this comment.
Manual host input state desynchronizes
Medium Severity
The host field now renders manualHostInput while execute still reads query.host and query.port. Changing storage clears those query fields without clearing the input, so Execute stays disabled against a filled-looking field. Toggling to manual entry leaves the prior dropdown host in query state with an empty input, so Execute can run against a host that is not shown.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 17db68b. Configure here.
2 participants
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_queryrejects 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 ofcopy_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_connectiongainsvalidate_node; the sudo variant already had it from feat(admin): Allow copy-tables CREATE on any target host #8411.system_queries.py:_validate_clusterless_nodegates_run_sql_query_on_host(sudo and read-only) and_run_explain_on_host.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=Falsepath is the one to scrutinize. It is reached only afterhost_is_allowlistedreturns 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_hostswhile now governing two tools. Only the schema description was updated here; renaming would need a coordinated options change.Tests
tests/adminpasses (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.