From c89aee37b9bfed6889722c1baed9c199a91e8faf Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Wed, 1 Sep 2021 11:34:07 -0700 Subject: [PATCH] fix: log errors for incorrect environment inputs (#13121) Invalid MINIO_ARGS, MINIO_ENDPOINTS would be silently ignored when using remoteEnv style, make sure to log errors to indicate invalid configuration. --- cmd/admin-handlers-config-kv.go | 6 +-- cmd/config-current.go | 52 ++++++++++++++++++------- cmd/config.go | 4 +- cmd/gateway-main.go | 4 +- cmd/server-main.go | 14 +++++-- cmd/storage-rest_test.go | 69 +++++++-------------------------- go.mod | 2 +- go.sum | 4 +- 8 files changed, 71 insertions(+), 84 deletions(-) diff --git a/cmd/admin-handlers-config-kv.go b/cmd/admin-handlers-config-kv.go index 08fbbd0ca..1813d1714 100644 --- a/cmd/admin-handlers-config-kv.go +++ b/cmd/admin-handlers-config-kv.go @@ -117,7 +117,7 @@ func (a adminAPIHandlers) SetConfigKVHandler(w http.ResponseWriter, r *http.Requ return } - if err = validateConfig(cfg, objectAPI.SetDriveCounts()); err != nil { + if err = validateConfig(cfg); err != nil { writeCustomErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAdminConfigBadJSON), err.Error(), r.URL) return } @@ -248,7 +248,7 @@ func (a adminAPIHandlers) RestoreConfigHistoryKVHandler(w http.ResponseWriter, r return } - if err = validateConfig(cfg, objectAPI.SetDriveCounts()); err != nil { + if err = validateConfig(cfg); err != nil { writeCustomErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAdminConfigBadJSON), err.Error(), r.URL) return } @@ -360,7 +360,7 @@ func (a adminAPIHandlers) SetConfigHandler(w http.ResponseWriter, r *http.Reques return } - if err = validateConfig(cfg, objectAPI.SetDriveCounts()); err != nil { + if err = validateConfig(cfg); err != nil { writeCustomErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAdminConfigBadJSON), err.Error(), r.URL) return } diff --git a/cmd/config-current.go b/cmd/config-current.go index 97176f349..20c71b2a9 100644 --- a/cmd/config-current.go +++ b/cmd/config-current.go @@ -227,7 +227,9 @@ var ( globalServerConfigMu sync.RWMutex ) -func validateConfig(s config.Config, setDriveCounts []int) error { +func validateConfig(s config.Config) error { + objAPI := newObjectLayerFn() + // We must have a global lock for this so nobody else modifies env while we do. defer env.LockSetEnv()() @@ -250,7 +252,10 @@ func validateConfig(s config.Config, setDriveCounts []int) error { } if globalIsErasure { - for _, setDriveCount := range setDriveCounts { + if objAPI == nil { + return errServerNotInitialized + } + for _, setDriveCount := range objAPI.SetDriveCounts() { if _, err := storageclass.LookupConfig(s[config.StorageClassSubSys][config.Default], setDriveCount); err != nil { return err } @@ -265,7 +270,7 @@ func validateConfig(s config.Config, setDriveCounts []int) error { if err != nil { return err } - objAPI := newObjectLayerFn() + if objAPI != nil { if compCfg.Enabled && !objAPI.IsCompressionSupported() { return fmt.Errorf("Backend does not support compression") @@ -325,7 +330,7 @@ func validateConfig(s config.Config, setDriveCounts []int) error { return notify.TestNotificationTargets(GlobalContext, s, NewGatewayHTTPTransport(), globalNotificationSys.ConfiguredTargetIDs()) } -func lookupConfigs(s config.Config, setDriveCounts []int) { +func lookupConfigs(s config.Config, objAPI ObjectLayer) { ctx := GlobalContext var err error @@ -337,7 +342,15 @@ func lookupConfigs(s config.Config, setDriveCounts []int) { } } - if dnsURL, dnsUser, dnsPass, ok := env.LookupEnv(config.EnvDNSWebhook); ok { + dnsURL, dnsUser, dnsPass, err := env.LookupEnv(config.EnvDNSWebhook) + if err != nil { + if globalIsGateway { + logger.FatalIf(err, "Unable to initialize remote webhook DNS config") + } else { + logger.LogIf(ctx, fmt.Errorf("Unable to initialize remote webhook DNS config %w", err)) + } + } + if err == nil && dnsURL != "" { globalDNSConfig, err = dns.NewOperatorDNS(dnsURL, dns.Authentication(dnsUser, dnsPass), dns.RootCAs(globalRootCAs)) @@ -417,7 +430,8 @@ func lookupConfigs(s config.Config, setDriveCounts []int) { getRemoteInstanceTransport = newGatewayHTTPTransport(apiConfig.RemoteTransportDeadline) }) - if globalIsErasure { + if globalIsErasure && objAPI != nil { + setDriveCounts := objAPI.SetDriveCounts() for i, setDriveCount := range setDriveCounts { sc, err := storageclass.LookupConfig(s[config.StorageClassSubSys][config.Default], setDriveCount) if err != nil { @@ -529,16 +543,18 @@ func lookupConfigs(s config.Config, setDriveCounts []int) { } // Apply dynamic config values - logger.LogIf(ctx, applyDynamicConfig(ctx, newObjectLayerFn(), s)) + if err := applyDynamicConfig(ctx, objAPI, s); err != nil { + if globalIsGateway { + logger.FatalIf(err, "Unable to initialize dynamic configuration") + } else { + logger.LogIf(ctx, err) + } + } } // applyDynamicConfig will apply dynamic config values. // Dynamic systems should be in config.SubSystemsDynamic as well. func applyDynamicConfig(ctx context.Context, objAPI ObjectLayer, s config.Config) error { - if objAPI == nil { - return nil - } - // Read all dynamic configs. // API apiConfig, err := api.LookupConfig(s[config.APISubSys][config.Default]) @@ -553,8 +569,10 @@ func applyDynamicConfig(ctx context.Context, objAPI ObjectLayer, s config.Config } // Validate if the object layer supports compression. - if cmpCfg.Enabled && !objAPI.IsCompressionSupported() { - return fmt.Errorf("Backend does not support compression") + if objAPI != nil { + if cmpCfg.Enabled && !objAPI.IsCompressionSupported() { + return fmt.Errorf("Backend does not support compression") + } } // Heal @@ -571,7 +589,11 @@ func applyDynamicConfig(ctx context.Context, objAPI ObjectLayer, s config.Config // Apply configurations. // We should not fail after this. - globalAPIConfig.init(apiConfig, objAPI.SetDriveCounts()) + var setDriveCounts []int + if objAPI != nil { + setDriveCounts = objAPI.SetDriveCounts() + } + globalAPIConfig.init(apiConfig, setDriveCounts) globalCompressConfigMu.Lock() globalCompressConfig = cmpCfg @@ -702,7 +724,7 @@ func loadConfig(objAPI ObjectLayer) error { } // Override any values from ENVs. - lookupConfigs(srvCfg, objAPI.SetDriveCounts()) + lookupConfigs(srvCfg, objAPI) // hold the mutex lock before a new config is assigned. globalServerConfigMu.Lock() diff --git a/cmd/config.go b/cmd/config.go index efa76af37..c5401af6b 100644 --- a/cmd/config.go +++ b/cmd/config.go @@ -155,7 +155,7 @@ func readServerConfig(ctx context.Context, objAPI ObjectLayer) (config.Config, e data, err := readConfig(ctx, objAPI, configFile) if err != nil { if errors.Is(err, errConfigNotFound) { - lookupConfigs(srvCfg, objAPI.SetDriveCounts()) + lookupConfigs(srvCfg, objAPI) return srvCfg, nil } return nil, err @@ -166,7 +166,7 @@ func readServerConfig(ctx context.Context, objAPI ObjectLayer) (config.Config, e minioMetaBucket: path.Join(minioMetaBucket, configFile), }) if err != nil { - lookupConfigs(srvCfg, objAPI.SetDriveCounts()) + lookupConfigs(srvCfg, objAPI) return nil, err } } diff --git a/cmd/gateway-main.go b/cmd/gateway-main.go index b97af7925..1dae42de1 100644 --- a/cmd/gateway-main.go +++ b/cmd/gateway-main.go @@ -218,8 +218,6 @@ func StartGateway(ctx *cli.Context, gw Gateway) { // Set when gateway is enabled globalIsGateway = true - enableConfigOps := false - // TODO: We need to move this code with globalConfigSys.Init() // for now keep it here such that "s3" gateway layer initializes // itself properly when KMS is set. @@ -245,7 +243,7 @@ func StartGateway(ctx *cli.Context, gw Gateway) { // Enable IAM admin APIs if etcd is enabled, if not just enable basic // operations such as profiling, server info etc. - registerAdminRouter(router, enableConfigOps) + registerAdminRouter(router, false) // Add healthcheck router registerHealthCheckRouter(router) diff --git a/cmd/server-main.go b/cmd/server-main.go index bec25f220..513a0ac65 100644 --- a/cmd/server-main.go +++ b/cmd/server-main.go @@ -105,10 +105,18 @@ EXAMPLES: } func serverCmdArgs(ctx *cli.Context) []string { - v := env.Get(config.EnvArgs, "") + v, _, _, err := env.LookupEnv(config.EnvArgs) + if err != nil { + logger.FatalIf(err, "Unable to validate passed arguments in %s:%s", + config.EnvArgs, os.Getenv(config.EnvArgs)) + } if v == "" { - // Fall back to older ENV MINIO_ENDPOINTS - v = env.Get(config.EnvEndpoints, "") + // Fall back to older environment value MINIO_ENDPOINTS + v, _, _, err = env.LookupEnv(config.EnvEndpoints) + if err != nil { + logger.FatalIf(err, "Unable to validate passed arguments in %s:%s", + config.EnvEndpoints, os.Getenv(config.EnvEndpoints)) + } } if v == "" { if !ctx.Args().Present() || ctx.Args().First() == "help" { diff --git a/cmd/storage-rest_test.go b/cmd/storage-rest_test.go index 3966d190d..16f2d8c53 100644 --- a/cmd/storage-rest_test.go +++ b/cmd/storage-rest_test.go @@ -26,7 +26,6 @@ import ( "testing" "github.com/gorilla/mux" - "github.com/minio/minio/internal/config" xnet "github.com/minio/pkg/net" ) @@ -416,7 +415,7 @@ func testStorageAPIRenameFile(t *testing.T, storage StorageAPI) { } } -func newStorageRESTHTTPServerClient(t *testing.T) (*httptest.Server, *storageRESTClient, config.Config, string) { +func newStorageRESTHTTPServerClient(t *testing.T) (*httptest.Server, *storageRESTClient, string) { prevHost, prevPort := globalMinioHost, globalMinioPort defer func() { globalMinioHost, globalMinioPort = prevHost, prevPort @@ -451,142 +450,102 @@ func newStorageRESTHTTPServerClient(t *testing.T) (*httptest.Server, *storageRES Endpoints: Endpoints{endpoint}, }}) - prevGlobalServerConfig := globalServerConfig - globalServerConfig = newServerConfig() - lookupConfigs(globalServerConfig, nil) - restClient := newStorageRESTClient(endpoint, false) - return httpServer, restClient, prevGlobalServerConfig, endpointPath + return httpServer, restClient, endpointPath } func TestStorageRESTClientDiskInfo(t *testing.T) { - httpServer, restClient, prevGlobalServerConfig, endpointPath := newStorageRESTHTTPServerClient(t) + httpServer, restClient, endpointPath := newStorageRESTHTTPServerClient(t) defer httpServer.Close() - defer func() { - globalServerConfig = prevGlobalServerConfig - }() defer os.RemoveAll(endpointPath) testStorageAPIDiskInfo(t, restClient) } func TestStorageRESTClientMakeVol(t *testing.T) { - httpServer, restClient, prevGlobalServerConfig, endpointPath := newStorageRESTHTTPServerClient(t) + httpServer, restClient, endpointPath := newStorageRESTHTTPServerClient(t) defer httpServer.Close() - defer func() { - globalServerConfig = prevGlobalServerConfig - }() defer os.RemoveAll(endpointPath) testStorageAPIMakeVol(t, restClient) } func TestStorageRESTClientListVols(t *testing.T) { - httpServer, restClient, prevGlobalServerConfig, endpointPath := newStorageRESTHTTPServerClient(t) + httpServer, restClient, endpointPath := newStorageRESTHTTPServerClient(t) defer httpServer.Close() - defer func() { - globalServerConfig = prevGlobalServerConfig - }() defer os.RemoveAll(endpointPath) testStorageAPIListVols(t, restClient) } func TestStorageRESTClientStatVol(t *testing.T) { - httpServer, restClient, prevGlobalServerConfig, endpointPath := newStorageRESTHTTPServerClient(t) + httpServer, restClient, endpointPath := newStorageRESTHTTPServerClient(t) defer httpServer.Close() - defer func() { - globalServerConfig = prevGlobalServerConfig - }() defer os.RemoveAll(endpointPath) testStorageAPIStatVol(t, restClient) } func TestStorageRESTClientDeleteVol(t *testing.T) { - httpServer, restClient, prevGlobalServerConfig, endpointPath := newStorageRESTHTTPServerClient(t) + httpServer, restClient, endpointPath := newStorageRESTHTTPServerClient(t) defer httpServer.Close() - defer func() { - globalServerConfig = prevGlobalServerConfig - }() defer os.RemoveAll(endpointPath) testStorageAPIDeleteVol(t, restClient) } func TestStorageRESTClientStatInfoFile(t *testing.T) { - httpServer, restClient, prevGlobalServerConfig, endpointPath := newStorageRESTHTTPServerClient(t) + httpServer, restClient, endpointPath := newStorageRESTHTTPServerClient(t) defer httpServer.Close() - defer func() { - globalServerConfig = prevGlobalServerConfig - }() defer os.RemoveAll(endpointPath) testStorageAPIStatInfoFile(t, restClient) } func TestStorageRESTClientListDir(t *testing.T) { - httpServer, restClient, prevGlobalServerConfig, endpointPath := newStorageRESTHTTPServerClient(t) + httpServer, restClient, endpointPath := newStorageRESTHTTPServerClient(t) defer httpServer.Close() - defer func() { - globalServerConfig = prevGlobalServerConfig - }() defer os.RemoveAll(endpointPath) testStorageAPIListDir(t, restClient) } func TestStorageRESTClientReadAll(t *testing.T) { - httpServer, restClient, prevGlobalServerConfig, endpointPath := newStorageRESTHTTPServerClient(t) + httpServer, restClient, endpointPath := newStorageRESTHTTPServerClient(t) defer httpServer.Close() - defer func() { - globalServerConfig = prevGlobalServerConfig - }() defer os.RemoveAll(endpointPath) testStorageAPIReadAll(t, restClient) } func TestStorageRESTClientReadFile(t *testing.T) { - httpServer, restClient, prevGlobalServerConfig, endpointPath := newStorageRESTHTTPServerClient(t) + httpServer, restClient, endpointPath := newStorageRESTHTTPServerClient(t) defer httpServer.Close() - defer func() { - globalServerConfig = prevGlobalServerConfig - }() defer os.RemoveAll(endpointPath) testStorageAPIReadFile(t, restClient) } func TestStorageRESTClientAppendFile(t *testing.T) { - httpServer, restClient, prevGlobalServerConfig, endpointPath := newStorageRESTHTTPServerClient(t) + httpServer, restClient, endpointPath := newStorageRESTHTTPServerClient(t) defer httpServer.Close() - defer func() { - globalServerConfig = prevGlobalServerConfig - }() defer os.RemoveAll(endpointPath) testStorageAPIAppendFile(t, restClient) } func TestStorageRESTClientDeleteFile(t *testing.T) { - httpServer, restClient, prevGlobalServerConfig, endpointPath := newStorageRESTHTTPServerClient(t) + httpServer, restClient, endpointPath := newStorageRESTHTTPServerClient(t) defer httpServer.Close() - defer func() { - globalServerConfig = prevGlobalServerConfig - }() defer os.RemoveAll(endpointPath) testStorageAPIDeleteFile(t, restClient) } func TestStorageRESTClientRenameFile(t *testing.T) { - httpServer, restClient, prevGlobalServerConfig, endpointPath := newStorageRESTHTTPServerClient(t) + httpServer, restClient, endpointPath := newStorageRESTHTTPServerClient(t) defer httpServer.Close() - defer func() { - globalServerConfig = prevGlobalServerConfig - }() defer os.RemoveAll(endpointPath) testStorageAPIRenameFile(t, restClient) diff --git a/go.mod b/go.mod index 5b789ef7f..60f88357f 100644 --- a/go.mod +++ b/go.mod @@ -48,7 +48,7 @@ require ( github.com/minio/madmin-go v1.1.0 github.com/minio/minio-go/v7 v7.0.13-0.20210823191913-cee488b95ff2 github.com/minio/parquet-go v1.0.0 - github.com/minio/pkg v1.0.11 + github.com/minio/pkg v1.1.2 github.com/minio/selfupdate v0.3.1 github.com/minio/sha256-simd v1.0.0 github.com/minio/simdjson-go v0.2.1 diff --git a/go.sum b/go.sum index aedb2966b..622a76e54 100644 --- a/go.sum +++ b/go.sum @@ -1047,8 +1047,8 @@ github.com/minio/parquet-go v1.0.0/go.mod h1:aQlkSOfOq2AtQKkuou3mosNVMwNokd+faTa github.com/minio/pkg v1.0.3/go.mod h1:obU54TZ9QlMv0TRaDgQ/JTzf11ZSXxnSfLrm4tMtBP8= github.com/minio/pkg v1.0.4/go.mod h1:obU54TZ9QlMv0TRaDgQ/JTzf11ZSXxnSfLrm4tMtBP8= github.com/minio/pkg v1.0.8/go.mod h1:32x/3OmGB0EOi1N+3ggnp+B5VFkSBBB9svPMVfpnf14= -github.com/minio/pkg v1.0.11 h1:gfpkP7SiznM7EyyHIfQ7lu98Ae4hV4Z+8YsoFQbH7PY= -github.com/minio/pkg v1.0.11/go.mod h1:32x/3OmGB0EOi1N+3ggnp+B5VFkSBBB9svPMVfpnf14= +github.com/minio/pkg v1.1.2 h1:A7zSlVIQCf71DPYM1kqBpybq6fmsRY3RuTMD0ZNcqZA= +github.com/minio/pkg v1.1.2/go.mod h1:32x/3OmGB0EOi1N+3ggnp+B5VFkSBBB9svPMVfpnf14= github.com/minio/selfupdate v0.3.1 h1:BWEFSNnrZVMUWXbXIgLDNDjbejkmpAmZvy/nCz1HlEs= github.com/minio/selfupdate v0.3.1/go.mod h1:b8ThJzzH7u2MkF6PcIra7KaXO9Khf6alWPvMSyTDCFM= github.com/minio/sha256-simd v0.1.1/go.mod h1:B5e1o+1/KgNmWrSQK08Y6Z1Vb5pwIktudl0J58iy0KM=