From 0c31e61343454b229743a2b75d50ef1a3fdc3734 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Wed, 10 Apr 2024 18:10:30 -0700 Subject: [PATCH] allow protection from invalid config values (#19460) we have had numerous reports on some config values not having default values, causing features misbehaving and not having default values set properly. This PR tries to address all these concerns once and for all. Each new sub-system that gets added - must check for invalid keys - must have default values set - must not "return err" when being saved into a global state() instead collate as part of other subsystem errors allow other sub-systems to independently initialize. --- cmd/config-current.go | 57 +++++++++++++++------------ cmd/utils.go | 4 ++ internal/config/browser/browser.go | 26 +++++++++---- internal/config/drive/drive.go | 23 ++++++----- internal/config/heal/heal.go | 9 +++-- internal/config/ilm/ilm.go | 9 +++++ internal/config/scanner/scanner.go | 62 +++++++++++++++++------------- 7 files changed, 118 insertions(+), 72 deletions(-) diff --git a/cmd/config-current.go b/cmd/config-current.go index ed45acc2e..062361887 100644 --- a/cmd/config-current.go +++ b/cmd/config-current.go @@ -566,6 +566,7 @@ func applyDynamicConfigForSubSys(ctx context.Context, objAPI ObjectLayer, s conf return errServerNotInitialized } + var errs []error setDriveCounts := objAPI.SetDriveCounts() switch subSys { case config.APISubSys: @@ -588,26 +589,29 @@ func applyDynamicConfigForSubSys(ctx context.Context, objAPI ObjectLayer, s conf case config.HealSubSys: healCfg, err := heal.LookupConfig(s[config.HealSubSys][config.Default]) if err != nil { - return fmt.Errorf("Unable to apply heal config: %w", err) + errs = append(errs, fmt.Errorf("Unable to apply heal config: %w", err)) + } else { + globalHealConfig.Update(healCfg) } - globalHealConfig.Update(healCfg) case config.BatchSubSys: batchCfg, err := batch.LookupConfig(s[config.BatchSubSys][config.Default]) if err != nil { - return fmt.Errorf("Unable to apply batch config: %w", err) + errs = append(errs, fmt.Errorf("Unable to apply batch config: %w", err)) + } else { + globalBatchConfig.Update(batchCfg) } - globalBatchConfig.Update(batchCfg) case config.ScannerSubSys: scannerCfg, err := scanner.LookupConfig(s[config.ScannerSubSys][config.Default]) if err != nil { - return fmt.Errorf("Unable to apply scanner config: %w", err) + errs = append(errs, fmt.Errorf("Unable to apply scanner config: %w", err)) + } else { + // update dynamic scanner values. + scannerIdleMode.Store(scannerCfg.IdleMode) + scannerCycle.Store(scannerCfg.Cycle) + scannerExcessObjectVersions.Store(scannerCfg.ExcessVersions) + scannerExcessFolders.Store(scannerCfg.ExcessFolders) + configLogIf(ctx, scannerSleeper.Update(scannerCfg.Delay, scannerCfg.MaxWait)) } - // update dynamic scanner values. - scannerIdleMode.Store(scannerCfg.IdleMode) - scannerCycle.Store(scannerCfg.Cycle) - scannerExcessObjectVersions.Store(scannerCfg.ExcessVersions) - scannerExcessFolders.Store(scannerCfg.ExcessFolders) - configLogIf(ctx, scannerSleeper.Update(scannerCfg.Delay, scannerCfg.MaxWait)) case config.LoggerWebhookSubSys: loggerCfg, err := logger.LookupConfigForSubSys(ctx, s, config.LoggerWebhookSubSys) if err != nil { @@ -693,11 +697,11 @@ func applyDynamicConfigForSubSys(ctx context.Context, objAPI ObjectLayer, s conf } } case config.DriveSubSys: - if driveConfig, err := drive.LookupConfig(s[config.DriveSubSys][config.Default]); err != nil { + driveConfig, err := drive.LookupConfig(s[config.DriveSubSys][config.Default]) + if err != nil { configLogIf(ctx, fmt.Errorf("Unable to load drive config: %w", err)) } else { - err := globalDriveConfig.Update(driveConfig) - if err != nil { + if err = globalDriveConfig.Update(driveConfig); err != nil { configLogIf(ctx, fmt.Errorf("Unable to update drive config: %v", err)) } } @@ -711,27 +715,32 @@ func applyDynamicConfigForSubSys(ctx context.Context, objAPI ObjectLayer, s conf case config.BrowserSubSys: browserCfg, err := browser.LookupConfig(s[config.BrowserSubSys][config.Default]) if err != nil { - return fmt.Errorf("Unable to apply browser config: %w", err) + errs = append(errs, fmt.Errorf("Unable to apply browser config: %w", err)) + } else { + globalBrowserConfig.Update(browserCfg) } - globalBrowserConfig.Update(browserCfg) case config.ILMSubSys: ilmCfg, err := ilm.LookupConfig(s[config.ILMSubSys][config.Default]) if err != nil { - return fmt.Errorf("Unable to apply ilm config: %w", err) + errs = append(errs, fmt.Errorf("Unable to apply ilm config: %w", err)) + } else { + if globalTransitionState != nil { + globalTransitionState.UpdateWorkers(ilmCfg.TransitionWorkers) + } + if globalExpiryState != nil { + globalExpiryState.ResizeWorkers(ilmCfg.ExpirationWorkers) + } + globalILMConfig.update(ilmCfg) } - if globalTransitionState != nil { - globalTransitionState.UpdateWorkers(ilmCfg.TransitionWorkers) - } - if globalExpiryState != nil { - globalExpiryState.ResizeWorkers(ilmCfg.ExpirationWorkers) - } - globalILMConfig.update(ilmCfg) } globalServerConfigMu.Lock() defer globalServerConfigMu.Unlock() if globalServerConfig != nil { globalServerConfig[subSys] = s[subSys] } + if len(errs) > 0 { + return errors.Join(errs...) + } return nil } diff --git a/cmd/utils.go b/cmd/utils.go index 554d3d949..e5e3454cc 100644 --- a/cmd/utils.go +++ b/cmd/utils.go @@ -626,6 +626,10 @@ func NewHTTPTransportWithClientCerts(clientCert, clientKey string) *http.Transpo if err != nil { internalLogIf(ctx, fmt.Errorf("Unable to load client key and cert, please check your client certificate configuration: %w", err)) } + if transport == nil { + // Client certs are not readable return default transport. + return s.NewHTTPTransportWithTimeout(1 * time.Minute) + } return transport } diff --git a/internal/config/browser/browser.go b/internal/config/browser/browser.go index a67451a92..6e4f11a6d 100644 --- a/internal/config/browser/browser.go +++ b/internal/config/browser/browser.go @@ -97,15 +97,30 @@ func (browseCfg *Config) Update(newCfg Config) { // LookupConfig - lookup api config and override with valid environment settings if any. func LookupConfig(kvs config.KVS) (cfg Config, err error) { - cspPolicy := env.Get(EnvBrowserCSPPolicy, kvs.GetWithDefault(browserCSPPolicy, DefaultKVS)) - hstsSeconds, err := strconv.Atoi(env.Get(EnvBrowserHSTSSeconds, kvs.GetWithDefault(browserHSTSSeconds, DefaultKVS))) - if err != nil { + cfg = Config{ + CSPPolicy: env.Get(EnvBrowserCSPPolicy, kvs.GetWithDefault(browserCSPPolicy, DefaultKVS)), + HSTSSeconds: 0, + HSTSIncludeSubdomains: true, + HSTSPreload: true, + ReferrerPolicy: "strict-origin-when-cross-origin", + } + + if err = config.CheckValidKeys(config.BrowserSubSys, kvs, DefaultKVS); err != nil { return cfg, err } hstsIncludeSubdomains := env.Get(EnvBrowserHSTSIncludeSubdomains, kvs.GetWithDefault(browserHSTSIncludeSubdomains, DefaultKVS)) == config.EnableOn hstsPreload := env.Get(EnvBrowserHSTSPreload, kvs.Get(browserHSTSPreload)) == config.EnableOn + hstsSeconds, err := strconv.Atoi(env.Get(EnvBrowserHSTSSeconds, kvs.GetWithDefault(browserHSTSSeconds, DefaultKVS))) + if err != nil { + return cfg, err + } + + cfg.HSTSSeconds = hstsSeconds + cfg.HSTSIncludeSubdomains = hstsIncludeSubdomains + cfg.HSTSPreload = hstsPreload + referrerPolicy := env.Get(EnvBrowserReferrerPolicy, kvs.GetWithDefault(browserReferrerPolicy, DefaultKVS)) switch referrerPolicy { case "no-referrer", "no-referrer-when-downgrade", "origin", "origin-when-cross-origin", "same-origin", "strict-origin", "strict-origin-when-cross-origin", "unsafe-url": @@ -114,11 +129,6 @@ func LookupConfig(kvs config.KVS) (cfg Config, err error) { return cfg, fmt.Errorf("invalid value %v for %s", referrerPolicy, browserReferrerPolicy) } - cfg.CSPPolicy = cspPolicy - cfg.HSTSSeconds = hstsSeconds - cfg.HSTSIncludeSubdomains = hstsIncludeSubdomains - cfg.HSTSPreload = hstsPreload - return cfg, nil } diff --git a/internal/config/drive/drive.go b/internal/config/drive/drive.go index 431086a95..91991135c 100644 --- a/internal/config/drive/drive.go +++ b/internal/config/drive/drive.go @@ -33,30 +33,35 @@ var DefaultKVS = config.KVS{ }, } +var configLk sync.RWMutex + // Config represents the subnet related configuration type Config struct { // MaxTimeout - maximum timeout for a drive operation MaxTimeout time.Duration `json:"maxTimeout"` - mutex sync.RWMutex } // Update - updates the config with latest values -func (c *Config) Update(new *Config) error { - c.mutex.Lock() - defer c.mutex.Unlock() +func (c *Config) Update(new Config) error { + configLk.Lock() + defer configLk.Unlock() c.MaxTimeout = getMaxTimeout(new.MaxTimeout) return nil } // GetMaxTimeout - returns the max timeout value. func (c *Config) GetMaxTimeout() time.Duration { - c.mutex.RLock() - defer c.mutex.RUnlock() + configLk.RLock() + defer configLk.RUnlock() + return getMaxTimeout(c.MaxTimeout) } // LookupConfig - lookup config and override with valid environment settings if any. -func LookupConfig(kvs config.KVS) (cfg *Config, err error) { +func LookupConfig(kvs config.KVS) (cfg Config, err error) { + cfg = Config{ + MaxTimeout: 30 * time.Second, + } if err = config.CheckValidKeys(config.DriveSubSys, kvs, DefaultKVS); err != nil { return cfg, err } @@ -68,9 +73,7 @@ func LookupConfig(kvs config.KVS) (cfg *Config, err error) { d = env.Get("_MINIO_DISK_MAX_TIMEOUT", "") } } - cfg = &Config{ - mutex: sync.RWMutex{}, - } + dur, _ := time.ParseDuration(d) if dur < time.Second { cfg.MaxTimeout = 30 * time.Second diff --git a/internal/config/heal/heal.go b/internal/config/heal/heal.go index 5c57820a7..7c0a7cda6 100644 --- a/internal/config/heal/heal.go +++ b/internal/config/heal/heal.go @@ -157,11 +157,14 @@ func LookupConfig(kvs config.KVS) (cfg Config, err error) { if err = config.CheckValidKeys(config.HealSubSys, kvs, DefaultKVS); err != nil { return cfg, err } - cfg.Bitrot = env.Get(EnvBitrot, kvs.GetWithDefault(Bitrot, DefaultKVS)) - _, err = parseBitrotConfig(cfg.Bitrot) - if err != nil { + + bitrot := env.Get(EnvBitrot, kvs.GetWithDefault(Bitrot, DefaultKVS)) + if _, err = parseBitrotConfig(bitrot); err != nil { return cfg, fmt.Errorf("'heal:bitrotscan' value invalid: %w", err) } + + cfg.Bitrot = bitrot + cfg.Sleep, err = time.ParseDuration(env.Get(EnvSleep, kvs.GetWithDefault(Sleep, DefaultKVS))) if err != nil { return cfg, fmt.Errorf("'heal:max_sleep' value invalid: %w", err) diff --git a/internal/config/ilm/ilm.go b/internal/config/ilm/ilm.go index b677647d5..7d2106a29 100644 --- a/internal/config/ilm/ilm.go +++ b/internal/config/ilm/ilm.go @@ -44,6 +44,15 @@ type Config struct { // LookupConfig - lookup ilm config and override with valid environment settings if any. func LookupConfig(kvs config.KVS) (cfg Config, err error) { + cfg = Config{ + TransitionWorkers: 100, + ExpirationWorkers: 100, + } + + if err = config.CheckValidKeys(config.ILMSubSys, kvs, DefaultKVS); err != nil { + return cfg, err + } + tw, err := strconv.Atoi(env.Get(EnvILMTransitionWorkers, kvs.GetWithDefault(transitionWorkers, DefaultKVS))) if err != nil { return cfg, err diff --git a/internal/config/scanner/scanner.go b/internal/config/scanner/scanner.go index 8fca3644d..02dabc92e 100644 --- a/internal/config/scanner/scanner.go +++ b/internal/config/scanner/scanner.go @@ -114,15 +114,44 @@ var DefaultKVS = config.KVS{ // LookupConfig - lookup config and override with valid environment settings if any. func LookupConfig(kvs config.KVS) (cfg Config, err error) { + cfg = Config{ + ExcessVersions: 100, + ExcessFolders: 50000, + IdleMode: 0, // Default is on + } + if err = config.CheckValidKeys(config.ScannerSubSys, kvs, DefaultKVS); err != nil { return cfg, err } + excessVersions, err := strconv.ParseInt(env.Get(EnvExcessVersions, kvs.GetWithDefault(ExcessVersions, DefaultKVS)), 10, 64) + if err != nil { + return cfg, err + } + cfg.ExcessVersions = excessVersions + + excessFolders, err := strconv.ParseInt(env.Get(EnvExcessFolders, kvs.GetWithDefault(ExcessFolders, DefaultKVS)), 10, 64) + if err != nil { + return cfg, err + } + cfg.ExcessFolders = excessFolders + + switch idleSpeed := env.Get(EnvIdleSpeed, kvs.GetWithDefault(IdleSpeed, DefaultKVS)); idleSpeed { + case "", config.EnableOn: + cfg.IdleMode = 0 + case config.EnableOff: + cfg.IdleMode = 1 + default: + return cfg, fmt.Errorf("unknown value: '%s'", idleSpeed) + } + // Stick to loading deprecated config/env if they are already set, and the Speed value // has not been changed from its "default" value, if it has been changed honor new settings. if kvs.GetWithDefault(Speed, DefaultKVS) == "default" { if kvs.Get(Delay) != "" && kvs.Get(MaxWait) != "" { - return lookupDeprecatedScannerConfig(kvs) + if err = lookupDeprecatedScannerConfig(kvs, &cfg); err != nil { + return cfg, err + } } } @@ -141,38 +170,17 @@ func LookupConfig(kvs config.KVS) (cfg Config, err error) { return cfg, fmt.Errorf("unknown '%s' value", speed) } - switch idleSpeed := env.Get(EnvIdleSpeed, kvs.GetWithDefault(IdleSpeed, DefaultKVS)); idleSpeed { - case "", config.EnableOn: - cfg.IdleMode = 0 - case config.EnableOff: - cfg.IdleMode = 1 - default: - return cfg, fmt.Errorf("unknown value: '%s'", idleSpeed) - } - - excessVersions, err := strconv.ParseInt(env.Get(EnvExcessVersions, kvs.GetWithDefault(ExcessVersions, DefaultKVS)), 10, 64) - if err != nil { - return cfg, err - } - cfg.ExcessVersions = excessVersions - - excessFolders, err := strconv.ParseInt(env.Get(EnvExcessFolders, kvs.GetWithDefault(ExcessFolders, DefaultKVS)), 10, 64) - if err != nil { - return cfg, err - } - cfg.ExcessFolders = excessFolders - return cfg, nil } -func lookupDeprecatedScannerConfig(kvs config.KVS) (cfg Config, err error) { +func lookupDeprecatedScannerConfig(kvs config.KVS, cfg *Config) (err error) { delay := env.Get(EnvDelayLegacy, "") if delay == "" { delay = env.Get(EnvDelay, kvs.GetWithDefault(Delay, DefaultKVS)) } cfg.Delay, err = strconv.ParseFloat(delay, 64) if err != nil { - return cfg, err + return err } maxWait := env.Get(EnvMaxWaitLegacy, "") if maxWait == "" { @@ -180,7 +188,7 @@ func lookupDeprecatedScannerConfig(kvs config.KVS) (cfg Config, err error) { } cfg.MaxWait, err = time.ParseDuration(maxWait) if err != nil { - return cfg, err + return err } cycle := env.Get(EnvCycle, kvs.GetWithDefault(Cycle, DefaultKVS)) if cycle == "" { @@ -188,7 +196,7 @@ func lookupDeprecatedScannerConfig(kvs config.KVS) (cfg Config, err error) { } cfg.Cycle, err = time.ParseDuration(cycle) if err != nil { - return cfg, err + return err } - return cfg, nil + return nil }