From 610dbe3479354c10910f1cf17ba9cbae0ea5a55d Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Sat, 6 May 2017 10:16:59 -0700 Subject: [PATCH] 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 | 176 +++++++++++++++++++++++-------------- cmd/config-migrate_test.go | 40 ++++++++- cmd/server-main.go | 2 +- pkg/quick/encoding.go | 12 ++- pkg/quick/errorutil.go | 25 ++---- pkg/quick/quick.go | 14 ++- pkg/quick/quick_test.go | 36 ++++++++ 7 files changed, 214 insertions(+), 91 deletions(-) diff --git a/cmd/config-migrate.go b/cmd/config-migrate.go index f06f5ddf8..f20c10434 100644 --- a/cmd/config-migrate.go +++ b/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 } - // Migrate version '2' to '3'. - if err := migrateV2ToV3(); err != nil { - return err - } - // Migrate version '3' to '4'. - if err := migrateV3ToV4(); err != nil { - return err - } - // Migrate version '4' to '5'. - if err := migrateV4ToV5(); err != nil { - return err - } - // Migrate version '5' to '6. - if err := migrateV5ToV6(); err != nil { - return err - } - // Migrate version '6' to '7'. - if err := migrateV6ToV7(); err != nil { - return err - } - // Migrate version '7' to '8'. - if err := migrateV7ToV8(); err != nil { - return err - } - // Migrate version '8' to '9'. - if err := migrateV8ToV9(); err != nil { - return err - } - // Migrate version '9' to '10'. - if err := migrateV9ToV10(); err != nil { - return err - } - // Migrate version '10' to '11'. - if err := migrateV10ToV11(); err != nil { - return err - } - // Migrate version '11' to '12'. - if err := migrateV11ToV12(); err != nil { - return err - } - // Migrate version '12' to '13'. - if err := migrateV12ToV13(); err != nil { - return err - } - // Migrate version '13' to '14'. - if err := migrateV13ToV14(); err != nil { - return err - } - // Migrate version '14' to '15'. - if err := migrateV14ToV15(); err != nil { - return err - } - // Migrate version '15' to '16'. - if err := migrateV15ToV16(); err != nil { - return err - } - // Migrate version '16' to '17'. - if err := migrateV16ToV17(); err != nil { - return err - } - // Migrate version '17' to '18'. - if err := migrateV17ToV18(); err != nil { + + // Load only config version information. + version, err := quick.GetVersion(getConfigFile()) + if err != nil { return err } - return nil + // 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 { + return err + } + fallthrough + case "3": + // Migrate version '3' to '4'. + if err = migrateV3ToV4(); err != nil { + return err + } + fallthrough + case "4": + // Migrate version '4' to '5'. + if err = migrateV4ToV5(); err != nil { + return err + } + fallthrough + case "5": + // Migrate version '5' to '6. + if err = migrateV5ToV6(); err != nil { + return err + } + fallthrough + case "6": + // Migrate version '6' to '7'. + if err = migrateV6ToV7(); err != nil { + return err + } + fallthrough + case "7": + // Migrate version '7' to '8'. + if err = migrateV7ToV8(); err != nil { + return err + } + fallthrough + case "8": + // Migrate version '8' to '9'. + if err = migrateV8ToV9(); err != nil { + return err + } + fallthrough + case "9": + // Migrate version '9' to '10'. + if err = migrateV9ToV10(); err != nil { + return err + } + fallthrough + case "10": + // Migrate version '10' to '11'. + if err = migrateV10ToV11(); err != nil { + return err + } + fallthrough + case "11": + // Migrate version '11' to '12'. + if err = migrateV11ToV12(); err != nil { + return err + } + fallthrough + case "12": + // Migrate version '12' to '13'. + if err = migrateV12ToV13(); err != nil { + return err + } + fallthrough + case "13": + // Migrate version '13' to '14'. + if err = migrateV13ToV14(); err != nil { + return err + } + fallthrough + case "14": + // Migrate version '14' to '15'. + if err = migrateV14ToV15(); err != nil { + return err + } + fallthrough + case "15": + // Migrate version '15' to '16'. + if err = migrateV15ToV16(); err != nil { + return err + } + fallthrough + case "16": + // Migrate version '16' to '17'. + if err = migrateV16ToV17(); err != nil { + return err + } + fallthrough + case "17": + // Migrate version '17' to '18'. + if err = migrateV17ToV18(); err != nil { + return err + } + fallthrough + 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. diff --git a/cmd/config-migrate_test.go b/cmd/config-migrate_test.go index fdd043fcb..fe4054255 100644 --- a/cmd/config-migrate_test.go +++ b/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") + } +} diff --git a/cmd/server-main.go b/cmd/server-main.go index e9378032f..ac3efa6a7 100644 --- a/cmd/server-main.go +++ b/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()) diff --git a/pkg/quick/encoding.go b/pkg/quick/encoding.go index a96f12314..a5100b6e2 100644 --- a/pkg/quick/encoding.go +++ b/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) - default: - return err + return fmt.Errorf("Unable to parse JSON schema due to a syntax error at '%s'", + FormatJSONSyntaxError(bytes.NewReader(b), jerr.Offset)) + 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 } diff --git a/pkg/quick/errorutil.go b/pkg/quick/errorutil.go index 063af3d5d..2c0bc9759 100644 --- a/pkg/quick/errorutil.go +++ b/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 { - if sErr == nil { - return nil - } - +func FormatJSONSyntaxError(data io.Reader, offset int64) (highlight string) { 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", - errLine, readLine.Len(), sErr) - errorStr += fmt.Sprintf(errorFmt, errLine, readLine.String()[idx:]) - - return errors.New(errorStr) + return fmt.Sprintf(errorFmt, errLine, readLine.String()[idx:]) } diff --git a/pkg/quick/quick.go b/pkg/quick/quick.go index 55cecb5db..e010c8f9a 100644 --- a/pkg/quick/quick.go +++ b/pkg/quick/quick.go @@ -127,7 +127,7 @@ func (d config) Diff(c Config) ([]structs.Field, error) { return fields, nil } -//DeepDiff - list fields in A that are missing or not equal to fields in B +// DeepDiff - list fields in A that are missing or not equal to fields in B func (d config) DeepDiff(c Config) ([]structs.Field, error) { var fields []structs.Field @@ -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 } diff --git a/pkg/quick/quick_test.go b/pkg/quick/quick_test.go index aed6b1d49..c1b3c70d4 100644 --- a/pkg/quick/quick_test.go +++ b/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)