Generalize loadConfig method to avoid reading from disk (#5819)

As we move to multiple config backends like local disk and etcd,
config file should not be read from the disk, instead the quick
package should load and verify for duplicate entries.

cmd/admin-handlers.go

@@ -33,6 +33,7 @@ import (
 	"github.com/minio/minio/pkg/auth"
 	"github.com/minio/minio/pkg/handlers"
 	"github.com/minio/minio/pkg/madmin"
+	"github.com/minio/minio/pkg/quick"
 )
 
 const (
@@ -693,7 +694,7 @@ func (a adminAPIHandlers) SetConfigHandler(w http.ResponseWriter, r *http.Reques
 
 	// Validate JSON provided in the request body: check the
 	// client has not sent JSON objects with duplicate keys.
-	if err = checkDupJSONKeys(string(configBytes)); err != nil {
+	if err = quick.CheckDuplicateKeys(string(configBytes)); err != nil {
 		logger.LogIf(ctx, err)
 		writeErrorResponse(w, ErrAdminConfigBadJSON, r.URL)
 		return

cmd/config-current.go

@@ -19,7 +19,6 @@ package cmd
 import (
 	"errors"
 	"fmt"
-	"io/ioutil"
 	"reflect"
 	"sync"
 
@@ -27,7 +26,6 @@ import (
 	"github.com/minio/minio/pkg/event"
 	"github.com/minio/minio/pkg/event/target"
 	"github.com/minio/minio/pkg/quick"
-	"github.com/tidwall/gjson"
 )
 
 // Steps to move from version N to version N+1
@@ -254,57 +252,6 @@ func newConfig() error {
 	return globalServerConfig.Save()
 }
 
-// doCheckDupJSONKeys recursively detects duplicate json keys
-func doCheckDupJSONKeys(key, value gjson.Result) error {
-	// Key occurrences map of the current scope to count
-	// if there is any duplicated json key.
-	keysOcc := make(map[string]int)
-
-	// Holds the found error
-	var checkErr error
-
-	// Iterate over keys in the current json scope
-	value.ForEach(func(k, v gjson.Result) bool {
-		// If current key is not null, check if its
-		// value contains some duplicated keys.
-		if k.Type != gjson.Null {
-			keysOcc[k.String()]++
-			checkErr = doCheckDupJSONKeys(k, v)
-		}
-		return checkErr == nil
-	})
-
-	// Check found err
-	if checkErr != nil {
-		return errors.New(key.String() + " => " + checkErr.Error())
-	}
-
-	// Check for duplicated keys
-	for k, v := range keysOcc {
-		if v > 1 {
-			return errors.New(key.String() + " => `" + k + "` entry is duplicated")
-		}
-	}
-
-	return nil
-}
-
-// Check recursively if a key is duplicated in the same json scope
-// e.g.:
-//  `{ "key" : { "key" ..` is accepted
-//  `{ "key" : { "subkey" : "val1", "subkey": "val2" ..` throws subkey duplicated error
-func checkDupJSONKeys(json string) error {
-	// Parse config with gjson library
-	config := gjson.Parse(json)
-
-	// Create a fake rootKey since root json doesn't seem to have representation
-	// in gjson library.
-	rootKey := gjson.Result{Type: gjson.String, Str: minioConfigFile}
-
-	// Check if loaded json contains any duplicated keys
-	return doCheckDupJSONKeys(rootKey, config)
-}
-
 // getValidConfig - returns valid server configuration
 func getValidConfig() (*serverConfig, error) {
 	srvCfg := &serverConfig{
@@ -312,8 +259,7 @@ func getValidConfig() (*serverConfig, error) {
 		Browser: true,
 	}
 
-	configFile := getConfigFile()
-	if _, err := quick.Load(configFile, srvCfg); err != nil {
+	if _, err := quick.Load(getConfigFile(), srvCfg); err != nil {
 		return nil, err
 	}
 
@@ -321,21 +267,11 @@ func getValidConfig() (*serverConfig, error) {
 		return nil, fmt.Errorf("configuration version mismatch. Expected: ‘%s’, Got: ‘%s’", serverConfigVersion, srvCfg.Version)
 	}
 
-	// Load config file json and check for duplication json keys
-	jsonBytes, err := ioutil.ReadFile(configFile)
-	if err != nil {
-		return nil, err
-	}
-	if err = checkDupJSONKeys(string(jsonBytes)); err != nil {
-		return nil, err
-	}
-
 	// Validate credential fields only when
 	// they are not set via the environment
-
 	// Error out if global is env credential is not set and config has invalid credential
 	if !globalIsEnvCreds && !srvCfg.Credential.IsValid() {
-		return nil, errors.New("invalid credential in config file " + configFile)
+		return nil, errors.New("invalid credential in config file " + getConfigFile())
 	}
 
 	return srvCfg, nil

cmd/config-current_test.go

@@ -24,7 +24,6 @@ import (
 
 	"github.com/minio/minio/pkg/auth"
 	"github.com/minio/minio/pkg/event/target"
-	"github.com/tidwall/gjson"
 )
 
 func TestServerConfig(t *testing.T) {
@@ -126,34 +125,6 @@ func TestServerConfigWithEnvs(t *testing.T) {
 	}
 }
 
-func TestCheckDupJSONKeys(t *testing.T) {
-	testCases := []struct {
-		json       string
-		shouldPass bool
-	}{
-		{`{}`, true},
-		{`{"version" : "13"}`, true},
-		{`{"version" : "13", "version": "14"}`, false},
-		{`{"version" : "13", "credential": {"accessKey": "12345"}}`, true},
-		{`{"version" : "13", "credential": {"accessKey": "12345", "accessKey":"12345"}}`, false},
-		{`{"version" : "13", "notify": {"amqp": {"1"}, "webhook":{"3"}}}`, true},
-		{`{"version" : "13", "notify": {"amqp": {"1"}, "amqp":{"3"}}}`, false},
-		{`{"version" : "13", "notify": {"amqp": {"1":{}, "2":{}}}}`, true},
-		{`{"version" : "13", "notify": {"amqp": {"1":{}, "1":{}}}}`, false},
-	}
-
-	for i, testCase := range testCases {
-		err := doCheckDupJSONKeys(gjson.Result{}, gjson.Parse(testCase.json))
-		if testCase.shouldPass && err != nil {
-			t.Errorf("Test %d, should pass but it failed with err = %v", i+1, err)
-		}
-		if !testCase.shouldPass && err == nil {
-			t.Errorf("Test %d, should fail but it succeed.", i+1)
-		}
-	}
-
-}
-
 // Tests config validator..
 func TestValidateConfig(t *testing.T) {
 	rootPath, err := newTestConfig(globalMinioDefaultRegion)

pkg/quick/encoding.go

@@ -136,6 +136,11 @@ func loadFileConfig(filename string, v interface{}) error {
 	if runtime.GOOS == "windows" {
 		fileData = []byte(strings.Replace(string(fileData), "\r\n", "\n", -1))
 	}
+
+	if err = checkDupJSONKeys(string(fileData)); err != nil {
+		return err
+	}
+
 	// Unmarshal file's content
 	return toUnmarshaller(filepath.Ext(filename))(fileData, v)
 }

pkg/quick/errorutil.go

@@ -21,10 +21,12 @@ package quick
 import (
 	"bufio"
 	"bytes"
+	"errors"
 	"fmt"
 	"io"
 
 	"github.com/cheggaaa/pb"
+	"github.com/tidwall/gjson"
 )
 
 const errorFmt = "%5d: %s  <<<<"
@@ -81,3 +83,59 @@ func FormatJSONSyntaxError(data io.Reader, offset int64) (highlight string) {
 
 	return fmt.Sprintf(errorFmt, errLine, readLine.String()[idx:])
 }
+
+// doCheckDupJSONKeys recursively detects duplicate json keys
+func doCheckDupJSONKeys(key, value gjson.Result) error {
+	// Key occurrences map of the current scope to count
+	// if there is any duplicated json key.
+	keysOcc := make(map[string]int)
+
+	// Holds the found error
+	var checkErr error
+
+	// Iterate over keys in the current json scope
+	value.ForEach(func(k, v gjson.Result) bool {
+		// If current key is not null, check if its
+		// value contains some duplicated keys.
+		if k.Type != gjson.Null {
+			keysOcc[k.String()]++
+			checkErr = doCheckDupJSONKeys(k, v)
+		}
+		return checkErr == nil
+	})
+
+	// Check found err
+	if checkErr != nil {
+		return errors.New(key.String() + " => " + checkErr.Error())
+	}
+
+	// Check for duplicated keys
+	for k, v := range keysOcc {
+		if v > 1 {
+			return errors.New(key.String() + " => `" + k + "` entry is duplicated")
+		}
+	}
+
+	return nil
+}
+
+// Check recursively if a key is duplicated in the same json scope
+// e.g.:
+//  `{ "key" : { "key" ..` is accepted
+//  `{ "key" : { "subkey" : "val1", "subkey": "val2" ..` throws subkey duplicated error
+func checkDupJSONKeys(json string) error {
+	// Parse config with gjson library
+	config := gjson.Parse(json)
+
+	// Create a fake rootKey since root json doesn't seem to have representation
+	// in gjson library.
+	rootKey := gjson.Result{Type: gjson.String, Str: "config.json"}
+
+	// Check if loaded json contains any duplicated keys
+	return doCheckDupJSONKeys(rootKey, config)
+}
+
+// CheckDuplicateKeys - checks for duplicate entries in a JSON file
+func CheckDuplicateKeys(json string) error {
+	return checkDupJSONKeys(json)
+}

pkg/quick/quick_test.go

@@ -27,6 +27,8 @@ import (
 	"runtime"
 	"strings"
 	"testing"
+
+	"github.com/tidwall/gjson"
 )
 
 func TestReadVersion(t *testing.T) {
@@ -504,3 +506,31 @@ func TestDeepDiff(t *testing.T) {
 	//		fmt.Printf("DeepDiff[%d]: %s=%v\n", i, field.Name(), field.Value())
 	//	}
 }
+
+func TestCheckDupJSONKeys(t *testing.T) {
+	testCases := []struct {
+		json       string
+		shouldPass bool
+	}{
+		{`{}`, true},
+		{`{"version" : "13"}`, true},
+		{`{"version" : "13", "version": "14"}`, false},
+		{`{"version" : "13", "credential": {"accessKey": "12345"}}`, true},
+		{`{"version" : "13", "credential": {"accessKey": "12345", "accessKey":"12345"}}`, false},
+		{`{"version" : "13", "notify": {"amqp": {"1"}, "webhook":{"3"}}}`, true},
+		{`{"version" : "13", "notify": {"amqp": {"1"}, "amqp":{"3"}}}`, false},
+		{`{"version" : "13", "notify": {"amqp": {"1":{}, "2":{}}}}`, true},
+		{`{"version" : "13", "notify": {"amqp": {"1":{}, "1":{}}}}`, false},
+	}
+
+	for i, testCase := range testCases {
+		err := doCheckDupJSONKeys(gjson.Result{}, gjson.Parse(testCase.json))
+		if testCase.shouldPass && err != nil {
+			t.Errorf("Test %d, should pass but it failed with err = %v", i+1, err)
+		}
+		if !testCase.shouldPass && err == nil {
+			t.Errorf("Test %d, should fail but it succeed.", i+1)
+		}
+	}
+
+}