diff --git a/docs/configuration.md b/docs/configuration.md index 68bb79c1..bf20c275 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -1275,7 +1275,7 @@ cooldown: | `cooldown.packages` | - | Per-package overrides (keyed by PURL) | | `cooldown.package_patterns` | - | Per-package glob overrides (keyed by PURL glob) | -Durations support days (`7d`), hours (`48h`), and minutes (`30m`). Set to `0` to disable. +Durations support days (`7d`), hours (`48h`), and minutes (`30m`). Set to `0` to disable. Other units such as weeks (`1w`) are not supported. A `default`, `ecosystems` or `packages` value that can't be parsed applies no cooldown, so the proxy logs a warning at startup naming it. Invalid durations in `package_patterns` and inside `cooldown.profiles` are rejected at startup. Package PURL keys are normalized to canonical form before matching, so `pkg:npm/@babel/core` and `pkg:npm/%40babel/core` are equivalent, as are `pkg:pypi/Django` and `pkg:pypi/django`. If both forms configure the same package, the canonical entry wins. diff --git a/internal/config/config.go b/internal/config/config.go index 0cfaa3b8..abba89b0 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -288,7 +288,7 @@ func (c *CooldownConfig) NormalizedPackages() map[string]string { // Shortest returns the shortest cooldown configured anywhere, by default, // ecosystem, package or package pattern, or 0 when none is. Unparsable -// durations are left out; Validate reports them. +// durations are left out; Config.Warnings reports them. func (c *CooldownConfig) Shortest() time.Duration { if c == nil { return 0 diff --git a/internal/config/cooldown_profiles.go b/internal/config/cooldown_profiles.go index 225dd201..44391a06 100644 --- a/internal/config/cooldown_profiles.go +++ b/internal/config/cooldown_profiles.go @@ -5,6 +5,7 @@ import ( "maps" "regexp" "slices" + "strings" "github.com/git-pkgs/cooldown" ) @@ -18,6 +19,26 @@ func (c *CooldownConfig) Policy() (*cooldown.Policy, error) { }, c.PackagePatterns) } +// durationWarning lists every top-level default, ecosystem or package cooldown +// duration that cannot be parsed, or returns "" when they all parse. +// +// The cooldown library reads such a duration as 0, so a typo such as "1w" +// turns cooldown off without notice. Profiles already reject these values, as +// does the library for package_patterns; the top level only warns, so configs +// that start today keep starting. +func (c *CooldownConfig) durationWarning() string { + top := CooldownConfig{Default: c.Default, Ecosystems: c.Ecosystems, Packages: c.Packages} + errs := top.durationErrors() + if len(errs) == 0 { + return "" + } + msgs := make([]string, len(errs)) + for i, err := range errs { + msgs[i] = err.Error() + } + return fmt.Sprintf("cooldown: %s; no cooldown applies where these values are used", strings.Join(msgs, "; ")) +} + // ValidateProfiles checks names and policies before they become routes. func (c *CooldownConfig) ValidateProfiles() error { for _, name := range slices.Sorted(maps.Keys(c.Profiles)) { @@ -39,6 +60,15 @@ func (c *CooldownConfig) ValidateProfiles() error { } func (c *CooldownConfig) validateProfileDurations() error { + if errs := c.durationErrors(); len(errs) > 0 { + return errs[0] + } + return nil +} + +// durationErrors returns one error per invalid duration, sorted by key. +func (c *CooldownConfig) durationErrors() []error { + var errs []error values := map[string]string{"default": c.Default} for group, overrides := range map[string]map[string]string{ "ecosystems": c.Ecosystems, "packages": c.Packages, "package_patterns": c.PackagePatterns, @@ -54,11 +84,10 @@ func (c *CooldownConfig) validateProfileDurations() error { } duration, err := cooldown.ParseDuration(value) if err != nil { - return fmt.Errorf("invalid %s: %w", key, err) - } - if duration < 0 { - return fmt.Errorf("invalid %s: duration must be non-negative", key) + errs = append(errs, fmt.Errorf("invalid %s: %w", key, err)) + } else if duration < 0 { + errs = append(errs, fmt.Errorf("invalid %s: duration must be non-negative", key)) } } - return nil + return errs } diff --git a/internal/config/cooldown_profiles_test.go b/internal/config/cooldown_profiles_test.go index 6f10362d..3be4666a 100644 --- a/internal/config/cooldown_profiles_test.go +++ b/internal/config/cooldown_profiles_test.go @@ -79,3 +79,89 @@ func TestLoadRejectsInvalidCooldownProfiles(t *testing.T) { }) } } + +// TestWarnsOnInvalidTopLevelCooldown checks that a top-level duration the +// cooldown library cannot parse, and would read as 0, is reported. It warns +// rather than failing Validate so that configs which start today still start. +func TestWarnsOnInvalidTopLevelCooldown(t *testing.T) { + for _, tc := range []struct { + name string + set func(*CooldownConfig) + want string + }{ + {"default weeks", func(c *CooldownConfig) { c.Default = "1w" }, "cooldown: invalid default"}, + {"default words", func(c *CooldownConfig) { c.Default = "7 days" }, "cooldown: invalid default"}, + {"default negative", func(c *CooldownConfig) { c.Default = "-3d" }, "cooldown: invalid default"}, + {"ecosystem", func(c *CooldownConfig) { c.Ecosystems = map[string]string{"npm": "3days"} }, "cooldown: invalid ecosystems.npm"}, + {"package", func(c *CooldownConfig) { c.Packages = map[string]string{"pkg:npm/lodash": "soon"} }, "cooldown: invalid packages.pkg:npm/lodash"}, + } { + t.Run(tc.name, func(t *testing.T) { + cfg := Default() + tc.set(&cfg.Cooldown) + if err := cfg.Validate(); err != nil { + t.Fatalf("Validate = %v, want nil: invalid top-level durations warn", err) + } + if !hasWarning(cfg.Warnings(), tc.want) { + t.Fatalf("Warnings = %q, want one containing %q", cfg.Warnings(), tc.want) + } + }) + } +} + +func TestWarnsOnEveryInvalidTopLevelCooldown(t *testing.T) { + cfg := Default() + cfg.Cooldown.Default = "1w" + cfg.Cooldown.Ecosystems = map[string]string{"npm": "3days", "pypi": "2d"} + cfg.Cooldown.Packages = map[string]string{"pkg:npm/lodash": "soon"} + for _, want := range []string{"invalid default", "invalid ecosystems.npm", "invalid packages.pkg:npm/lodash"} { + if !hasWarning(cfg.Warnings(), want) { + t.Errorf("Warnings = %q, want one containing %q", cfg.Warnings(), want) + } + } + if hasWarning(cfg.Warnings(), "ecosystems.pypi") { + t.Errorf("Warnings = %q, want the valid pypi value left out", cfg.Warnings()) + } +} + +func TestNoWarningForValidTopLevelCooldown(t *testing.T) { + for _, value := range []string{"", "0", "3d", "1.5d", "48h", "90m"} { + cfg := Default() + cfg.Cooldown.Default = value + cfg.Cooldown.Ecosystems = map[string]string{"npm": value} + if hasWarning(cfg.Warnings(), "cooldown") { + t.Errorf("Warnings(%q) = %q, want no cooldown warning", value, cfg.Warnings()) + } + } +} + +func TestWarnsOnInvalidCooldownFromEnv(t *testing.T) { + t.Setenv("PROXY_COOLDOWN_DEFAULT", "1w") + cfg := Default() + cfg.LoadFromEnv() + if !hasWarning(cfg.Warnings(), "cooldown: invalid default") { + t.Fatalf("Warnings = %q, want invalid cooldown default", cfg.Warnings()) + } +} + +func hasWarning(warnings []string, substr string) bool { + for _, w := range warnings { + if strings.Contains(w, substr) { + return true + } + } + return false +} + +// TestInvalidCooldownPatternFailsPolicy checks that an invalid package_patterns +// duration is left to the cooldown library, which already refuses it when the +// policy is built at startup, rather than reported as a warning. +func TestInvalidCooldownPatternFailsPolicy(t *testing.T) { + cfg := Default() + cfg.Cooldown.PackagePatterns = map[string]string{"pkg:npm/*": "1w"} + if _, err := cfg.Cooldown.Policy(); err == nil { + t.Fatal("Policy() = nil error, want the invalid pattern duration rejected") + } + if hasWarning(cfg.Warnings(), "cooldown") { + t.Errorf("Warnings = %q, want no cooldown warning for a pattern", cfg.Warnings()) + } +} diff --git a/internal/config/mirror.go b/internal/config/mirror.go index 3b56fd87..cd0bdd2f 100644 --- a/internal/config/mirror.go +++ b/internal/config/mirror.go @@ -144,6 +144,9 @@ func (c *Config) Warnings() []string { if c.Mirror.Refresh != nil && !c.CacheMetadata { warnings = append(warnings, "mirror.refresh without cache_metadata does not refresh Debian, Alpine or RPM packages: their indexes are found through the metadata cache") } + if w := c.Cooldown.durationWarning(); w != "" { + warnings = append(warnings, w) + } return warnings }