Fail fast for invalid RESTIC_PACK_SIZE env values (#5592)

Co-authored-by: Michael Eischer <michael.eischer@fau.de>

changelog/unreleased/pull-5592

@@ -0,0 +1,7 @@
+Bugfix: Return error if `RESTIC_PACK_SIZE` contains invalid value
+
+If the environment variable `RESTIC_PACK_SIZE` could not be parsed, then
+restic ignored its value. Now, the restic commands fail with an error, unless
+the command-line option `--pack-size` was specified.
+
+https://github.com/restic/restic/pull/5592

cmd/restic/main.go

@@ -49,6 +49,10 @@ The full documentation can be found at https://restic.readthedocs.io/ .
 		DisableAutoGenTag: true,
 
 		PersistentPreRunE: func(c *cobra.Command, _ []string) error {
+			switch c.Name() {
+			case "__complete", "__completeNoDesc":
+				return nil
+			}
 			return globalOptions.PreRun(needsPassword(c.Name()))
 		},
 	}
@@ -114,7 +118,7 @@ The full documentation can be found at https://restic.readthedocs.io/ .
 // user for authentication).
 func needsPassword(cmd string) bool {
 	switch cmd {
-	case "cache", "generate", "help", "options", "self-update", "version", "__complete":
+	case "cache", "generate", "help", "options", "self-update", "version", "__complete", "__completeNoDesc":
 		return false
 	default:
 		return true

internal/global/global.go

@@ -81,6 +81,9 @@ type Options struct {
 	Options []string
 
 	Extended options.Options
+
+	// packSizeFlag is used to detect if --pack-size was set (CLI overrides env).
+	packSizeFlag *pflag.Flag
 }
 
 func (opts *Options) AddFlags(f *pflag.FlagSet) {
@@ -106,7 +109,8 @@ func (opts *Options) AddFlags(f *pflag.FlagSet) {
 	f.BoolVar(&opts.NoExtraVerify, "no-extra-verify", false, "skip additional verification of data before upload (see documentation)")
 	f.IntVar(&opts.Limits.UploadKb, "limit-upload", 0, "limits uploads to a maximum `rate` in KiB/s. (default: unlimited)")
 	f.IntVar(&opts.Limits.DownloadKb, "limit-download", 0, "limits downloads to a maximum `rate` in KiB/s. (default: unlimited)")
-	f.UintVar(&opts.PackSize, "pack-size", 0, "set target pack `size` in MiB, created pack files may be larger (default: $RESTIC_PACK_SIZE)")
+	const packSizeFlag = "pack-size"
+	f.UintVar(&opts.PackSize, packSizeFlag, 0, "set target pack `size` in MiB, created pack files may be larger (default: $RESTIC_PACK_SIZE)")
 	f.StringSliceVarP(&opts.Options, "option", "o", []string{}, "set extended option (`key=value`, can be specified multiple times)")
 	f.StringVar(&opts.HTTPUserAgent, "http-user-agent", "", "set a http user agent for outgoing http requests")
 	f.DurationVar(&opts.StuckRequestTimeout, "stuck-request-timeout", 5*time.Minute, "`duration` after which to retry stuck requests")
@@ -125,9 +129,7 @@ func (opts *Options) AddFlags(f *pflag.FlagSet) {
 		// ignore error as there's no good way to handle it
 		_ = opts.Compression.Set(comp)
 	}
-	// parse target pack size from env, on error the default value will be used
-	targetPackSize, _ := strconv.ParseUint(os.Getenv("RESTIC_PACK_SIZE"), 10, 32)
-	opts.PackSize = uint(targetPackSize)
+	opts.packSizeFlag = f.Lookup(packSizeFlag)
 
 	if os.Getenv("RESTIC_HTTP_USER_AGENT") != "" {
 		opts.HTTPUserAgent = os.Getenv("RESTIC_HTTP_USER_AGENT")
@@ -135,6 +137,15 @@ func (opts *Options) AddFlags(f *pflag.FlagSet) {
 }
 
 func (opts *Options) PreRun(needsPassword bool) error {
+	if envVal := os.Getenv("RESTIC_PACK_SIZE"); envVal != "" && !opts.packSizeFlag.Changed {
+		targetPackSize, err := strconv.ParseUint(envVal, 10, 32)
+		if err != nil {
+			// Failing fast here keeps backups from running for a long time with the wrong pack size.
+			return errors.Fatalf("invalid value for RESTIC_PACK_SIZE %q: %v", envVal, err)
+		}
+		opts.PackSize = uint(targetPackSize)
+	}
+
 	// set verbosity, default is one
 	opts.Verbosity = 1
 	if opts.Quiet && opts.Verbose > 0 {

internal/global/global_test.go

@@ -7,6 +7,9 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/spf13/pflag"
+
+	"github.com/restic/restic/internal/errors"
 	rtest "github.com/restic/restic/internal/test"
 )
 
@@ -49,3 +52,41 @@ func TestReadEmptyPassword(t *testing.T) {
 	_, err = readPassword(context.TODO(), opts, "test")
 	rtest.Assert(t, strings.Contains(err.Error(), "must not be specified together with providing a password via a cli option or environment variable"), "unexpected error message, got %v", err)
 }
+
+func TestPackSizeEnvParseError(t *testing.T) {
+	t.Setenv("RESTIC_PACK_SIZE", "64MiB")
+
+	var gopts Options
+	gopts.AddFlags(pflag.NewFlagSet("test", pflag.ContinueOnError))
+
+	err := gopts.PreRun(false)
+	rtest.Assert(t, err != nil, "expected error for invalid pack size env")
+	rtest.Assert(t, errors.IsFatal(err), "expected fatal error for invalid pack size env, got %T", err)
+	rtest.Assert(t, strings.Contains(err.Error(), "RESTIC_PACK_SIZE"), "error should mention RESTIC_PACK_SIZE, got %v", err)
+}
+
+func TestPackSizeEnvApplied(t *testing.T) {
+	t.Setenv("RESTIC_PACK_SIZE", "64")
+
+	var gopts Options
+	gopts.AddFlags(pflag.NewFlagSet("test", pflag.ContinueOnError))
+
+	err := gopts.PreRun(false)
+	rtest.OK(t, err)
+	rtest.Equals(t, uint(64), gopts.PackSize)
+}
+
+func TestPackSizeEnvIgnoredWhenFlagSet(t *testing.T) {
+	t.Setenv("RESTIC_PACK_SIZE", "64MiB")
+
+	var gopts Options
+	fs := pflag.NewFlagSet("test", pflag.ContinueOnError)
+	gopts.AddFlags(fs)
+
+	err := fs.Set("pack-size", "64")
+	rtest.OK(t, err)
+
+	err = gopts.PreRun(false)
+	rtest.OK(t, err)
+	rtest.Equals(t, uint(64), gopts.PackSize)
+}