Export command prints turned-off sub-sys as comments (#8594)

This PR also tries to

- Preserve the order of keys printed in export command
- Fix cache to be enabled with _STATE env to keep
  backward compatibility

cmd/config/cache/config.go

@@ -61,22 +61,26 @@ func (cfg *Config) UnmarshalJSON(data []byte) (err error) {
 		return errors.New("config quota value should not be null or negative")
 	}
 
-	if _, err = parseCacheDrives(_cfg.Drives); err != nil {
-		return err
-	}
-	if _, err = parseCacheExcludes(_cfg.Exclude); err != nil {
-		return err
-	}
 	return nil
 }
 
 // Parses given cacheDrivesEnv and returns a list of cache drives.
-func parseCacheDrives(drives []string) ([]string, error) {
+func parseCacheDrives(drives string) ([]string, error) {
+	var drivesSlice []string
 	if len(drives) == 0 {
-		return drives, nil
+		return drivesSlice, nil
 	}
+
+	drivesSlice = strings.Split(drives, cacheDelimiterLegacy)
+	if len(drivesSlice) == 1 && drivesSlice[0] == drives {
+		drivesSlice = strings.Split(drives, cacheDelimiter)
+	}
+
 	var endpoints []string
-	for _, d := range drives {
+	for _, d := range drivesSlice {
+		if len(d) == 0 {
+			return nil, config.ErrInvalidCacheDrivesValue(nil).Msg("cache dir cannot be an empty path")
+		}
 		if ellipses.HasEllipses(d) {
 			s, err := parseCacheDrivePaths(d)
 			if err != nil {
@@ -89,9 +93,6 @@ func parseCacheDrives(drives []string) ([]string, error) {
 	}
 
 	for _, d := range endpoints {
-		if len(d) == 0 {
-			return nil, config.ErrInvalidCacheDrivesValue(nil).Msg("cache dir cannot be an empty path")
-		}
 		if !filepath.IsAbs(d) {
 			return nil, config.ErrInvalidCacheDrivesValue(nil).Msg("cache dir should be absolute path: %s", d)
 		}
@@ -114,8 +115,18 @@ func parseCacheDrivePaths(arg string) (ep []string, err error) {
 }
 
 // Parses given cacheExcludesEnv and returns a list of cache exclude patterns.
-func parseCacheExcludes(excludes []string) ([]string, error) {
+func parseCacheExcludes(excludes string) ([]string, error) {
-	for _, e := range excludes {
+	var excludesSlice []string
+	if len(excludes) == 0 {
+		return excludesSlice, nil
+	}
+
+	excludesSlice = strings.Split(excludes, cacheDelimiterLegacy)
+	if len(excludesSlice) == 1 && excludesSlice[0] == excludes {
+		excludesSlice = strings.Split(excludes, cacheDelimiter)
+	}
+
+	for _, e := range excludesSlice {
 		if len(e) == 0 {
 			return nil, config.ErrInvalidCacheExcludesValue(nil).Msg("cache exclude path (%s) cannot be empty", e)
 		}
@@ -123,5 +134,6 @@ func parseCacheExcludes(excludes []string) ([]string, error) {
 			return nil, config.ErrInvalidCacheExcludesValue(nil).Msg("cache exclude pattern (%s) cannot start with / as prefix", e)
 		}
 	}
-	return excludes, nil
+
+	return excludesSlice, nil
 }

cmd/config/cache/config_test.go

@@ -19,7 +19,6 @@ package cache
 import (
 	"reflect"
 	"runtime"
-	"strings"
 	"testing"
 )
 
@@ -61,6 +60,11 @@ func TestParseCacheDrives(t *testing.T) {
 			expectedPatterns []string
 			success          bool
 		}{"/home/drive1;/home/drive2;/home/drive3", []string{"/home/drive1", "/home/drive2", "/home/drive3"}, true})
+		testCases = append(testCases, struct {
+			driveStr         string
+			expectedPatterns []string
+			success          bool
+		}{"/home/drive1,/home/drive2,/home/drive3", []string{"/home/drive1", "/home/drive2", "/home/drive3"}, true})
 		testCases = append(testCases, struct {
 			driveStr         string
 			expectedPatterns []string
@@ -73,7 +77,7 @@ func TestParseCacheDrives(t *testing.T) {
 		}{"/home/drive{1..3}", []string{}, false})
 	}
 	for i, testCase := range testCases {
-		drives, err := parseCacheDrives(strings.Split(testCase.driveStr, cacheDelimiterLegacy))
+		drives, err := parseCacheDrives(testCase.driveStr)
 		if err != nil && testCase.success {
 			t.Errorf("Test %d: Expected success but failed instead %s", i+1, err)
 		}
@@ -102,11 +106,12 @@ func TestParseCacheExclude(t *testing.T) {
 
 		// valid input
 		{"bucket1/*;*.png;images/trip/barcelona/*", []string{"bucket1/*", "*.png", "images/trip/barcelona/*"}, true},
+		{"bucket1/*,*.png,images/trip/barcelona/*", []string{"bucket1/*", "*.png", "images/trip/barcelona/*"}, true},
 		{"bucket1", []string{"bucket1"}, true},
 	}
 
 	for i, testCase := range testCases {
-		excludes, err := parseCacheExcludes(strings.Split(testCase.excludeStr, cacheDelimiterLegacy))
+		excludes, err := parseCacheExcludes(testCase.excludeStr)
 		if err != nil && testCase.success {
 			t.Errorf("Test %d: Expected success but failed instead %s", i+1, err)
 		}

cmd/config/cache/lookup.go

@@ -19,7 +19,6 @@ package cache
 import (
 	"errors"
 	"strconv"
-	"strings"
 
 	"github.com/minio/minio/cmd/config"
 	"github.com/minio/minio/pkg/env"
@@ -84,42 +83,43 @@ func LookupConfig(kvs config.KVS) (Config, error) {
 		return cfg, err
 	}
 
-	// Check if cache is explicitly disabled
-	stateBool, err := config.ParseBool(env.Get(EnvCacheState, kvs.Get(config.State)))
-	if err != nil {
-		// Parsing failures happen due to empty KVS, ignore it.
-		if kvs.Empty() {
-			return cfg, nil
-		}
-		return cfg, err
-	}
-
 	drives := env.Get(EnvCacheDrives, kvs.Get(Drives))
-	if stateBool {
+	if len(drives) > 0 {
-		if len(drives) == 0 {
+		// Drives is not-empty means user wishes to enable this explicitly, but
-			return cfg, config.Error("'drives' key cannot be empty if you wish to enable caching")
+		// check if ENV is set to false to disable caching.
-		}
+		stateBool, err := config.ParseBool(env.Get(EnvCacheState, config.StateOn))
-	}
-	if len(drives) == 0 {
-		return cfg, nil
-	}
-
-	cfg.Drives, err = parseCacheDrives(strings.Split(drives, cacheDelimiter))
-	if err != nil {
-		cfg.Drives, err = parseCacheDrives(strings.Split(drives, cacheDelimiterLegacy))
 		if err != nil {
 			return cfg, err
 		}
+		if !stateBool {
+			return cfg, nil
+		}
+	} else {
+		// Check if cache is explicitly disabled
+		stateBool, err := config.ParseBool(env.Get(EnvCacheState, kvs.Get(config.State)))
+		if err != nil {
+			if kvs.Empty() {
+				return cfg, nil
+			}
+			return cfg, err
+		}
+		if stateBool {
+			return cfg, config.Error("'drives' key cannot be empty to enable caching")
+		}
+		return cfg, nil
+	}
+
+	var err error
+	cfg.Drives, err = parseCacheDrives(drives)
+	if err != nil {
+		return cfg, err
 	}
 
 	cfg.Enabled = true
 	if excludes := env.Get(EnvCacheExclude, kvs.Get(Exclude)); excludes != "" {
-		cfg.Exclude, err = parseCacheExcludes(strings.Split(excludes, cacheDelimiter))
+		cfg.Exclude, err = parseCacheExcludes(excludes)
 		if err != nil {
-			cfg.Exclude, err = parseCacheExcludes(strings.Split(excludes, cacheDelimiterLegacy))
+			return cfg, err
-			if err != nil {
-				return cfg, err
-			}
 		}
 	}
 

cmd/config/config.go

@@ -46,12 +46,12 @@ func (e Error) Error() string {
 // Default keys
 const (
 	Default = madmin.Default
-	State   = "state"
+	State   = madmin.StateKey
-	Comment = "comment"
+	Comment = madmin.CommentKey
 
 	// State values
-	StateOn  = "on"
+	StateOn  = madmin.StateOn
-	StateOff = "off"
+	StateOff = madmin.StateOff
 
 	RegionName = "name"
 	AccessKey  = "access_key"
@@ -137,7 +137,7 @@ const (
 	SubSystemSeparator = madmin.SubSystemSeparator
 	KvSeparator        = madmin.KvSeparator
 	KvSpaceSeparator   = madmin.KvSpaceSeparator
-	KvComment          = `#`
+	KvComment          = madmin.KvComment
 	KvNewline          = madmin.KvNewline
 	KvDoubleQuote      = madmin.KvDoubleQuote
 	KvSingleQuote      = madmin.KvSingleQuote
@@ -232,9 +232,11 @@ type Config map[string]map[string]KVS
 
 func (c Config) String() string {
 	var s strings.Builder
-	for k, v := range c {
+	hkvs := HelpSubSysMap[""]
+	for _, hkv := range hkvs {
+		v := c[hkv.Key]
 		for target, kv := range v {
-			s.WriteString(k)
+			s.WriteString(hkv.Key)
 			if target != Default {
 				s.WriteString(SubSystemSeparator)
 				s.WriteString(target)
@@ -252,10 +254,11 @@ func (c Config) DelFrom(r io.Reader) error {
 	scanner := bufio.NewScanner(r)
 	for scanner.Scan() {
 		// Skip any empty lines, or comment like characters
-		if scanner.Text() == "" || strings.HasPrefix(scanner.Text(), KvComment) {
+		text := scanner.Text()
+		if text == "" || strings.HasPrefix(text, KvComment) {
 			continue
 		}
-		if err := c.DelKVS(scanner.Text()); err != nil {
+		if err := c.DelKVS(text); err != nil {
 			return err
 		}
 	}
@@ -271,13 +274,14 @@ func (c Config) ReadFrom(r io.Reader) (int64, error) {
 	scanner := bufio.NewScanner(r)
 	for scanner.Scan() {
 		// Skip any empty lines, or comment like characters
-		if scanner.Text() == "" || strings.HasPrefix(scanner.Text(), KvComment) {
+		text := scanner.Text()
+		if text == "" || strings.HasPrefix(text, KvComment) {
 			continue
 		}
-		if err := c.SetKVS(scanner.Text(), DefaultKVS); err != nil {
+		if err := c.SetKVS(text, DefaultKVS); err != nil {
 			return 0, err
 		}
-		n += len(scanner.Text())
+		n += len(text)
 	}
 	if err := scanner.Err(); err != nil {
 		return 0, err
@@ -411,6 +415,7 @@ func New() Config {
 	srvCfg := make(Config)
 	for _, k := range SubSystems.ToSlice() {
 		srvCfg[k] = map[string]KVS{}
+		srvCfg[k][Default] = DefaultKVS[k]
 	}
 	return srvCfg
 }
@@ -502,12 +507,6 @@ func (c Config) DelKVS(s string) error {
 	return nil
 }
 
-// This function is needed, to trim off single or double quotes, creeping into the values.
-func sanitizeValue(v string) string {
-	v = strings.TrimSuffix(strings.TrimPrefix(strings.TrimSpace(v), KvDoubleQuote), KvDoubleQuote)
-	return strings.TrimSuffix(strings.TrimPrefix(v, KvSingleQuote), KvSingleQuote)
-}
-
 // Clone - clones a config map entirely.
 func (c Config) Clone() Config {
 	cp := New()
@@ -551,8 +550,11 @@ func (c Config) SetKVS(s string, defaultKVS map[string]KVS) error {
 		}
 		if len(kv) == 1 && prevK != "" {
 			kvs = append(kvs, KV{
-				Key:   prevK,
+				Key: prevK,
-				Value: strings.Join([]string{kvs.Get(prevK), sanitizeValue(kv[0])}, KvSpaceSeparator),
+				Value: strings.Join([]string{
+					kvs.Get(prevK),
+					madmin.SanitizeValue(kv[0]),
+				}, KvSpaceSeparator),
 			})
 			continue
 		}
@@ -562,7 +564,7 @@ func (c Config) SetKVS(s string, defaultKVS map[string]KVS) error {
 		prevK = kv[0]
 		kvs = append(kvs, KV{
 			Key:   kv[0],
-			Value: sanitizeValue(kv[1]),
+			Value: madmin.SanitizeValue(kv[1]),
 		})
 	}
 
@@ -574,7 +576,6 @@ func (c Config) SetKVS(s string, defaultKVS map[string]KVS) error {
 
 	// Save client sent kvs
 	c[subSys][tgt] = kvs
-
 	_, ok := c[subSys][tgt].Lookup(State)
 	if !ok {
 		// implicit state "on" if not specified.

cmd/config/storageclass/help.go

@@ -22,14 +22,14 @@ import "github.com/minio/minio/cmd/config"
 var (
 	Help = config.HelpKVS{
 		config.HelpKV{
-			Key:         ClassRRS,
+			Key:         ClassStandard,
-			Description: `Set reduced redundancy storage class parity ratio. eg: "EC:2"`,
+			Description: `Set standard storage class parity ratio. eg: "EC:4"`,
 			Optional:    true,
 			Type:        "string",
 		},
 		config.HelpKV{
-			Key:         ClassStandard,
+			Key:         ClassRRS,
-			Description: `Set standard storage class parity ratio. eg: "EC:4"`,
+			Description: `Set reduced redundancy storage class parity ratio. eg: "EC:2"`,
 			Optional:    true,
 			Type:        "string",
 		},

pkg/madmin/config-kv-commands.go

@@ -51,11 +51,7 @@ func (adm *AdminClient) DelConfigKV(k string) (err error) {
 
 // SetConfigKV - set key value config to server.
 func (adm *AdminClient) SetConfigKV(kv string) (err error) {
-	targets, err := ParseSubSysTarget([]byte(kv))
+	econfigBytes, err := EncryptData(adm.secretAccessKey, []byte(kv))
-	if err != nil {
-		return err
-	}
-	econfigBytes, err := EncryptData(adm.secretAccessKey, []byte(targets.String()))
 	if err != nil {
 		return err
 	}

pkg/madmin/parse-kv.go

@@ -59,22 +59,33 @@ func (kvs KVS) Lookup(key string) (string, bool) {
 	return "", false
 }
 
-// Targets sub-system targets
+// Target signifies an individual target
-type Targets map[string]map[string]KVS
+type Target struct {
+	SubSystem string `json:"subSys"`
+	KVS       KVS    `json:"kvs"`
+}
 
+// Targets sub-system targets
+type Targets []Target
+
+// Standard config keys and values.
 const (
-	stateKey   = "state"
+	StateKey   = "state"
-	commentKey = "comment"
+	CommentKey = "comment"
+
+	// State values
+	StateOn  = "on"
+	StateOff = "off"
 )
 
 func (kvs KVS) String() string {
 	var s strings.Builder
 	for _, kv := range kvs {
-		// Do not need to print state
+		// Do not need to print state which is on.
-		if kv.Key == stateKey {
+		if kv.Key == StateKey && kv.Value == StateOn {
 			continue
 		}
-		if kv.Key == commentKey && kv.Value == "" {
+		if kv.Key == CommentKey && kv.Value == "" {
 			continue
 		}
 		s.WriteString(kv.Key)
@@ -94,13 +105,7 @@ func (kvs KVS) String() string {
 
 // Count - returns total numbers of target
 func (t Targets) Count() int {
-	var count int
+	return len(t)
-	for _, targetKV := range t {
-		for range targetKV {
-			count++
-		}
-	}
-	return count
 }
 
 func hasSpace(s string) bool {
@@ -115,19 +120,15 @@ func hasSpace(s string) bool {
 func (t Targets) String() string {
 	var s strings.Builder
 	count := t.Count()
-	for subSys, targetKV := range t {
+	// Print all "on" states entries
-		for target, kv := range targetKV {
+	for _, targetKV := range t {
-			count--
+		kv := targetKV.KVS
-			s.WriteString(subSys)
+		count--
-			if target != Default {
+		s.WriteString(targetKV.SubSystem)
-				s.WriteString(SubSystemSeparator)
+		s.WriteString(KvSpaceSeparator)
-				s.WriteString(target)
+		s.WriteString(kv.String())
-			}
+		if len(t) > 1 && count > 0 {
-			s.WriteString(KvSpaceSeparator)
+			s.WriteString(KvNewline)
-			s.WriteString(kv.String())
-			if (len(t) > 1 || len(targetKV) > 1) && count > 0 {
-				s.WriteString(KvNewline)
-			}
 		}
 	}
 	return s.String()
@@ -137,6 +138,7 @@ func (t Targets) String() string {
 const (
 	SubSystemSeparator = `:`
 	KvSeparator        = `=`
+	KvComment          = `#`
 	KvSpaceSeparator   = ` `
 	KvNewline          = "\n"
 	KvDoubleQuote      = `"`
@@ -145,13 +147,14 @@ const (
 	Default = `_`
 )
 
-// This function is needed, to trim off single or double quotes, creeping into the values.
+// SanitizeValue - this function is needed, to trim off single or double quotes, creeping into the values.
-func sanitizeValue(v string) string {
+func SanitizeValue(v string) string {
 	v = strings.TrimSuffix(strings.TrimPrefix(strings.TrimSpace(v), KvDoubleQuote), KvDoubleQuote)
 	return strings.TrimSuffix(strings.TrimPrefix(v, KvSingleQuote), KvSingleQuote)
 }
 
-func convertTargets(s string, targets Targets) error {
+// AddTarget - adds new targets, by parsing the input string s.
+func (t *Targets) AddTarget(s string) error {
 	inputs := strings.SplitN(s, KvSpaceSeparator, 2)
 	if len(inputs) <= 1 {
 		return fmt.Errorf("invalid number of arguments '%s'", s)
@@ -170,7 +173,7 @@ func convertTargets(s string, targets Targets) error {
 		if len(kv) == 1 && prevK != "" {
 			kvs = append(kvs, KV{
 				Key:   prevK,
-				Value: strings.Join([]string{kvs.Get(prevK), sanitizeValue(kv[0])}, KvSpaceSeparator),
+				Value: strings.Join([]string{kvs.Get(prevK), SanitizeValue(kv[0])}, KvSpaceSeparator),
 			})
 			continue
 		}
@@ -180,28 +183,24 @@ func convertTargets(s string, targets Targets) error {
 		prevK = kv[0]
 		kvs = append(kvs, KV{
 			Key:   kv[0],
-			Value: sanitizeValue(kv[1]),
+			Value: SanitizeValue(kv[1]),
 		})
 	}
 
-	_, ok := targets[subSystemValue[0]]
+	*t = append(*t, Target{
-	if !ok {
+		SubSystem: inputs[0],
-		targets[subSystemValue[0]] = map[string]KVS{}
+		KVS:       kvs,
-	}
+	})
-	if len(subSystemValue) == 2 {
+
-		targets[subSystemValue[0]][subSystemValue[1]] = kvs
-	} else {
-		targets[subSystemValue[0]][Default] = kvs
-	}
 	return nil
 }
 
 // ParseSubSysTarget - parse sub-system target
 func ParseSubSysTarget(buf []byte) (Targets, error) {
-	targets := make(map[string]map[string]KVS)
+	var targets Targets
 	bio := bufio.NewScanner(bytes.NewReader(buf))
 	for bio.Scan() {
-		if err := convertTargets(bio.Text(), targets); err != nil {
+		if err := targets.AddTarget(bio.Text()); err != nil {
 			return nil, err
 		}
 	}