config: Do not migrate config file if not needed. (#4264)

Also improve the error message returned by `pkg/quick`.

Fixes #4233

cmd/config-migrate.go

@@ -27,77 +27,125 @@ import (
 // DO NOT EDIT following message template, please open a github issue to discuss instead.
 var configMigrateMSGTemplate = "Configuration file %s migrated from version '%s' to '%s' successfully.\n"
 
+// Migrates all config versions from "1" to "18".
 func migrateConfig() error {
-	// Purge all configs with version '1'.
+	// Purge all configs with version '1',
+	// this is a special case since version '1' used
+	// to be a filename 'fsUsers.json' not 'config.json'.
 	if err := purgeV1(); err != nil {
 		return err
 	}
+
+	// Load only config version information.
+	version, err := quick.GetVersion(getConfigFile())
+	if err != nil {
+		return err
+	}
+
+	// Conditional to migrate only relevant config versions.
+	// Upon success migration continues to the next version in sequence.
+	switch version {
+	case "2":
 		// Migrate version '2' to '3'.
-	if err := migrateV2ToV3(); err != nil {
+		if err = migrateV2ToV3(); err != nil {
 			return err
 		}
+		fallthrough
+	case "3":
 		// Migrate version '3' to '4'.
-	if err := migrateV3ToV4(); err != nil {
+		if err = migrateV3ToV4(); err != nil {
 			return err
 		}
+		fallthrough
+	case "4":
 		// Migrate version '4' to '5'.
-	if err := migrateV4ToV5(); err != nil {
+		if err = migrateV4ToV5(); err != nil {
 			return err
 		}
+		fallthrough
+	case "5":
 		// Migrate version '5' to '6.
-	if err := migrateV5ToV6(); err != nil {
+		if err = migrateV5ToV6(); err != nil {
 			return err
 		}
+		fallthrough
+	case "6":
 		// Migrate version '6' to '7'.
-	if err := migrateV6ToV7(); err != nil {
+		if err = migrateV6ToV7(); err != nil {
 			return err
 		}
+		fallthrough
+	case "7":
 		// Migrate version '7' to '8'.
-	if err := migrateV7ToV8(); err != nil {
+		if err = migrateV7ToV8(); err != nil {
 			return err
 		}
+		fallthrough
+	case "8":
 		// Migrate version '8' to '9'.
-	if err := migrateV8ToV9(); err != nil {
+		if err = migrateV8ToV9(); err != nil {
 			return err
 		}
+		fallthrough
+	case "9":
 		// Migrate version '9' to '10'.
-	if err := migrateV9ToV10(); err != nil {
+		if err = migrateV9ToV10(); err != nil {
 			return err
 		}
+		fallthrough
+	case "10":
 		// Migrate version '10' to '11'.
-	if err := migrateV10ToV11(); err != nil {
+		if err = migrateV10ToV11(); err != nil {
 			return err
 		}
+		fallthrough
+	case "11":
 		// Migrate version '11' to '12'.
-	if err := migrateV11ToV12(); err != nil {
+		if err = migrateV11ToV12(); err != nil {
 			return err
 		}
+		fallthrough
+	case "12":
 		// Migrate version '12' to '13'.
-	if err := migrateV12ToV13(); err != nil {
+		if err = migrateV12ToV13(); err != nil {
 			return err
 		}
+		fallthrough
+	case "13":
 		// Migrate version '13' to '14'.
-	if err := migrateV13ToV14(); err != nil {
+		if err = migrateV13ToV14(); err != nil {
 			return err
 		}
+		fallthrough
+	case "14":
 		// Migrate version '14' to '15'.
-	if err := migrateV14ToV15(); err != nil {
+		if err = migrateV14ToV15(); err != nil {
 			return err
 		}
+		fallthrough
+	case "15":
 		// Migrate version '15' to '16'.
-	if err := migrateV15ToV16(); err != nil {
+		if err = migrateV15ToV16(); err != nil {
 			return err
 		}
+		fallthrough
+	case "16":
 		// Migrate version '16' to '17'.
-	if err := migrateV16ToV17(); err != nil {
+		if err = migrateV16ToV17(); err != nil {
 			return err
 		}
+		fallthrough
+	case "17":
 		// Migrate version '17' to '18'.
-	if err := migrateV17ToV18(); err != nil {
+		if err = migrateV17ToV18(); err != nil {
 			return err
 		}
-
+		fallthrough
-	return nil
+	case v18:
+		// No migration needed. this always points to current version.
+		err = nil
+	}
+	return err
 }
 
 // Version '1' is not supported anymore and deprecated, safe to delete.

cmd/config-migrate_test.go

@@ -17,6 +17,7 @@
 package cmd
 
 import (
+	"fmt"
 	"io/ioutil"
 	"os"
 	"testing"
@@ -153,6 +154,7 @@ func TestServerConfigMigrateV2toV18(t *testing.T) {
 	if err := ioutil.WriteFile(configPath, []byte(configJSON), 0644); err != nil {
 		t.Fatal("Unexpected error: ", err)
 	}
+
 	// Fire a migrateConfig()
 	if err := migrateConfig(); err != nil {
 		t.Fatal("Unexpected error: ", err)
@@ -191,7 +193,7 @@ func TestServerConfigMigrateFaultyConfig(t *testing.T) {
 	configPath := rootPath + "/" + minioConfigFile
 
 	// Create a corrupted config file
-	if err := ioutil.WriteFile(configPath, []byte("{ \"version\":\""), 0644); err != nil {
+	if err := ioutil.WriteFile(configPath, []byte("{ \"version\":\"2\", \"test\":"), 0644); err != nil {
 		t.Fatal("Unexpected error: ", err)
 	}
 
@@ -245,3 +247,39 @@ func TestServerConfigMigrateFaultyConfig(t *testing.T) {
 		t.Fatal("migrateConfigV17ToV18() should fail with a corrupted json")
 	}
 }
+
+// Test if all migrate code returns error with corrupted config files
+func TestServerConfigMigrateCorruptedConfig(t *testing.T) {
+	rootPath, err := newTestConfig(globalMinioDefaultRegion)
+	if err != nil {
+		t.Fatalf("Init Test config failed")
+	}
+	// remove the root directory after the test ends.
+	defer removeAll(rootPath)
+
+	setConfigDir(rootPath)
+	configPath := rootPath + "/" + minioConfigFile
+
+	for i := 3; i <= 17; i++ {
+		// Create a corrupted config file
+		if err = ioutil.WriteFile(configPath, []byte(fmt.Sprintf("{ \"version\":\"%d\", \"credential\": { \"accessKey\": 1 } }", i)),
+			0644); err != nil {
+			t.Fatal("Unexpected error: ", err)
+		}
+
+		// Test different migrate versions and be sure they are returning an error
+		if err = migrateConfig(); err == nil {
+			t.Fatal("migrateConfig() should fail with a corrupted json")
+		}
+	}
+
+	// Create a corrupted config file for version '2'.
+	if err = ioutil.WriteFile(configPath, []byte("{ \"version\":\"2\", \"credentials\": { \"accessKeyId\": 1 } }"), 0644); err != nil {
+		t.Fatal("Unexpected error: ", err)
+	}
+
+	// Test different migrate versions and be sure they are returning an error
+	if err = migrateConfig(); err == nil {
+		t.Fatal("migrateConfig() should fail with a corrupted json")
+	}
+}

cmd/server-main.go

@@ -107,7 +107,7 @@ func initConfig() {
 	// Config file does not exist, we create it fresh and return upon success.
 	if isFile(getConfigFile()) {
 		fatalIf(migrateConfig(), "Config migration failed.")
-		fatalIf(loadConfig(), "Unable to load minio config file")
+		fatalIf(loadConfig(), "Unable to load config version: '%s'.", v18)
 	} else {
 		fatalIf(newConfig(), "Unable to initialize minio config for the first time.")
 		log.Println("Created minio configuration file successfully at " + getConfigDir())

pkg/quick/encoding.go

@@ -21,6 +21,7 @@ package quick
 import (
 	"bytes"
 	"encoding/json"
+	"fmt"
 	"io/ioutil"
 	"path/filepath"
 	"runtime"
@@ -55,12 +56,15 @@ func (j jsonEncoding) Unmarshal(b []byte, v interface{}) error {
 	err := json.Unmarshal(b, v)
 	if err != nil {
 		// Try to return a sophisticated json error message if possible
-		switch err := err.(type) {
+		switch jerr := err.(type) {
 		case *json.SyntaxError:
-			return FormatJSONSyntaxError(bytes.NewReader(b), err)
+			return fmt.Errorf("Unable to parse JSON schema due to a syntax error at '%s'",
-		default:
+				FormatJSONSyntaxError(bytes.NewReader(b), jerr.Offset))
-			return err
+		case *json.UnmarshalTypeError:
+			return fmt.Errorf("Unable to parse JSON, type '%v' cannot be converted into the Go '%v' type",
+				jerr.Value, jerr.Type)
 		}
+		return err
 	}
 	return nil
 }

pkg/quick/errorutil.go

@@ -21,28 +21,22 @@ package quick
 import (
 	"bufio"
 	"bytes"
-	"encoding/json"
-	"errors"
 	"fmt"
 	"io"
 
 	"github.com/cheggaaa/pb"
 )
 
-const errorFmt = "%5d: %s <--  "
+const errorFmt = "%5d: %s  <<<<"
 
 // FormatJSONSyntaxError generates a pretty printed json syntax error since
 // golang doesn't provide an easy way to report the location of the error
-func FormatJSONSyntaxError(data io.Reader, sErr *json.SyntaxError) error {
+func FormatJSONSyntaxError(data io.Reader, offset int64) (highlight string) {
-	if sErr == nil {
-		return nil
-	}
-
 	var readLine bytes.Buffer
+	var errLine = 1
+	var readBytes int64
 
 	bio := bufio.NewReader(data)
-	errLine := int64(1)
-	readBytes := int64(0)
 
 	// termWidth is set to a default one to use when we are
 	// not able to calculate terminal width via OS syscalls
@@ -60,13 +54,10 @@ func FormatJSONSyntaxError(data io.Reader, sErr *json.SyntaxError) error {
 	for {
 		b, err := bio.ReadByte()
 		if err != nil {
-			if err != io.EOF {
-				return err
-			}
 			break
 		}
 		readBytes++
-		if readBytes > sErr.Offset {
+		if readBytes > offset {
 			break
 		}
 		switch b {
@@ -88,9 +79,5 @@ func FormatJSONSyntaxError(data io.Reader, sErr *json.SyntaxError) error {
 		idx = 0
 	}
 
-	errorStr := fmt.Sprintf("JSON syntax error at line %d, col %d : %s.\n",
+	return fmt.Sprintf(errorFmt, errLine, readLine.String()[idx:])
-		errLine, readLine.Len(), sErr)
-	errorStr += fmt.Sprintf(errorFmt, errLine, readLine.String()[idx:])
-
-	return errors.New(errorStr)
 }

pkg/quick/quick.go

@@ -196,12 +196,22 @@ func New(data interface{}) (Config, error) {
 	return d, nil
 }
 
+// GetVersion - extracts the version information.
+func GetVersion(filename string) (version string, err error) {
+	var qc Config
+	if qc, err = Load(filename, &struct {
+		Version string
+	}{}); err != nil {
+		return "", err
+	}
+	return qc.Version(), err
+}
+
 // Load - loads json config from filename for the a given struct data
 func Load(filename string, data interface{}) (qc Config, err error) {
 	if qc, err = New(data); err == nil {
 		err = qc.Load(filename)
 	}
-
 	return qc, err
 }
 

pkg/quick/quick_test.go

@@ -36,6 +36,42 @@ type MySuite struct{}
 
 var _ = Suite(&MySuite{})
 
+func (s *MySuite) TestReadVersion(c *C) {
+	type myStruct struct {
+		Version string
+	}
+	saveMe := myStruct{"1"}
+	config, err := New(&saveMe)
+	c.Assert(err, IsNil)
+	err = config.Save("test.json")
+	c.Assert(err, IsNil)
+
+	version, err := GetVersion("test.json")
+	c.Assert(err, IsNil)
+	c.Assert(version, Equals, "1")
+}
+
+func (s *MySuite) TestReadVersionErr(c *C) {
+	type myStruct struct {
+		Version int
+	}
+	saveMe := myStruct{1}
+	_, err := New(&saveMe)
+	c.Assert(err, Not(IsNil))
+
+	err = ioutil.WriteFile("test.json", []byte("{ \"version\":2,"), 0644)
+	c.Assert(err, IsNil)
+
+	_, err = GetVersion("test.json")
+	c.Assert(err, Not(IsNil))
+
+	err = ioutil.WriteFile("test.json", []byte("{ \"version\":2 }"), 0644)
+	c.Assert(err, IsNil)
+
+	_, err = GetVersion("test.json")
+	c.Assert(err, Not(IsNil))
+}
+
 func (s *MySuite) TestSaveFailOnDir(c *C) {
 	defer os.RemoveAll("test.json")
 	e := os.MkdirAll("test.json", 0644)