fail of RESTIC_READ_CONCURRENCY or RESTIC_COMPRESSION are invalid

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

changelog/unreleased/pull-5592

@@ -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

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 != "" {

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

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