Merge #45704 #45727 #45759

45704: colexec: add distinct mode to hashTable r=Azhng a=Azhng

Previously hashTable will buffer all tuples before building `head` and
`same` vector.
Now hashTable supports distinct mode where it only buffers
the distinct tuples from upstream operator. This removes the need of traversing
through the `head` and `same` vectors. Instead, now user of the hashTable
can directly build hashTable in distinct mode and copy the buffered tuples.

Closes #44404

Release note: None

45727: cli: allow for `cockroach demo` to start in secure mode r=knz a=rohany

Fixes #45607.

Release note (cli change): This PR adds support for `cockroach demo`
to start in secure mode using the flag `--insecure=false`.

45759: storage: Do not create nil data pointers in rocksdb slices r=miretskiy a=miretskiy

Fixes #45524

Libroach's DBSlice data type gets converted to the rocksdb::Slice
whenever we need to access underlying rocksdb (iterators, get/set, etc).

However, internally, rocksdb::Slice asserts that the underlying data
pointer may not be null in the rocksdb::Slice::compare as observed in #45524

This change modifies our rocksdb bridge code to never generate
DBSlices with nullptr data.

Release notes: None

Co-authored-by: Azhng <archer.xn@gmail.com>
Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu>
Co-authored-by: Yevgeniy Miretskiy <yevgeniy@cockroachlabs.com>

c-deps/libroach/db.h

@@ -62,8 +62,12 @@ inline std::string ToString(DBSlice s) { return std::string(s.data, s.len); }
 inline std::string ToString(DBString s) { return std::string(s.data, s.len); }
 
 // ToSlice converts a DBSlice/DBString to a rocksdb::Slice.
-inline rocksdb::Slice ToSlice(DBSlice s) { return rocksdb::Slice(s.data, s.len); }
-inline rocksdb::Slice ToSlice(DBString s) { return rocksdb::Slice(s.data, s.len); }
+// Unfortunately, rocksdb::Slice represents empty slice with an
+// empty string (rocksdb::Slice("", 0)), as opposed to nullptr.
+// This is problematic since rocksdb has various
+// assertions checking for slice.data != nullptr.
+inline rocksdb::Slice ToSlice(DBSlice s) { return rocksdb::Slice(s.data == nullptr ? "" : s.data, s.len); }
+inline rocksdb::Slice ToSlice(DBString s) { return rocksdb::Slice(s.data == nullptr ? "" : s.data, s.len); }
 
 // MVCCComputeStatsInternal returns the mvcc stats of the data in an iterator.
 // Stats are only computed for keys between the given range.

pkg/cli/cli_test.go

@@ -408,8 +408,16 @@ func Example_demo() {
 		{`demo`, `--set=errexit=0`, `-e`, `select nonexistent`, `-e`, `select 123 as "123"`},
 		{`demo`, `startrek`, `-e`, `show databases`},
 		{`demo`, `startrek`, `-e`, `show databases`, `--format=table`},
+		// Test that if we start with --insecure=false we can perform
+		// commands that require a secure cluster.
+		{`demo`, `--insecure=false`, `-e`, `CREATE USER test WITH PASSWORD 'testpass'`},
+		{`demo`, `-e`, `CREATE USER test WITH PASSWORD 'testpass'`},
 	}
 	setCLIDefaultsForTests()
+	// We must reset the security asset loader here, otherwise the dummy
+	// asset loader that is set by default in tests will not be able to
+	// find the certs that demo sets up.
+	security.ResetAssetLoader()
 	for _, cmd := range testData {
 		c.RunWithArgs(cmd)
 	}
@@ -457,6 +465,11 @@ func Example_demo() {
 	//   startrek
 	//   system
 	// (4 rows)
+	// demo --insecure=false -e CREATE USER test WITH PASSWORD 'testpass'
+	// CREATE USER 1
+	// demo -e CREATE USER test WITH PASSWORD 'testpass'
+	// ERROR: setting or updating a password is not supported in insecure mode
+	// SQLSTATE: 28P01
 }
 
 func Example_sql() {

pkg/cli/context.go

@@ -162,6 +162,7 @@ func initCLIDefaults() {
 	demoCtx.disableTelemetry = false
 	demoCtx.disableLicenseAcquisition = false
 	demoCtx.transientCluster = nil
+	demoCtx.insecure = true
 
 	authCtx.validityPeriod = 1 * time.Hour
 
@@ -378,4 +379,5 @@ var demoCtx struct {
 	geoPartitionedReplicas    bool
 	simulateLatency           bool
 	transientCluster          *transientCluster
+	insecure                  bool
 }

pkg/cli/demo.go

@@ -14,7 +14,10 @@ import (
 	"context"
 	gosql "database/sql"
 	"fmt"
+	"io/ioutil"
 	"net/url"
+	"os"
+	"path/filepath"
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
@@ -69,6 +72,8 @@ const demoOrg = "Cockroach Demo"
 
 const defaultGeneratorName = "movr"
 
+const defaultRootPassword = "admin"
+
 var defaultGenerator workload.Generator
 
 // maxNodeInitTime is the maximum amount of time to wait for nodes to be connected.
@@ -167,10 +172,12 @@ var GetAndApplyLicense func(dbConn *gosql.DB, clusterID uuid.UUID, org string) (
 
 type transientCluster struct {
 	connURL string
-	stopper *stop.Stopper
-	s       *server.TestServer
-	servers []*server.TestServer
-	cleanup func()
+	// certsDir is only set when demoCtx.insecure is false.
+	certsDir string
+	stopper  *stop.Stopper
+	s        *server.TestServer
+	servers  []*server.TestServer
+	cleanup  func()
 }
 
 // DrainNode will gracefully attempt to drain a node in the cluster.
@@ -287,7 +294,6 @@ func testServerArgsForTransientCluster(nodeID roachpb.NodeID, joinAddr string) b
 
 	args := base.TestServerArgs{
 		PartOfCluster: true,
-		Insecure:      true,
 		Stopper: initBacktrace(
 			fmt.Sprintf("%s/demo-node%d", startCtx.backtraceOutputDir, nodeID),
 		),
@@ -326,6 +332,56 @@ This server is running at increased risk of memory-related failures.`,
 	}
 }
 
+// generateCerts generates some temporary certificates for cockroach demo.
+func generateCerts() (certsDir string, cleanup func(), err error) {
+	certsDir, err = ioutil.TempDir("", "demo_certs")
+	if err != nil {
+		return "", nil, err
+	}
+	caKeyPath := filepath.Join(certsDir, security.EmbeddedCAKey)
+	// Create a CA-Key.
+	if err := security.CreateCAPair(
+		certsDir,
+		caKeyPath,
+		defaultKeySize,
+		defaultCALifetime,
+		false, /* allowKeyReuse */
+		false, /*overwrite */
+	); err != nil {
+		return "", nil, err
+	}
+	// Generate a certificate for the demo nodes.
+	if err := security.CreateNodePair(
+		certsDir,
+		caKeyPath,
+		defaultKeySize,
+		defaultCertLifetime,
+		false, /* overwrite */
+		[]string{"127.0.0.1"},
+	); err != nil {
+		return "", nil, err
+	}
+	// Create a certificate for the root user.
+	if err := security.CreateClientPair(
+		certsDir,
+		caKeyPath,
+		defaultKeySize,
+		defaultCertLifetime,
+		false, /* overwrite */
+		security.RootUser,
+		false, /* generatePKCS8Key */
+	); err != nil {
+		return "", nil, err
+	}
+	cleanup = func() {
+		if err := checkAndMaybeShout(os.RemoveAll(certsDir)); err != nil {
+			// There's nothing to do here anymore if err != nil.
+			_ = err
+		}
+	}
+	return certsDir, cleanup, nil
+}
+
 func setupTransientCluster(
 	ctx context.Context, cmd *cobra.Command, gen workload.Generator,
 ) (c transientCluster, err error) {
@@ -368,6 +424,16 @@ func setupTransientCluster(
 		c.stopper.Stop(ctx)
 	}
 
+	if !demoCtx.insecure {
+		certs, cleanup, err := generateCerts()
+		if err != nil {
+			return c, err
+		}
+		c.certsDir = certs
+		_ = cleanup
+		c.stopper.AddCloser(stop.CloserFn(cleanup))
+	}
+
 	serverFactory := server.TestServerFactory
 	var servers []*server.TestServer
 
@@ -402,6 +468,12 @@ func setupTransientCluster(
 				},
 			}
 		}
+		if demoCtx.insecure {
+			args.Insecure = true
+		} else {
+			args.Insecure = false
+			args.SSLCertsDir = c.certsDir
+		}
 
 		serv := serverFactory.New(args).(*server.TestServer)
 
@@ -524,7 +596,10 @@ func setupTransientCluster(
 	// Prepare the URL for use by the SQL shell.
 	// TODO (rohany): there should be a way that the user can request a specific node
 	//  to connect to to see the effects of the artificial latency.
-	c.connURL = makeURLForServer(c.s, gen)
+	c.connURL, err = makeURLForServer(c.s, gen, c.certsDir)
+	if err != nil {
+		return c, err
+	}
 
 	// Start up the update check loop.
 	// We don't do this in (*server.Server).Start() because we don't want it
@@ -616,7 +691,11 @@ func (c *transientCluster) setupWorkload(ctx context.Context, gen workload.Gener
 		if demoCtx.runWorkload {
 			var sqlURLs []string
 			for i := range c.servers {
-				sqlURLs = append(sqlURLs, makeURLForServer(c.servers[i], gen))
+				sqlURL, err := makeURLForServer(c.servers[i], gen, c.certsDir)
+				if err != nil {
+					return err
+				}
+				sqlURLs = append(sqlURLs, sqlURL)
 			}
 			if err := c.runWorkload(ctx, gen, sqlURLs); err != nil {
 				return errors.Wrapf(err, "starting background workload")
@@ -675,9 +754,18 @@ func (c *transientCluster) runWorkload(
 	return nil
 }
 
-func makeURLForServer(s *server.TestServer, gen workload.Generator) string {
+func makeURLForServer(
+	s *server.TestServer, gen workload.Generator, certPath string,
+) (string, error) {
 	options := url.Values{}
-	options.Add("sslmode", "disable")
+	cfg := base.Config{
+		SSLCertsDir: certPath,
+		User:        security.RootUser,
+		Insecure:    demoCtx.insecure,
+	}
+	if err := cfg.LoadSecurityOptions(options, security.RootUser); err != nil {
+		return "", err
+	}
 	options.Add("application_name", sqlbase.ReportableAppNamePrefix+"cockroach demo")
 	sqlURL := url.URL{
 		Scheme:   "postgres",
@@ -688,7 +776,23 @@ func makeURLForServer(s *server.TestServer, gen workload.Generator) string {
 	if gen != nil {
 		sqlURL.Path = gen.Meta().Name
 	}
-	return sqlURL.String()
+	return sqlURL.String(), nil
+}
+
+func (c *transientCluster) setupUserAuth(ctx context.Context) error {
+	db, err := gosql.Open("postgres", c.connURL)
+	if err != nil {
+		return err
+	}
+	if _, err := db.ExecContext(
+		ctx,
+		`ALTER USER $1 WITH PASSWORD $2`,
+		security.RootUser,
+		defaultRootPassword,
+	); err != nil {
+		return err
+	}
+	return nil
 }
 
 func incrementTelemetryCounters(cmd *cobra.Command) {
@@ -822,7 +926,13 @@ func runDemo(cmd *cobra.Command, gen workload.Generator) (err error) {
 	}
 
 	if err := c.setupWorkload(ctx, gen); err != nil {
-		return err
+		return checkAndMaybeShout(err)
+	}
+
+	if !demoCtx.insecure {
+		if err := c.setupUserAuth(ctx); err != nil {
+			return checkAndMaybeShout(err)
+		}
 	}
 
 	if cliCtx.isInteractive {
@@ -837,6 +947,14 @@ func runDemo(cmd *cobra.Command, gen workload.Generator) (err error) {
 # Web UI: %s
 #
 `, c.s.AdminURL())
+
+		if !demoCtx.insecure {
+			fmt.Printf(
+				"# The user %q with password %q has been created. Use it to access the Web UI!\n#\n",
+				security.RootUser,
+				defaultRootPassword,
+			)
+		}
 	}
 
 	conn := makeSQLConn(c.connURL)

pkg/cli/demo_locality_test.go

@@ -12,6 +12,8 @@
 
 package cli
 
+import "github.com/cockroachdb/cockroach/pkg/security"
+
 func Example_demo_locality() {
 	c := newCLITest(cliTestParams{noServer: true})
 	defer c.cleanup()
@@ -23,6 +25,10 @@ func Example_demo_locality() {
 			`-e`, `select node_id, locality from crdb_internal.gossip_nodes order by node_id`},
 	}
 	setCLIDefaultsForTests()
+	// We must reset the security asset loader here, otherwise the dummy
+	// asset loader that is set by default in tests will not be able to
+	// find the certs that demo sets up.
+	security.ResetAssetLoader()
 	for _, cmd := range testData {
 		c.RunWithArgs(cmd)
 	}

pkg/cli/demo_test.go

@@ -38,7 +38,6 @@ func TestTestServerArgsForTransientCluster(t *testing.T) {
 			cacheSize:         1 << 10,
 			expected: base.TestServerArgs{
 				PartOfCluster:     true,
-				Insecure:          true,
 				JoinAddr:          "127.0.0.1",
 				SQLMemoryPoolSize: 2 << 10,
 				CacheSize:         1 << 10,
@@ -51,7 +50,6 @@ func TestTestServerArgsForTransientCluster(t *testing.T) {
 			cacheSize:         4 << 10,
 			expected: base.TestServerArgs{
 				PartOfCluster:     true,
-				Insecure:          true,
 				JoinAddr:          "127.0.0.1",
 				SQLMemoryPoolSize: 4 << 10,
 				CacheSize:         4 << 10,

pkg/cli/flags.go

@@ -605,6 +605,7 @@ func init() {
 	BoolFlag(demoFlags, &demoCtx.geoPartitionedReplicas, cliflags.DemoGeoPartitionedReplicas, false)
 	VarFlag(demoFlags, demoNodeSQLMemSizeValue, cliflags.DemoNodeSQLMemSize)
 	VarFlag(demoFlags, demoNodeCacheSizeValue, cliflags.DemoNodeCacheSize)
+	BoolFlag(demoFlags, &demoCtx.insecure, cliflags.ServerInsecure, true)
 	// Mark the --global flag as hidden until we investigate it more.
 	BoolFlag(demoFlags, &demoCtx.simulateLatency, cliflags.Global, false)
 	_ = demoFlags.MarkHidden(cliflags.Global.Name)

pkg/cli/interactive_tests/test_demo.tcl

@@ -19,3 +19,23 @@ eexpect root@
 # Ensure db is movr.
 eexpect "movr>"
 end_test
+
+start_test "Check that demo secure mode says hello properly"
+spawn $argv demo --insecure=false
+# Be polite.
+eexpect "Welcome"
+# Warn the user that they won't get persistence.
+eexpect "your changes to data stored in the demo session will not be saved!"
+# Inform the necessary URL.
+eexpect "Web UI: https:"
+# Ensure that user login information is present.
+eexpect "The user \"root\" with password \"admin\" has been created."
+# Ensure same messages as cockroach sql.
+eexpect "Server version"
+eexpect "Cluster ID"
+eexpect "brief introduction"
+# Ensure user is root.
+eexpect root@
+# Ensure db is movr.
+eexpect "movr>"
+end_test