From 5d65428b293594329057068c52a9512d3254a2cf Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Tue, 26 Nov 2019 11:42:10 -0800 Subject: [PATCH] Handle localhost distributed setups properly (#8577) Fixes an issue reported by @klauspost and @vadmeste This PR also allows users to expand their clusters from single node XL deployment to distributed mode. --- buildscripts/verify-build.sh | 13 +----- cmd/endpoint-ellipses.go | 22 +++++---- cmd/endpoint.go | 36 ++++----------- cmd/endpoint_test.go | 83 +++++++++++++++++----------------- cmd/net.go | 42 ++++++----------- cmd/net_test.go | 69 +++++++++++++++------------- cmd/notification.go | 4 +- cmd/server-main.go | 7 +-- cmd/server-startup-msg.go | 1 - cmd/server-startup-msg_test.go | 2 +- cmd/storage-rest_test.go | 10 +++- pkg/net/host.go | 5 +- pkg/net/host_test.go | 56 +++++++++++++---------- pkg/net/port.go | 6 +++ pkg/net/port_test.go | 3 +- pkg/net/url.go | 17 +++++-- 16 files changed, 189 insertions(+), 187 deletions(-) diff --git a/buildscripts/verify-build.sh b/buildscripts/verify-build.sh index b9c12448c..b0a8e13d0 100755 --- a/buildscripts/verify-build.sh +++ b/buildscripts/verify-build.sh @@ -134,17 +134,6 @@ function start_minio_zone_erasure_sets() declare -a minio_pids export MINIO_ACCESS_KEY=$ACCESS_KEY export MINIO_SECRET_KEY=$SECRET_KEY - "${MINIO[@]}" server --address=:9000 "http://127.0.0.1:9000${WORK_DIR}/zone-disk-sets{1...4}" >/dev/null 2>&1 & - current_pid=$! - - sleep 10 - kill -9 "${current_pid}" - - "${MINIO[@]}" server --address=:9001 "http://127.0.0.1:9001${WORK_DIR}/zone-disk-sets{5...8}" >/dev/null 2>&1 & - current_pid=$! - - sleep 10 - kill -9 "${current_pid}" "${MINIO[@]}" server --address=:9000 "http://127.0.0.1:9000${WORK_DIR}/zone-disk-sets{1...4}" "http://127.0.0.1:9001${WORK_DIR}/zone-disk-sets{5...8}" >"$WORK_DIR/zone-minio-9000.log" 2>&1 & minio_pids[0]=$! @@ -152,7 +141,7 @@ function start_minio_zone_erasure_sets() "${MINIO[@]}" server --address=:9001 "http://127.0.0.1:9000${WORK_DIR}/zone-disk-sets{1...4}" "http://127.0.0.1:9001${WORK_DIR}/zone-disk-sets{5...8}" >"$WORK_DIR/zone-minio-9001.log" 2>&1 & minio_pids[1]=$! - sleep 10 + sleep 35 echo "${minio_pids[@]}" } diff --git a/cmd/endpoint-ellipses.go b/cmd/endpoint-ellipses.go index 46b8be411..9e5f816fe 100644 --- a/cmd/endpoint-ellipses.go +++ b/cmd/endpoint-ellipses.go @@ -283,9 +283,17 @@ func createServerEndpoints(serverAddr string, args ...string) (EndpointZones, in return endpointZones, len(setArgs[0]), setupType, nil } - // Look for duplicate args. - if _, err := GetAllSets(args...); err != nil { - return nil, -1, -1, err + // Verify the args setup-type appropriately. + { + setArgs, err := GetAllSets(args...) + if err != nil { + return nil, -1, -1, err + } + + _, setupType, err = CreateEndpoints(serverAddr, setArgs...) + if err != nil { + return nil, -1, -1, err + } } for _, arg := range args { @@ -293,14 +301,10 @@ func createServerEndpoints(serverAddr string, args ...string) (EndpointZones, in if err != nil { return nil, -1, -1, err } - endpointList, newSetupType, err := CreateEndpoints(serverAddr, setArgs...) + endpointList, _, err := CreateEndpoints(serverAddr, setArgs...) if err != nil { return nil, -1, -1, err } - if setupType != 0 && setupType != newSetupType { - return nil, -1, -1, fmt.Errorf("Mixed modes of operation %s and %s are not allowed", - setupType, newSetupType) - } if drivesPerSet != 0 && drivesPerSet != len(setArgs[0]) { return nil, -1, -1, fmt.Errorf("All zones should have same drive per set ratio - expected %d, got %d", drivesPerSet, len(setArgs[0])) } @@ -310,7 +314,7 @@ func createServerEndpoints(serverAddr string, args ...string) (EndpointZones, in Endpoints: endpointList, }) drivesPerSet = len(setArgs[0]) - setupType = newSetupType } + return endpointZones, drivesPerSet, setupType, nil } diff --git a/cmd/endpoint.go b/cmd/endpoint.go index 999d7d577..1c3f49e8c 100644 --- a/cmd/endpoint.go +++ b/cmd/endpoint.go @@ -80,14 +80,12 @@ func (endpoint Endpoint) HTTPS() bool { } // UpdateIsLocal - resolves the host and updates if it is local or not. -func (endpoint *Endpoint) UpdateIsLocal() error { +func (endpoint *Endpoint) UpdateIsLocal() (err error) { if !endpoint.IsLocal { - isLocal, err := isLocalHost(endpoint.Hostname()) + endpoint.IsLocal, err = isLocalHost(endpoint.Hostname(), endpoint.Port(), globalMinioPort) if err != nil { return err } - endpoint.IsLocal = isLocal - } return nil } @@ -261,7 +259,7 @@ func (endpoints Endpoints) UpdateIsLocal() error { // return err if not Docker or Kubernetes // We use IsDocker() to check for Docker environment // We use IsKubernetes() to check for Kubernetes environment - isLocal, err := isLocalHost(endpoints[i].Hostname()) + isLocal, err := isLocalHost(endpoints[i].Hostname(), endpoints[i].Port(), globalMinioPort) if err != nil { if !IsDocker() && !IsKubernetes() { return err @@ -447,6 +445,10 @@ func CreateEndpoints(serverAddr string, args ...[]string) (Endpoints, SetupType, endpoints = append(endpoints, newEndpoints...) } + if len(endpoints) == 0 { + return endpoints, setupType, config.ErrInvalidErasureEndpoints(nil).Msg("invalid number of endpoints") + } + // Return XL setup when all endpoints are path style. if endpoints[0].Type() == PathEndpointType { setupType = XLSetupType @@ -520,33 +522,11 @@ func CreateEndpoints(serverAddr string, args ...[]string) (Endpoints, SetupType, return endpoints, setupType, config.ErrInvalidErasureEndpoints(nil).Msg("all local endpoints should not have different hostnames/ips") } - endpointPaths := endpointPathSet.ToSlice() - endpoints, _ := NewEndpoints(endpointPaths...) - setupType = XLSetupType - return endpoints, setupType, nil + return endpoints, XLSetupType, nil } // Even though all endpoints are local, but those endpoints use different ports. // This means it is DistXL setup. - } else { - // This is DistXL setup. - // Check whether local server address are not 127.x.x.x - for _, localHost := range localServerHostSet.ToSlice() { - ipList, err := getHostIP(localHost) - logger.FatalIf(err, "unexpected error when resolving host '%s'", localHost) - - // Filter ipList by IPs those start with '127.' or '::1' - loopBackIPs := ipList.FuncMatch(func(ip string, matchString string) bool { - return net.ParseIP(ip).IsLoopback() - }, "") - - // If loop back IP is found and ipList contains only loop back IPs, then error out. - if len(loopBackIPs) > 0 && len(loopBackIPs) == len(ipList) { - err = fmt.Errorf("'%s' resolves to loopback address is not allowed for distributed XL", - localHost) - return endpoints, setupType, err - } - } } // Add missing port in all endpoints. diff --git a/cmd/endpoint_test.go b/cmd/endpoint_test.go index 72746d3cd..a8b7c0acb 100644 --- a/cmd/endpoint_test.go +++ b/cmd/endpoint_test.go @@ -67,9 +67,7 @@ func TestSubOptimalEndpointInput(t *testing.T) { } func TestNewEndpoint(t *testing.T) { - u1, _ := url.Parse("http://localhost/path") u2, _ := url.Parse("https://example.org/path") - u3, _ := url.Parse("http://127.0.0.1:8080/path") u4, _ := url.Parse("http://192.168.253.200/path") testCases := []struct { @@ -91,10 +89,7 @@ func TestNewEndpoint(t *testing.T) { {"http:path", Endpoint{URL: &url.URL{Path: "http:path"}, IsLocal: true}, PathEndpointType, nil}, {"http:/path", Endpoint{URL: &url.URL{Path: "http:/path"}, IsLocal: true}, PathEndpointType, nil}, {"http:///path", Endpoint{URL: &url.URL{Path: "http:/path"}, IsLocal: true}, PathEndpointType, nil}, - {"http://localhost/path", Endpoint{URL: u1, IsLocal: true}, URLEndpointType, nil}, - {"http://localhost/path//", Endpoint{URL: u1, IsLocal: true}, URLEndpointType, nil}, {"https://example.org/path", Endpoint{URL: u2, IsLocal: false}, URLEndpointType, nil}, - {"http://127.0.0.1:8080/path", Endpoint{URL: u3, IsLocal: true}, URLEndpointType, nil}, {"http://192.168.253.200/path", Endpoint{URL: u4, IsLocal: false}, URLEndpointType, nil}, {"", Endpoint{}, -1, fmt.Errorf("empty or root endpoint is not supported")}, {SlashSeparator, Endpoint{}, -1, fmt.Errorf("empty or root endpoint is not supported")}, @@ -112,28 +107,31 @@ func TestNewEndpoint(t *testing.T) { } for _, testCase := range testCases { - endpoint, err := NewEndpoint(testCase.arg) - if err == nil { - err = endpoint.UpdateIsLocal() - } - - if testCase.expectedErr == nil { - if err != nil { - t.Fatalf("error: expected = , got = %v", err) + testCase := testCase + t.Run("", func(t *testing.T) { + endpoint, err := NewEndpoint(testCase.arg) + if err == nil { + err = endpoint.UpdateIsLocal() } - } else if err == nil { - t.Fatalf("error: expected = %v, got = ", testCase.expectedErr) - } else if testCase.expectedErr.Error() != err.Error() { - t.Fatalf("error: expected = %v, got = %v", testCase.expectedErr, err) - } - if err == nil && !reflect.DeepEqual(testCase.expectedEndpoint, endpoint) { - t.Fatalf("endpoint: expected = %+v, got = %+v", testCase.expectedEndpoint, endpoint) - } + if testCase.expectedErr == nil { + if err != nil { + t.Errorf("error: expected = , got = %v", err) + } + } else if err == nil { + t.Errorf("error: expected = %v, got = ", testCase.expectedErr) + } else if testCase.expectedErr.Error() != err.Error() { + t.Errorf("error: expected = %v, got = %v", testCase.expectedErr, err) + } - if err == nil && testCase.expectedType != endpoint.Type() { - t.Fatalf("type: expected = %+v, got = %+v", testCase.expectedType, endpoint.Type()) - } + if err == nil && !reflect.DeepEqual(testCase.expectedEndpoint, endpoint) { + t.Errorf("endpoint: expected = %#v, got = %#v", testCase.expectedEndpoint, endpoint) + } + + if err == nil && testCase.expectedType != endpoint.Type() { + t.Errorf("type: expected = %+v, got = %+v", testCase.expectedType, endpoint.Type()) + } + }) } } @@ -281,26 +279,20 @@ func TestCreateEndpoints(t *testing.T) { Endpoint{URL: &url.URL{Path: "d3"}, IsLocal: true}, Endpoint{URL: &url.URL{Path: "d4"}, IsLocal: true}, }, XLSetupType, nil}, - // XL Setup with URLEndpointType + // DistXL Setup with URLEndpointType {":9000", [][]string{{"http://localhost/d1", "http://localhost/d2", "http://localhost/d3", "http://localhost/d4"}}, ":9000", Endpoints{ - Endpoint{URL: &url.URL{Path: "/d1"}, IsLocal: true}, - Endpoint{URL: &url.URL{Path: "/d2"}, IsLocal: true}, - Endpoint{URL: &url.URL{Path: "/d3"}, IsLocal: true}, - Endpoint{URL: &url.URL{Path: "/d4"}, IsLocal: true}, + Endpoint{URL: &url.URL{Scheme: "http", Host: "localhost", Path: "/d1"}, IsLocal: true}, + Endpoint{URL: &url.URL{Scheme: "http", Host: "localhost", Path: "/d2"}, IsLocal: true}, + Endpoint{URL: &url.URL{Scheme: "http", Host: "localhost", Path: "/d3"}, IsLocal: true}, + Endpoint{URL: &url.URL{Scheme: "http", Host: "localhost", Path: "/d4"}, IsLocal: true}, }, XLSetupType, nil}, - // XL Setup with URLEndpointType having mixed naming to local host. - {"127.0.0.1:10000", [][]string{{"http://localhost/d1", "http://localhost/d2", "http://127.0.0.1/d3", "http://127.0.0.1/d4"}}, ":10000", Endpoints{ - Endpoint{URL: &url.URL{Path: "/d1"}, IsLocal: true}, - Endpoint{URL: &url.URL{Path: "/d2"}, IsLocal: true}, - Endpoint{URL: &url.URL{Path: "/d3"}, IsLocal: true}, - Endpoint{URL: &url.URL{Path: "/d4"}, IsLocal: true}, - }, XLSetupType, fmt.Errorf("all local endpoints should not have different hostname/ips")}, + // DistXL Setup with URLEndpointType having mixed naming to local host. + {"127.0.0.1:10000", [][]string{{"http://localhost/d1", "http://localhost/d2", "http://127.0.0.1/d3", "http://127.0.0.1/d4"}}, "", Endpoints{}, -1, fmt.Errorf("all local endpoints should not have different hostnames/ips")}, + {":9001", [][]string{{"http://10.0.0.1:9000/export", "http://10.0.0.2:9000/export", "http://" + nonLoopBackIP + ":9001/export", "http://10.0.0.2:9001/export"}}, "", Endpoints{}, -1, fmt.Errorf("path '/export' can not be served by different port on same address")}, {":9000", [][]string{{"http://127.0.0.1:9000/export", "http://" + nonLoopBackIP + ":9000/export", "http://10.0.0.1:9000/export", "http://10.0.0.2:9000/export"}}, "", Endpoints{}, -1, fmt.Errorf("path '/export' cannot be served by different address on same server")}, - {":9000", [][]string{{"http://localhost/d1", "http://localhost/d2", "http://example.org/d3", "http://example.com/d4"}}, "", Endpoints{}, -1, fmt.Errorf("'localhost' resolves to loopback address is not allowed for distributed XL")}, - // DistXL type {"127.0.0.1:10000", [][]string{{case1Endpoint1, case1Endpoint2, "http://example.org/d3", "http://example.com/d4"}}, "127.0.0.1:10000", Endpoints{ Endpoint{URL: case1URLs[0], IsLocal: case1LocalFlags[0]}, @@ -354,12 +346,21 @@ func TestCreateEndpoints(t *testing.T) { t.Errorf("error: expected = %v, got = ", testCase.expectedErr) } if err == nil { - if !reflect.DeepEqual(endpoints, testCase.expectedEndpoints) { - t.Errorf("endpoints: expected = %v, got = %v", testCase.expectedEndpoints, endpoints) - } if setupType != testCase.expectedSetupType { t.Errorf("setupType: expected = %v, got = %v", testCase.expectedSetupType, setupType) } + if len(endpoints) != len(testCase.expectedEndpoints) { + t.Errorf("endpoints: expected = %d, got = %d", len(testCase.expectedEndpoints), + len(endpoints)) + } else { + for i, endpoint := range endpoints { + if testCase.expectedEndpoints[i].String() != endpoint.String() { + t.Errorf("endpoints: expected = %s, got = %s", + testCase.expectedEndpoints[i], + endpoint) + } + } + } } if err != nil && testCase.expectedErr == nil { t.Errorf("error: expected = , got = %v, testCase: %v", err, testCase) diff --git a/cmd/net.go b/cmd/net.go index b2b9ed8fb..2287ed74d 100644 --- a/cmd/net.go +++ b/cmd/net.go @@ -22,13 +22,13 @@ import ( "net" "net/url" "sort" - "strconv" "strings" "syscall" "github.com/minio/minio-go/v6/pkg/set" "github.com/minio/minio/cmd/config" "github.com/minio/minio/cmd/logger" + xnet "github.com/minio/minio/pkg/net" ) // IPv4 addresses of local host. @@ -39,13 +39,11 @@ var localIP6 = mustGetLocalIP6() // mustSplitHostPort is a wrapper to net.SplitHostPort() where error is assumed to be a fatal. func mustSplitHostPort(hostPort string) (host, port string) { - host, port, err := net.SplitHostPort(hostPort) - // Strip off IPv6 zone information. - if i := strings.Index(host, "%"); i > -1 { - host = host[:i] + xh, err := xnet.ParseHost(hostPort) + if err != nil { + logger.FatalIf(err, "Unable to split host port %s", hostPort) } - logger.FatalIf(err, "Unable to split host port %s", hostPort) - return host, port + return xh.Name, xh.Port.String() } // mustGetLocalIP4 returns IPv4 addresses of localhost. It panics on error. @@ -273,7 +271,7 @@ func extractHostPort(hostAddr string) (string, string, error) { // isLocalHost - checks if the given parameter // correspond to one of the local IP of the // current machine -func isLocalHost(host string) (bool, error) { +func isLocalHost(host string, port string, localPort string) (bool, error) { hostIPs, err := getHostIP(host) if err != nil { return false, err @@ -282,6 +280,9 @@ func isLocalHost(host string) (bool, error) { // If intersection of two IP sets is not empty, then the host is localhost. isLocalv4 := !localIP4.Intersection(hostIPs).IsEmpty() isLocalv6 := !localIP6.Intersection(hostIPs).IsEmpty() + if port != "" { + return (isLocalv4 || isLocalv6) && (port == localPort), nil + } return isLocalv4 || isLocalv6, nil } @@ -307,7 +308,7 @@ func sameLocalAddrs(addr1, addr2 string) (bool, error) { addr1Local = true } else { // Host not empty, check if it is local - if addr1Local, err = isLocalHost(host1); err != nil { + if addr1Local, err = isLocalHost(host1, port1, port1); err != nil { return false, err } } @@ -317,7 +318,7 @@ func sameLocalAddrs(addr1, addr2 string) (bool, error) { addr2Local = true } else { // Host not empty, check if it is local - if addr2Local, err = isLocalHost(host2); err != nil { + if addr2Local, err = isLocalHost(host2, port2, port2); err != nil { return false, err } } @@ -334,33 +335,20 @@ func sameLocalAddrs(addr1, addr2 string) (bool, error) { // CheckLocalServerAddr - checks if serverAddr is valid and local host. func CheckLocalServerAddr(serverAddr string) error { - host, port, err := net.SplitHostPort(serverAddr) + host, err := xnet.ParseHost(serverAddr) if err != nil { return config.ErrInvalidAddressFlag(err) } - // Strip off IPv6 zone information. - if i := strings.Index(host, "%"); i > -1 { - host = host[:i] - } - - // Check whether port is a valid port number. - p, err := strconv.Atoi(port) - if err != nil { - return config.ErrInvalidAddressFlag(err).Msg("invalid port number") - } else if p < 1 || p > 65535 { - return config.ErrInvalidAddressFlag(nil).Msg("port number must be between 1 to 65535") - } - // 0.0.0.0 is a wildcard address and refers to local network // addresses. I.e, 0.0.0.0:9000 like ":9000" refers to port // 9000 on localhost. - if host != "" && host != net.IPv4zero.String() && host != net.IPv6zero.String() { - isLocalHost, err := isLocalHost(host) + if host.Name != "" && host.Name != net.IPv4zero.String() && host.Name != net.IPv6zero.String() { + localHost, err := isLocalHost(host.Name, host.Port.String(), host.Port.String()) if err != nil { return err } - if !isLocalHost { + if !localHost { return config.ErrInvalidAddressFlag(nil).Msg("host in server address should be this server") } } diff --git a/cmd/net_test.go b/cmd/net_test.go index ee5e9d21c..1b1903c8d 100644 --- a/cmd/net_test.go +++ b/cmd/net_test.go @@ -35,11 +35,9 @@ func TestMustSplitHostPort(t *testing.T) { }{ {":54321", "", "54321"}, {"server:54321", "server", "54321"}, - {":", "", ""}, {":0", "", "0"}, - {":-10", "", "-10"}, - {"server:100000000", "server", "100000000"}, - {"server:https", "server", "https"}, + {"server:https", "server", "443"}, + {"server:http", "server", "80"}, } for _, testCase := range testCases { @@ -242,24 +240,27 @@ func TestCheckLocalServerAddr(t *testing.T) { {":54321", nil}, {"localhost:54321", nil}, {"0.0.0.0:9000", nil}, - {"", fmt.Errorf("missing port in address")}, - {"localhost", fmt.Errorf("address localhost: missing port in address")}, + {":0", nil}, + {"localhost", nil}, + {"", fmt.Errorf("invalid argument")}, {"example.org:54321", fmt.Errorf("host in server address should be this server")}, - {":0", fmt.Errorf("port number must be between 1 to 65535")}, - {":-10", fmt.Errorf("port number must be between 1 to 65535")}, + {":-10", fmt.Errorf("port must be between 0 to 65535")}, } for _, testCase := range testCases { - err := CheckLocalServerAddr(testCase.serverAddr) - if testCase.expectedErr == nil { - if err != nil { - t.Fatalf("error: expected = , got = %v", err) + testCase := testCase + t.Run("", func(t *testing.T) { + err := CheckLocalServerAddr(testCase.serverAddr) + if testCase.expectedErr == nil { + if err != nil { + t.Errorf("error: expected = , got = %v", err) + } + } else if err == nil { + t.Errorf("error: expected = %v, got = ", testCase.expectedErr) + } else if testCase.expectedErr.Error() != err.Error() { + t.Errorf("error: expected = %v, got = %v", testCase.expectedErr, err) } - } else if err == nil { - t.Fatalf("error: expected = %v, got = ", testCase.expectedErr) - } else if testCase.expectedErr.Error() != err.Error() { - t.Fatalf("error: expected = %v, got = %v", testCase.expectedErr, err) - } + }) } } @@ -318,23 +319,27 @@ func TestSameLocalAddrs(t *testing.T) { {"http://8.8.8.8:9000", "http://localhost:9000", false, nil}, } - for i, testCase := range testCases { - sameAddr, err := sameLocalAddrs(testCase.addr1, testCase.addr2) - if testCase.expectedErr != nil && err == nil { - t.Fatalf("Test %d: should fail but succeeded", i+1) - } - if testCase.expectedErr == nil && err != nil { - t.Fatalf("Test %d: should succeed but failed with %v", i+1, err) - } - if err == nil { - if sameAddr != testCase.sameAddr { - t.Fatalf("Test %d: expected: %v, found: %v", i+1, testCase.sameAddr, sameAddr) + for _, testCase := range testCases { + testCase := testCase + t.Run("", func(t *testing.T) { + sameAddr, err := sameLocalAddrs(testCase.addr1, testCase.addr2) + if testCase.expectedErr != nil && err == nil { + t.Errorf("should fail but succeeded") } - } else { - if err.Error() != testCase.expectedErr.Error() { - t.Fatalf("Test %d: failed with different error, expected: '%v', found:'%v'.", i+1, testCase.expectedErr, err) + if testCase.expectedErr == nil && err != nil { + t.Errorf("should succeed but failed with %v", err) } - } + if err == nil { + if sameAddr != testCase.sameAddr { + t.Errorf("expected: %v, found: %v", testCase.sameAddr, sameAddr) + } + } else { + if err.Error() != testCase.expectedErr.Error() { + t.Errorf("failed with different error, expected: '%v', found:'%v'.", + testCase.expectedErr, err) + } + } + }) } } func TestIsHostIP(t *testing.T) { diff --git a/cmd/notification.go b/cmd/notification.go index 52be1490c..b1f4e2bb9 100644 --- a/cmd/notification.go +++ b/cmd/notification.go @@ -742,9 +742,9 @@ func (sys *NotificationSys) initListeners(ctx context.Context, objAPI ObjectLaye } for _, args := range listenerList { - found, err := isLocalHost(args.Addr.Name) + found, err := isLocalHost(args.Addr.Name, args.Addr.Port.String(), args.Addr.Port.String()) if err != nil { - logger.GetReqInfo(ctx).AppendTags("host", args.Addr.Name) + logger.GetReqInfo(ctx).AppendTags("host", args.Addr.String()) logger.LogIf(ctx, err) return err } diff --git a/cmd/server-main.go b/cmd/server-main.go index 03775cb15..292db60fe 100644 --- a/cmd/server-main.go +++ b/cmd/server-main.go @@ -144,6 +144,10 @@ func serverHandleCmdArgs(ctx *cli.Context) { var setupType SetupType var err error + globalMinioAddr = globalCLIContext.Addr + + globalMinioHost, globalMinioPort = mustSplitHostPort(globalMinioAddr) + endpoints := strings.Fields(env.Get(config.EnvEndpoints, "")) if len(endpoints) > 0 { globalEndpoints, globalXLSetDriveCount, setupType, err = createServerEndpoints(globalCLIContext.Addr, endpoints...) @@ -152,11 +156,8 @@ func serverHandleCmdArgs(ctx *cli.Context) { } logger.FatalIf(err, "Invalid command line arguments") - globalMinioAddr = globalCLIContext.Addr logger.LogIf(context.Background(), checkEndpointsSubOptimal(ctx, setupType, globalEndpoints)) - globalMinioHost, globalMinioPort = mustSplitHostPort(globalMinioAddr) - // On macOS, if a process already listens on LOCALIPADDR:PORT, net.Listen() falls back // to IPv6 address ie minio will start listening on IPv6 address whereas another // (non-)minio process is listening on IPv4 of given port. diff --git a/cmd/server-startup-msg.go b/cmd/server-startup-msg.go index a6b245c6c..9bdc4f3ae 100644 --- a/cmd/server-startup-msg.go +++ b/cmd/server-startup-msg.go @@ -158,7 +158,6 @@ func stripStandardPorts(apiEndpoints []string) (newAPIEndpoints []string) { for i, apiEndpoint := range apiEndpoints { u, err := xnet.ParseHTTPURL(apiEndpoint) if err != nil { - newAPIEndpoints[i] = apiEndpoint continue } if globalMinioHost == "" && isNotIPv4(u.Host) { diff --git a/cmd/server-startup-msg_test.go b/cmd/server-startup-msg_test.go index eca74fe8d..3f0f00efd 100644 --- a/cmd/server-startup-msg_test.go +++ b/cmd/server-startup-msg_test.go @@ -103,7 +103,7 @@ func TestStripStandardPorts(t *testing.T) { apiEndpoints = []string{"http://%%%%%:9000"} newAPIEndpoints = stripStandardPorts(apiEndpoints) - if !reflect.DeepEqual(apiEndpoints, newAPIEndpoints) { + if !reflect.DeepEqual([]string{""}, newAPIEndpoints) { t.Fatalf("Expected %#v, got %#v", apiEndpoints, newAPIEndpoints) } diff --git a/cmd/storage-rest_test.go b/cmd/storage-rest_test.go index dcf0eb4dd..b84325cf7 100644 --- a/cmd/storage-rest_test.go +++ b/cmd/storage-rest_test.go @@ -490,6 +490,11 @@ func testStorageAPIRenameFile(t *testing.T, storage StorageAPI) { } func newStorageRESTHTTPServerClient(t *testing.T) (*httptest.Server, *storageRESTClient, config.Config, string) { + prevHost, prevPort := globalMinioHost, globalMinioPort + defer func() { + globalMinioHost, globalMinioPort = prevHost, prevPort + }() + endpointPath, err := ioutil.TempDir("", ".TestStorageREST.") if err != nil { t.Fatalf("unexpected error %v", err) @@ -504,18 +509,21 @@ func newStorageRESTHTTPServerClient(t *testing.T) (*httptest.Server, *storageRES } url.Path = endpointPath + globalMinioHost, globalMinioPort = mustSplitHostPort(url.Host) + endpoint, err := NewEndpoint(url.String()) if err != nil { t.Fatalf("NewEndpoint failed %v", endpoint) } - if err := endpoint.UpdateIsLocal(); err != nil { + if err = endpoint.UpdateIsLocal(); err != nil { t.Fatalf("UpdateIsLocal failed %v", err) } registerStorageRESTHandlers(router, []ZoneEndpoints{{ Endpoints: Endpoints{endpoint}, }}) + restClient := newStorageRESTClient(endpoint) prevGlobalServerConfig := globalServerConfig globalServerConfig = newServerConfig() diff --git a/pkg/net/host.go b/pkg/net/host.go index 9ae7ac022..d20568fe8 100644 --- a/pkg/net/host.go +++ b/pkg/net/host.go @@ -81,9 +81,12 @@ func (host *Host) UnmarshalJSON(data []byte) (err error) { // ParseHost - parses string into Host func ParseHost(s string) (*Host, error) { + if s == "" { + return nil, errors.New("invalid argument") + } isValidHost := func(host string) bool { if host == "" { - return false + return true } if ip := net.ParseIP(host); ip != nil { diff --git a/pkg/net/host_test.go b/pkg/net/host_test.go index 040a7e9ad..57e786c9f 100644 --- a/pkg/net/host_test.go +++ b/pkg/net/host_test.go @@ -137,7 +137,7 @@ func TestHostUnmarshalJSON(t *testing.T) { {[]byte(`"12play"`), &Host{"12play", 0, false}, false}, {[]byte(`"play-minio-io"`), &Host{"play-minio-io", 0, false}, false}, {[]byte(`"play--min.io"`), &Host{"play--min.io", 0, false}, false}, - {[]byte(`":9000"`), nil, true}, + {[]byte(`":9000"`), &Host{"", 9000, true}, false}, {[]byte(`"[fe80::8097:76eb:b397:e067%wlp2s0]"`), &Host{"fe80::8097:76eb:b397:e067%wlp2s0", 0, false}, false}, {[]byte(`"[fe80::8097:76eb:b397:e067]:9000"`), &Host{"fe80::8097:76eb:b397:e067", 9000, true}, false}, {[]byte(`"fe80::8097:76eb:b397:e067%wlp2s0"`), nil, true}, @@ -154,20 +154,23 @@ func TestHostUnmarshalJSON(t *testing.T) { {[]byte(`":"`), nil, true}, } - for i, testCase := range testCases { - var host Host - err := host.UnmarshalJSON(testCase.data) - expectErr := (err != nil) + for _, testCase := range testCases { + testCase := testCase + t.Run("", func(t *testing.T) { + var host Host + err := host.UnmarshalJSON(testCase.data) + expectErr := (err != nil) - if expectErr != testCase.expectErr { - t.Fatalf("test %v: error: expected: %v, got: %v", i+1, testCase.expectErr, expectErr) - } - - if !testCase.expectErr { - if !reflect.DeepEqual(&host, testCase.expectedHost) { - t.Fatalf("test %v: host: expected: %#v, got: %#v", i+1, testCase.expectedHost, host) + if expectErr != testCase.expectErr { + t.Errorf("error: expected: %v, got: %v", testCase.expectErr, expectErr) } - } + + if !testCase.expectErr { + if !reflect.DeepEqual(&host, testCase.expectedHost) { + t.Errorf("host: expected: %#v, got: %#v", testCase.expectedHost, host) + } + } + }) } } @@ -188,7 +191,7 @@ func TestParseHost(t *testing.T) { {"12play", &Host{"12play", 0, false}, false}, {"play-minio-io", &Host{"play-minio-io", 0, false}, false}, {"play--min.io", &Host{"play--min.io", 0, false}, false}, - {":9000", nil, true}, + {":9000", &Host{"", 9000, true}, false}, {"play:", nil, true}, {"play::", nil, true}, {"play:90000", nil, true}, @@ -199,19 +202,22 @@ func TestParseHost(t *testing.T) { {"", nil, true}, } - for i, testCase := range testCases { - host, err := ParseHost(testCase.s) - expectErr := (err != nil) + for _, testCase := range testCases { + testCase := testCase + t.Run("", func(t *testing.T) { + host, err := ParseHost(testCase.s) + expectErr := (err != nil) - if expectErr != testCase.expectErr { - t.Fatalf("test %v: error: expected: %v, got: %v", i+1, testCase.expectErr, expectErr) - } - - if !testCase.expectErr { - if !reflect.DeepEqual(host, testCase.expectedHost) { - t.Fatalf("test %v: host: expected: %#v, got: %#v", i+1, testCase.expectedHost, host) + if expectErr != testCase.expectErr { + t.Errorf("error: expected: %v, got: %v", testCase.expectErr, expectErr) } - } + + if !testCase.expectErr { + if !reflect.DeepEqual(host, testCase.expectedHost) { + t.Errorf("host: expected: %#v, got: %#v", testCase.expectedHost, host) + } + } + }) } } diff --git a/pkg/net/port.go b/pkg/net/port.go index 960d24287..db7319706 100644 --- a/pkg/net/port.go +++ b/pkg/net/port.go @@ -31,6 +31,12 @@ func (p Port) String() string { // ParsePort - parses string into Port func ParsePort(s string) (p Port, err error) { + if s == "https" { + return Port(443), nil + } else if s == "http" { + return Port(80), nil + } + var i int if i, err = strconv.Atoi(s); err != nil { return p, errors.New("invalid port number") diff --git a/pkg/net/port_test.go b/pkg/net/port_test.go index d24874623..db04e9e0d 100644 --- a/pkg/net/port_test.go +++ b/pkg/net/port_test.go @@ -49,10 +49,11 @@ func TestParsePort(t *testing.T) { {"0", Port(0), false}, {"9000", Port(9000), false}, {"65535", Port(65535), false}, + {"http", Port(80), false}, + {"https", Port(443), false}, {"90000", Port(0), true}, {"-10", Port(0), true}, {"", Port(0), true}, - {"http", Port(0), true}, {" 1024", Port(0), true}, } diff --git a/pkg/net/url.go b/pkg/net/url.go index 7e9dc4909..d781fd0dc 100644 --- a/pkg/net/url.go +++ b/pkg/net/url.go @@ -126,12 +126,23 @@ func ParseURL(s string) (u *URL, err error) { return nil, err } - if uu.Host == "" { + if uu.Hostname() == "" { if uu.Scheme != "" { return nil, errors.New("scheme appears with empty host") } - } else if _, err = ParseHost(uu.Host); err != nil { - return nil, err + } else { + portStr := uu.Port() + if portStr == "" { + switch uu.Scheme { + case "http": + portStr = "80" + case "https": + portStr = "443" + } + } + if _, err = ParseHost(net.JoinHostPort(uu.Hostname(), portStr)); err != nil { + return nil, err + } } // Clean path in the URL.