From 7f417262c5f3f7296aea50d34c094182b80b1916 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Thu, 1 Oct 2026 11:32:25 +0000 Subject: [PATCH] feat(job-array-one-based-index): clamp the apiserver array size limit (task 5.2) job.max_array_size is resolved by a pure resolveMaxArraySize: unset or 0 gives 1000, 1..32767 is used as is, values above 32767 are clamped to 32767 with a startup warning naming the configured and effective value, and a negative value refuses startup. The old >2147483646 rejection is gone; the shipped config documents the bounds. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../specs/job-array-one-based-index/tasks.md | 2 +- src/frontend/apiserver/config/app.yaml | 2 +- src/frontend/apiserver/utils/config.go | 65 +++++--- src/frontend/apiserver/utils/config_test.go | 144 ++++++++++++++++++ 4 files changed, 189 insertions(+), 24 deletions(-) diff --git a/.kiro/specs/job-array-one-based-index/tasks.md b/.kiro/specs/job-array-one-based-index/tasks.md index fdc15338..f3636903 100644 --- a/.kiro/specs/job-array-one-based-index/tasks.md +++ b/.kiro/specs/job-array-one-based-index/tasks.md @@ -101,7 +101,7 @@ - _Requirements: 1.1, 1.2, 5.8, 5.14_ - _Depends: 1.3_ -- [ ] 5.2 (P) Clamp the array size limit setting +- [x] 5.2 (P) Clamp the array size limit setting - Move the limit rule into a pure resolver: unset or 0 gives 1000, 1..32767 used as is, above 32767 clamped to 32767, negative refuses startup; drop the old upper-bound rejection - Log a startup warning with the configured and effective values when clamped; update the shipped config's comment for the setting (default 1000, at most 32767) - Done when the config tests pass for 0, 1, 32767, 32768, 3000000000 and -1, and a clamped startup logs the warning diff --git a/src/frontend/apiserver/config/app.yaml b/src/frontend/apiserver/config/app.yaml index e52b1820..c986c34b 100644 --- a/src/frontend/apiserver/config/app.yaml +++ b/src/frontend/apiserver/config/app.yaml @@ -68,7 +68,7 @@ file_upload: - "application/javascript" # .js file job: - max_array_size: 1000 # default max array size for job submission + max_array_size: 1000 # max elements per job array (default 1000 if unset or 0, at most 32767; larger values are clamped to 32767 with a startup warning; negative refuses startup) cache: refresh_interval: 30 # seconds, available values: 1-300 diff --git a/src/frontend/apiserver/utils/config.go b/src/frontend/apiserver/utils/config.go index 24b5845a..260bfa2a 100644 --- a/src/frontend/apiserver/utils/config.go +++ b/src/frontend/apiserver/utils/config.go @@ -10,13 +10,14 @@ import ( "strings" "github.com/spf13/viper" + "go.uber.org/zap" ) type BackendConfig struct { - Address string `yaml:"address"` - Port int `yaml:"port"` - From string `yaml:"from"` - Key string `yaml:"key"` + Address string `yaml:"address"` + Port int `yaml:"port"` + From string `yaml:"from"` + Key string `yaml:"key"` // Peers lists additional controller candidates for HA mode in "host:port" // format. When non-empty, RegisterToBackend tries each candidate in order // and connects to whichever accepts the join as primary. Address+Port @@ -206,24 +207,19 @@ func InitConf() error { DisallowedTypes: viper.GetStringSlice("file_upload.disallowed_types"), } - // parse job config section - // parse job config section - if !viper.IsSet("job") { - // if job section is not set, use default values - JobYamlConfig = &JobConfig{ - MaxArraySize: 1000, - } - } else { - maxArraySize := viper.GetInt("job.max_array_size") - if maxArraySize == 0 { - maxArraySize = 1000 - } - if maxArraySize < 1 || maxArraySize > 2147483646 { - return fmt.Errorf("invalid job.max_array_size value: %d, must be between 1 and 2147483646", maxArraySize) - } - JobYamlConfig = &JobConfig{ - MaxArraySize: maxArraySize, - } + // parse job config section; an absent section or key reads as 0 (default) + configuredArraySize := viper.GetInt("job.max_array_size") + maxArraySize, clamped, err := resolveMaxArraySize(configuredArraySize) + if err != nil { + return err + } + if clamped { + Warn("job.max_array_size above supported maximum", + zap.Int("configured", configuredArraySize), + zap.Int("effective", maxArraySize)) + } + JobYamlConfig = &JobConfig{ + MaxArraySize: maxArraySize, } // parse cache config section @@ -289,6 +285,31 @@ func ParseMaxSize(maxSize string) (int64, error) { return size, nil } +const ( + // defaultMaxArraySize is used when job.max_array_size is unset or 0. + defaultMaxArraySize = 1000 + // supportedMaxArraySize is the largest array the API server creates + // (array element numbers are 1..32767). + supportedMaxArraySize = 32767 +) + +// resolveMaxArraySize turns the configured job.max_array_size into the +// effective array size limit: 0 (unset) gives the default 1000, 1..32767 is +// used as is, anything above 32767 is clamped to 32767 (clamped = true, the +// caller logs a warning), and a negative value is an error. +func resolveMaxArraySize(v int) (effective int, clamped bool, err error) { + switch { + case v < 0: + return 0, false, fmt.Errorf("invalid job.max_array_size value: %d, must not be negative", v) + case v == 0: + return defaultMaxArraySize, false, nil + case v > supportedMaxArraySize: + return supportedMaxArraySize, true, nil + default: + return v, false, nil + } +} + // resolveJWTSecret returns the JWT secret from XCE_JWT_SECRET env var if set and non-empty, // otherwise falls back to the value from config file. func resolveJWTSecret(configValue string) string { diff --git a/src/frontend/apiserver/utils/config_test.go b/src/frontend/apiserver/utils/config_test.go index 2bb86b7c..5fb36a15 100644 --- a/src/frontend/apiserver/utils/config_test.go +++ b/src/frontend/apiserver/utils/config_test.go @@ -2,7 +2,13 @@ package utils import ( "os" + "path/filepath" + "strings" "testing" + + "github.com/spf13/viper" + "go.uber.org/zap" + "go.uber.org/zap/zaptest/observer" ) func TestResolveJWTSecret_EnvOverridesConfig(t *testing.T) { @@ -33,3 +39,141 @@ func TestResolveJWTSecret_EmptyEnvUsesConfig(t *testing.T) { t.Errorf("expected config-secret, got %q", got) } } + +func TestResolveMaxArraySize(t *testing.T) { + cases := []struct { + name string + in int + want int + wantClamped bool + wantErr bool + }{ + {"unset or zero gives default", 0, 1000, false, false}, + {"one used as is", 1, 1, false, false}, + {"default value used as is", 1000, 1000, false, false}, + {"supported maximum used as is", 32767, 32767, false, false}, + {"one above maximum clamped", 32768, 32767, true, false}, + {"three billion clamped", 3000000000, 32767, true, false}, + {"negative refused", -1, 0, false, true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, clamped, err := resolveMaxArraySize(tc.in) + if tc.wantErr { + if err == nil { + t.Fatalf("resolveMaxArraySize(%d): expected error, got %d", tc.in, got) + } + if !strings.Contains(err.Error(), "job.max_array_size") { + t.Errorf("error %q does not name the setting", err) + } + return + } + if err != nil { + t.Fatalf("resolveMaxArraySize(%d): unexpected error %v", tc.in, err) + } + if got != tc.want || clamped != tc.wantClamped { + t.Errorf("resolveMaxArraySize(%d) = (%d, %v), want (%d, %v)", + tc.in, got, clamped, tc.want, tc.wantClamped) + } + }) + } +} + +// setupInitConf writes a copy of the shipped config/app.yaml into a temp +// top dir, with the job section replaced by jobSection ("" drops it), points +// Global_TopDir at it, and installs an observed logger. Globals are restored +// on cleanup. +func setupInitConf(t *testing.T, jobSection string) *observer.ObservedLogs { + t.Helper() + shipped, err := os.ReadFile(filepath.Join("..", "config", "app.yaml")) + if err != nil { + t.Fatalf("read shipped app.yaml: %v", err) + } + var out []string + skipping := false + for _, line := range strings.Split(string(shipped), "\n") { + if line == "job:" { + skipping = true + if jobSection != "" { + out = append(out, strings.Split(jobSection, "\n")...) + } + continue + } + if skipping { + if strings.HasPrefix(line, " ") { + continue + } + skipping = false + } + out = append(out, line) + } + dir := t.TempDir() + if err := os.MkdirAll(filepath.Join(dir, "config"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "config", "app.yaml"), + []byte(strings.Join(out, "\n")), 0o644); err != nil { + t.Fatal(err) + } + + core, logs := observer.New(zap.DebugLevel) + oldLog, oldTop, oldJob := Log, Global_TopDir, JobYamlConfig + Log = zap.New(core) + Global_TopDir = dir + viper.Reset() + t.Cleanup(func() { + Log, Global_TopDir, JobYamlConfig = oldLog, oldTop, oldJob + viper.Reset() + }) + return logs +} + +func TestInitConf_MaxArraySizeClampedLogsWarning(t *testing.T) { + logs := setupInitConf(t, "job:\n max_array_size: 40000") + if err := InitConf(); err != nil { + t.Fatalf("InitConf: %v", err) + } + if JobYamlConfig.MaxArraySize != 32767 { + t.Errorf("MaxArraySize = %d, want 32767", JobYamlConfig.MaxArraySize) + } + warns := logs.FilterMessage("job.max_array_size above supported maximum"). + FilterLevelExact(zap.WarnLevel).All() + if len(warns) != 1 { + t.Fatalf("expected 1 clamp warning, got %d (all logs: %v)", len(warns), logs.All()) + } + f := warns[0].ContextMap() + if f["configured"] != int64(40000) || f["effective"] != int64(32767) { + t.Errorf("warning fields = %v, want configured=40000 effective=32767", f) + } +} + +func TestInitConf_MaxArraySizeInRangeNoWarning(t *testing.T) { + logs := setupInitConf(t, "job:\n max_array_size: 32767") + if err := InitConf(); err != nil { + t.Fatalf("InitConf: %v", err) + } + if JobYamlConfig.MaxArraySize != 32767 { + t.Errorf("MaxArraySize = %d, want 32767", JobYamlConfig.MaxArraySize) + } + if n := logs.FilterMessage("job.max_array_size above supported maximum").Len(); n != 0 { + t.Errorf("unexpected clamp warning (%d)", n) + } +} + +func TestInitConf_MaxArraySizeAbsentSectionDefaults(t *testing.T) { + setupInitConf(t, "") + if err := InitConf(); err != nil { + t.Fatalf("InitConf: %v", err) + } + if JobYamlConfig.MaxArraySize != 1000 { + t.Errorf("MaxArraySize = %d, want 1000", JobYamlConfig.MaxArraySize) + } +} + +func TestInitConf_MaxArraySizeNegativeRefused(t *testing.T) { + setupInitConf(t, "job:\n max_array_size: -1") + err := InitConf() + if err == nil || !strings.Contains(err.Error(), "job.max_array_size") { + t.Fatalf("InitConf with -1: expected job.max_array_size error, got %v", err) + } +}