Improve options handling and add --error-retry-base-interval

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.<Location>.timeout`* - HTTP request timeout used in HTTP stores (not S3) in nanoseconds
-- `http-error-retry` *DEPRECATED, see `store-options.<Location>.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.

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 {

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
 		}

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.

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)
+		})
+	}
+}

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

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{})