diff --git a/changelog/unreleased/issue-4759 b/changelog/unreleased/issue-4759 new file mode 100644 index 000000000..e4ef0f836 --- /dev/null +++ b/changelog/unreleased/issue-4759 @@ -0,0 +1,10 @@ +Bugfix: Return error if environment variables contain invalid values + +If the value of one of the environment variables `RESTIC_COMPRESSION`, `RESTIC_PACK_SIZE` +or `RESTIC_READ_CONCURRENCY` could not be parsed, then restic ignored their value. +Now, the restic commands fail with an error, unless they were overwritten with their +corresponding command-line option. + +https://github.com/restic/restic/issues/4759 +https://github.com/restic/restic/pull/5592 +https://github.com/restic/restic/pull/5700 diff --git a/changelog/unreleased/pull-5592 b/changelog/unreleased/pull-5592 deleted file mode 100644 index 87a1bfd8b..000000000 --- a/changelog/unreleased/pull-5592 +++ /dev/null @@ -1,7 +0,0 @@ -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 diff --git a/cmd/restic/cmd_backup.go b/cmd/restic/cmd_backup.go index f9b45fe51..e0f901738 100644 --- a/cmd/restic/cmd_backup.go +++ b/cmd/restic/cmd_backup.go @@ -52,15 +52,23 @@ Exit status is 10 if the repository does not exist. Exit status is 11 if the repository is already locked. Exit status is 12 if the password is incorrect. `, - PreRun: func(_ *cobra.Command, _ []string) { + PreRunE: func(_ *cobra.Command, _ []string) error { + if envVal := os.Getenv("RESTIC_READ_CONCURRENCY"); envVal != "" && !opts.readConcurrencyFlag.Changed { + n, err := strconv.ParseUint(envVal, 10, 32) + if err != nil { + return errors.Fatalf("invalid value for RESTIC_READ_CONCURRENCY %q: %v", envVal, err) + } + opts.ReadConcurrency = uint(n) + } if opts.Host == "" { hostname, err := os.Hostname() if err != nil { debug.Log("os.Hostname() returned err: %v", err) - return + return nil } opts.Host = hostname } + return nil }, GroupID: cmdGroupDefault, DisableAutoGenTag: true, @@ -102,6 +110,8 @@ type BackupOptions struct { ReadConcurrency uint NoScan bool SkipIfUnchanged bool + + readConcurrencyFlag *pflag.Flag } func (opts *BackupOptions) AddFlags(f *pflag.FlagSet) { @@ -145,9 +155,7 @@ func (opts *BackupOptions) AddFlags(f *pflag.FlagSet) { } f.BoolVar(&opts.SkipIfUnchanged, "skip-if-unchanged", false, "skip snapshot creation if identical to parent snapshot") - // parse read concurrency from env, on error the default value will be used - readConcurrency, _ := strconv.ParseUint(os.Getenv("RESTIC_READ_CONCURRENCY"), 10, 32) - opts.ReadConcurrency = uint(readConcurrency) + opts.readConcurrencyFlag = f.Lookup("read-concurrency") // parse host from env, if not exists or empty the default value will be used if host := os.Getenv("RESTIC_HOST"); host != "" { diff --git a/internal/global/global.go b/internal/global/global.go index 88439462a..6930bd1c7 100644 --- a/internal/global/global.go +++ b/internal/global/global.go @@ -82,8 +82,10 @@ type Options struct { Extended options.Options - // packSizeFlag is used to detect if --pack-size was set (CLI overrides env). - packSizeFlag *pflag.Flag + // packSizeFlag and compressionFlag detect if the corresponding CLI flag was set (CLI overrides env). + // Lookup cannot return nil as the flags are added to the same FlagSet just above. + packSizeFlag *pflag.Flag + compressionFlag *pflag.Flag } func (opts *Options) AddFlags(f *pflag.FlagSet) { @@ -105,7 +107,8 @@ func (opts *Options) AddFlags(f *pflag.FlagSet) { f.BoolVar(&opts.InsecureNoPassword, "insecure-no-password", false, "use an empty password for the repository, must be passed to every restic command (insecure)") f.BoolVar(&opts.InsecureTLS, "insecure-tls", false, "skip TLS certificate verification when connecting to the repository (insecure)") f.BoolVar(&opts.CleanupCache, "cleanup-cache", false, "auto remove old cache directories") - f.Var(&opts.Compression, "compression", "compression mode (only available for repository format version 2), one of (auto|off|fastest|better|max) (default: $RESTIC_COMPRESSION)") + const compressionFlag = "compression" + f.Var(&opts.Compression, compressionFlag, "compression mode (only available for repository format version 2), one of (auto|off|fastest|better|max) (default: $RESTIC_COMPRESSION)") 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)") @@ -124,12 +127,8 @@ func (opts *Options) AddFlags(f *pflag.FlagSet) { opts.RootCertFilenames = strings.Split(os.Getenv("RESTIC_CACERT"), ",") } opts.TLSClientCertKeyFilename = os.Getenv("RESTIC_TLS_CLIENT_CERT") - comp := os.Getenv("RESTIC_COMPRESSION") - if comp != "" { - // ignore error as there's no good way to handle it - _ = opts.Compression.Set(comp) - } opts.packSizeFlag = f.Lookup(packSizeFlag) + opts.compressionFlag = f.Lookup(compressionFlag) if os.Getenv("RESTIC_HTTP_USER_AGENT") != "" { opts.HTTPUserAgent = os.Getenv("RESTIC_HTTP_USER_AGENT") @@ -145,6 +144,11 @@ func (opts *Options) PreRun(needsPassword bool) error { } opts.PackSize = uint(targetPackSize) } + if envVal := os.Getenv("RESTIC_COMPRESSION"); envVal != "" && !opts.compressionFlag.Changed { + if err := opts.Compression.Set(envVal); err != nil { + return errors.Fatalf("invalid value for RESTIC_COMPRESSION %q: %v", envVal, err) + } + } // set verbosity, default is one opts.Verbosity = 1 diff --git a/internal/global/global_test.go b/internal/global/global_test.go index 474d9d0e1..90ec972c3 100644 --- a/internal/global/global_test.go +++ b/internal/global/global_test.go @@ -90,3 +90,41 @@ func TestPackSizeEnvIgnoredWhenFlagSet(t *testing.T) { rtest.OK(t, err) rtest.Equals(t, uint(64), gopts.PackSize) } + +func TestCompressionEnvParseError(t *testing.T) { + t.Setenv("RESTIC_COMPRESSION", "invalid") + + var gopts Options + gopts.AddFlags(pflag.NewFlagSet("test", pflag.ContinueOnError)) + + err := gopts.PreRun(false) + rtest.Assert(t, err != nil, "expected error for invalid compression env") + rtest.Assert(t, errors.IsFatal(err), "expected fatal error for invalid compression env, got %T", err) + rtest.Assert(t, strings.Contains(err.Error(), "RESTIC_COMPRESSION"), "error should mention RESTIC_COMPRESSION, got %v", err) +} + +func TestCompressionEnvApplied(t *testing.T) { + t.Setenv("RESTIC_COMPRESSION", "max") + + var gopts Options + gopts.AddFlags(pflag.NewFlagSet("test", pflag.ContinueOnError)) + + err := gopts.PreRun(false) + rtest.OK(t, err) + rtest.Equals(t, "max", gopts.Compression.String()) +} + +func TestCompressionEnvIgnoredWhenFlagSet(t *testing.T) { + t.Setenv("RESTIC_COMPRESSION", "invalid") + + var gopts Options + fs := pflag.NewFlagSet("test", pflag.ContinueOnError) + gopts.AddFlags(fs) + + err := fs.Set("compression", "off") + rtest.OK(t, err) + + err = gopts.PreRun(false) + rtest.OK(t, err) + rtest.Equals(t, "off", gopts.Compression.String()) +}