fix(configuration): apply the rate limiter warn_action default

What does this MR do?

ApplyDefaults fills in the rate limiter defaults while ranging over config.RateLimiter.Limiters by value (configuration.go#L1311-1326), so every assignment goes to a copy of the limiter and is thrown away. No parsed configuration carries the defaults: limit.period stays "" instead of second, and action.warn_action and action.hard_action stay "" instead of none. The loop has been like this since 32b3f2e3 (docs: rate-limiting configuration settings, 2025-04-25).

The rate limiter middleware validates every limiter and rejects those empty values (ratelimiter.go#L159-184), and NewApp returns the error (app.go#L278-284). So a registry whose limiter leaves out action.warn_action, which docs/configuration.md describes as optional with a default of none, does not start: it exits with action.warn_action must be one of: [none log]. The same happens when limit.period or action.hard_action, which the docs list as required, is left out.

This MR takes a pointer to each element, so the warn_action default reaches the parsed configuration. It also drops the Match.Type lowercasing from the same loop: that line never had an effect, the middleware already trims and lowercases the match type before it uses it (ratelimiter.go#L148), and applying it in ApplyDefaults would change the parsed configuration that TestConfigSuite expects (Type: "IP", as written).

As agreed in review, it removes the limit.period and action.hard_action defaults from the same loop instead of making them effective, so both fields stay required, as docs/configuration.md says. The docs are not changed.

The test for the warn_action default asserted the empty value, so it pinned the defect; its default case now expects none. The new TestParseRateLimiter_Limiters_Defaults checks that every limiter in the list gets that default, not only the first, and keeps the period and hard_action it sets.

Behavior change: a limiter that passed validation before set warn_action, and a default never replaces a configured value, so a configuration that started before behaves the same. A limiter that leaves out warn_action used to stop the registry from starting and now gets none. One that leaves out period or hard_action still fails validation, as on master.

The GitLab chart writes period, warn_action and hard_action into a limiter only when the values set them (_rate_limiter.tpl#L29-53). Rate limiting is off there by default and the chart's examples set all three, so a chart setup that works today sees no change, and one whose limiter leaves out warn_action now starts instead of failing. Omnibus has no gitlab.rb setting for the rate limiter: its registry configuration template writes no rate_limiter section (registry-config.yml.erb), so a gitlab.rb setup is not affected.

Closes #2992 (closed)

How the tests fail on master and pass with the change

On master at 672bd753, with Go 1.26.10 and 1.27.3 (the CI matrix), with only the test changes on the unchanged code, two tests fail: the default case of TestParseRateLimiter_Limiters_Action_WarnAction, under both yaml and env, and the new test. Trimmed to the assertion lines:

--- FAIL: TestParseRateLimiter_Limiters_Action_WarnAction/yaml/default
        expected: "none"
        actual  : ""
--- FAIL: TestParseRateLimiter_Limiters_Action_WarnAction/env/default
        expected: "none"
        actual  : ""
--- FAIL: TestParseRateLimiter_Limiters_Defaults
        expected: "none"
        actual  : ""

With the change, the rate limiter tests pass on both versions, and so does go test ./configuration/ ./registry/middleware/ratelimiter/ ./registry/ -count=1.

I also broke the fix in four ways (copying each limiter instead of taking a pointer, applying the default to the first limiter only, dropping the warn_action default, and restoring the period and hard_action defaults), and the tests failed each time.

Starting the registry on master and on this branch

With the steps in the issue (Redis 7.2.16, the limiter of the docs/configuration.md example without warn_action and log_only: false), go run ./cmd/registry serve /tmp/config.yml on master exits with status 1:

creating new registry instance: configuring application: failed to configure rate limiting: 1 error occurred:
	* action.warn_action must be one of: [none log]

From this branch it starts, logs Configured rate limiter with warn_action set to none and rate limiting has been successfully configured, and answers GET /v2/ with 200.

I also started both builds with a limiter that sets all three values (both start, with the configured values) and with one that sets none of them (master exits with the three validation errors, and this branch with the period and hard_action ones, since both stay required).

Other checks I ran
  • go test -tags integration ./registry/middleware/ratelimiter/ -count=1 with KV_ADDR pointing at a Redis 8.10.3 server, as the kv:rate-limiter:handlers job runs it, on Go 1.26.10 and 1.27.3: pass (the cluster test skips with a single node, as in that job).
  • golangci-lint run ./configuration/... ./registry/middleware/ratelimiter/... with v2.15.0 (the version CI pins) and the project's .golangci.yml, for amd64, 386 and arm64: 0 issues. golangci-lint fmt --diff, gofmt -l and go vet on the touched packages: clean.
  • make validate-log-fields: no new offenses.
  • commitlint with the project's .commitlintrc.yml in node:lts-alpine, on both commits: 0 problems.

I found this while maintaining gitlab-mcp-server, whose upstream-bugs.md records everything done on its dependencies and sibling projects.

Author checklist

  • CODEOWNERS Review: This MR requires approval from at least one CODEOWNER per category/file.
    • If codeowners are absent or the change is urgent, any registry maintainer can temporarily disable CODEOWNERS reviews in the project settings. When doing so:
      • Add a comment on the MR justifying the bypass
      • Mention/CC the designated codeowners in the MR for async review
      • Re-enable CODEOWNERS reviews in project settings once the MR is merged
    • If you lack permissions to disable CODEOWNERS reviews, reach out to the Engineering Manager (@jaime) or Senior Engineering Manager (@crystalpoole) for assistance.
  • Assign one of conventional-commit prefixes to the MR.
    • fix: Indicates a bug fix, triggers a patch release.
    • feat: Signals the introduction of a new feature, triggers a minor release.
    • perf: Focuses on performance improvements that don't introduce new features or fix bugs, triggers a patch release.
    • docs: Updates or changes to documentation. Does not trigger a release.
    • style: Changes that do not affect the code's functionality. Does not trigger a release.
    • refactor: Modifications to the code that do not fix bugs or add features but improve code structure or readability. Does not trigger a release.
    • test: Changes related to adding or modifying tests. Does not trigger a release.
    • chore: Routine tasks that don't affect the application, such as updating build processes, package manager configs, etc. Does not trigger a release.
    • build: Changes that affect the build system or external dependencies. May trigger a release.
    • ci: Modifications to continuous integration configuration files and scripts. Does not trigger a release.
    • revert: Reverts a previous commit. It could result in a patch, minor, or major release.
  • MR contains database changes including schema/background migrations: not applicable.
  • Change contains a breaking change - apply the breaking change label: I do not think it is one, since a configuration that started before behaves the same; only a limiter that kept the registry from starting by leaving out warn_action now gets its default.
  • Change is considered high risk - apply the label high-risk-change
  • I created or linked to an existing issue for every added or updated TODO, BUG, FIXME or OPTIMIZE prefixed comment (there are none).
  • Changes cannot be rolled back: not applicable, a revert restores the previous behavior.
Documentation/resources

Code review guidelines

Go Style guidelines

Feature flags

When documentation is required

Documentation workflow

Reviewer checklist

  • Ensure the commit and MR tittle are still accurate.
  • If the change contains a breaking change, verify the breaking change label.
  • If the change is considered high risk, verify the label high-risk-change
  • Identify if the change can be rolled back safely. (note: all other reasons for not being able to rollback will be sufficiently captured by major version changes).
Edited by José M. Requena Plens

Merge request reports

Loading
Loading