Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli,sql: stop using RPC conns in CLI client commands #51454

Open
knz opened this issue Jul 15, 2020 · 5 comments
Open

cli,sql: stop using RPC conns in CLI client commands #51454

knz opened this issue Jul 15, 2020 · 5 comments
Labels
A-cc-enablement Pertains to current CC production issues or short-term projects A-cli-admin CLI commands that pertain to controlling and configuring nodes A-security A-server-networking Pertains to network addressing,routing,initialization C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-server-and-security DB Server & Security

Comments

@knz
Copy link
Contributor

knz commented Jul 15, 2020

Today the CLI admin commands use a mix of RPC and SQL connections.
(They don't use the HTTP interface.)

For example, node decommission is RPC-only. node ls is SQL-only. debug zip uses both RPC and SQL.

We want everything to use a single protocol, ideally SQL. The reason why we want SQL is that the SQL server has the strongest authentication policies. IT would also enable using Kerberos tokens with the CLI client commands (as requested by customers, e.g. cockroach workloads - cc @BramGruneir )

Discussed this with @bdarnell . We can achieve this as follows: define SQL built-in functions for the various CLI commands. The built-in function would generate the command's results. The client-side program would then become a "dumb" SQL client that just issues the appropriate SQL queries.

The other advantage of this approach is that it then becomes possible to "run CLI admin commands" from a SQL shell, for example a UI tool.

Note that this feature might be rendered more secure by combining it with #51453.


A technical implementation detail for this approach: whatever service we make available via a SQL built-in function should also simultaneously become available over the HTTP REST API, using a similarly (ideally, identically) named endpoint.

For example, if I have a "command" SELECT crdb_internal.admin_node_status() I should have /api/v2/node/status (or something like that).

With this system, the debug zip command would send a SQL request for crdb_internal.admin_debug_zip() (or something like that) and there would also be a HTTP endpoint /api/v2/debug/zip (or something like that), which is incidentally a requirement for #51008.

cc @celiala who might be interested.

Jira issue: CRDB-6312

@knz knz added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-cli A-security labels Jul 15, 2020
@knz knz added this to To do in DB Server & Security via automation Jul 15, 2020
@BramGruneir
Copy link
Member

I am 100% behind this.
We should also make this a policy going forward.

@bdarnell
Copy link
Member

In some cases a virtual table might make more sense than a built-in function. For example, cockroach node status already displays its results in a tabular form; it should really become an alias for SELECT * from crdb_internal.admin_node_status instead of SELECT crdb_internal.admin_node_status().

@knz
Copy link
Contributor Author

knz commented Jul 16, 2020

Built-in functions can return tables (we have those already, called set-returning functions)

craig bot pushed a commit that referenced this issue Jul 20, 2020
51563: builtins: handle builtins involving string casts to geometry r=rytaft a=otan

Release note (sql change): Implement geospatial functions that take in a
string argument such that when an ambiguous function that could be
either geometry or geography is encountered, geometry is chosen. This is
the case for the following functions:
* st_area
* st_asewkt
* st_asgeojson
* st_buffer
* st_coveredby
* st_covers
* st_distance
* st_dwithin
* st_intersects
* st_length

51570: cli/sql: support Kerberos auth r=mjibson a=mjibson

Informs #42488
Informs #51454

Release note: add Kerberos (GSS) support to `cockroach sql` and other CLI commands that only use the SQL protocol (such as `node ls`, or `node status`). Other `cockroach` CLI commands that also use the RPC protocol still cannot use Kerberos (such as `node decommission`, `debug zip`, etc).

Co-authored-by: Oliver Tan <otan@cockroachlabs.com>
Co-authored-by: Matt Jibson <matt.jibson@gmail.com>
@knz knz moved this from To do to Linked issues (see roadmap) in DB Server & Security Jul 30, 2020
@knz
Copy link
Contributor Author

knz commented Nov 30, 2020

Here's the analysis of the current behavior, with what we can aim for:

Command Port used New port used
node ls SQL same
node status SQL same
node decommission RPC SQL via new built-in, with HTTP option
node recommission RPC SQL via new built-in, with HTTP option
node drain RPC SQL via new built-in, with HTTP option
init RPC SQL TBD, with HTTP option
quit (deprecated) RPC (REMOVED)
nodelocal SQL same
userfile SQL same
gen haproxy RPC SQL
other gen sub-commands N/A (entirely local) N/A
workload SQL same
import SQL same
debug tsdump RPC SQL via new built-in, with HTTP option
debug doctor SQL same
debug gossip-values RPC SQL TBD, with HTTP option
debug zip SQL and RPC SQL with HTTP option

There's a tricky bit: for gossip-values, init and possibly others, we want the option via HTTP and/or SQL to work even when the cluster is not initialized, and even when some system ranges are unavailable:

  • if we want this to work via SQL, we need a way to establish a SQL connection and route a RPC-over-SQL command without engaging the regular SQL machinery, including a KV-less authentication story.

  • if we want this to work via HTTP, we need a KV-less authentication story.

@knz knz added A-cli-admin CLI commands that pertain to controlling and configuring nodes and removed A-cli labels Mar 20, 2021
@jlinder jlinder added the T-server-and-security DB Server & Security label Jun 16, 2021
@knz knz added A-server-networking Pertains to network addressing,routing,initialization A-cc-enablement Pertains to current CC production issues or short-term projects labels Jul 29, 2021
@github-actions
Copy link

We have marked this issue as stale because it has been inactive for
18 months. If this issue is still relevant, removing the stale label
or adding a comment will keep it active. Otherwise, we'll close it in
10 days to keep the issue queue tidy. Thank you for your contribution
to CockroachDB!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cc-enablement Pertains to current CC production issues or short-term projects A-cli-admin CLI commands that pertain to controlling and configuring nodes A-security A-server-networking Pertains to network addressing,routing,initialization C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-server-and-security DB Server & Security
Projects
DB Server & Security
  
Linked issues (from the roadmap colum...
Development

No branches or pull requests

4 participants