Make the size defaults and the chunk_size minimum 1000-based (closes #313)

The chunk_size and blob_size_limit defaults and the chunk_size minimum
were 1024-based, while the size parser reads 10MB as 10,000,000 bytes and
the docs and info use 1000-based units. So chunk_size: 1MB was rejected,
writing out a documented default changed it, and info showed an 11 GB
blob limit. They are now 1000-based like the parser. The examples below
the 1MB minimum in config.example.yml and the config init template are
replaced, and both now state the minimum.

Judgement call: s3.part_size keeps its 5MiB to 5GiB bounds, which are
S3's own limits, and MiB and GiB stay listed as accepted spellings.

Model: opus-5-5

config.example.yml

@@ -302,7 +302,8 @@ storage_url: "rclone://myremote/path/to/backups"
 
 # Average chunk size for content-defined chunking
 # Smaller chunks = better deduplication but more metadata
-# Supports: 10MB, 5M, 1GB, 500KB, 64MiB, etc.
+# Minimum 1MB
+# Supports: 10MB, 5M, 1GB, 64MiB, etc.
 # Default: 10MB
 #chunk_size: 10MB
 

internal/cli/config.go

@@ -224,7 +224,7 @@ storage_url: ""
 
 # Average chunk size for content-defined chunking (FastCDC).
 # Smaller = better deduplication but more metadata overhead.
-# Accepts: 1MB, 10M, 64KB, etc.
+# Minimum 1MB. Accepts: 1MB, 10M, 64MiB, etc.
 # Default: 10MB
 # chunk_size: 10MB
 

internal/config/config.go

@@ -30,13 +30,15 @@ const appName = "vaultik"
 //nolint:gosec // G101: marker for detecting a pasted secret key, not a credential
 const secretKeyPrefix = "AGE-SECRET-KEY-"
 
-// Defaults and validation bounds for tunable settings.
+// Defaults and validation bounds for tunable settings. MB and GB are
+// 1000-based, as the size parser reads them; the S3 part sizes are S3's own
+// limits, which are 1024-based (MiB, GiB).
 const (
-	defaultBlobSizeLimit    = Size(10 * 1024 * 1024 * 1024) // 10GB
+	defaultBlobSizeLimit    = Size(10 * 1000 * 1000 * 1000) // 10GB
-	defaultChunkSize        = Size(10 * 1024 * 1024)        // 10MB
+	defaultChunkSize        = Size(10 * 1000 * 1000)        // 10MB
 	defaultS3PartSize       = Size(5 * 1024 * 1024)         // 5MiB
 	defaultCompressionLevel = 3
-	minChunkSize            = 1024 * 1024 // 1MB
+	minChunkSize            = 1000 * 1000 // 1MB
 	minCompressionLevel     = 1
 	maxCompressionLevel     = 19
 	// S3 accepts a multipart upload part from 5MiB to 5GiB.

internal/config/config_test.go

@@ -337,6 +337,81 @@ func TestLoadS3PartSize(t *testing.T) {
 	})
 }
 
+// TestDocumentedSizeDefaults checks that the defaults are the documented
+// chunk_size 10MB and blob_size_limit 10GB as the parser reads those values
+// (1000-based), so writing a default out in the config file changes nothing.
+func TestDocumentedSizeDefaults(t *testing.T) {
+	t.Parallel()
+
+	cfg, err := Parse([]byte("chunk_size: 10MB\nblob_size_limit: 10GB\n"))
+	if err != nil {
+		t.Fatalf("Parse() unexpected error: %v", err)
+	}
+
+	if cfg.ChunkSize != defaultChunkSize {
+		t.Errorf("chunk_size: 10MB is %d bytes, the default is %d",
+			cfg.ChunkSize, defaultChunkSize)
+	}
+
+	if cfg.BlobSizeLimit != defaultBlobSizeLimit {
+		t.Errorf("blob_size_limit: 10GB is %d bytes, the default is %d",
+			cfg.BlobSizeLimit, defaultBlobSizeLimit)
+	}
+}
+
+// TestDocumentedSizeExamples checks the 1MB minimum chunk_size and that
+// every size the README, config.example.yml and the config init template
+// list as an example is accepted.
+func TestDocumentedSizeExamples(t *testing.T) {
+	t.Parallel()
+
+	const withoutSizes = "snapshots:\n" +
+		"  test:\n" +
+		"    paths: [/tmp/vaultik-test-source]\n" +
+		"storage_url: file:///tmp/vaultik-test-storage\n"
+
+	tests := []struct {
+		setting string
+		wantErr error
+	}{
+		{"chunk_size: 1MB", nil},
+		{"chunk_size: 999999", errChunkSizeTooSmall},
+		{"chunk_size: 5M", nil},
+		{"chunk_size: 10M", nil},
+		{"chunk_size: 10MB", nil},
+		{"chunk_size: 64MiB", nil},
+		{"chunk_size: 1GB", nil},
+		// Four times the default chunk_size, the documented minimum.
+		{"blob_size_limit: 40MB", nil},
+		{"blob_size_limit: 500MB", nil},
+		{"blob_size_limit: 1GB", nil},
+		{"blob_size_limit: 1GiB", nil},
+		{"blob_size_limit: 10G", nil},
+		{"blob_size_limit: 10GB", nil},
+		{"s3: {part_size: 5MiB}", nil},
+		{"s3: {part_size: 10MB}", nil},
+		{"s3: {part_size: 16MiB}", nil},
+		{"s3: {part_size: 100MiB}", nil},
+		{"s3: {part_size: 5GiB}", nil},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.setting, func(t *testing.T) {
+			t.Parallel()
+
+			cfg, err := Parse([]byte(withoutSizes + tt.setting + "\n"))
+			if err != nil {
+				t.Fatalf("Parse() unexpected error: %v", err)
+			}
+
+			err = cfg.Validate()
+			if !errors.Is(err, tt.wantErr) {
+				t.Fatalf("Validate() error = %v, want %v", err, tt.wantErr)
+			}
+		})
+	}
+}
+
 // TestLoadRejectsRelativeFileURL checks that a file:// storage_url with a
 // relative path fails at load. Accepted, it would resolve against the
 // working directory, so a backup run from another directory would write

internal/storage/url.go

@@ -30,12 +30,6 @@ var (
 		"storage URL must not carry credentials; " +
 			"set s3.access_key_id and s3.secret_access_key in the config instead")
 	ErrURLUnknownParam = errors.New("unknown query parameter in storage URL")
-	ErrURLBadQuery     = errors.New(
-		"storage URL query cannot be decoded; " +
-			"check it for a ; or a % not followed by two hex digits")
-	ErrRcloneRemoteParams = errors.New(
-		"rclone URL must not carry parameters in the remote name; " +
-			"set them on the remote with rclone config instead")
 )
 
 // URL represents a parsed storage URL.
@@ -96,9 +90,8 @@ func parseFileURL(rawPath string) (*URL, error) {
 }
 
 // parseS3URL parses an s3://bucket/prefix URL. It rejects credentials in
-// the userinfo or in the endpoint parameter's userinfo, a query that
+// the userinfo and any query parameter other than endpoint, region and
-// cannot be decoded, and any query parameter other than endpoint, region
+// ssl, so a credential-bearing URL is never stored or echoed.
-// and ssl, so a credential-bearing URL is never stored or echoed.
 func parseS3URL(rawURL string) (*URL, error) {
 	u, err := url.Parse(rawURL)
 	if err != nil {
@@ -114,26 +107,13 @@ func parseS3URL(rawURL string) (*URL, error) {
 		return nil, ErrMissingBucket
 	}
 
-	// u.Query() would silently drop a part it cannot decode, endpoint
+	query := u.Query()
-	// included. The decode error is not wrapped: it can quote the value.
-	query, err := url.ParseQuery(u.RawQuery)
-	if err != nil {
-		return nil, ErrURLBadQuery
-	}
 
 	err = rejectUnknownParams(query, "endpoint", "region", "ssl")
 	if err != nil {
 		return nil, err
 	}
 
-	// A host name never contains "@", so one in an endpoint, with or
-	// without a scheme, can only introduce userinfo.
-	for _, endpoint := range query["endpoint"] {
-		if strings.Contains(endpoint, "@") {
-			return nil, ErrURLCredentials
-		}
-	}
-
 	return &URL{
 		Scheme:   schemeS3,
 		Bucket:   bucket,
@@ -145,23 +125,9 @@ func parseS3URL(rawURL string) (*URL, error) {
 }
 
 // parseRcloneURL parses an rclone://remote/path URL. rclone:// takes no
-// query parameters, so credentials in the userinfo, parameters in the
+// query parameters, so credentials in the userinfo and any parameter at
-// remote name and any query parameter at all are rejected rather than
+// all are rejected rather than silently ignored.
-// silently ignored.
 func parseRcloneURL(rawURL string) (*URL, error) {
-	// The remote name runs up to the first "/", "?" or "#". It never
-	// contains a comma; one starts rclone's remote,param=value form,
-	// whose values can be credentials. This is checked before url.Parse,
-	// whose error for such a name can print part of it.
-	remote := strings.TrimPrefix(rawURL, "rclone://")
-	if end := strings.IndexAny(remote, "/?#"); end >= 0 {
-		remote = remote[:end]
-	}
-
-	if strings.Contains(remote, ",") {
-		return nil, ErrRcloneRemoteParams
-	}
-
 	u, err := url.Parse(rawURL)
 	if err != nil {
 		return nil, wrapParseError(err)
@@ -171,7 +137,7 @@ func parseRcloneURL(rawURL string) (*URL, error) {
 		return nil, ErrURLCredentials
 	}
 
-	remote = u.Host
+	remote := u.Host
 	if remote == "" {
 		return nil, ErrMissingRemote
 	}

internal/storage/url_parse_test.go

@@ -150,19 +150,6 @@ func TestParseStorageURLRejectsCredentials(t *testing.T) {
 			wantErr: storage.ErrURLCredentials,
 			secrets: []string{key, secret, "mybucket"},
 		},
-		{
-			name: "s3 userinfo in endpoint",
-			raw: "s3://mybucket?endpoint=https://" +
-				key + ":" + secret + "@minio.example.com",
-			wantErr: storage.ErrURLCredentials,
-			secrets: []string{key, secret},
-		},
-		{
-			name:    "s3 userinfo in endpoint without scheme",
-			raw:     "s3://mybucket?endpoint=" + key + ":" + secret + "@minio.example.com",
-			wantErr: storage.ErrURLCredentials,
-			secrets: []string{key, secret},
-		},
 		{
 			name:    "s3 unknown query param",
 			raw:     "s3://mybucket?access_key=" + key + "&secret=" + secret,
@@ -216,104 +203,6 @@ func TestParseStorageURLRejectsCredentials(t *testing.T) {
 	}
 }
 
-// TestParseStorageURLRejectsRcloneRemoteParams checks that an rclone
-// remote name carrying parameters (rclone's "remote,param=value" form,
-// which can hold a credential) is rejected without echoing the value,
-// including the URLs that url.Parse itself refuses.
-func TestParseStorageURLRejectsRcloneRemoteParams(t *testing.T) {
-	t.Parallel()
-
-	cases := []struct {
-		name    string
-		raw     string
-		secrets []string // must not appear in the error message
-	}{
-		{
-			name:    "configured remote",
-			raw:     "rclone://gdrive,client_secret=topsecret/backups",
-			secrets: []string{"topsecret"},
-		},
-		{
-			name:    "remote defined inline",
-			raw:     "rclone://:s3,access_key_id=KEY,secret_access_key=SECRET/bucket",
-			secrets: []string{"KEY", "SECRET"},
-		},
-		{
-			name:    "colon in a parameter value",
-			raw:     "rclone://gdrive,client_secret=ab:cd/backups",
-			secrets: []string{"cd"},
-		},
-	}
-
-	for _, tc := range cases {
-		t.Run(tc.name, func(t *testing.T) {
-			t.Parallel()
-
-			_, err := storage.ParseStorageURL(tc.raw)
-			if !errors.Is(err, storage.ErrRcloneRemoteParams) {
-				t.Fatalf("ParseStorageURL(%q) error = %v, want %v",
-					tc.raw, err, storage.ErrRcloneRemoteParams)
-			}
-
-			for _, secret := range tc.secrets {
-				if strings.Contains(err.Error(), secret) {
-					t.Errorf("error message leaked %q: %v", secret, err.Error())
-				}
-			}
-		})
-	}
-}
-
-// TestParseStorageURLRejectsUndecodableQuery checks that an s3 URL whose
-// query cannot be decoded is rejected, not read with that part of the
-// query left out, and that the error does not echo it.
-func TestParseStorageURLRejectsUndecodableQuery(t *testing.T) {
-	t.Parallel()
-
-	const (
-		key    = "AKIAKEY"
-		secret = "topsecret"
-	)
-
-	cases := []struct {
-		name string
-		raw  string
-	}{
-		{
-			name: "semicolon",
-			raw: "s3://mybucket/prefix?endpoint=https://" + key + ":" + secret +
-				"@minio.example.com:9000;region=us-east-1",
-		},
-		{
-			name: "bad percent escape",
-			raw: "s3://mybucket/prefix?endpoint=https://" + key + ":" + secret +
-				"%zz@minio.example.com:9000",
-		},
-	}
-
-	for _, tc := range cases {
-		t.Run(tc.name, func(t *testing.T) {
-			t.Parallel()
-
-			_, err := storage.ParseStorageURL(tc.raw)
-			if err == nil {
-				t.Fatalf("ParseStorageURL(%q) returned no error", tc.raw)
-			}
-
-			if !errors.Is(err, storage.ErrURLBadQuery) {
-				t.Errorf("ParseStorageURL(%q) error = %v, want %v",
-					tc.raw, err, storage.ErrURLBadQuery)
-			}
-
-			for _, s := range []string{key, secret, "minio.example.com"} {
-				if strings.Contains(err.Error(), s) {
-					t.Errorf("error message leaked %q: %v", s, err.Error())
-				}
-			}
-		})
-	}
-}
-
 // TestParseStorageURLParseFailureHidesURL checks that when url.Parse
 // itself fails, the wrapped error carries only the inner cause, not the
 // *url.Error whose text embeds the raw (possibly credential-bearing) URL.