roachprod: seperate syncedcluster.SQL() into c.ExecSQL() and

c.ExecOrInteractiveSQL()

Previously, a user of syncedCluster.SQL() could unknowingly cause a panic if
they called this function on a single node cluster, as this open an interactive
SQL shell. This patch prevents this footgun by seperating the c.SQL() call into
c.ExecSQL() and c.ExecOrInteractiveSQL().

Epic: none

Release note: None.

pkg/roachprod/install/BUILD.bazel

@@ -30,6 +30,7 @@ go_library(
         "//pkg/roachprod/ssh",
         "//pkg/roachprod/ui",
         "//pkg/roachprod/vm/aws",
+        "//pkg/roachprod/vm/gce",
         "//pkg/roachprod/vm/local",
         "//pkg/util/intsets",
         "//pkg/util/log",

pkg/roachprod/install/cockroach.go

@@ -290,37 +290,30 @@ func (c *SyncedCluster) NodeUIPort(node Node) int {
 	return c.VMs[node-1].AdminUIPort
 }
 
-// SQL runs `cockroach sql`, which starts a SQL shell or runs a SQL command.
+// ExecOrInteractiveSQL ssh's onto a single node and executes `./ cockroach sql`
+// with the provided args, potentially opening an interactive session. Note
+// that the caller can pass the `--e` flag to execute sql cmds and exit the
+// session. See `./cockroch sql -h` for more options.
 //
-// In interactive mode, there must be exactly one node target (as per
-// TargetNodes).
-//
-// In non-interactive mode, a command specified via the `-e` flag is run against
-// all nodes.
-func (c *SyncedCluster) SQL(
+// CAUTION: this function should not be used by roachtest writers. Use ExecSQL below.
+func (c *SyncedCluster) ExecOrInteractiveSQL(
 	ctx context.Context, l *logger.Logger, tenantName string, args []string,
 ) error {
-	if len(args) == 0 || len(c.Nodes) == 1 {
-		// If no arguments, we're going to get an interactive SQL shell. Require
-		// exactly one target and ask SSH to provide a pseudoterminal.
-		if len(args) == 0 && len(c.Nodes) != 1 {
-			return fmt.Errorf("invalid number of nodes for interactive sql: %d", len(c.Nodes))
-		}
-		url := c.NodeURL("localhost", c.NodePort(c.Nodes[0]), tenantName)
-		binary := cockroachNodeBinary(c, c.Nodes[0])
-		allArgs := []string{binary, "sql", "--url", url}
-		allArgs = append(allArgs, ssh.Escape(args))
-		return c.SSH(ctx, l, []string{"-t"}, allArgs)
+	if len(c.Nodes) != 1 {
+		return fmt.Errorf("invalid number of nodes for interactive sql: %d", len(c.Nodes))
 	}
-
-	// Otherwise, assume the user provided the "-e" flag, so we can reasonably
-	// execute the query on all specified nodes.
-	return c.RunSQL(ctx, l, args)
+	url := c.NodeURL("localhost", c.NodePort(c.Nodes[0]), tenantName)
+	binary := cockroachNodeBinary(c, c.Nodes[0])
+	allArgs := []string{binary, "sql", "--url", url}
+	allArgs = append(allArgs, ssh.Escape(args))
+	return c.SSH(ctx, l, []string{"-t"}, allArgs)
 }
 
-// RunSQL runs a `cockroach sql` command.
+// ExecSQL runs a `cockroach sql` .
 // It is assumed that the args include the -e flag.
-func (c *SyncedCluster) RunSQL(ctx context.Context, l *logger.Logger, args []string) error {
+func (c *SyncedCluster) ExecSQL(
+	ctx context.Context, l *logger.Logger, tenantName string, args []string,
+) error {
 	type result struct {
 		node   Node
 		output string
@@ -336,7 +329,7 @@ func (c *SyncedCluster) RunSQL(ctx context.Context, l *logger.Logger, args []str
 			cmd = fmt.Sprintf(`cd %s ; `, c.localVMDir(node))
 		}
 		cmd += cockroachNodeBinary(c, node) + " sql --url " +
-			c.NodeURL("localhost", c.NodePort(node), "" /* tenantName */) + " " +
+			c.NodeURL("localhost", c.NodePort(node), tenantName) + " " +
 			ssh.Escape(args)
 
 		sess := c.newSession(l, node, cmd, withDebugName("run-sql"))
@@ -806,7 +799,7 @@ WITH SCHEDULE OPTIONS first_run = 'now'`
 	createScheduleCmd := fmt.Sprintf(`-e
 CREATE SCHEDULE IF NOT EXISTS test_only_backup FOR BACKUP INTO '%s' %s`,
 		collectionPath, scheduleArgs)
-	return c.SQL(ctx, l, "" /* tenantName */, []string{createScheduleCmd})
+	return c.ExecSQL(ctx, l, "" /*tenantName*/, []string{createScheduleCmd})
 }
 
 // getEnvVars returns all COCKROACH_* environment variables, in the form

pkg/roachprod/multitenant.go

@@ -66,7 +66,7 @@ func StartTenant(
 	saveNodes := hc.Nodes
 	hc.Nodes = hc.Nodes[:1]
 	l.Printf("Creating tenant metadata")
-	if err := hc.RunSQL(ctx, l, []string{
+	if err := hc.ExecSQL(ctx, l, "", []string{
 		`-e`,
 		fmt.Sprintf(createTenantIfNotExistsQuery, startOpts.TenantID),
 	}); err != nil {

pkg/roachprod/roachprod.go

@@ -419,7 +419,14 @@ func RunWithDetails(
 	return c.RunWithDetails(ctx, l, c.Nodes, title, cmd)
 }
 
-// SQL runs `cockroach sql` on a remote cluster.
+// SQL runs `cockroach sql` on a remote cluster. If a single node is passed,
+// an interactive session may start.
+//
+// NOTE: When querying a single-node in a cluster, a pseudo-terminal is attached
+// to ssh which may result in an _interactive_ ssh session.
+//
+// CAUTION: this function should not be used by roachtest writers. Use syncedCluser.ExecSQL()
+// instead.
 func SQL(
 	ctx context.Context,
 	l *logger.Logger,
@@ -435,7 +442,10 @@ func SQL(
 	if err != nil {
 		return err
 	}
-	return c.SQL(ctx, l, tenantName, cmdArray)
+	if len(c.Nodes) == 1 {
+		return c.ExecOrInteractiveSQL(ctx, l, tenantName, cmdArray)
+	}
+	return c.ExecSQL(ctx, l, tenantName, cmdArray)
 }
 
 // IP gets the ip addresses of the nodes in a cluster.