Merge #60854 #61029 #61194 #61240

60854: cli: enhance the `cockroach connect` command r=aaron-crl,itsbilal a=knz

Puts pieces together for #60632. 

See individual commits for details.

Release justification: low risk, high benefit changes to existing functionality


61029: bazel: generate `//go:generate sh generate.sh` within Bazel r=rickystewart a=alan-mas

Part of #57787 work. 

As gazelle is not taking care by itself in any changes related to //go:generate ... (anything that has a go run as a prefix) we are creating a genrule and we need to handle every each of them separately.

We are creating a genrule based on pkg/geo/wkt/generate.sh to avoid change that script as it seems it is only used to "change" a flag (has a TODO task inside it).

Release note: None
Release justification: non-production code change

61194: storage: add noop behavior for bounds setting in pebbleIterator r=sumeerbhola a=sumeerbhola

This is to compensate for the bug fix in Pebble which removes
the noop behavior from Pebble
cockroachdb/pebble#1073

Added a test that checks that the noop behavior is working,
the contract regarding slice stability is followed, and that
the bounds are correct.

Additionally, there are some minor tweaks involving removal
and addition of some defensive code. The additions are
related to the pebbleIterator.reusable field.

Release justification: Fix for high-severity bug in existing
functionality. The Pebble bug fix that needs to be compensated
here could lead to incorrect results from iterators, even prior
to the latest seek optimization in Pebble (though our existing
tests were not failing prior to that latest seek optimization).

Release note: None

61240: vendor: upgrade go-geom to pickup more GEOMETRYCOLLECTION EMPTY fixes r=andyyang890 a=otan

This allows us to further differentiate GEOMETRYCOLLECTION EMPTY from
GEOMETRYCOLLECTION Z/ZM/M EMPTY.

Release justification: low risk change to new functionality

Release note: None

Co-authored-by: Raphael 'kena' Poss <[email protected]>
Co-authored-by: Alanmas <[email protected]>
Co-authored-by: sumeerbhola <[email protected]>
Co-authored-by: Oliver Tan <[email protected]>

BUILD.bazel

@@ -118,6 +118,7 @@ load("@bazel_gazelle//:def.bzl", "gazelle")
 # gazelle:exclude pkg/util/timeutil/pgdate/field_string.go
 # gazelle:exclude pkg/util/timeutil/pgdate/parsemode_string.go
 # gazelle:exclude pkg/workload/schemachange/optype_string.go
+# gazelle:exclude pkg/geo/wkt/wkt_generated.go
 # gazelle:exclude pkg/sql/schemachanger/scop/backfill_visitor_generated.go
 # gazelle:exclude pkg/sql/schemachanger/scop/mutation_visitor_generated.go
 # gazelle:exclude pkg/sql/schemachanger/scop/validation_visitor_generated.go

DEPS.bzl

@@ -3240,8 +3240,8 @@ def go_deps():
         name = "com_github_twpayne_go_geom",
         build_file_proto_mode = "disable_global",
         importpath = "github.com/twpayne/go-geom",
-        sum = "h1:yh2fcro1FLk9uTYi3OSXxtI3JRzaghtsNgaku2ASZbE=",
-        version = "v1.3.7-0.20210224233516-acd1d64d533a",
+        sum = "h1:SRMQNnhXCCgFBGAYFnM8iOSMYcOlOwkaTP3pwRCcuOY=",
+        version = "v1.3.7-0.20210228220813-9d9885b50d3e",
     )
     go_repository(
         name = "com_github_twpayne_go_kml",

go.mod

@@ -139,7 +139,7 @@ require (
 	github.com/spf13/cobra v0.0.5
 	github.com/spf13/pflag v1.0.5
 	github.com/stretchr/testify v1.6.1
-	github.com/twpayne/go-geom v1.3.7-0.20210224233516-acd1d64d533a
+	github.com/twpayne/go-geom v1.3.7-0.20210228220813-9d9885b50d3e
 	github.com/wadey/gocovmerge v0.0.0-20160331181800-b5bfa59ec0ad
 	github.com/xdg/scram v0.0.0-20180814205039-7eeb5667e42c
 	github.com/zabawaba99/go-gitignore v0.0.0-20200117185801-39e6bddfb292

go.sum

@@ -964,8 +964,8 @@ github.com/tinylib/msgp v1.1.1/go.mod h1:+d+yLhGm8mzTaHzB+wgMYrodPfmZrzkirds8fDW
 github.com/tmc/grpc-websocket-proxy v0.0.0-20170815181823-89b8d40f7ca8/go.mod h1:ncp9v5uamzpCO7NfCPTXjqaC+bZgJeR0sMTm6dMHP7U=
 github.com/tmc/grpc-websocket-proxy v0.0.0-20190109142713-0ad062ec5ee5/go.mod h1:ncp9v5uamzpCO7NfCPTXjqaC+bZgJeR0sMTm6dMHP7U=
 github.com/tv42/httpunix v0.0.0-20150427012821-b75d8614f926/go.mod h1:9ESjWnEqriFuLhtthL60Sar/7RFoluCcXsuvEwTV5KM=
-github.com/twpayne/go-geom v1.3.7-0.20210224233516-acd1d64d533a h1:yh2fcro1FLk9uTYi3OSXxtI3JRzaghtsNgaku2ASZbE=
-github.com/twpayne/go-geom v1.3.7-0.20210224233516-acd1d64d533a/go.mod h1:XTyWHR6+l9TUYONbbK4ImUTYbWDCu2ySSPrZmmiA0Pg=
+github.com/twpayne/go-geom v1.3.7-0.20210228220813-9d9885b50d3e h1:SRMQNnhXCCgFBGAYFnM8iOSMYcOlOwkaTP3pwRCcuOY=
+github.com/twpayne/go-geom v1.3.7-0.20210228220813-9d9885b50d3e/go.mod h1:XTyWHR6+l9TUYONbbK4ImUTYbWDCu2ySSPrZmmiA0Pg=
 github.com/twpayne/go-kml v1.5.1 h1:RI0JKh/VzdK/d+ZxdJzt8Ar921KMYPfg9qkw7vsbAGw=
 github.com/twpayne/go-kml v1.5.1/go.mod h1:kz8jAiIz6FIdU2Zjce9qGlVtgFYES9vt7BTPBHf5jl4=
 github.com/twpayne/go-polyline v1.0.0/go.mod h1:ICh24bcLYBX8CknfvNPKqoTbe+eg+MX1NPyJmSBo7pU=

pkg/base/config.go

@@ -167,10 +167,6 @@ type Config struct {
 	// SSLCertsDir is the path to the certificate/key directory.
 	SSLCertsDir string
 
-	// InitToken is a shared initialization token for generating TLS certificates
-	// across multiple nodes.
-	InitToken string
-
 	// User running this process. It could be the user under which
 	// the server is running or the user passed in client calls.
 	User security.SQLUsername

pkg/cli/BUILD.bazel

@@ -189,6 +189,7 @@ go_library(
         "@com_github_cockroachdb_cockroach_go//crdb",
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_cockroachdb_errors//oserror",
+        "@com_github_cockroachdb_logtags//:logtags",
         "@com_github_cockroachdb_pebble//:pebble",
         "@com_github_cockroachdb_pebble//tool",
         "@com_github_cockroachdb_redact//:redact",

pkg/cli/cli.go

@@ -250,6 +250,7 @@ func init() {
 	cockroachCmd.AddCommand(
 		startCmd,
 		startSingleNodeCmd,
+		connectCmd,
 		initCmd,
 		certCmd,
 		// TODO(bilal): Uncomment this when the connect command does something useful.

pkg/cli/cli_test.go

@@ -1408,6 +1408,7 @@ func TestFlagUsage(t *testing.T) {
 Available Commands:
   start             start a node in a multi-node cluster
   start-single-node start a single-node cluster
+  connect           auto-build TLS certificates for use with the start command
   init              initialize a cluster
   cert              create ca, node, and client certs
   sql               open a sql shell

pkg/cli/cliflags/flags.go

@@ -717,8 +717,33 @@ Disable use of "external" IO, such as to S3, GCS, or the file system (nodelocal)
 	}
 
 	InitToken = FlagInfo{
-		Name:        "init-token",
-		Description: `Shared token for initialization of node TLS certificates`,
+		Name: "init-token",
+		Description: `Shared token for initialization of node TLS certificates.
+
+This flag is optional for the 'start' command. When omitted, the 'start'
+command expects the operator to prepare TLS certificates beforehand using
+the 'cert' command.
+
+This flag must be combined with --num-expected-initial-nodes.`,
+	}
+
+	NumExpectedInitialNodes = FlagInfo{
+		Name: "num-expected-initial-nodes",
+		Description: `Number of expected nodes during TLS certificate creation,
+including the node where the connect command is run.
+
+This flag must be combined with --init-token.`,
+	}
+
+	SingleNode = FlagInfo{
+		Name: "single-node",
+		Description: `Prepare the certificates for a subsequent 'start-single-node'
+command. The 'connect' command only runs cursory checks on the network
+configuration and does not wait for peers to auto-negotiate a common
+set of credentials.
+
+The --single-node flag is exclusive with the --init-num-peers and --init-token
+flags.`,
 	}
 
 	CertsDir = FlagInfo{

pkg/cli/connect.go

@@ -13,17 +13,22 @@ package cli
 import (
 	"context"
 	"fmt"
-	"net"
+	"os"
 
+	"github.com/cockroachdb/cockroach/pkg/cli/cliflags"
+	"github.com/cockroachdb/cockroach/pkg/security"
 	"github.com/cockroachdb/cockroach/pkg/server"
+	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/errors"
+	"github.com/cockroachdb/logtags"
 	"github.com/spf13/cobra"
 )
 
 // connectCmd triggers a TLS initialization handshake and writes
 // certificates in the specified certs-dir for use with start.
 var connectCmd = &cobra.Command{
 	Use:   "connect --certs-dir=<path to cockroach certs dir> --init-token=<shared secret> --join=<host 1>,<host 2>,...,<host N>",
-	Short: "build TLS certificates for use with the start command",
+	Short: "auto-build TLS certificates for use with the start command",
 	Long: `
 Connects to other nodes and negotiates an initialization bundle for use with
 secure inter-node connections.
@@ -34,18 +39,115 @@ secure inter-node connections.
 
 // runConnect connects to other nodes and negotiates an initialization bundle
 // for use with secure inter-node connections.
-func runConnect(cmd *cobra.Command, args []string) error {
+func runConnect(cmd *cobra.Command, args []string) (retErr error) {
+	if err := validateConnectFlags(cmd, true /* requireExplicitFlags */); err != nil {
+		return err
+	}
+
+	// If the node cert already exists, skip all the complexity of setting up
+	// servers, etc.
+	cl := security.MakeCertsLocator(baseCfg.SSLCertsDir)
+	if exists, err := cl.HasNodeCert(); err != nil {
+		return err
+	} else if exists {
+		return errors.Newf("node certificate already exists in %s", baseCfg.SSLCertsDir)
+	}
+
+	// Ensure that log files are populated when the process terminates.
+	defer log.Flush()
+
 	peers := []string(serverCfg.JoinList)
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
-	listener, err := net.Listen("tcp", fmt.Sprintf("%s:%s", startCtx.serverListenAddr, serverListenPort))
+	ctx = logtags.AddTag(ctx, "connect", nil)
+
+	log.Infof(ctx, "validating the command line network arguments")
+
+	// Ensure that the default hostnames / ports are filled in for the
+	// various address fields in baseCfg.
+	if err := baseCfg.ValidateAddrs(ctx); err != nil {
+		return err
+	}
+
+	log.Ops.Infof(ctx, "starting the initial network listeners")
+
+	// We are creating listeners so that if the host part of the listen
+	// address means "all interfaces", the Listen call will resolve this
+	// into a concrete network address. We need all separate listeners
+	// because the certs will want to use the advertised addresses.
+	rpcLn, err := server.ListenAndUpdateAddrs(ctx, &baseCfg.Addr, &baseCfg.AdvertiseAddr, "rpc")
+	if err != nil {
+		return err
+	}
+	log.Ops.Infof(ctx, "started rpc listener at: %s", rpcLn.Addr())
+	defer func() {
+		_ = rpcLn.Close()
+	}()
+	httpLn, err := server.ListenAndUpdateAddrs(ctx, &baseCfg.HTTPAddr, &baseCfg.HTTPAdvertiseAddr, "http")
 	if err != nil {
 		return err
 	}
+	log.Ops.Infof(ctx, "started http listener at: %s", httpLn.Addr())
+	if baseCfg.SplitListenSQL {
+		sqlLn, err := server.ListenAndUpdateAddrs(ctx, &baseCfg.SQLAddr, &baseCfg.SQLAdvertiseAddr, "sql")
+		if err != nil {
+			return err
+		}
+		log.Ops.Infof(ctx, "started sql listener at: %s", sqlLn.Addr())
+		_ = sqlLn.Close()
+	}
+	// Note: we want the http listener to remain open while we open the
+	// SQL listener above to detect port conflict in the configuration
+	// properly.
+	_ = httpLn.Close()
+
 	defer func() {
-		_ = listener.Close()
+		if retErr == nil {
+			fmt.Println("server certificate generation complete.\n\n" +
+				"cert files generated in: " + os.ExpandEnv(baseCfg.SSLCertsDir) + "\n\n" +
+				"Do not forget to generate a client certificate for the 'root' user!\n" +
+				"This must be done manually, preferably from a different unix user account\n" +
+				"than the one running the server. Eample command:\n\n" +
+				"   " + os.Args[0] + " cert create-client root --ca-key=<path-to-client-ca-key>\n")
+		}
 	}()
 
-	return server.InitHandshake(ctx, baseCfg, baseCfg.InitToken, len(peers), peers, baseCfg.SSLCertsDir, listener)
+	reporter := func(format string, args ...interface{}) {
+		fmt.Printf(format+"\n", args...)
+	}
+
+	return server.InitHandshake(ctx, reporter, baseCfg, startCtx.initToken, startCtx.numExpectedNodes, peers, baseCfg.SSLCertsDir, rpcLn)
+}
+
+func validateConnectFlags(cmd *cobra.Command, requireExplicitFlags bool) error {
+	if requireExplicitFlags {
+		f := flagSetForCmd(cmd)
+		if !(f.Lookup(cliflags.SingleNode.Name).Changed ||
+			(f.Lookup(cliflags.NumExpectedInitialNodes.Name).Changed && f.Lookup(cliflags.InitToken.Name).Changed)) {
+			return errors.Newf("either --%s must be passed, or both --%s and --%s",
+				cliflags.SingleNode.Name, cliflags.NumExpectedInitialNodes.Name, cliflags.InitToken.Name)
+		}
+		if f.Lookup(cliflags.SingleNode.Name).Changed &&
+			(f.Lookup(cliflags.NumExpectedInitialNodes.Name).Changed || f.Lookup(cliflags.InitToken.Name).Changed) {
+			return errors.Newf("--%s cannot be specified together with --%s or --%s",
+				cliflags.SingleNode.Name, cliflags.NumExpectedInitialNodes.Name, cliflags.InitToken.Name)
+		}
+	}
+
+	if startCtx.genCertsForSingleNode {
+		startCtx.numExpectedNodes = 1
+		startCtx.initToken = "start-single-node"
+		return nil
+	}
+
+	if startCtx.numExpectedNodes < 1 {
+		return errors.Newf("flag --%s must be set to a value greater than or equal to 1",
+			cliflags.NumExpectedInitialNodes.Name)
+	}
+	if startCtx.initToken == "" {
+		return errors.Newf("flag --%s must be set to a non-empty string",
+			cliflags.InitToken.Name)
+	}
+	return nil
 }

pkg/cli/context.go

@@ -413,6 +413,11 @@ var startCtx struct {
 	serverCertPrincipalMap []string
 	serverListenAddr       string
 
+	// The TLS auto-handshake parameters.
+	initToken             string
+	numExpectedNodes      int
+	genCertsForSingleNode bool
+
 	// if specified, this forces the HTTP listen addr to localhost
 	// and disables TLS on the HTTP listener.
 	unencryptedLocalhostHTTP bool
@@ -448,6 +453,9 @@ func setStartContextDefaults() {
 	startCtx.serverSSLCertsDir = base.DefaultCertsDirectory
 	startCtx.serverCertPrincipalMap = nil
 	startCtx.serverListenAddr = ""
+	startCtx.initToken = ""
+	startCtx.numExpectedNodes = 0
+	startCtx.genCertsForSingleNode = false
 	startCtx.unencryptedLocalhostHTTP = false
 	startCtx.tempDir = ""
 	startCtx.externalIODir = ""