Skip to content

Commit

Permalink
Merge #38924
Browse files Browse the repository at this point in the history 
38924: cli: IPv6-related fixes r=knz a=knz

Fixes #38882.

Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
  • Loading branch information
@knz
craig[bot] and knz committed Jul 17, 2019
2 parents 90fc142 + 3e809df commit a70553f
Show file tree
Hide file tree
Showing 12 changed files with 260 additions and 56 deletions.
24 changes: 23 additions & 1 deletion pkg/base/store_spec.go
View file
Expand Up @@ -13,6 +13,7 @@ package base
import (
"bytes"
"fmt"
"net"
"path/filepath"
"regexp"
"sort"
Expand All @@ -22,6 +23,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/cli/cliflags"
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/util/humanizeutil"
"github.com/cockroachdb/cockroach/pkg/util/netutil"
humanize "github.com/dustin/go-humanize"
"github.com/pkg/errors"
"github.com/spf13/pflag"
Expand Down Expand Up @@ -398,6 +400,26 @@ func (jls *JoinListType) Type() string {
// Set adds a new value to the JoinListType. It is the important part of
// pflag's value interface.
func (jls *JoinListType) Set(value string) error {
*jls = append(*jls, value)
if strings.TrimSpace(value) == "" {
// No value, likely user error.
return errors.New("no address specified in --join")
}
for _, v := range strings.Split(value, ",") {
v = strings.TrimSpace(v)
if v == "" {
// --join=a,,b equivalent to --join=a,b
continue
}
// Try splitting the address. This validates the format
// of the address and tolerates a missing delimiter colon
// between the address and port number.
addr, port, err := netutil.SplitHostPort(v, "")
if err != nil {
return err
}
// Re-join the parts. This guarantees an address that
// will be valid for net.SplitHostPort().
*jls = append(*jls, net.JoinHostPort(addr, port))
}
return nil
}
54 changes: 50 additions & 4 deletions pkg/base/store_spec_test.go
View file
Expand Up @@ -8,14 +8,16 @@
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

package base
package base_test

import (
"fmt"
"reflect"
"testing"

"github.com/cockroachdb/cockroach/pkg/base"
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/testutils"
"github.com/cockroachdb/cockroach/pkg/util/leaktest"
)

Expand All @@ -27,7 +29,7 @@ func TestNewStoreSpec(t *testing.T) {
testCases := []struct {
value string
expectedErr string
expected StoreSpec
expected base.StoreSpec
}{
// path
{"path=/mnt/hda1", "", StoreSpec{Path: "/mnt/hda1"}},
Expand Down Expand Up @@ -137,7 +139,7 @@ func TestNewStoreSpec(t *testing.T) {
}

for i, testCase := range testCases {
storeSpec, err := NewStoreSpec(testCase.value)
storeSpec, err := base.NewStoreSpec(testCase.value)
if err != nil {
if len(testCase.expectedErr) == 0 {
t.Errorf("%d(%s): no expected error, got %s", i, testCase.value, err)
Expand All @@ -159,7 +161,7 @@ func TestNewStoreSpec(t *testing.T) {

// Now test String() to make sure the result can be parsed.
storeSpecString := storeSpec.String()
storeSpec2, err := NewStoreSpec(storeSpecString)
storeSpec2, err := base.NewStoreSpec(storeSpecString)
if err != nil {
t.Errorf("%d(%s): error parsing String() result: %s", i, testCase.value, err)
continue
Expand All @@ -171,3 +173,47 @@ func TestNewStoreSpec(t *testing.T) {
}
}
}

// StoreSpec aliases base.StoreSpec for convenience.
type StoreSpec = base.StoreSpec

// SizeSpec aliases base.SizeSpec for convenience.
type SizeSpec = base.SizeSpec

func TestJoinListType(t *testing.T) {
defer leaktest.AfterTest(t)()

testData := []struct {
join string
exp string
err string
}{
{"", "", "no address specified in --join"},
{":", "--join=:", ""},
{"a", "--join=a:", ""},
{"a,b", "--join=a: --join=b:", ""},
{"a,,b", "--join=a: --join=b:", ""},
{",a", "--join=a:", ""},
{"a,", "--join=a:", ""},
{"a:123,b", "--join=a:123 --join=b:", ""},
{"[::1]:123,b", "--join=[::1]:123 --join=b:", ""},
{"[::1,b", "", `address \[::1: missing ']' in address`},
}

for _, test := range testData {
t.Run(test.join, func(t *testing.T) {
var jls base.JoinListType
err := jls.Set(test.join)
if !testutils.IsError(err, test.err) {
t.Fatalf("error: expected %q, got: %+v", test.err, err)
}
if test.err != "" {
return
}
actual := jls.String()
if actual != test.exp {
t.Errorf("expected: %q, got: %q", test.exp, actual)
}
})
}
}
2 changes: 2 additions & 0 deletions pkg/cli/context.go
View file
Expand Up @@ -106,6 +106,8 @@ func initCLIDefaults() {
serverCfg.ReadyFn = nil
serverCfg.DelayedBootstrapFn = nil
serverCfg.SocketFile = ""
serverCfg.JoinList = nil

startCtx.serverInsecure = baseCfg.Insecure
startCtx.serverSSLCertsDir = base.DefaultCertsDirectory
startCtx.serverListenAddr = ""
Expand Down
24 changes: 2 additions & 22 deletions pkg/cli/flags.go
View file
Expand Up @@ -12,7 +12,6 @@ package cli

import (
"flag"
"fmt"
"net"
"strings"
"time"
Expand All @@ -24,6 +23,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/util/humanizeutil"
"github.com/cockroachdb/cockroach/pkg/util/log"
"github.com/cockroachdb/cockroach/pkg/util/log/logflags"
"github.com/cockroachdb/cockroach/pkg/util/netutil"
"github.com/spf13/cobra"
"github.com/spf13/pflag"
)
Expand Down Expand Up @@ -172,27 +172,7 @@ func (a addrSetter) Type() string { return "<addr/host>[:<port>]" }

// Set implement the pflag.Value interface.
func (a addrSetter) Set(v string) error {
addr, port, err := net.SplitHostPort(v)
if err != nil {
if aerr, ok := err.(*net.AddrError); ok {
if strings.HasPrefix(aerr.Err, "too many colons") {
// Maybe this was an IPv6 address using the deprecated syntax
// without '[...]'. Try that.
// Note: the following is valid even if *a.port is empty.
// (An empty port number is always a valid listen address.)
maybeAddr := "[" + v + "]:" + *a.port
addr, port, err = net.SplitHostPort(maybeAddr)
if err == nil {
fmt.Fprintf(stderr,
"warning: the syntax \"%s\" for IPv6 addresses is deprecated; use \"[%s]\"\n", v, v)
}
} else if strings.HasPrefix(aerr.Err, "missing port") {
// It's inconvenient that SplitHostPort doesn't know how to ignore
// a missing port number. Oh well.
addr, port, err = net.SplitHostPort(v + ":" + *a.port)
}
}
}
addr, port, err := netutil.SplitHostPort(v, *a.port)
if err != nil {
return err
}
Expand Down
66 changes: 59 additions & 7 deletions pkg/cli/flags_test.go
View file
Expand Up @@ -15,13 +15,16 @@ import (
"flag"
"fmt"
"io/ioutil"
"os"
"path/filepath"
"reflect"
"strings"
"testing"
"time"

"github.com/cockroachdb/cockroach/pkg/base"
"github.com/cockroachdb/cockroach/pkg/cli/cliflags"
"github.com/cockroachdb/cockroach/pkg/gossip/resolver"
"github.com/cockroachdb/cockroach/pkg/server/status"
"github.com/cockroachdb/cockroach/pkg/testutils"
"github.com/cockroachdb/cockroach/pkg/testutils/buildutil"
Expand Down Expand Up @@ -397,8 +400,8 @@ func TestServerConnSettings(t *testing.T) {
{[]string{"start", "--port", "12345"}, ":12345", ":12345", "[]"},
{[]string{"start", "--advertise-host", "192.168.0.111"}, ":" + base.DefaultPort, "192.168.0.111:" + base.DefaultPort, "[]"},
{[]string{"start", "--advertise-addr", "192.168.0.111", "--advertise-port", "12345"}, ":" + base.DefaultPort, "192.168.0.111:12345", "[]"},
{[]string{"start", "--listen-addr", "::1"}, "[::1]:" + base.DefaultPort, "[::1]:" + base.DefaultPort, "[]"},
{[]string{"start", "--listen-addr", "2622:6221:e663:4922:fc2b:788b:fadd:7b48", "[]"},
{[]string{"start", "--listen-addr", "[::1]"}, "[::1]:" + base.DefaultPort, "[::1]:" + base.DefaultPort, "[]"},
{[]string{"start", "--listen-addr", "[2622:6221:e663:4922:fc2b:788b:fadd:7b48]", "[]"},
"[2622:6221:e663:4922:fc2b:788b:fadd:7b48]:" + base.DefaultPort, "[2622:6221:e663:4922:fc2b:788b:fadd:7b48]:" + base.DefaultPort, "[]"},
{[]string{"start", "--listen-addr", "127.0.0.1", "--port", "12345"}, "127.0.0.1:12345", "127.0.0.1:12345", "[]"},
{[]string{"start", "--listen-addr", "127.0.0.1", "--advertise-addr", "192.168.0.111", "--port", "12345"}, "127.0.0.1:12345", "192.168.0.111:12345", "[]"},
Expand Down Expand Up @@ -449,6 +452,58 @@ func TestServerConnSettings(t *testing.T) {
}
}

func TestServerJoinSettings(t *testing.T) {
defer leaktest.AfterTest(t)()

// Avoid leaking configuration changes after the tests end.
defer initCLIDefaults()

f := StartCmd.Flags()
testData := []struct {
args []string
expectedJoin []string
}{
{[]string{"start", "--join=a"}, []string{"a:" + base.DefaultPort}},
{[]string{"start", "--join=:"}, []string{"HOSTNAME:" + base.DefaultPort}},
{[]string{"start", "--join=:123"}, []string{"HOSTNAME:123"}},
{[]string{"start", "--join=a,b,c"}, []string{"a:" + base.DefaultPort, "b:" + base.DefaultPort, "c:" + base.DefaultPort}},
{[]string{"start", "--join=a", "--join=b"}, []string{"a:" + base.DefaultPort, "b:" + base.DefaultPort}},
{[]string{"start", "--join=127.0.0.1"}, []string{"127.0.0.1:" + base.DefaultPort}},
{[]string{"start", "--join=127.0.0.1:"}, []string{"127.0.0.1:" + base.DefaultPort}},
{[]string{"start", "--join=127.0.0.1,abc"}, []string{"127.0.0.1:" + base.DefaultPort, "abc:" + base.DefaultPort}},
{[]string{"start", "--join=[::1],[::2]"}, []string{"[::1]:" + base.DefaultPort, "[::2]:" + base.DefaultPort}},
{[]string{"start", "--join=[::1]:123,[::2]"}, []string{"[::1]:123", "[::2]:" + base.DefaultPort}},
{[]string{"start", "--join=[::1],127.0.0.1"}, []string{"[::1]:" + base.DefaultPort, "127.0.0.1:" + base.DefaultPort}},
{[]string{"start", "--join=[::1]:123", "--join=[::2]"}, []string{"[::1]:123", "[::2]:" + base.DefaultPort}},
}

for i, td := range testData {
initCLIDefaults()
if err := f.Parse(td.args); err != nil {
t.Fatalf("Parse(%#v) got unexpected error: %v", td.args, err)
}

extraClientFlagInit()

var actual []string
myHostname, _ := os.Hostname()
for _, addr := range serverCfg.JoinList {
res, err := resolver.NewResolver(addr)
if err != nil {
t.Error(err)
}
actualAddr := res.Addr()
// Normalize the local hostname to make the test location-agnostic.
actualAddr = strings.ReplaceAll(actualAddr, myHostname, "HOSTNAME")
actual = append(actual, actualAddr)
}
if !reflect.DeepEqual(td.expectedJoin, actual) {
t.Errorf("%d. serverCfg.JoinList expected %#v, but got %#v. td.args was '%#v'.",
i, td.expectedJoin, actual, td.args)
}
}
}

func TestClientConnSettings(t *testing.T) {
defer leaktest.AfterTest(t)()

Expand Down Expand Up @@ -482,9 +537,6 @@ func TestClientConnSettings(t *testing.T) {
// Deprecated syntax.
{[]string{"quit", "--port", "12345"}, ":12345"},
{[]string{"quit", "--host", "127.0.0.1", "--port", "12345"}, "127.0.0.1:12345"},
{[]string{"quit", "--host", "::1"}, "[::1]:" + base.DefaultPort},
{[]string{"quit", "--host", "2622:6221:e663:4922:fc2b:788b:fadd:7b48"},
"[2622:6221:e663:4922:fc2b:788b:fadd:7b48]:" + base.DefaultPort},
}

for i, td := range testData {
Expand Down Expand Up @@ -524,8 +576,8 @@ func TestHttpHostFlagValue(t *testing.T) {
{[]string{"start", "--" + cliflags.ListenHTTPAddr.Name, "my.host.name"}, "my.host.name:" + base.DefaultHTTPPort},
{[]string{"start", "--" + cliflags.ListenHTTPAddr.Name, "myhostname"}, "myhostname:" + base.DefaultHTTPPort},
// confirm IPv6 works too
{[]string{"start", "--" + cliflags.ListenHTTPAddr.Name, "::1"}, "[::1]:" + base.DefaultHTTPPort},
{[]string{"start", "--" + cliflags.ListenHTTPAddr.Name, "2622:6221:e663:4922:fc2b:788b:fadd:7b48"}, "[2622:6221:e663:4922:fc2b:788b:fadd:7b48]:" + base.DefaultHTTPPort},
{[]string{"start", "--" + cliflags.ListenHTTPAddr.Name, "[::1]"}, "[::1]:" + base.DefaultHTTPPort},
{[]string{"start", "--" + cliflags.ListenHTTPAddr.Name, "[2622:6221:e663:4922:fc2b:788b:fadd:7b48]"}, "[2622:6221:e663:4922:fc2b:788b:fadd:7b48]:" + base.DefaultHTTPPort},
}

for i, td := range testData {
Expand Down
5 changes: 2 additions & 3 deletions pkg/cli/start.go
View file
Expand Up @@ -907,9 +907,8 @@ func hintServerCmdFlags(ctx context.Context, cmd *cobra.Command) {

func clientFlags() string {
flags := []string{os.Args[0], "<client cmd>"}
host, port, err := net.SplitHostPort(serverCfg.AdvertiseAddr)
if err == nil {
flags = append(flags, "--host="+host+":"+port)
if serverCfg.AdvertiseAddr != "" {
flags = append(flags, "--host="+serverCfg.AdvertiseAddr)
}
if startCtx.serverInsecure {
flags = append(flags, "--insecure")
Expand Down
6 changes: 3 additions & 3 deletions pkg/cli/start_test.go
View file
Expand Up @@ -44,15 +44,15 @@ func TestInitInsecure(t *testing.T) {
{[]string{"--insecure=false"}, false, ""},
{[]string{"--listen-addr", "localhost"}, false, ""},
{[]string{"--listen-addr", "127.0.0.1"}, false, ""},
{[]string{"--listen-addr", "::1"}, false, ""},
{[]string{"--listen-addr", "[::1]"}, false, ""},
{[]string{"--listen-addr", "192.168.1.1"}, false,
`specify --insecure to listen on external address 192\.168\.1\.1`},
{[]string{"--insecure", "--listen-addr", "192.168.1.1"}, true, ""},
{[]string{"--listen-addr", "localhost", "--advertise-addr", "192.168.1.1"}, false, ""},
{[]string{"--listen-addr", "127.0.0.1", "--advertise-addr", "192.168.1.1"}, false, ""},
{[]string{"--listen-addr", "127.0.0.1", "--advertise-addr", "192.168.1.1", "--advertise-port", "36259"}, false, ""},
{[]string{"--listen-addr", "::1", "--advertise-addr", "192.168.1.1"}, false, ""},
{[]string{"--listen-addr", "::1", "--advertise-addr", "192.168.1.1", "--advertise-port", "36259"}, false, ""},
{[]string{"--listen-addr", "[::1]", "--advertise-addr", "192.168.1.1"}, false, ""},
{[]string{"--listen-addr", "[::1]", "--advertise-addr", "192.168.1.1", "--advertise-port", "36259"}, false, ""},
{[]string{"--insecure", "--listen-addr", "192.168.1.1", "--advertise-addr", "192.168.1.1"}, true, ""},
{[]string{"--insecure", "--listen-addr", "192.168.1.1", "--advertise-addr", "192.168.2.2"}, true, ""},
{[]string{"--insecure", "--listen-addr", "192.168.1.1", "--advertise-addr", "192.168.2.2", "--advertise-port", "36259"}, true, ""},
Expand Down
19 changes: 6 additions & 13 deletions pkg/server/config.go
View file
Expand Up @@ -141,8 +141,7 @@ type Config struct {
Attrs string

// JoinList is a list of node addresses that act as bootstrap hosts for
// connecting to the gossip network. Each item in the list can actually be
// multiple comma-separated addresses, kept for backward-compatibility.
// connecting to the gossip network.
JoinList base.JoinListType

// RetryOptions controls the retry behavior of the server.
Expand Down Expand Up @@ -576,18 +575,12 @@ func (cfg *Config) readEnvironmentVariables() {
// parseGossipBootstrapResolvers parses list of gossip bootstrap resolvers.
func (cfg *Config) parseGossipBootstrapResolvers() ([]resolver.Resolver, error) {
var bootstrapResolvers []resolver.Resolver
for _, commaSeparatedAddresses := range cfg.JoinList {
addresses := strings.Split(commaSeparatedAddresses, ",")
for _, address := range addresses {
if len(address) == 0 {
continue
}
resolver, err := resolver.NewResolver(address)
if err != nil {
return nil, err
}
bootstrapResolvers = append(bootstrapResolvers, resolver)
for _, address := range cfg.JoinList {
resolver, err := resolver.NewResolver(address)
if err != nil {
return nil, err
}
bootstrapResolvers = append(bootstrapResolvers, resolver)
}

return bootstrapResolvers, nil
Expand Down
8 changes: 6 additions & 2 deletions pkg/server/config_test.go
View file
Expand Up @@ -51,7 +51,7 @@ func TestParseInitNodeAttributes(t *testing.T) {
func TestParseJoinUsingAddrs(t *testing.T) {
defer leaktest.AfterTest(t)()
cfg := MakeConfig(context.TODO(), cluster.MakeTestingClusterSettings())
cfg.JoinList = []string{"localhost:12345,,localhost:23456", "localhost:34567"}
cfg.JoinList = []string{"localhost:12345", "localhost:23456", "localhost:34567", "localhost"}
cfg.Stores = base.StoreSpecList{Specs: []base.StoreSpec{{InMemory: true, Size: base.SizeSpec{InBytes: base.MinimumStoreSize * 100}}}}
engines, err := cfg.CreateEngines(context.TODO())
if err != nil {
Expand All @@ -73,7 +73,11 @@ func TestParseJoinUsingAddrs(t *testing.T) {
if err != nil {
t.Fatal(err)
}
expected := []resolver.Resolver{r1, r2, r3}
r4, err := resolver.NewResolver("localhost:26257")
if err != nil {
t.Fatal(err)
}
expected := []resolver.Resolver{r1, r2, r3, r4}
if !reflect.DeepEqual(cfg.GossipBootstrapResolvers, expected) {
t.Fatalf("Unexpected bootstrap addresses: %v, expected: %v", cfg.GossipBootstrapResolvers, expected)
}
Expand Down
4 changes: 3 additions & 1 deletion pkg/server/testserver.go
View file
Expand Up @@ -152,7 +152,9 @@ func makeTestConfigFromParams(params base.TestServerArgs) Config {
cfg.SQLMemoryPoolSize = params.SQLMemoryPoolSize
}

cfg.JoinList = []string{params.JoinAddr}
if params.JoinAddr != "" {
cfg.JoinList = []string{params.JoinAddr}
}
if cfg.Insecure {
// Whenever we can (i.e. in insecure mode), use IsolatedTestAddr
// to prevent issues that can occur when running a test under
Expand Down

0 comments on commit a70553f

Please sign in to comment.