From 095c9b3ba8b5e9efcfc076786d628d4a1bb1164b Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Fri, 7 Apr 2023 17:16:14 +0200 Subject: [PATCH 1/4] cli/sql: warn the user when connecting to the system tenant This change ensures that a clear warning is printed in the SQL shell when the user connects to the system tenant and there are secondary tenants defined. For example: ``` % ./cockroach sql --certs-dir=~/.cockroach-demo -d cluster:system \# \# Welcome to the CockroachDB SQL shell. \[...] \# \# \# ATTENTION: YOUR ARE CONNECTED TO THE SYSTEM TENANT OF A MULTI-TENANT CLUSTER. \# PROCEED WITH CAUTION. YOU ARE RESPONSIBLE FOR ENSURING THAT YOU DO NOT \# PERFORM ANY OPERATIONS THAT COULD DAMAGE THE CLUSTER OR OTHER TENANTS. \# [...] root@localhost:26257/defaultdb> ``` Release note: None --- pkg/cli/clisqlclient/conn.go | 36 +++++++++++++++++++ .../test_demo_cli_integration.tcl.disabled | 3 ++ .../test_sql_version_reporting.tcl | 8 +---- 3 files changed, 40 insertions(+), 7 deletions(-) diff --git a/pkg/cli/clisqlclient/conn.go b/pkg/cli/clisqlclient/conn.go index a5653e466a3f..c1c7739d9ac2 100644 --- a/pkg/cli/clisqlclient/conn.go +++ b/pkg/cli/clisqlclient/conn.go @@ -80,6 +80,11 @@ type sqlConn struct { clusterID string clusterOrganization string + // isSystemTenantUnderSecondaryTenants is true if the current + // connection is to the system tenant and there are secondary + // tenants defined. + isSystemTenantUnderSecondaryTenants bool + // infow and errw are the streams where informational, error and // warning messages are printed. // Echoed queries, if Echo is enabled, are printed to errw too. @@ -271,6 +276,30 @@ func (c *sqlConn) tryEnableServerExecutionTimings(ctx context.Context) error { func (c *sqlConn) GetServerMetadata( ctx context.Context, ) (nodeID int32, version, clusterID string, retErr error) { + c.isSystemTenantUnderSecondaryTenants = false + val, err := c.QueryRow(ctx, ` + SELECT EXISTS (SELECT 1 FROM system.information_schema.tables WHERE table_name='tenants')`) + if c.conn.IsClosed() { + return 0, "", "", MarkWithConnectionClosed(err) + } + if err != nil { + return 0, "", "", err + } + // We use toString() instead of casting val[0] to string because we + // get either a go string or bool depending on the SQL driver in + // use. + if toString(val[0])[0] == 't' { + val, err = c.QueryRow(ctx, ` + SELECT EXISTS (SELECT 1 FROM system.public.tenants LIMIT 1)`) + if c.conn.IsClosed() { + return 0, "", "", MarkWithConnectionClosed(err) + } + if err != nil { + return 0, "", "", err + } + c.isSystemTenantUnderSecondaryTenants = toString(val[0])[0] == 't' + } + // Retrieve the node ID and server build info. // Be careful to query against the empty database string, which avoids taking // a lease against the current database (in case it's currently unavailable). @@ -328,6 +357,7 @@ func (c *sqlConn) GetServerMetadata( c.serverBuild = fmt.Sprintf("CockroachDB %s %s (%s, built %s, %s)", v10fields[0], version, v10fields[2], v10fields[3], v10fields[4]) } + return nodeID, version, clusterID, nil } @@ -395,6 +425,12 @@ func (c *sqlConn) checkServerMetadata(ctx context.Context) error { fmt.Fprintf(c.errw, "warning: unable to retrieve the server's version: %s\n", err) } + if c.isSystemTenantUnderSecondaryTenants { + fmt.Fprintln(c.infow, "#\n# ATTENTION: YOU ARE CONNECTED TO THE SYSTEM TENANT OF A MULTI-TENANT CLUSTER.\n"+ + "# PROCEED WITH CAUTION. YOU ARE RESPONSIBLE FOR ENSURING THAT YOU DO NOT\n"+ + "# PERFORM ANY OPERATIONS THAT COULD DAMAGE THE CLUSTER OR OTHER TENANTS.\n#") + } + // Report the server version only if it the revision has been // fetched successfully, and the revision has changed since the last // connection. diff --git a/pkg/cli/interactive_tests/test_demo_cli_integration.tcl.disabled b/pkg/cli/interactive_tests/test_demo_cli_integration.tcl.disabled index ade396e08862..3b5f4fa357de 100644 --- a/pkg/cli/interactive_tests/test_demo_cli_integration.tcl.disabled +++ b/pkg/cli/interactive_tests/test_demo_cli_integration.tcl.disabled @@ -52,6 +52,7 @@ end_test start_test "Check that a SQL shell can connect to the system tenant without special arguments as demo" spawn $argv sql --no-line-editor -u demo -d cluster:system eexpect "Welcome" +eexpect "ATTENTION: YOU ARE CONNECTED TO THE SYSTEM TENANT" eexpect demo@ eexpect "defaultdb>" send "table system.tenants;\r" @@ -64,6 +65,7 @@ end_test start_test "Check that a SQL shell can connect to the system tenant without special arguments as root" spawn $argv sql --no-line-editor -u root -d cluster:system eexpect "Welcome" +eexpect "ATTENTION: YOU ARE CONNECTED TO THE SYSTEM TENANT" eexpect root@ eexpect "defaultdb>" send "table system.tenants;\r" @@ -93,6 +95,7 @@ system "$argv sql -d cluster:system/demo -u root -e \"alter user root with passw set ssldir $env(HOME)/.cockroach-demo spawn $argv sql --no-line-editor --url "postgresql://root:abc@/defaultdb?host=$ssldir&options=-ccluster%3Dsystem&port=26257" eexpect "Welcome" +eexpect "ATTENTION: YOU ARE CONNECTED TO THE SYSTEM TENANT" eexpect root@ eexpect "defaultdb>" send "table system.tenants;\r" diff --git a/pkg/cli/interactive_tests/test_sql_version_reporting.tcl b/pkg/cli/interactive_tests/test_sql_version_reporting.tcl index 1d3797b045ea..1953546d4a6f 100755 --- a/pkg/cli/interactive_tests/test_sql_version_reporting.tcl +++ b/pkg/cli/interactive_tests/test_sql_version_reporting.tcl @@ -35,13 +35,7 @@ eexpect "connection closed unexpectedly" # Check that the prompt immediately succeeds the error message eexpect "connection lost" eexpect "opening new connection: all session settings will be lost" -expect { - "\r\n# " { - report "unexpected server message" - exit 1 - } - "root@" {} -} +eexpect "root@" # Check the reconnect did succeed - this also resets the connection state to good. send "select 1;\r" From c7a932aeb2cf9e0fa9da318ad9941fa92ef5bed2 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Fri, 7 Apr 2023 17:37:30 +0200 Subject: [PATCH 2/4] cli/sql: include the tenant name in the prompt when known This change extends the SQL prompt with the name of the tenant the prompt is connected to, if it was specified in the URL. For example: ``` % ./cockroach sql --certs-dir=~/.cockroach-demo -d cluster:foo ... root@localhost:26257/foo/defaultdb> ``` Release note: None --- pkg/cli/clisqlshell/sql.go | 41 ++++++++++++++----- .../test_client_side_checking.tcl | 2 +- .../test_demo_cli_integration.tcl.disabled | 19 ++++++--- 3 files changed, 44 insertions(+), 18 deletions(-) diff --git a/pkg/cli/clisqlshell/sql.go b/pkg/cli/clisqlshell/sql.go index 6cff6c097669..a46c68cabf54 100644 --- a/pkg/cli/clisqlshell/sql.go +++ b/pkg/cli/clisqlshell/sql.go @@ -16,7 +16,6 @@ import ( "context" "fmt" "io" - "net/url" "os" "os/exec" "os/signal" @@ -130,10 +129,10 @@ Commands specific to the demo shell (EXPERIMENTAL): \demo add add a node (locality specified as "region=,zone="). ` - defaultPromptPattern = "%n@%M/%/%x>" + defaultPromptPattern = "%n@%M/%C%/%x>" // debugPromptPattern avoids substitution patterns that require a db roundtrip. - debugPromptPattern = "%n@%M>" + debugPromptPattern = "%n@%M %C>" ) // cliState defines the current state of the CLI during @@ -934,7 +933,7 @@ func (c *cliState) doRefreshPrompts(nextState cliStateEnum) cliStateEnum { } else { // Configure the editor to use the new prompt. - parsedURL, err := url.Parse(c.conn.GetURL()) + parsedURL, err := pgurl.Parse(c.conn.GetURL()) if err != nil { // If parsing fails, we'll keep the entire URL. The Open call succeeded, and that // is the important part. @@ -943,10 +942,7 @@ func (c *cliState) doRefreshPrompts(nextState cliStateEnum) cliStateEnum { return nextState } - userName := "" - if parsedURL.User != nil { - userName = parsedURL.User.Username() - } + userName := parsedURL.GetUsername() dbName := unknownDbName c.lastKnownTxnStatus = unknownTxnStatus @@ -961,14 +957,35 @@ func (c *cliState) doRefreshPrompts(nextState cliStateEnum) cliStateEnum { dbName = c.refreshDatabaseName() } + // Do we have a "cluster" option; either as an options= parameter + // or as a prefix to the database name? + opts := parsedURL.GetExtraOptions() + var logicalCluster string + if extOptsS := opts.Get("options"); extOptsS != "" { + extOpts, err := pgurl.ParseExtendedOptions(extOptsS) + if err == nil { + logicalCluster = extOpts.Get("cluster") + } + } + if urlDB := parsedURL.GetDatabase(); strings.HasPrefix(urlDB, "cluster:") { + parts := strings.SplitN(urlDB, "/", 2) + logicalCluster = parts[0][len("cluster:"):] + } + if logicalCluster != "" { + logicalCluster += "/" + } + c.fullPrompt = rePromptFmt.ReplaceAllStringFunc(c.iCtx.customPromptPattern, func(m string) string { switch m { case "%M": - return parsedURL.Host // full host name. + _, host, port := parsedURL.GetNetworking() + return host + ":" + port // server:port case "%m": - return parsedURL.Hostname() // host name. + _, host, _ := parsedURL.GetNetworking() + return host case "%>": - return parsedURL.Port() // port. + _, _, port := parsedURL.GetNetworking() + return port case "%n": // user name. return userName case "%/": // database name. @@ -977,6 +994,8 @@ func (c *cliState) doRefreshPrompts(nextState cliStateEnum) cliStateEnum { return c.lastKnownTxnStatus case "%%": return "%" + case "%C": + return logicalCluster default: err = fmt.Errorf("unrecognized format code in prompt: %q", m) return "" diff --git a/pkg/cli/interactive_tests/test_client_side_checking.tcl b/pkg/cli/interactive_tests/test_client_side_checking.tcl index 5427ce29c039..5a417ad142e6 100644 --- a/pkg/cli/interactive_tests/test_client_side_checking.tcl +++ b/pkg/cli/interactive_tests/test_client_side_checking.tcl @@ -96,7 +96,7 @@ eexpect "root@" send "\\set display_format csv\r\\set\r" eexpect "check_syntax,false" eexpect "echo,true" -eexpect "prompt1,%n@%M>" +eexpect "prompt1,%n@%M %C>" eexpect "root@" send "\\q\r" eexpect ":/# " diff --git a/pkg/cli/interactive_tests/test_demo_cli_integration.tcl.disabled b/pkg/cli/interactive_tests/test_demo_cli_integration.tcl.disabled index 3b5f4fa357de..76ce918afdd6 100644 --- a/pkg/cli/interactive_tests/test_demo_cli_integration.tcl.disabled +++ b/pkg/cli/interactive_tests/test_demo_cli_integration.tcl.disabled @@ -47,6 +47,13 @@ eexpect "does not exist" eexpect "defaultdb>" send "\\q\r" eexpect eof + +spawn $argv sql --no-line-editor -u root -d cluster:demo-tenant +eexpect "Welcome" +eexpect root@ +eexpect "demo-tenant/defaultdb>" +send "\\q\r" +eexpect eof end_test start_test "Check that a SQL shell can connect to the system tenant without special arguments as demo" @@ -54,10 +61,10 @@ spawn $argv sql --no-line-editor -u demo -d cluster:system eexpect "Welcome" eexpect "ATTENTION: YOU ARE CONNECTED TO THE SYSTEM TENANT" eexpect demo@ -eexpect "defaultdb>" +eexpect "system/defaultdb>" send "table system.tenants;\r" eexpect "2 rows" -eexpect "defaultdb>" +eexpect "system/defaultdb>" send "\\q\r" eexpect eof end_test @@ -67,10 +74,10 @@ spawn $argv sql --no-line-editor -u root -d cluster:system eexpect "Welcome" eexpect "ATTENTION: YOU ARE CONNECTED TO THE SYSTEM TENANT" eexpect root@ -eexpect "defaultdb>" +eexpect "system/defaultdb>" send "table system.tenants;\r" eexpect "2 rows" -eexpect "defaultdb>" +eexpect "system/defaultdb>" send "\\q\r" eexpect eof end_test @@ -97,10 +104,10 @@ spawn $argv sql --no-line-editor --url "postgresql://root:abc@/defaultdb?host=$s eexpect "Welcome" eexpect "ATTENTION: YOU ARE CONNECTED TO THE SYSTEM TENANT" eexpect root@ -eexpect "defaultdb>" +eexpect "system/defaultdb>" send "table system.tenants;\r" eexpect "2 rows" -eexpect "defaultdb>" +eexpect "system/defaultdb>" send "\\q\r" eexpect eof end_test From 27c0a29016d1f82f78138f24107c3e892b7b86ea Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Mon, 10 Apr 2023 12:20:26 +0200 Subject: [PATCH 3/4] cli/interactive_tests: make test_client_side_checking easier to run locally Sometimes the shell invoked by `expect` adds additional escape characters in-between the output of a process and the next prompt. This patch makes the test more robust in that case. Release note: None --- .../interactive_tests/test_client_side_checking.tcl | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/pkg/cli/interactive_tests/test_client_side_checking.tcl b/pkg/cli/interactive_tests/test_client_side_checking.tcl index 5a417ad142e6..5405fe6d0653 100644 --- a/pkg/cli/interactive_tests/test_client_side_checking.tcl +++ b/pkg/cli/interactive_tests/test_client_side_checking.tcl @@ -65,16 +65,18 @@ send "(echo '\\unset errexit'; echo 'begin;'; echo 'select 1+;'; echo 'select 1; eexpect "syntax error" eexpect "current transaction is aborted" eexpect ":/# " -send "echo \$?\r" -eexpect "1\r\n:/# " +send "echo ::\$?::\r" +eexpect "::1::" +eexpect ":/# " send "(echo '\\unset errexit'; echo '\\set check_syntax'; echo 'begin;'; echo 'select 1+;'; echo 'select 1;'; echo 'commit;') | $argv sql\r" eexpect "syntax error" eexpect "1 row" eexpect "COMMIT" eexpect ":/# " -send "echo \$?\r" -eexpect "0\r\n:/# " +send "echo ::\$?::\r" +eexpect "::0::" +eexpect ":/# " end_test start_test "Check that --debug-sql-cli sets suitable simplified client-side options." From 66755e93d14e11f3e0d54300646ddb2205013350 Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Mon, 10 Apr 2023 12:23:11 +0200 Subject: [PATCH 4/4] cli/democluster: rename "demo-tenant" to "demoapp" We aim to de-emphasize the word "tenant" in user-facing features. This patch achieves this for `cockroach demo` by making the tenant name "demoapp" instead of "demo-tenant". Release note: None --- pkg/cli/democluster/demo_cluster.go | 2 +- .../interactive_tests/test_demo_cli_integration.tcl.disabled | 4 ++-- pkg/cli/interactive_tests/test_demo_multitenant.tcl.disabled | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/cli/democluster/demo_cluster.go b/pkg/cli/democluster/demo_cluster.go index 02a591065170..5ace22770783 100644 --- a/pkg/cli/democluster/demo_cluster.go +++ b/pkg/cli/democluster/demo_cluster.go @@ -114,7 +114,7 @@ const demoOrg = "Cockroach Demo" const demoUsername = "demo" // demoTenantName is the name of the demo tenant. -const demoTenantName = "demo-tenant" +const demoTenantName = "demoapp" // LoggerFn is the type of a logger function to use by the // demo cluster to report special events. diff --git a/pkg/cli/interactive_tests/test_demo_cli_integration.tcl.disabled b/pkg/cli/interactive_tests/test_demo_cli_integration.tcl.disabled index 76ce918afdd6..3fe7fe7e7c29 100644 --- a/pkg/cli/interactive_tests/test_demo_cli_integration.tcl.disabled +++ b/pkg/cli/interactive_tests/test_demo_cli_integration.tcl.disabled @@ -48,10 +48,10 @@ eexpect "defaultdb>" send "\\q\r" eexpect eof -spawn $argv sql --no-line-editor -u root -d cluster:demo-tenant +spawn $argv sql --no-line-editor -u root -d cluster:demoapp eexpect "Welcome" eexpect root@ -eexpect "demo-tenant/defaultdb>" +eexpect "demoapp/defaultdb>" send "\\q\r" eexpect eof end_test diff --git a/pkg/cli/interactive_tests/test_demo_multitenant.tcl.disabled b/pkg/cli/interactive_tests/test_demo_multitenant.tcl.disabled index 825d1ba07ec0..3541ebe4ec5b 100644 --- a/pkg/cli/interactive_tests/test_demo_multitenant.tcl.disabled +++ b/pkg/cli/interactive_tests/test_demo_multitenant.tcl.disabled @@ -11,7 +11,7 @@ start_test "Check --multitenant flag runs as expected" spawn $argv demo --no-line-editor --empty --nodes 3 --multitenant eexpect "Welcome" -eexpect "You are connected to tenant \"demo-tenant\"" +eexpect "You are connected to tenant \"demoapp\"" eexpect "(sql)" eexpect "127.0.0.1:26257/defaultdb" eexpect "defaultdb>"