From b1705599e199d8652066946e8cdb6fb2ef3a008d Mon Sep 17 00:00:00 2001 From: Praveen raj Mani Date: Thu, 25 Jun 2020 15:59:28 +0530 Subject: [PATCH] Fix config leaks and deprecate file-based config setters in NAS gateway (#9884) This PR has the following changes - Removing duplicate lookupConfigs() calls. - Deprecate admin config APIs for NAS gateways. This will avoid repeated reloads of the config from the disk. - WatchConfigNASDisk will be removed - Migration guide for NAS gateways users to migrate to ENV settings. NOTE: THIS PR HAS A BREAKING CHANGE Fixes #9875 Co-authored-by: Harshavardhana --- cmd/config-current.go | 7 ++----- cmd/config-encrypted.go | 5 +---- cmd/config.go | 18 ------------------ cmd/config/notify/parse.go | 4 +--- cmd/gateway-main.go | 18 ++---------------- cmd/server-main.go | 2 +- docs/gateway/nas.md | 30 ++++++++++++++++++++++++++++++ 7 files changed, 37 insertions(+), 47 deletions(-) diff --git a/cmd/config-current.go b/cmd/config-current.go index 5a591228f..ed3a090e5 100644 --- a/cmd/config-current.go +++ b/cmd/config-current.go @@ -482,12 +482,12 @@ func lookupConfigs(s config.Config) { } } - globalConfigTargetList, err = notify.GetNotificationTargets(s, GlobalContext.Done(), NewGatewayHTTPTransport()) + globalConfigTargetList, err = notify.GetNotificationTargets(s, GlobalContext.Done(), NewGatewayHTTPTransport(), false) if err != nil { logger.LogIf(ctx, fmt.Errorf("Unable to initialize notification target(s): %w", err)) } - globalEnvTargetList, err = notify.GetNotificationTargets(newServerConfig(), GlobalContext.Done(), NewGatewayHTTPTransport()) + globalEnvTargetList, err = notify.GetNotificationTargets(newServerConfig(), GlobalContext.Done(), NewGatewayHTTPTransport(), true) if err != nil { logger.LogIf(ctx, fmt.Errorf("Unable to initialize notification target(s): %w", err)) } @@ -579,9 +579,6 @@ func newSrvConfig(objAPI ObjectLayer) error { // Initialize server config. srvCfg := newServerConfig() - // Override any values from ENVs. - lookupConfigs(srvCfg) - // hold the mutex lock before a new config is assigned. globalServerConfigMu.Lock() globalServerConfig = srvCfg diff --git a/cmd/config-encrypted.go b/cmd/config-encrypted.go index 170365381..0a669d4b9 100644 --- a/cmd/config-encrypted.go +++ b/cmd/config-encrypted.go @@ -31,10 +31,7 @@ import ( "github.com/minio/minio/pkg/madmin" ) -func handleEncryptedConfigBackend(objAPI ObjectLayer, server bool) error { - if !server { - return nil - } +func handleEncryptedConfigBackend(objAPI ObjectLayer) error { encrypted, err := checkBackendEncrypted(objAPI) if err != nil { diff --git a/cmd/config.go b/cmd/config.go index a06addb58..d2f866ca5 100644 --- a/cmd/config.go +++ b/cmd/config.go @@ -23,7 +23,6 @@ import ( "path" "sort" "strings" - "time" jsoniter "github.com/json-iterator/go" "github.com/minio/minio/cmd/config" @@ -184,23 +183,6 @@ func (sys *ConfigSys) Load(objAPI ObjectLayer) error { return sys.Init(objAPI) } -// WatchConfigNASDisk - watches nas disk on periodic basis. -func (sys *ConfigSys) WatchConfigNASDisk(ctx context.Context, objAPI ObjectLayer) { - configInterval := globalRefreshIAMInterval - watchDisk := func() { - for { - select { - case <-ctx.Done(): - return - case <-time.After(configInterval): - loadConfig(objAPI) - } - } - } - // Refresh configSys in background for NAS gateway. - go watchDisk() -} - // Init - initializes config system from config.json. func (sys *ConfigSys) Init(objAPI ObjectLayer) error { if objAPI == nil { diff --git a/cmd/config/notify/parse.go b/cmd/config/notify/parse.go index 93f603d49..28f9883ad 100644 --- a/cmd/config/notify/parse.go +++ b/cmd/config/notify/parse.go @@ -60,8 +60,7 @@ func TestNotificationTargets(cfg config.Config, doneCh <-chan struct{}, transpor // GetNotificationTargets registers and initializes all notification // targets, returns error if any. -func GetNotificationTargets(cfg config.Config, doneCh <-chan struct{}, transport *http.Transport) (*event.TargetList, error) { - test := false +func GetNotificationTargets(cfg config.Config, doneCh <-chan struct{}, transport *http.Transport, test bool) (*event.TargetList, error) { returnOnTargetError := false return RegisterNotificationTargets(cfg, doneCh, transport, nil, test, returnOnTargetError) } @@ -72,7 +71,6 @@ func GetNotificationTargets(cfg config.Config, doneCh <-chan struct{}, transport // * Add newly added target configuration to serverConfig.Notify.. // * Handle the configuration in this function to create/add into TargetList. func RegisterNotificationTargets(cfg config.Config, doneCh <-chan struct{}, transport *http.Transport, targetIDs []event.TargetID, test bool, returnOnTargetError bool) (*event.TargetList, error) { - targetList, err := FetchRegisteredTargets(cfg, doneCh, transport, test, returnOnTargetError) if err != nil { return targetList, err diff --git a/cmd/gateway-main.go b/cmd/gateway-main.go index 1eb9fb2c2..fe778f6c6 100644 --- a/cmd/gateway-main.go +++ b/cmd/gateway-main.go @@ -152,7 +152,7 @@ func StartGateway(ctx *cli.Context, gw Gateway) { // Set when gateway is enabled globalIsGateway = true - enableConfigOps := gatewayName == "nas" + enableConfigOps := false // TODO: We need to move this code with globalConfigSys.Init() // for now keep it here such that "s3" gateway layer initializes @@ -242,29 +242,15 @@ func StartGateway(ctx *cli.Context, gw Gateway) { globalObjectAPI = newObject globalObjLayerMutex.Unlock() - // Migrate all backend configs to encrypted backend, also handles rotation as well. - // For "nas" gateway we need to specially handle the backend migration as well. - // Internally code handles migrating etcd if enabled automatically. - logger.FatalIf(handleEncryptedConfigBackend(newObject, enableConfigOps), - "Unable to handle encrypted backend for config, iam and policies") - // Calls all New() for all sub-systems. newAllSubsystems() - // **** WARNING **** - // Migrating to encrypted backend should happen before initialization of any - // sub-systems, make sure that we do not move the above codeblock elsewhere. - if enableConfigOps { - logger.FatalIf(globalConfigSys.Init(newObject), "Unable to initialize config system") + if gatewayName == "nas" { buckets, err := newObject.ListBuckets(GlobalContext) if err != nil { logger.Fatal(err, "Unable to list buckets") } - logger.FatalIf(globalNotificationSys.Init(buckets, newObject), "Unable to initialize notification system") - // Start watching disk for reloading config, this - // is only enabled for "NAS" gateway. - globalConfigSys.WatchConfigNASDisk(GlobalContext, newObject) } if globalEtcdClient != nil { diff --git a/cmd/server-main.go b/cmd/server-main.go index 21eb9af8a..172a27bb1 100644 --- a/cmd/server-main.go +++ b/cmd/server-main.go @@ -235,7 +235,7 @@ func initSafeMode(ctx context.Context, newObject ObjectLayer) (err error) { // Migrate all backend configs to encrypted backend configs, optionally // handles rotating keys for encryption, if there is any retriable failure // that shall be retried if there is an error. - if err = handleEncryptedConfigBackend(newObject, true); err == nil { + if err = handleEncryptedConfigBackend(newObject); err == nil { // Upon success migrating the config, initialize all sub-systems // if all sub-systems initialized successfully return right away if err = initAllSubsystems(retryCtx, newObject); err == nil { diff --git a/docs/gateway/nas.md b/docs/gateway/nas.md index 4a6ecf061..cfb5d5b86 100644 --- a/docs/gateway/nas.md +++ b/docs/gateway/nas.md @@ -49,6 +49,36 @@ mc ls mynas [2017-02-26 22:10:11 PST] 0B test-bucket1/ ``` +## Breaking changes + +There will be a breaking change after the release version 'RELEASE.2020-06-22T03-12-50Z'. + +### The file-based config settings are deprecated in NAS + +The support for admin config APIs will be removed. This will include getters and setters like `mc admin config get` and `mc admin config` and any other `mc admin config` options. The reason for this change is to avoid un-necessary reloads of the config from the disk. And to comply with the Environment variable based settings like other gateways. + +### Migration guide + +The users who have been using the older config approach should migrate to ENV settings by setting environment variables accordingly. + +For example, + +Consider the following webhook target config. + +``` +notify_webhook:1 endpoint=http://localhost:8080/ auth_token= queue_limit=0 queue_dir=/tmp/webhk client_cert= client_key= +``` + +The corresponding environment variable setting can be + +``` +export MINIO_NOTIFY_WEBHOOK_ENABLE_1=on +export MINIO_NOTIFY_WEBHOOK_ENDPOINT_1=http://localhost:8080/ +export MINIO_NOTIFY_WEBHOOK_QUEUE_DIR_1=/tmp/webhk +``` + +> NOTE: Please check the docs for the corresponding ENV setting. Alternatively, We can obtain other ENVs in the form `mc admin config set alias/ --env` + ## Explore Further - [`mc` command-line interface](https://docs.min.io/docs/minio-client-quickstart-guide) - [`aws` command-line interface](https://docs.min.io/docs/aws-cli-with-minio)