Skip to content

Commit

Permalink
feat: Add upgrade proposal plan validation to CLI (#10379)
Browse files Browse the repository at this point in the history
<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

Closes: #10286

When submitting a software upgrade proposal (e.g. `$DAEMON tx gov submit-proposal software-upgrade`)
* Validate the plan info by default.
* Add flag `--no-validate` to allow skipping that validation.
* Add flag `--daemon-name` to designate the executable name (needed for validation).
* The daemon name comes first from the `--daemon-name` flag. If that's not provided, it looks for a `DAEMON_NAME` environment variable (to match what's used by Cosmovisor). If that's not set, the name of the currently running executable is used.

Things that are validated:
* The plan info cannot be empty or blank.
* If the plan info is a url:
  * It must have a `checksum` query parameter.
  * It must return properly formatted plan info JSON.
  * The `checksum` is correct.
* If the plan info is not a url:
  * It must be propery formatted plan info JSON.
* There is at least one entry in the `binaries` field.
* The keys of the `binaries` field are either "any" or in the format of "os/arch".
* All URLs contain a `checksum` query parameter.
* Each URL contains a usable response.
* The `checksum` is correct for each URL.

Note: With this change, either a valid `--upgrade-info` will need to be provided, or else `--no-validate` must be provided. If no `--upgrade-info` is given, a validation error is returned.

---

### 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
- [ ] ~~added `!` to the type prefix if API or client breaking change~~ _N/A_
- [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)
- [ ] updated the relevant documentation or specification
- [ ] 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)
  • Loading branch information
dwedul-figure authored Nov 12, 2021
1 parent 363b51e commit 181ba0e
Show file tree
Hide file tree
Showing 8 changed files with 970 additions and 1 deletion.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [\#9837](https://github.com/cosmos/cosmos-sdk/issues/9837) `--generate-only` flag will accept the keyname now.
* [\#10326](https://github.com/cosmos/cosmos-sdk/pull/10326) `x/authz` add query all grants by granter query.
* [\#10348](https://github.com/cosmos/cosmos-sdk/pull/10348) Add `fee.{payer,granter}` and `tip` fields to StdSignDoc for signing tipped transactions.
* [\#10379](https://github.com/cosmos/cosmos-sdk/pull/10379) Add validation to `x/upgrade` CLI `software-upgrade` command `--plan-info` value.

### Improvements

Expand Down
18 changes: 18 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ require (
github.com/gorilla/mux v1.8.0
github.com/grpc-ecosystem/go-grpc-middleware v1.3.0
github.com/grpc-ecosystem/grpc-gateway v1.16.0
github.com/hashicorp/go-cleanhttp v0.5.1 // indirect
github.com/hashicorp/go-getter v1.4.1
github.com/hashicorp/golang-lru v0.5.5-0.20210104140557-80c98217689d
github.com/hdevalence/ed25519consensus v0.0.0-20210204194344-59a8610d2b87
github.com/improbable-eng/grpc-web v0.15.0
Expand Down Expand Up @@ -56,10 +58,14 @@ require (
)

require (
cloud.google.com/go v0.93.3 // indirect
cloud.google.com/go/storage v1.10.0 // indirect
filippo.io/edwards25519 v1.0.0-beta.2 // indirect
github.com/ChainSafe/go-schnorrkel v0.0.0-20200405005733-88cbf1b4c40d // indirect
github.com/Workiva/go-datastructures v1.0.52 // indirect
github.com/aws/aws-sdk-go v1.27.0 // indirect
github.com/beorn7/perks v1.0.1 // indirect
github.com/bgentry/go-netrc v0.0.0-20140422174119-9fd32a8b3d3d // indirect
github.com/cenkalti/backoff/v4 v4.1.1 // indirect
github.com/cespare/xxhash v1.1.0 // indirect
github.com/cespare/xxhash/v2 v2.1.1 // indirect
Expand All @@ -78,16 +84,21 @@ require (
github.com/go-logfmt/logfmt v0.5.0 // indirect
github.com/godbus/dbus v0.0.0-20190726142602-4481cbc300e2 // indirect
github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b // indirect
github.com/golang/groupcache v0.0.0-20200121045136-8c9f03a8e57e // indirect
github.com/golang/snappy v0.0.4 // indirect
github.com/google/btree v1.0.0 // indirect
github.com/google/orderedcode v0.0.1 // indirect
github.com/googleapis/gax-go/v2 v2.1.0 // indirect
github.com/gorilla/websocket v1.4.2 // indirect
github.com/gsterjov/go-libsecret v0.0.0-20161001094733-a6f4afe4910c // indirect
github.com/gtank/merlin v0.1.1 // indirect
github.com/gtank/ristretto255 v0.1.2 // indirect
github.com/hashicorp/go-immutable-radix v1.0.0 // indirect
github.com/hashicorp/go-safetemp v1.0.0 // indirect
github.com/hashicorp/go-version v1.2.0 // indirect
github.com/hashicorp/hcl v1.0.0 // indirect
github.com/inconshreveable/mousetrap v1.0.0 // indirect
github.com/jmespath/go-jmespath v0.4.0 // indirect
github.com/jmhodges/levigo v1.0.0 // indirect
github.com/keybase/go-keychain v0.0.0-20190712205309-48d3d31d256d // indirect
github.com/klauspost/compress v1.12.3 // indirect
Expand All @@ -96,6 +107,8 @@ require (
github.com/matttproud/golang_protobuf_extensions v1.0.1 // indirect
github.com/mimoo/StrobeGo v0.0.0-20181016162300-f8f6d4d2b643 // indirect
github.com/minio/highwayhash v1.0.1 // indirect
github.com/mitchellh/go-homedir v1.1.0 // indirect
github.com/mitchellh/go-testing-interface v1.0.0 // indirect
github.com/mitchellh/mapstructure v1.4.2 // indirect
github.com/mtibben/percent v0.2.1 // indirect
github.com/pelletier/go-toml v1.9.4 // indirect
Expand All @@ -111,12 +124,17 @@ require (
github.com/subosito/gotenv v1.2.0 // indirect
github.com/syndtr/goleveldb v1.0.1-0.20210819022825-2ae1ddf74ef7 // indirect
github.com/tecbot/gorocksdb v0.0.0-20191217155057-f0fad39f321c // indirect
github.com/ulikunitz/xz v0.5.5 // indirect
github.com/zondax/hid v0.9.0 // indirect
go.etcd.io/bbolt v1.3.5 // indirect
go.opencensus.io v0.23.0 // indirect
golang.org/x/net v0.0.0-20210903162142-ad29c8ab022f // indirect
golang.org/x/oauth2 v0.0.0-20210819190943-2bc19b11175f // indirect
golang.org/x/sys v0.0.0-20210903071746-97244b99971b // indirect
golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1 // indirect
golang.org/x/text v0.3.6 // indirect
google.golang.org/api v0.56.0 // indirect
google.golang.org/appengine v1.6.7 // indirect
gopkg.in/ini.v1 v1.63.2 // indirect
gopkg.in/yaml.v2 v2.4.0 // indirect
gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b // indirect
Expand Down
31 changes: 31 additions & 0 deletions go.sum

Large diffs are not rendered by default.

40 changes: 39 additions & 1 deletion x/upgrade/client/cli/tx.go
Original file line number Diff line number Diff line change
@@ -1,19 +1,25 @@
package cli

import (
"os"
"path/filepath"

"github.com/spf13/cobra"

"github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/client/tx"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/gov/client/cli"
gov "github.com/cosmos/cosmos-sdk/x/gov/types"
"github.com/cosmos/cosmos-sdk/x/upgrade/plan"
"github.com/cosmos/cosmos-sdk/x/upgrade/types"
)

const (
FlagUpgradeHeight = "upgrade-height"
FlagUpgradeInfo = "upgrade-info"
FlagNoValidate = "no-validate"
FlagDaemonName = "daemon-name"
)

// GetTxCmd returns the transaction commands for this module
Expand Down Expand Up @@ -45,6 +51,24 @@ func NewCmdSubmitUpgradeProposal() *cobra.Command {
if err != nil {
return err
}
noValidate, err := cmd.Flags().GetBool(FlagNoValidate)
if err != nil {
return err
}
if !noValidate {
prop := content.(*types.SoftwareUpgradeProposal)
var daemonName string
if daemonName, err = cmd.Flags().GetString(FlagDaemonName); err != nil {
return err
}
var planInfo *plan.Info
if planInfo, err = plan.ParseInfo(prop.Plan.Info); err != nil {
return err
}
if err = planInfo.ValidateFull(daemonName); err != nil {
return err
}
}

from := clientCtx.GetFromAddress()

Expand All @@ -70,7 +94,9 @@ func NewCmdSubmitUpgradeProposal() *cobra.Command {
cmd.Flags().String(cli.FlagDescription, "", "description of proposal")
cmd.Flags().String(cli.FlagDeposit, "", "deposit of proposal")
cmd.Flags().Int64(FlagUpgradeHeight, 0, "The height at which the upgrade must happen")
cmd.Flags().String(FlagUpgradeInfo, "", "Optional info for the planned upgrade such as commit hash, etc.")
cmd.Flags().String(FlagUpgradeInfo, "", "Info for the upgrade plan such as new version download urls, etc.")
cmd.Flags().Bool(FlagNoValidate, false, "Skip validation of the upgrade info")
cmd.Flags().String(FlagDaemonName, getDefaultDaemonName(), "The name of the executable being upgraded (for upgrade-info validation). Default is the DAEMON_NAME env var if set, or else this executable")

return cmd
}
Expand Down Expand Up @@ -154,3 +180,15 @@ func parseArgsToContent(cmd *cobra.Command, name string) (gov.Content, error) {
content := types.NewSoftwareUpgradeProposal(title, description, plan)
return content, nil
}

// getDefaultDaemonName gets the default name to use for the daemon.
// If a DAEMON_NAME env var is set, that is used.
// Otherwise, the last part of the currently running executable is used.
func getDefaultDaemonName() string {
// DAEMON_NAME is specifically used here to correspond with the Comsovisor setup env vars.
name := os.Getenv("DAEMON_NAME")
if len(name) == 0 {
_, name = filepath.Split(os.Args[0])
}
return name
}
141 changes: 141 additions & 0 deletions x/upgrade/plan/downloader.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
package plan

import (
"errors"
"fmt"
neturl "net/url"
"os"
"path/filepath"
"strings"

"github.com/hashicorp/go-getter"
)

// DownloadUpgrade downloads the given url into the provided directory and ensures it's valid.
// The provided url must contain a checksum parameter that matches the file being downloaded.
// If this returns nil, the download was successful, and {dstRoot}/bin/{daemonName} is a regular executable file.
// This is an opinionated directory structure that corresponds with Cosmovisor requirements.
// If the url is not an archive, it is downloaded and saved to {dstRoot}/bin/{daemonName}.
// If the url is an archive, it is downloaded and unpacked to {dstRoot}.
// If the archive does not contain a /bin/{daemonName} file, then this will attempt to move /{daemonName} to /bin/{daemonName}.
// If the archive does not contain either /bin/{daemonName} or /{daemonName}, an error is returned.
// Note: Because a checksum is required, this function cannot be used to download non-archive directories.
// If dstRoot already exists, some or all of its contents might be updated.
func DownloadUpgrade(dstRoot, url, daemonName string) error {
if err := ValidateIsURLWithChecksum(url); err != nil {
return err
}
target := filepath.Join(dstRoot, "bin", daemonName)
// First try to download it as a single file. If there's no error, it's okay and we're done.
if err := getter.GetFile(target, url); err != nil {
// If it was a checksum error, no need to try as directory.
if _, ok := err.(*getter.ChecksumError); ok {
return err
}
// File download didn't work, try it as an archive.
if err = downloadUpgradeAsArchive(dstRoot, url, daemonName); err != nil {
// Out of options, send back the error.
return err
}
}
return EnsureBinary(target)
}

// downloadUpgradeAsArchive tries to download the given url as an archive.
// The archive is unpacked and saved in dstDir.
// If the archive contains /{daemonName} and not /bin/{daemonName}, then /{daemonName} will be moved to /bin/{daemonName}.
// If this returns nil, the download was successful, and {dstDir}/bin/{daemonName} is a regular executable file.
func downloadUpgradeAsArchive(dstDir, url, daemonName string) error {
err := getter.Get(dstDir, url)
if err != nil {
return err
}

// If bin/{daemonName} exists, we're done.
dstDirBinFile := filepath.Join(dstDir, "bin", daemonName)
err = EnsureBinary(dstDirBinFile)
if err == nil {
return nil
}

// Otherwise, check for a root {daemonName} file and move it to the bin/ directory if found.
dstDirFile := filepath.Join(dstDir, daemonName)
err = EnsureBinary(dstDirFile)
if err == nil {
err = os.Rename(dstDirFile, dstDirBinFile)
if err != nil {
return fmt.Errorf("could not move %s to the bin directory: %w", daemonName, err)
}
return nil
}

return fmt.Errorf("url \"%s\" result does not contain a bin/%s or %s file", url, daemonName, daemonName)
}

// EnsureBinary checks that the given file exists as a regular file and is executable.
// An error is returned if:
// - The file does not exist.
// - The path exists, but is one of: Dir, Symlink, NamedPipe, Socket, Device, CharDevice, or Irregular.
// - The file exists, is not executable by all three of User, Group, and Other, and cannot be made executable.
func EnsureBinary(path string) error {
info, err := os.Stat(path)
if err != nil {
return err
}
if !info.Mode().IsRegular() {
_, f := filepath.Split(path)
return fmt.Errorf("%s is not a regular file", f)
}
// Make sure all executable bits are set.
oldMode := info.Mode().Perm()
newMode := oldMode | 0111 // Set the three execute bits to on (a+x).
if oldMode != newMode {
return os.Chmod(path, newMode)
}
return nil
}

// DownloadURLWithChecksum gets the contents of the given url, ensuring the checksum is correct.
// The provided url must contain a checksum parameter that matches the file being downloaded.
// If there isn't an error, the content returned by the url will be returned as a string.
// Returns an error if:
// - The url is not a URL or does not contain a checksum parameter.
// - Downloading the URL fails.
// - The checksum does not match what is returned by the URL.
// - The URL does not return a regular file.
// - The downloaded file is empty or only whitespace.
func DownloadURLWithChecksum(url string) (string, error) {
if err := ValidateIsURLWithChecksum(url); err != nil {
return "", err
}
tempDir, err := os.MkdirTemp("", "reference")
if err != nil {
return "", fmt.Errorf("could not create temp directory: %w", err)
}
defer os.RemoveAll(tempDir)
tempFile := filepath.Join(tempDir, "content")
if err = getter.GetFile(tempFile, url); err != nil {
return "", fmt.Errorf("could not download url \"%s\": %w", url, err)
}
tempFileBz, rerr := os.ReadFile(tempFile)
if rerr != nil {
return "", fmt.Errorf("could not read downloaded temporary file: %w", rerr)
}
tempFileStr := strings.TrimSpace(string(tempFileBz))
if len(tempFileStr) == 0 {
return "", fmt.Errorf("no content returned by \"%s\"", url)
}
return tempFileStr, nil
}

// ValidateIsURLWithChecksum checks that the given string is a url and contains a checksum query parameter.
func ValidateIsURLWithChecksum(urlStr string) error {
url, err := neturl.Parse(urlStr)
if err != nil {
return err
}
if len(url.Query().Get("checksum")) == 0 {
return errors.New("missing checksum query parameter")
}
return nil
}
Loading

0 comments on commit 181ba0e

Please sign in to comment.