diff --git a/README.md b/README.md index fe0a9e7..b731f16 100644 --- a/README.md +++ b/README.md @@ -240,8 +240,6 @@ For most use cases, it is sufficient to use the tool's default configuration not Available configuration values: -- `http-timeout` *DEPRECATED, see `store-options..timeout`* - HTTP request timeout used in HTTP stores (not S3) in nanoseconds -- `http-error-retry` *DEPRECATED, see `store-options..error-retry` - Number of times to retry failed chunk requests from HTTP stores - `s3-credentials` - Defines credentials for use with S3 stores. Especially useful if more than one S3 store is used. The key in the config needs to be the URL scheme and host used for the store, excluding the path, but including the port number if used in the store URL. The key can also contain glob patterns, and the available wildcards are `*`, `?` and `[…]`. Please refer to the [filepath.Match](https://pkg.go.dev/path/filepath#Match) documentation for additional information. It is also possible to use a [standard aws credentials file](https://docs.aws.amazon.com/cli/latest/userguide/cli-config-files.html) in order to store s3 credentials. - `store-options` - Allows customization of chunk and index stores, for example compression settings, timeouts, retry behavior and keys. Not all options are applicable to every store, some of these like `timeout` are ignored for local stores. Some of these options, such as the client certificates are overwritten with any values set in the command line. Note that the store location used in the command line needs to match the key under `store-options` exactly for these options to be used. As for the `s3-credentials`, glob patterns are also supported. A configuration file where more than one key matches a single store location, is considered invalid. - `timeout` - Time limit for chunk read or write operation in nanoseconds. Default: 1 minute. If set to a negative value, timeout is infinite. diff --git a/cmd/desync/config.go b/cmd/desync/config.go index 10b894d..448f137 100644 --- a/cmd/desync/config.go +++ b/cmd/desync/config.go @@ -32,10 +32,8 @@ type S3Creds struct { // Config is used to hold the global tool configuration. It's used to customize // store features and provide credentials where needed. type Config struct { - HTTPTimeout time.Duration `json:"http-timeout,omitempty"` - HTTPErrorRetry int `json:"http-error-retry,omitempty"` - S3Credentials map[string]S3Creds `json:"s3-credentials"` - StoreOptions map[string]desync.StoreOptions `json:"store-options"` + S3Credentials map[string]S3Creds `json:"s3-credentials"` + StoreOptions map[string]desync.StoreOptions `json:"store-options"` } // GetS3CredentialsFor attempts to find creds and region for an S3 location in the @@ -80,7 +78,7 @@ func (c Config) GetS3CredentialsFor(u *url.URL) (*credentials.Credentials, strin // config file. func (c Config) GetStoreOptionsFor(location string) (options desync.StoreOptions, err error) { found := false - options = desync.StoreOptions{} + options = desync.NewStoreOptionsWithDefaults() for k, v := range c.StoreOptions { if locationMatch(k, location) { if found { diff --git a/cmd/desync/info.go b/cmd/desync/info.go index e5806fc..33f2662 100644 --- a/cmd/desync/info.go +++ b/cmd/desync/info.go @@ -122,7 +122,7 @@ func runInfo(ctx context.Context, opt infoOptions, args []string) error { results.Unique = len(deduped) if len(opt.stores) > 0 { - store, err := multiStoreWithRouter(cmdStoreOptions{n: opt.n}, opt.stores...) + store, err := multiStoreWithRouter(opt.cmdStoreOptions, opt.stores...) if err != nil { return err } diff --git a/cmd/desync/options.go b/cmd/desync/options.go index 70981b2..b1f39c8 100644 --- a/cmd/desync/options.go +++ b/cmd/desync/options.go @@ -2,6 +2,7 @@ package main import ( "errors" + "time" "github.com/folbricht/desync" "github.com/spf13/pflag" @@ -10,38 +11,44 @@ import ( // cmdStoreOptions are used to pass additional options to store initialization from the // commandline. These generally override settings from the config file. type cmdStoreOptions struct { - n int - clientCert string - clientKey string - caCert string - skipVerify bool - trustInsecure bool - cacheRepair bool - errorRetry int + n int + clientCert string + clientKey string + caCert string + skipVerify bool + trustInsecure bool + cacheRepair bool + errorRetry int + errorRetryBaseInterval time.Duration + pflag.FlagSet } -// MergeWith takes store options as read from the config, and applies command-line +// MergedWith takes store options as read from the config, and applies command-line // provided options on top of them and returns the merged result. func (o cmdStoreOptions) MergedWith(opt desync.StoreOptions) desync.StoreOptions { opt.N = o.n - if o.clientCert != "" { + + if o.FlagSet.Lookup("client-cert").Changed { opt.ClientCert = o.clientCert } - if o.clientKey != "" { + if o.FlagSet.Lookup("client-key").Changed { opt.ClientKey = o.clientKey } - if o.caCert != "" { + if o.FlagSet.Lookup("ca-cert").Changed { opt.CACert = o.caCert } if o.skipVerify { opt.SkipVerify = true } - if o.trustInsecure { + if o.FlagSet.Lookup("trust-insecure").Changed { opt.TrustInsecure = true } - if o.errorRetry > 0 { + if o.FlagSet.Lookup("error-retry").Changed { opt.ErrorRetry = o.errorRetry } + if o.FlagSet.Lookup("error-retry-base-interval").Changed { + opt.ErrorRetryBaseInterval = o.errorRetryBaseInterval + } return opt } @@ -61,7 +68,10 @@ func addStoreOptions(o *cmdStoreOptions, f *pflag.FlagSet) { f.StringVar(&o.caCert, "ca-cert", "", "trust authorities in this file, instead of OS trust store") f.BoolVarP(&o.trustInsecure, "trust-insecure", "t", false, "trust invalid certificates") f.BoolVarP(&o.cacheRepair, "cache-repair", "r", true, "replace invalid chunks in the cache from source") - f.IntVarP(&o.errorRetry, "error-retry", "e", 3, "number of times to retry in case of network error") + f.IntVarP(&o.errorRetry, "error-retry", "e", desync.DefaultErrorRetry, "number of times to retry in case of network error") + f.DurationVarP(&o.errorRetryBaseInterval, "error-retry-base-interval", "b", desync.DefaultErrorRetryBaseInterval, "initial retry delay, increases linearly with each subsequent attempt") + + o.FlagSet = *f } // cmdServerOptions hold command line options used in HTTP servers. diff --git a/cmd/desync/options_test.go b/cmd/desync/options_test.go new file mode 100644 index 0000000..f1c90bd --- /dev/null +++ b/cmd/desync/options_test.go @@ -0,0 +1,170 @@ +package main + +import ( + "github.com/spf13/cobra" + "github.com/stretchr/testify/require" + "os" + "testing" + "time" +) + +const defaultErrorRetry = 3 +const DefaultErrorRetryBaseInterval = 500 * time.Millisecond + +func newTestOptionsCommand(opt *cmdStoreOptions) *cobra.Command { + cmd := &cobra.Command{} + + addStoreOptions(opt, cmd.Flags()) + return cmd +} + +func TestErrorRetryOptions(t *testing.T) { + for _, test := range []struct { + name string + args []string + cfgFileContent []byte + errorRetryStoreHit int + errorRetryStoreMiss int + baseIntervalStoreHit time.Duration + baseIntervalStoreMiss time.Duration + }{ + {"Config with the error retry and base interval set", + []string{""}, + []byte(`{"store-options": {"/store/*/":{"error-retry": 20, "error-retry-base-interval": 250000000}}}`), + 20, defaultErrorRetry, 250000000, DefaultErrorRetryBaseInterval, + }, + {"Error retry and base interval via command line args", + []string{"--error-retry", "10", "--error-retry-base-interval", "1s"}, + []byte(`{"store-options": {"/store/*/":{"error-retry": 20, "error-retry-base-interval": 250000000}}}`), + 10, 10, 1000000000, 1000000000, + }, + {"Config without error retry nor base interval", + []string{""}, + []byte(`{"store-options": {"/store/*/":{"uncompressed": true}}}`), + defaultErrorRetry, defaultErrorRetry, DefaultErrorRetryBaseInterval, DefaultErrorRetryBaseInterval, + }, + {"Config with default error retry and base interval", + []string{""}, + []byte(`{"store-options": {"/store/*/":{"error-retry": 3, "error-retry-base-interval": 500000000}}}`), + defaultErrorRetry, defaultErrorRetry, DefaultErrorRetryBaseInterval, DefaultErrorRetryBaseInterval, + }, + {"Config that disables error retry and base interval", + []string{""}, + []byte(`{"store-options": {"/store/*/":{"error-retry": 0, "error-retry-base-interval": 0}}}`), + 0, defaultErrorRetry, 0, DefaultErrorRetryBaseInterval, + }, + {"Disables error retry and base interval via command line args", + []string{"--error-retry", "0", "--error-retry-base-interval", "0"}, + []byte(`{"store-options": {"/store/*/":{"error-retry": 20, "error-retry-base-interval": 250000000}}}`), + 0, 0, 0, 0, + }, + {"Force the default values via command line args", + []string{"--error-retry", "3", "--error-retry-base-interval", "500ms"}, + []byte(`{"store-options": {"/store/*/":{"error-retry": 20, "error-retry-base-interval": 750000000}}}`), + defaultErrorRetry, defaultErrorRetry, DefaultErrorRetryBaseInterval, DefaultErrorRetryBaseInterval, + }, + } { + t.Run(test.name, func(t *testing.T) { + f, err := os.CreateTemp("", "desync-options") + require.NoError(t, err) + defer os.Remove(f.Name()) + _, err = f.Write(test.cfgFileContent) + require.NoError(t, err) + + // Set the global config file name + cfgFile = f.Name() + + initConfig() + + var cmdOpt cmdStoreOptions + + cmd := newTestOptionsCommand(&cmdOpt) + cmd.SetArgs(test.args) + + // Execute the mock command, to load the options provided in the launch arguments + _, err = cmd.ExecuteC() + require.NoError(t, err) + + configOptions, err := cfg.GetStoreOptionsFor("/store/20230901") + opt := cmdOpt.MergedWith(configOptions) + require.Equal(t, test.errorRetryStoreHit, opt.ErrorRetry) + require.Equal(t, test.baseIntervalStoreHit, opt.ErrorRetryBaseInterval) + + configOptions, err = cfg.GetStoreOptionsFor("/missingStore") + opt = cmdOpt.MergedWith(configOptions) + require.NoError(t, err) + require.Equal(t, test.errorRetryStoreMiss, opt.ErrorRetry) + require.Equal(t, test.baseIntervalStoreMiss, opt.ErrorRetryBaseInterval) + }) + } +} + +func TestStringOptions(t *testing.T) { + for _, test := range []struct { + name string + args []string + cfgFileContent []byte + clientCertStoreHit string + clientCertStoreMiss string + clientKeyStoreHit string + clientKeyStoreMiss string + caCertStoreHit string + caCertStoreMiss string + }{ + {"Config with options set", + []string{""}, + []byte(`{"store-options": {"/store/*/":{"client-cert": "/foo", "client-key": "/bar", "ca-cert": "/baz"}}}`), + "/foo", "", "/bar", "", "/baz", "", + }, + {"Configs set via command line args", + []string{"--client-cert", "/aa/bb", "--client-key", "/another", "--ca-cert", "/ca"}, + []byte(`{"store-options": {"/store/*/":{"client-cert": "/foo", "client-key": "/bar", "ca-cert": "/baz"}}}`), + "/aa/bb", "/aa/bb", "/another", "/another", "/ca", "/ca", + }, + {"Config without any of those string options set", + []string{""}, + []byte(`{"store-options": {"/store/*/":{"uncompressed": true}}}`), + "", "", "", "", "", "", + }, + {"Disable values from CLI args", + []string{"--client-cert", "", "--client-key", "", "--ca-cert", ""}, + []byte(`{"store-options": {"/store/*/":{"client-cert": "/foo", "client-key": "/bar", "ca-cert": "/baz"}}}`), + "", "", "", "", "", "", + }, + } { + t.Run(test.name, func(t *testing.T) { + f, err := os.CreateTemp("", "desync-options") + require.NoError(t, err) + defer os.Remove(f.Name()) + _, err = f.Write(test.cfgFileContent) + require.NoError(t, err) + + // Set the global config file name + cfgFile = f.Name() + + initConfig() + + var cmdOpt cmdStoreOptions + + cmd := newTestOptionsCommand(&cmdOpt) + cmd.SetArgs(test.args) + + // Execute the mock command, to load the options provided in the launch arguments + _, err = cmd.ExecuteC() + require.NoError(t, err) + + configOptions, err := cfg.GetStoreOptionsFor("/store/20230901") + opt := cmdOpt.MergedWith(configOptions) + require.Equal(t, test.clientCertStoreHit, opt.ClientCert) + require.Equal(t, test.clientKeyStoreHit, opt.ClientKey) + require.Equal(t, test.caCertStoreHit, opt.CACert) + + configOptions, err = cfg.GetStoreOptionsFor("/missingStore") + opt = cmdOpt.MergedWith(configOptions) + require.NoError(t, err) + require.Equal(t, test.clientCertStoreMiss, opt.ClientCert) + require.Equal(t, test.clientKeyStoreMiss, opt.ClientKey) + require.Equal(t, test.caCertStoreMiss, opt.CACert) + }) + } +} diff --git a/cmd/desync/store.go b/cmd/desync/store.go index ee098b7..8190a90 100644 --- a/cmd/desync/store.go +++ b/cmd/desync/store.go @@ -123,16 +123,6 @@ func storeFromLocation(location string, cmdOpt cmdStoreOptions) (desync.Store, e return nil, err } case "http", "https": - // This is for backwards compatibility only, to support http-timeout and - // http-error-retry in the config file for a bit longer. If those are - // set, and the options for the store aren't, then use the old values. - // TODO: Remove this code and drop these config options in the future. - if opt.Timeout == 0 && cfg.HTTPTimeout > 0 { - opt.Timeout = cfg.HTTPTimeout - } - if opt.ErrorRetry == 0 && cfg.HTTPErrorRetry > 0 { - opt.ErrorRetry = cfg.HTTPErrorRetry - } s, err = desync.NewRemoteHTTPStore(loc, opt) if err != nil { return nil, err @@ -252,16 +242,6 @@ func indexStoreFromLocation(location string, cmdOpt cmdStoreOptions) (desync.Ind return nil, "", err } case "http", "https": - // This is for backwards compatibility only, to support http-timeout and - // http-error-retry in the config file for a bit longer. If those are - // set, and the options for the store aren't, then use the old values. - // TODO: Remove this code and drop these config options in the future. - if opt.Timeout == 0 && cfg.HTTPTimeout > 0 { - opt.Timeout = cfg.HTTPTimeout - } - if opt.ErrorRetry == 0 && cfg.HTTPErrorRetry > 0 { - opt.ErrorRetry = cfg.HTTPErrorRetry - } s, err = desync.NewRemoteHTTPIndexStore(&p, opt) if err != nil { return nil, "", err diff --git a/store.go b/store.go index fc2496b..d4d985d 100644 --- a/store.go +++ b/store.go @@ -2,11 +2,15 @@ package desync import ( "context" + "encoding/json" "fmt" "io" "time" ) +const DefaultErrorRetry = 3 +const DefaultErrorRetryBaseInterval = 500 * time.Millisecond + // Store is a generic interface implemented by read-only stores, like SSH or // HTTP remote stores currently. type Store interface { @@ -71,12 +75,11 @@ type StoreOptions struct { Timeout time.Duration `json:"timeout,omitempty"` // Number of times object retrieval should be attempted on error. Useful when dealing - // with unreliable connections. Default: 0 + // with unreliable connections. ErrorRetry int `json:"error-retry,omitempty"` // Number of nanoseconds to wait before first retry attempt. // Retry attempt number N for the same request will wait N times this interval. - // Default: 0 nanoseconds ErrorRetryBaseInterval time.Duration `json:"error-retry-base-interval,omitempty"` // If SkipVerify is true, this store will not verify the data it reads and serves up. This is @@ -91,12 +94,27 @@ type StoreOptions struct { Uncompressed bool `json:"uncompressed"` } +// NewStoreOptionsWithDefaults creates a new StoreOptions struct with the default values set +func NewStoreOptionsWithDefaults() (o StoreOptions) { + o.ErrorRetry = DefaultErrorRetry + o.ErrorRetryBaseInterval = DefaultErrorRetryBaseInterval + return o +} + +func (o *StoreOptions) UnmarshalJSON(data []byte) error { + // Set all the default values before loading the JSON store options + o.ErrorRetry = DefaultErrorRetry + o.ErrorRetryBaseInterval = DefaultErrorRetryBaseInterval + type Alias StoreOptions + return json.Unmarshal(data, (*Alias)(o)) +} + // Returns data converters that convert between plain and storage-format. Each layer // represents a modification such as compression or encryption and is applied in order // depending the direction of data. If data is written to storage, the layer's toStorage // method is called in the order they are returned. If data is read, the fromStorage // method is called in reverse order. -func (o StoreOptions) converters() []converter { +func (o *StoreOptions) converters() []converter { var m []converter if !o.Uncompressed { m = append(m, Compressor{})