fix: preserve multipart defaults and rollback compatibility

Signed-off-by: Feng Ruohang <rh@vonng.com>

internal/config/api/api.go

@@ -135,7 +135,6 @@ var (
 			Key:   apiStaleUploadsExpiry,
 			Value: "24h",
 		},
-		config.KV{Key: apiMultipartListing, Value: "strict"},
 		config.KV{
 			Key:   apiDeleteCleanupInterval,
 			Value: "5m",
@@ -210,6 +209,7 @@ func LookupConfig(kvs config.KVS) (cfg Config, err error) {
 		apiReplicationWorkers,
 		apiReplicationFailedWorkers,
 		"expiry_workers",
+		apiMultipartListing, // Ignore the retired shared-config key during migration.
 	}
 
 	disableODirect := env.Get(EnvAPIDisableODirect, kvs.Get(apiDisableODirect)) == config.EnableOn
@@ -324,10 +324,6 @@ func LookupConfig(kvs config.KVS) (cfg Config, err error) {
 		return cfg, err
 	}
 	cfg.StaleUploadsExpiry = staleUploadsExpiry
-	cfg.MultipartListing = env.Get(EnvAPIMultipartListing, kvs.GetWithDefault(apiMultipartListing, DefaultKVS))
-	if cfg.MultipartListing != "strict" && cfg.MultipartListing != "legacy" {
-		return cfg, fmt.Errorf("%s must be strict or legacy", apiMultipartListing)
-	}
 
 	cfg.SyncEvents = env.Get(EnvAPISyncEvents, kvs.Get(apiSyncEvents)) == config.EnableOn
 
@@ -348,5 +344,11 @@ func LookupConfig(kvs config.KVS) (cfg Config, err error) {
 		cfg.ObjectMaxVersions = math.MaxInt64
 	}
 
+	cfg.MultipartListing = env.Get(EnvAPIMultipartListing, "legacy")
+	if cfg.MultipartListing != "strict" && cfg.MultipartListing != "legacy" {
+		cfg.MultipartListing = "legacy"
+		return cfg, fmt.Errorf("%s must be strict or legacy; using legacy", EnvAPIMultipartListing)
+	}
+
 	return cfg, nil
 }

internal/config/api/api_test.go

@@ -4,38 +4,111 @@
 package api
 
 import (
+	"encoding/json"
+	"strings"
 	"testing"
 
 	"github.com/minio/minio/internal/config"
 )
 
 func TestMultipartListingMigrationMode(t *testing.T) {
+	t.Setenv(EnvAPIRequestsMax, "")
+	t.Setenv(EnvAPISyncEvents, "")
+	t.Setenv(EnvAPIObjectMaxVersions, "")
 	for _, tc := range []struct {
 		name, stored, override, want string
 		invalid                      bool
 	}{
-		{name: "default", want: "strict"},
-		{name: "explicit-migration", stored: "legacy", want: "legacy"},
-		{name: "environment-override", stored: "strict", override: "legacy", want: "legacy"},
-		{name: "invalid-stored", stored: "automatic", invalid: true},
-		{name: "invalid-environment", stored: "strict", override: "automatic", invalid: true},
+		{name: "default", want: "legacy"},
+		{name: "explicit-legacy", override: "legacy", want: "legacy"},
+		{name: "explicit-strict", override: "strict", want: "strict"},
+		{name: "retired-stored-strict", stored: "strict", want: "legacy"},
+		{name: "retired-stored-legacy", stored: "legacy", want: "legacy"},
+		{name: "retired-stored-invalid", stored: "automatic", want: "legacy"},
+		{name: "environment-override", stored: "legacy", override: "strict", want: "strict"},
+		{name: "invalid-environment", stored: "strict", override: "automatic", want: "legacy", invalid: true},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			t.Setenv(EnvAPIMultipartListing, tc.override)
-			var kvs config.KVS
+			kvs := DefaultKVS.Clone()
+			kvs.Set(apiRequestsMax, "17")
+			kvs.Set(apiSyncEvents, config.EnableOn)
+			kvs.Set(apiObjectMaxVersions, "71")
 			if tc.stored != "" {
-				kvs = config.KVS{{Key: apiMultipartListing, Value: tc.stored}}
+				kvs.Set(apiMultipartListing, tc.stored)
 			}
 			cfg, err := LookupConfig(kvs)
-			if tc.invalid {
-				if err == nil {
-					t.Fatal("invalid migration mode silently accepted")
-				}
-				return
+			if (err != nil) != tc.invalid || cfg.MultipartListing != tc.want {
+				t.Fatalf("mode=%q err=%v, want %q invalid=%v", cfg.MultipartListing, err, tc.want, tc.invalid)
 			}
-			if err != nil || cfg.MultipartListing != tc.want {
-				t.Fatalf("mode=%q err=%v, want %q", cfg.MultipartListing, err, tc.want)
+			if cfg.RequestsMax != 17 || !cfg.SyncEvents || cfg.ObjectMaxVersions != 71 {
+				t.Fatalf("migration setting discarded unrelated config: %+v", cfg)
 			}
 		})
 	}
 }
+
+func TestMultipartListingConfigPersistence(t *testing.T) {
+	t.Setenv(EnvAPIRequestsMax, "")
+	t.Setenv(EnvAPISyncEvents, "")
+	t.Setenv(EnvAPIObjectMaxVersions, "")
+	t.Setenv(EnvAPIMultipartListing, "strict")
+	previous := config.DefaultKVS
+	config.DefaultKVS = map[string]config.KVS{config.APISubSys: DefaultKVS}
+	t.Cleanup(func() { config.DefaultKVS = previous })
+	for _, help := range Help {
+		if help.Key == apiMultipartListing {
+			t.Fatal("process-only setting advertised as shared config")
+		}
+	}
+	for _, input := range []string{
+		"",
+		"api requests_max=17 sync_events=on object_max_versions=71",
+		`api requests_max=17 comment="later disable multipart_listing=strict"`,
+	} {
+		t.Run(input, func(t *testing.T) {
+			c := config.New().Clone()
+			if _, err := c.ReadConfig(strings.NewReader(input)); err != nil {
+				t.Fatal(err)
+			}
+			// Exercise the JSON save/load and Merge paths used for shared config.
+			data, err := json.Marshal(c)
+			if err != nil {
+				t.Fatal(err)
+			}
+			var loaded config.Config
+			if err = json.Unmarshal(data, &loaded); err != nil {
+				t.Fatal(err)
+			}
+			kvs := loaded.Merge()[config.APISubSys][config.Default]
+			if _, present := kvs.Lookup(apiMultipartListing); present {
+				t.Fatal("process-only setting persisted in shared config")
+			}
+			cfg, err := LookupConfig(kvs)
+			if err != nil || cfg.MultipartListing != "strict" {
+				t.Fatalf("environment mode lost: %+v %v", cfg, err)
+			}
+			if input != "" && cfg.RequestsMax != 17 {
+				t.Fatalf("unrelated setting lost: %+v", cfg)
+			}
+			if strings.Contains(input, "comment=") && kvs.Get(config.Comment) != "later disable multipart_listing=strict" {
+				t.Fatalf("literal comment changed: %q", kvs.Get(config.Comment))
+			}
+		})
+	}
+	// A key persisted by a previous development build is tolerated, retained for
+	// explicit cleanup, and removable without resetting other API settings.
+	c := config.New().Clone()
+	kvs := c[config.APISubSys][config.Default]
+	kvs.Set(apiMultipartListing, "strict")
+	kvs.Set(apiRequestsMax, "17")
+	c[config.APISubSys][config.Default] = kvs
+	c = c.Merge()
+	if err := c.DelKVS("api multipart_listing"); err != nil {
+		t.Fatal(err)
+	}
+	kvs = c.Merge()[config.APISubSys][config.Default]
+	if _, present := kvs.Lookup(apiMultipartListing); present || kvs.Get(apiRequestsMax) != "17" {
+		t.Fatalf("targeted reset changed unrelated config or restored retired key: %v", kvs)
+	}
+}

internal/config/api/help.go

@@ -26,12 +26,6 @@ var (
 
 	// Help holds configuration keys and their default values for api subsystem.
 	Help = config.HelpKVS{
-		config.HelpKV{
-			Key:         apiMultipartListing,
-			Description: "multipart listing mode: strict, or temporary legacy mode during coordinated upgrade" + defaultHelpPostfix(apiMultipartListing),
-			Type:        "string",
-			Optional:    true,
-		},
 		config.HelpKV{
 			Key:         apiRequestsMax,
 			Description: `set the maximum number of concurrent requests (default: auto)`,