From ccd95f3ed33f9613e2e6188201d4b6d760966b2a Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Mon, 30 Aug 2021 17:17:12 +0200 Subject: [PATCH] feat(cosmovisor): strict boolean argument parsing (#10018) ## Description Closes: #10017 --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [x] added `!` to the type prefix if API or client breaking change - [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [x] provided a link to the relevant issue or specification - [x] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [x] added a changelog entry to `CHANGELOG.md` - [x] included comments for [documenting Go code](https://blog.golang.org/godoc) - [x] updated the relevant documentation or specification - [x] reviewed "Files changed" and left comments if necessary - [x] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable) --- cosmovisor/CHANGELOG.md | 6 +++-- cosmovisor/args.go | 54 ++++++++++++++++++++++++++++++----------- cosmovisor/args_test.go | 41 +++++++++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 16 deletions(-) diff --git a/cosmovisor/CHANGELOG.md b/cosmovisor/CHANGELOG.md index 454c39fcf..97ae18eab 100644 --- a/cosmovisor/CHANGELOG.md +++ b/cosmovisor/CHANGELOG.md @@ -35,13 +35,15 @@ Ref: https://keepachangelog.com/en/1.0.0/ # Changelog - ## [Unreleased] -## Features +### Features + [\#8590](https://github.com/cosmos/cosmos-sdk/pull/8590) File watcher for cosmovisor. Instead of parsing logs from stdin and stderr, we watch the `/data/upgrade-info.json` file updates using polling mechanism. +### Improvements + ++ [\#10018](https://github.com/cosmos/cosmos-sdk/pull/10018) Strict boolean argument parsing: cosmovisor will fail if user will not set correctly a boolean variable. Correct values are: "true", "false", "" (not setting) - all case not sensitive. ## v0.1 2021-08-06 diff --git a/cosmovisor/args.go b/cosmovisor/args.go index e6dcf2c84..ebcb5994d 100644 --- a/cosmovisor/args.go +++ b/cosmovisor/args.go @@ -9,9 +9,20 @@ import ( "os" "path/filepath" "strconv" + "strings" "time" ) +// environment variable names +const ( + envHome = "DAEMON_HOME" + envName = "DAEMON_NAME" + envDownloadBin = "DAEMON_ALLOW_DOWNLOAD_BINARIES" + envRestartUpgrade = "DAEMON_RESTART_AFTER_UPGRADE" + envSkipBackup = "UNSAFE_SKIP_BACKUP" + envInterval = "DAEMON_POLL_INTERVAL" +) + const ( rootName = "cosmovisor" genesisDir = "genesis" @@ -106,19 +117,22 @@ func (cfg *Config) CurrentBin() (string, error) { // and then validate it is reasonable func GetConfigFromEnv() (*Config, error) { cfg := &Config{ - Home: os.Getenv("DAEMON_HOME"), - Name: os.Getenv("DAEMON_NAME"), + Home: os.Getenv(envHome), + Name: os.Getenv(envName), } - if os.Getenv("DAEMON_ALLOW_DOWNLOAD_BINARIES") == "true" { - cfg.AllowDownloadBinaries = true + var err error + if cfg.AllowDownloadBinaries, err = booleanOption(envDownloadBin, false); err != nil { + return nil, err + } + if cfg.RestartAfterUpgrade, err = booleanOption(envRestartUpgrade, false); err != nil { + return nil, err + } + if cfg.UnsafeSkipBackup, err = booleanOption(envSkipBackup, false); err != nil { + return nil, err } - if os.Getenv("DAEMON_RESTART_AFTER_UPGRADE") == "true" { - cfg.RestartAfterUpgrade = true - } - - interval := os.Getenv("DAEMON_POLL_INTERVAL") + interval := os.Getenv(envInterval) if interval != "" { i, err := strconv.ParseUint(interval, 10, 32) if err != nil { @@ -129,8 +143,6 @@ func GetConfigFromEnv() (*Config, error) { cfg.PollInterval = 300 * time.Millisecond } - cfg.UnsafeSkipBackup = os.Getenv("UNSAFE_SKIP_BACKUP") == "true" - if err := cfg.validate(); err != nil { return nil, err } @@ -142,15 +154,15 @@ func GetConfigFromEnv() (*Config, error) { // and that Name is set func (cfg *Config) validate() error { if cfg.Name == "" { - return errors.New("DAEMON_NAME is not set") + return errors.New(envName + " is not set") } if cfg.Home == "" { - return errors.New("DAEMON_HOME is not set") + return errors.New(envHome + " is not set") } if !filepath.IsAbs(cfg.Home) { - return errors.New("DAEMON_HOME must be an absolute path") + return errors.New(envHome + " must be an absolute path") } // ensure the root directory exists @@ -231,3 +243,17 @@ returnError: cfg.currentUpgrade.Name = "_" return cfg.currentUpgrade } + +// checks and validates env option +func booleanOption(name string, defaultVal bool) (bool, error) { + p := strings.ToLower(os.Getenv(name)) + switch p { + case "": + return defaultVal, nil + case "false": + return false, nil + case "true": + return true, nil + } + return false, fmt.Errorf("env variable %q must have a boolean value (\"true\" or \"false\"), got %q", name, p) +} diff --git a/cosmovisor/args_test.go b/cosmovisor/args_test.go index 9e0ce3bb0..4b0bd39d2 100644 --- a/cosmovisor/args_test.go +++ b/cosmovisor/args_test.go @@ -2,6 +2,7 @@ package cosmovisor import ( "fmt" + "os" "path/filepath" "testing" @@ -129,3 +130,43 @@ func (s *argsTestSuite) TestEnsureBin() { } } } + +func (s *argsTestSuite) TestBooleanOption() { + require := s.Require() + name := "COSMOVISOR_TEST_VAL" + + check := func(def, expected, isErr bool, msg string) { + v, err := booleanOption(name, def) + if isErr { + require.Error(err) + return + } + require.NoError(err) + require.Equal(expected, v, msg) + } + + os.Setenv(name, "") + check(true, true, false, "should correctly set default value") + check(false, false, false, "should correctly set default value") + + os.Setenv(name, "wrong") + check(true, true, true, "should error on wrong value") + os.Setenv(name, "truee") + check(true, true, true, "should error on wrong value") + + os.Setenv(name, "false") + check(true, false, false, "should handle false value") + check(false, false, false, "should handle false value") + os.Setenv(name, "faLSe") + check(true, false, false, "should handle false value case not sensitive") + check(false, false, false, "should handle false value case not sensitive") + + os.Setenv(name, "true") + check(true, true, false, "should handle true value") + check(false, true, false, "should handle true value") + + os.Setenv(name, "TRUE") + check(true, true, false, "should handle true value case not sensitive") + check(false, true, false, "should handle true value case not sensitive") + +}