server: checkEndpoints syntax properly. (#3451)

cmd/server-main.go

@@ -17,7 +17,6 @@
 package cmd
 
 import (
-	"errors"
 	"fmt"
 	"io/ioutil"
 	"net"
@@ -261,48 +260,36 @@ func isDistributedSetup(eps []*url.URL) bool {
 	return false
 }
 
-// We just exit for invalid endpoints.
+// Check if the endpoints are following expected syntax, i.e
-func checkEndpointsSyntax(eps []*url.URL, disks []string) {
+// valid scheme, valid path across all platforms.
+func checkEndpointsSyntax(eps []*url.URL, disks []string) error {
 	for i, u := range eps {
 		switch u.Scheme {
-		case "", "http", "https":
+		case "":
-			// Scheme is "" for FS and singlenode-XL, hence pass.
+			// "/" is not allowed.
+			if u.Path == "" || u.Path == "/" {
+				return fmt.Errorf("Root path is not allowed : %s (%s)", u.Path, disks[i])
+			}
+		case "http", "https":
+			// "http://server1/" is not allowed
+			if u.Path == "" || u.Path == "/" || u.Path == "\\" {
+				return fmt.Errorf("Root path is not allowed : %s (%s)", u.Path, disks[i])
+			}
 		default:
 			if runtime.GOOS == "windows" {
 				// On windows for "C:\export" scheme will be "C"
 				matched, err := regexp.MatchString("^[a-zA-Z]$", u.Scheme)
-				fatalIf(err, "Parsing scheme : %s (%s)", u.Scheme, disks[i])
+				if err != nil {
+					return fmt.Errorf("Invalid scheme : %s (%s), ERROR %s", u.Scheme, disks[i], err)
+				}
 				if matched {
 					break
 				}
 			}
-			fatalIf(errInvalidArgument, "Invalid scheme : %s (%s)", u.Scheme, disks[i])
+			return fmt.Errorf("Invalid scheme : %s (%s)", u.Scheme, disks[i])
 		}
-		if runtime.GOOS == "windows" {
-			if u.Scheme == "http" || u.Scheme == "https" {
-				// "http://server1/" is not allowed
-				if u.Path == "" || u.Path == "/" || u.Path == "\\" {
-					fatalIf(errInvalidArgument, "Empty path for %s", disks[i])
-				}
-			}
-		} else {
-			if u.Scheme == "http" || u.Scheme == "https" {
-				// "http://server1/" is not allowed.
-				if u.Path == "" || u.Path == "/" {
-					fatalIf(errInvalidArgument, "Empty path for %s", disks[i])
-				}
-			} else {
-				// "/" is not allowed.
-				if u.Path == "" || u.Path == "/" {
-					fatalIf(errInvalidArgument, "Empty path for %s", disks[i])
-				}
-			}
-		}
-	}
-
-	if err := checkDuplicateEndpoints(eps); err != nil {
-		fatalIf(errInvalidArgument, "Duplicate entries in %s", strings.Join(disks, " "))
 	}
+	return nil
 }
 
 // Make sure all the command line parameters are OK and exit in case of invalid parameters.
@@ -315,11 +302,18 @@ func checkServerSyntax(c *cli.Context) {
 	// Verify syntax for all the XL disks.
 	disks := c.Args()
 	endpoints, err := parseStorageEndpoints(disks)
-	fatalIf(err, "Unable to parse storage endpoints %s", disks)
+	fatalIf(err, "Unable to parse storage endpoints %s", strings.Join(disks, " "))
-	checkEndpointsSyntax(endpoints, disks)
+
+	// Validate if endpoints follow the expected syntax.
+	err = checkEndpointsSyntax(endpoints, disks)
+	fatalIf(err, "Invalid endpoints found %s", strings.Join(disks, " "))
+
+	// Validate for duplicate endpoints are supplied.
+	err = checkDuplicateEndpoints(endpoints)
+	fatalIf(err, "Duplicate entries in %s", strings.Join(disks, " "))
 
 	if len(endpoints) > 1 {
-		// For XL setup.
+		// Validate if we have sufficient disks for XL setup.
 		err = checkSufficientDisks(endpoints)
 		fatalIf(err, "Storage endpoint error.")
 	}
@@ -361,7 +355,7 @@ func checkServerSyntax(c *cli.Context) {
 	}
 }
 
-// Checks if any of the endpoints supplied is local to a server instance.
+// Checks if any of the endpoints supplied is local to this server.
 func isAnyEndpointLocal(eps []*url.URL) bool {
 	anyLocalEp := false
 	for _, ep := range eps {
@@ -385,6 +379,7 @@ func serverMain(c *cli.Context) {
 	// Initialization routine, such as config loading, enable logging, ..
 	minioInit()
 
+	// Check for minio updates from dl.minio.io
 	checkUpdate()
 
 	// Server address.
@@ -414,9 +409,10 @@ func serverMain(c *cli.Context) {
 	endpoints, err := parseStorageEndpoints(c.Args())
 	fatalIf(err, "Unable to parse storage endpoints %s", c.Args())
 
-	// Should exit gracefully if none of the endpoints passed as command line argument is local to this server.
+	// Should exit gracefully if none of the endpoints passed
+	// as command line args are local to this server.
 	if !isAnyEndpointLocal(endpoints) {
-		fatalIf(errors.New("No endpoint is local to this server"), "None of the disks supplied are local to this instance. Please check the disks supplied.")
+		fatalIf(errInvalidArgument, "None of the disks passed as command line args are local to this server.")
 	}
 
 	storageDisks, err := initStorageDisks(endpoints)

cmd/server-main_test.go

@@ -197,6 +197,8 @@ func TestParseStorageEndpoints(t *testing.T) {
 	globalMinioHost = ""
 }
 
+// Test check endpoints syntax function for syntax verification
+// across various scenarios of inputs.
 func TestCheckEndpointsSyntax(t *testing.T) {
 	var testCases []string
 	if runtime.GOOS == "windows" {
@@ -221,19 +223,36 @@ func TestCheckEndpointsSyntax(t *testing.T) {
 	for _, disk := range testCases {
 		eps, err := parseStorageEndpoints([]string{disk})
 		if err != nil {
-			t.Error(disk, err)
+			t.Fatalf("Unable to parse %s, error %s", disk, err)
-			continue
 		}
-		// This will fatalIf() if endpoint is invalid.
+		if err = checkEndpointsSyntax(eps, []string{disk}); err != nil {
-		checkEndpointsSyntax(eps, []string{disk})
+			t.Errorf("Invalid endpoints %s", err)
+		}
+	}
+	eps, err := parseStorageEndpoints([]string{"/"})
+	if err != nil {
+		t.Fatalf("Unable to parse /, error %s", err)
+	}
+	if err = checkEndpointsSyntax(eps, []string{"/"}); err == nil {
+		t.Error("Should fail, passed instead")
+	}
+	eps, err = parseStorageEndpoints([]string{"http://localhost/"})
+	if err != nil {
+		t.Fatalf("Unable to parse http://localhost/, error %s", err)
+	}
+	if err = checkEndpointsSyntax(eps, []string{"http://localhost/"}); err == nil {
+		t.Error("Should fail, passed instead")
 	}
 }
 
+// Tests check server syntax.
 func TestCheckServerSyntax(t *testing.T) {
 	app := cli.NewApp()
 	app.Commands = []cli.Command{serverCmd}
 	serverFlagSet := flag.NewFlagSet("server", 0)
-	cli.NewContext(app, serverFlagSet, nil)
+	serverFlagSet.String("address", ":9000", "")
+	ctx := cli.NewContext(app, serverFlagSet, serverFlagSet)
+
 	disksGen := func(n int) []string {
 		disks, err := getRandomDisks(n)
 		if err != nil {
@@ -247,21 +266,14 @@ func TestCheckServerSyntax(t *testing.T) {
 		disksGen(8),
 		disksGen(16),
 	}
+
 	for i, disks := range testCases {
 		err := serverFlagSet.Parse(disks)
 		if err != nil {
 			t.Errorf("Test %d failed to parse arguments %s", i+1, disks)
 		}
 		defer removeRoots(disks)
-		endpoints, err := parseStorageEndpoints(disks)
+		checkServerSyntax(ctx)
-		if err != nil {
-			t.Fatalf("Test %d : Unexpected error %s", i+1, err)
-		}
-		checkEndpointsSyntax(endpoints, disks)
-		_, err = initStorageDisks(endpoints)
-		if err != nil {
-			t.Errorf("Test %d : disk init failed : %s", i+1, err)
-		}
 	}
 }