server: Validate server arguments for duplicates. (#2554)

- Validates invalid format inputs.
- Validates duplicate entries.
- Validates sufficient amount of disks.

Partially fixes #2502

.travis.yml

@@ -4,9 +4,9 @@ language: go
 
 os:
 - linux
-#- osx
+- osx
 
-#osx_image: xcode7.2
+osx_image: xcode7.2
 
 env:
 - ARCH=x86_64

cmd/bucket-notification-utils.go

@@ -268,18 +268,17 @@ func validateTopicConfigs(topicConfigs []topicConfig) APIErrorCode {
 
 // Check all the queue configs for any duplicates.
 func checkDuplicateQueueConfigs(configs []queueConfig) APIErrorCode {
-	configMaps := make(map[string]int)
+	var queueConfigARNS []string
 
 	// Navigate through each configs and count the entries.
 	for _, config := range configs {
-		configMaps[config.QueueARN]++
+		queueConfigARNS = append(queueConfigARNS, config.QueueARN)
 	}
 
-	// Validate if there are any duplicate counts.
+	// Check if there are any duplicate counts.
-	for _, count := range configMaps {
+	if err := checkDuplicates(queueConfigARNS); err != nil {
-		if count != 1 {
+		errorIf(err, "Invalid queue configs found.")
-			return ErrOverlappingConfigs
+		return ErrOverlappingConfigs
-		}
 	}
 
 	// Success.
@@ -288,18 +287,17 @@ func checkDuplicateQueueConfigs(configs []queueConfig) APIErrorCode {
 
 // Check all the topic configs for any duplicates.
 func checkDuplicateTopicConfigs(configs []topicConfig) APIErrorCode {
-	configMaps := make(map[string]int)
+	var topicConfigARNS []string
 
 	// Navigate through each configs and count the entries.
 	for _, config := range configs {
-		configMaps[config.TopicARN]++
+		topicConfigARNS = append(topicConfigARNS, config.TopicARN)
 	}
 
-	// Validate if there are any duplicate counts.
+	// Check if there are any duplicate counts.
-	for _, count := range configMaps {
+	if err := checkDuplicates(topicConfigARNS); err != nil {
-		if count != 1 {
+		errorIf(err, "Invalid topic configs found.")
-			return ErrOverlappingConfigs
+		return ErrOverlappingConfigs
-		}
 	}
 
 	// Success.
@@ -320,12 +318,17 @@ func validateNotificationConfig(nConfig notificationConfig) APIErrorCode {
 	}
 
 	// Check for duplicate queue configs.
-	if s3Error := checkDuplicateQueueConfigs(nConfig.QueueConfigs); s3Error != ErrNone {
+	if len(nConfig.QueueConfigs) > 1 {
-		return s3Error
+		if s3Error := checkDuplicateQueueConfigs(nConfig.QueueConfigs); s3Error != ErrNone {
+			return s3Error
+		}
 	}
+
 	// Check for duplicate topic configs.
-	if s3Error := checkDuplicateTopicConfigs(nConfig.TopicConfigs); s3Error != ErrNone {
+	if len(nConfig.TopicConfigs) > 1 {
-		return s3Error
+		if s3Error := checkDuplicateTopicConfigs(nConfig.TopicConfigs); s3Error != ErrNone {
+			return s3Error
+		}
 	}
 
 	// Add validation for other configurations.

cmd/server-main.go

@@ -202,16 +202,70 @@ func initServerConfig(c *cli.Context) {
 	// Do not fail if this is not allowed, lower limits are fine as well.
 }
 
+// Validate if input disks are sufficient for initializing XL.
+func checkSufficientDisks(disks []string) error {
+	// Verify total number of disks.
+	totalDisks := len(disks)
+	if totalDisks > maxErasureBlocks {
+		return errXLMaxDisks
+	}
+	if totalDisks < minErasureBlocks {
+		return errXLMinDisks
+	}
+
+	// isEven function to verify if a given number if even.
+	isEven := func(number int) bool {
+		return number%2 == 0
+	}
+
+	// Verify if we have even number of disks.
+	// only combination of 4, 6, 8, 10, 12, 14, 16 are supported.
+	if !isEven(totalDisks) {
+		return errXLNumDisks
+	}
+
+	// Success.
+	return nil
+}
+
+// Validates if disks are of supported format, invalid arguments are rejected.
+func checkNamingDisks(disks []string) error {
+	for _, disk := range disks {
+		_, _, err := splitNetPath(disk)
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 // Check server arguments.
 func checkServerSyntax(c *cli.Context) {
 	if !c.Args().Present() || c.Args().First() == "help" {
 		cli.ShowCommandHelpAndExit(c, "server", 1)
 	}
+	disks := c.Args()
+	if len(disks) > 1 {
+		// Validate if input disks have duplicates in them.
+		err := checkDuplicates(disks)
+		fatalIf(err, "Invalid disk arguments for server.")
+
+		// Validate if input disks are sufficient for erasure coded setup.
+		err = checkSufficientDisks(disks)
+		fatalIf(err, "Invalid disk arguments for server.")
+
+		// Validate if input disks are properly named in accordance with either
+		//  - /mnt/disk1
+		//  - ip:/mnt/disk1
+		err = checkNamingDisks(disks)
+		fatalIf(err, "Invalid disk arguments for server.")
+	}
 }
 
 // Extract port number from address address should be of the form host:port.
 func getPort(address string) int {
 	_, portStr, _ := net.SplitHostPort(address)
+
 	// If port empty, default to port '80'
 	if portStr == "" {
 		portStr = "80"

cmd/utils.go

@@ -20,6 +20,7 @@ import (
 	"encoding/base64"
 	"encoding/xml"
 	"errors"
+	"fmt"
 	"io"
 	"net"
 	"net/http"
@@ -46,6 +47,33 @@ func cloneHeader(h http.Header) http.Header {
 	return h2
 }
 
+// checkDuplicates - function to validate if there are duplicates in a slice of strings.
+func checkDuplicates(list []string) error {
+	// Empty lists are not allowed.
+	if len(list) == 0 {
+		return errInvalidArgument
+	}
+	// Empty keys are not allowed.
+	for _, key := range list {
+		if key == "" {
+			return errInvalidArgument
+		}
+	}
+	listMaps := make(map[string]int)
+	// Navigate through each configs and count the entries.
+	for _, key := range list {
+		listMaps[key]++
+	}
+	// Validate if there are any duplicate counts.
+	for key, count := range listMaps {
+		if count != 1 {
+			return fmt.Errorf("Duplicate key: \"%s\" found of count: \"%d\"", key, count)
+		}
+	}
+	// No duplicates.
+	return nil
+}
+
 // splits network path into its components Address and Path.
 func splitNetPath(networkPath string) (netAddr, netPath string, err error) {
 	if runtime.GOOS == "windows" {
@@ -54,16 +82,15 @@ func splitNetPath(networkPath string) (netAddr, netPath string, err error) {
 		}
 	}
 	networkParts := strings.SplitN(networkPath, ":", 2)
-	switch len(networkParts) {
+	if len(networkParts) == 1 {
-	case 1:
 		return "", networkPath, nil
-	case 2:
+	}
-		if networkParts[1] == "" {
+	if networkParts[1] == "" {
-			return "", "", &net.AddrError{Err: "missing path in network path", Addr: networkPath}
+		return "", "", &net.AddrError{Err: "Missing path in network path", Addr: networkPath}
-		} else if networkParts[0] == "" {
+	} else if networkParts[0] == "" {
-			return "", "", &net.AddrError{Err: "missing address in network path", Addr: networkPath}
+		return "", "", &net.AddrError{Err: "Missing address in network path", Addr: networkPath}
-		}
+	} else if !filepath.IsAbs(networkParts[1]) {
-		return networkParts[0], networkParts[1], nil
+		return "", "", &net.AddrError{Err: "Network path should be absolute", Addr: networkPath}
 	}
 	return networkParts[0], networkParts[1], nil
 }

cmd/utils_nix_test.go

@@ -31,11 +31,17 @@ func TestSplitNetPath(t *testing.T) {
 		netPath     string
 		err         error
 	}{
-		{"10.1.10.1:", "", "", &net.AddrError{Err: "missing path in network path", Addr: "10.1.10.1:"}},
+		// Invalid cases 1-5.
+		{"10.1.10.1:", "", "", &net.AddrError{Err: "Missing path in network path", Addr: "10.1.10.1:"}},
+		{"10.1.10.1:../1", "", "", &net.AddrError{Err: "Network path should be absolute", Addr: "10.1.10.1:../1"}},
+		{":/tmp/1", "", "", &net.AddrError{Err: "Missing address in network path", Addr: ":/tmp/1"}},
+		{"10.1.10.1:disk/1", "", "", &net.AddrError{Err: "Network path should be absolute", Addr: "10.1.10.1:disk/1"}},
+		{"10.1.10.1:\\path\\test", "", "", &net.AddrError{Err: "Network path should be absolute", Addr: "10.1.10.1:\\path\\test"}},
+
+		// Valid cases 6-8
 		{"10.1.10.1", "", "10.1.10.1", nil},
 		{"10.1.10.1://", "10.1.10.1", "//", nil},
 		{"10.1.10.1:/disk/1", "10.1.10.1", "/disk/1", nil},
-		{"10.1.10.1:\\path\\test", "10.1.10.1", "\\path\\test", nil},
 	}
 
 	for i, test := range testCases {

cmd/utils_test.go

@@ -17,6 +17,7 @@
 package cmd
 
 import (
+	"fmt"
 	"net/http"
 	"reflect"
 	"testing"
@@ -46,6 +47,57 @@ func TestCloneHeader(t *testing.T) {
 	}
 }
 
+// Tests check duplicates function.
+func TestCheckDuplicates(t *testing.T) {
+	tests := []struct {
+		list       []string
+		err        error
+		shouldPass bool
+	}{
+		// Test 1 - for '/tmp/1' repeated twice.
+		{
+			list:       []string{"/tmp/1", "/tmp/1", "/tmp/2", "/tmp/3"},
+			err:        fmt.Errorf("Duplicate key: \"/tmp/1\" found of count: \"2\""),
+			shouldPass: false,
+		},
+		// Test 2 - for '/tmp/1' repeated thrice.
+		{
+			list:       []string{"/tmp/1", "/tmp/1", "/tmp/1", "/tmp/3"},
+			err:        fmt.Errorf("Duplicate key: \"/tmp/1\" found of count: \"3\""),
+			shouldPass: false,
+		},
+		// Test 3 - empty string.
+		{
+			list:       []string{""},
+			err:        errInvalidArgument,
+			shouldPass: false,
+		},
+		// Test 4 - empty string.
+		{
+			list:       nil,
+			err:        errInvalidArgument,
+			shouldPass: false,
+		},
+		// Test 5 - non repeated strings.
+		{
+			list:       []string{"/tmp/1", "/tmp/2", "/tmp/3"},
+			err:        nil,
+			shouldPass: true,
+		},
+	}
+
+	// Validate if function runs as expected.
+	for i, test := range tests {
+		err := checkDuplicates(test.list)
+		if test.shouldPass && err != test.err {
+			t.Errorf("Test: %d, Expected %s got %s", i+1, test.err, err)
+		}
+		if !test.shouldPass && err.Error() != test.err.Error() {
+			t.Errorf("Test: %d, Expected %s got %s", i+1, test.err, err)
+		}
+	}
+}
+
 // Tests maximum object size.
 func TestMaxObjectSize(t *testing.T) {
 	sizes := []struct {

cmd/utils_windows_test.go

@@ -31,11 +31,26 @@ func TestSplitNetPath(t *testing.T) {
 		netPath     string
 		err         error
 	}{
+		// Invalid cases 1-8.
+		{":C:", "", "", &net.AddrError{Err: "Missing address in network path", Addr: ":C:"}},
+		{"10.1.10.1:", "", "", &net.AddrError{Err: "Missing path in network path", Addr: "10.1.10.1:"}},
+		{"10.1.10.1:C", "", "", &net.AddrError{Err: "Network path should be absolute", Addr: "10.1.10.1:C"}},
+		{"10.1.10.1:C:", "", "", &net.AddrError{Err: "Network path should be absolute", Addr: "10.1.10.1:C:"}},
+		{"10.1.10.1:C:../path", "", "", &net.AddrError{Err: "Network path should be absolute", Addr: "10.1.10.1:C:../path"}},
+		{"10.1.10.1:C:tmp/1", "", "", &net.AddrError{Err: "Network path should be absolute", Addr: "10.1.10.1:C:tmp/1"}},
+		{"10.1.10.1::C:\\path\\test", "", "", &net.AddrError{
+			Err:  "Network path should be absolute",
+			Addr: "10.1.10.1::C:\\path\\test",
+		}},
+		{"10.1.10.1:\\path\\test", "", "", &net.AddrError{
+			Err:  "Network path should be absolute",
+			Addr: "10.1.10.1:\\path\\test",
+		}},
+
+		// Valid cases 9-11.
 		{"10.1.10.1:C:\\path\\test", "10.1.10.1", "C:\\path\\test", nil},
-		{"10.1.10.1:C:", "10.1.10.1", "C:", nil},
-		{":C:", "", "", &net.AddrError{Err: "missing address in network path", Addr: ":C:"}},
 		{"C:\\path\\test", "", "C:\\path\\test", nil},
-		{"10.1.10.1::C:\\path\\test", "10.1.10.1", ":C:\\path\\test", nil},
+		{`10.1.10.1:\\?\UNC\path\test`, "10.1.10.1", `\\?\UNC\path\test`, nil},
 	}
 
 	for i, test := range testCases {

cmd/xl-v1-errors.go

@@ -27,6 +27,9 @@ var errXLMinDisks = errors.New("Minimum '4' disks are required to enable erasure
 // errXLNumDisks - returned for odd number of disks.
 var errXLNumDisks = errors.New("Total number of disks should be multiples of '2'")
 
+// errXLDuplicateArguments - returned for duplicate disks.
+var errXLDuplicateArguments = errors.New("Duplicate disks found.")
+
 // errXLReadQuorum - did not meet read quorum.
 var errXLReadQuorum = errors.New("Read failed. Insufficient number of disks online")
 

cmd/xl-v1.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 	"sort"
 
+	"github.com/minio/minio-go/pkg/set"
 	"github.com/minio/minio/pkg/disk"
 	"github.com/minio/minio/pkg/objcache"
 )
@@ -66,49 +67,20 @@ type xlObjects struct {
 	objCacheEnabled bool
 }
 
-// Validate if input disks are sufficient for initializing XL.
-func checkSufficientDisks(disks []string) error {
-	// Verify total number of disks.
-	totalDisks := len(disks)
-	if totalDisks > maxErasureBlocks {
-		return errXLMaxDisks
-	}
-	if totalDisks < minErasureBlocks {
-		return errXLMinDisks
-	}
-
-	// isEven function to verify if a given number if even.
-	isEven := func(number int) bool {
-		return number%2 == 0
-	}
-
-	// Verify if we have even number of disks.
-	// only combination of 4, 6, 8, 10, 12, 14, 16 are supported.
-	if !isEven(totalDisks) {
-		return errXLNumDisks
-	}
-
-	// Success.
-	return nil
-}
-
-// isDiskFound - validates if the disk is found in a list of input disks.
-func isDiskFound(disk string, disks []string) bool {
-	return contains(disks, disk)
-}
-
 // newXLObjects - initialize new xl object layer.
 func newXLObjects(disks, ignoredDisks []string) (ObjectLayer, error) {
-	// Validate if input disks are sufficient.
+	if disks == nil {
-	if err := checkSufficientDisks(disks); err != nil {
+		return nil, errInvalidArgument
-		return nil, err
+	}
+	disksSet := set.NewStringSet()
+	if len(ignoredDisks) > 0 {
+		disksSet = set.CreateStringSet(ignoredDisks...)
 	}
-
 	// Bootstrap disks.
 	storageDisks := make([]StorageAPI, len(disks))
 	for index, disk := range disks {
 		// Check if disk is ignored.
-		if isDiskFound(disk, ignoredDisks) {
+		if disksSet.Contains(disk) {
 			storageDisks[index] = nil
 			continue
 		}

cmd/xl-v1_test.go

@@ -130,8 +130,21 @@ func TestNewXL(t *testing.T) {
 		erasureDisks = append(erasureDisks, disk)
 		defer removeAll(disk)
 	}
+
+	// No disks input.
+	_, err := newXLObjects(nil, nil)
+	if err != errInvalidArgument {
+		t.Fatalf("Unable to initialize erasure, %s", err)
+	}
+
 	// Initializes all erasure disks
-	_, err := newXLObjects(erasureDisks, nil)
+	_, err = newXLObjects(erasureDisks, nil)
+	if err != nil {
+		t.Fatalf("Unable to initialize erasure, %s", err)
+	}
+
+	// Initializes all erasure disks, ignoring first two.
+	_, err = newXLObjects(erasureDisks, erasureDisks[:2])
 	if err != nil {
 		t.Fatalf("Unable to initialize erasure, %s", err)
 	}