feat: add optional name field to RateLimitRule for improved observability#9466
feat: add optional name field to RateLimitRule for improved observability#9466Aias00 wants to merge 4 commits into
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
Adds an optional, user-defined name to RateLimitRule to produce stable, human-meaningful rate limit keys (improving observability) while preserving the existing index-based key format when name is unset.
Changes:
- Introduces
RateLimitRule.namewith validation (pattern/length, “not purely numeric”, and per-policy uniqueness via CEL). - Updates BackendTrafficPolicy IR translation to use
<ns>/<policy>/rule/<name>when provided (else<rule-index>). - Adds/updates CEL validation tests and Gateway API / xDS translator golden testdata, plus release note and generated artifacts.
Reviewed changes
Copilot reviewed 13 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/cel-validation/backendtrafficpolicy_test.go | Adds CEL validation coverage for named rules (valid, duplicate, numeric-only, invalid chars). |
| site/content/en/latest/api/extension_types.md | Documents the new name field on RateLimitRule in the rendered API docs. |
| release-notes/current/new_features/9151-named-ratelimit-rules-backendtrafficpolicy.md | Release note fragment for the new optional rule name behavior. |
| internal/xds/translator/testdata/out/ratelimit-config/global-shared-named-rule.yaml | Golden output verifying descriptor keys include the named rule. |
| internal/xds/translator/testdata/out/ratelimit-config/global-shared-named-rule.routes.yaml | Golden output verifying route rate limit actions use the named rule key. |
| internal/xds/translator/testdata/in/ratelimit-config/global-shared-named-rule.yaml | Test input covering shared named-rule behavior around /rule/ handling. |
| internal/utils/testdata/backendtrafficpolicy_ratelimitrule_merge.strategicmerge.out.yaml | Golden output ensuring merges preserve the new name field. |
| internal/utils/testdata/backendtrafficpolicy_ratelimitrule_merge.jsonmerge.out.yaml | Golden output ensuring JSON merge preserves the new name field. |
| internal/gatewayapi/testdata/backendtrafficpolicy-with-ratelimit-rule-names.out.yaml | Golden output showing mixed named/unnamed rule key generation. |
| internal/gatewayapi/testdata/backendtrafficpolicy-with-ratelimit-rule-names.in.yaml | Test input for multiple named global rules plus an unnamed fallback. |
| internal/gatewayapi/testdata/backendtrafficpolicy-with-ratelimit-named-rule.out.yaml | Golden output for named shared + named non-shared + unnamed rules. |
| internal/gatewayapi/testdata/backendtrafficpolicy-with-ratelimit-named-rule.in.yaml | Test input for named shared/non-shared rules and unnamed fallback. |
| internal/gatewayapi/helpers.go | Updates helper to format rule keys using optional rule name. |
| internal/gatewayapi/backendtrafficpolicy.go | Uses the optional rule name when setting IR rule Name for local/global rate limits. |
| charts/gateway-helm/charts/crds/crds/generated/gateway.envoyproxy.io_backendtrafficpolicies.yaml | Updates generated CRD schema to include name + validations. |
| charts/gateway-crds-helm/templates/generated/gateway.envoyproxy.io_backendtrafficpolicies.yaml | Updates generated CRD schema to include name + validations. |
| api/v1alpha1/zz_generated.deepcopy.go | Ensures RateLimitRule.Name is deep-copied. |
| api/v1alpha1/ratelimit_types.go | Adds RateLimitRule.Name and related kubebuilder + CEL validations. |
Files not reviewed (1)
- api/v1alpha1/zz_generated.deepcopy.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| | Field | Type | Required | Default | Description | | ||
| | --- | --- | --- | --- | --- | | ||
| | `name` | _string_ | false | | Name is a user-facing name for this rule that can be used for debugging<br />and observability. When set, the rate limit key will include this name,<br />making it easier to identify in metrics and dashboards.<br />The name must be unique within a policy and should be a stable identifier<br />that won't change when the rule order changes.<br />When name is set, the rate limit key format becomes:<br /><policy-namespace>/<policy-name>/rule/<rule-name><br />When name is not set, the format remains:<br /><policy-namespace>/<policy-name>/rule/<rule-index> | |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79454224d9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Set the Name field as <policy-ns>/<policy-name>/rule/<rule-index> | ||
| irRules[i].Name = irRuleName(policy.Namespace, policy.Name, i) | ||
| // Set the Name field as <policy-ns>/<policy-name>/rule/<rule-name-or-index> | ||
| irRules[i].Name = irRuleName(policy.Namespace, policy.Name, i, rule.Name) |
There was a problem hiding this comment.
For the default global case (shared unset/false), this IR name is never used by the xDS/RLS config: I checked internal/xds/translator/ratelimit.go, where buildRouteRateLimits and addRateLimitDescriptor still choose getRouteDescriptor(route.Name) plus getRouteRuleDescriptor(domainRuleIdx, ...). A policy like the added example with name: basic-limit but no shared: true therefore still emits rule-0-* descriptors and will churn counters/metrics when rules are reordered. Please thread the named key into the non-shared descriptor path, or restrict the API/docs to the shared-only case.
Useful? React with 👍 / 👎.
| // Set the Name field as <policy-ns>/<policy-name>/rule/<rule-index> | ||
| irRule.Name = irRuleName(policy.Namespace, policy.Name, i) | ||
| // Set the Name field as <policy-ns>/<policy-name>/rule/<rule-name-or-index> | ||
| irRule.Name = irRuleName(policy.Namespace, policy.Name, i, rule.Name) |
There was a problem hiding this comment.
This accepts spec.rateLimit.local.rules[].name and stores it in the IR, but I checked internal/xds/translator/local_ratelimit.go and local descriptor/action construction never reads rule.Name; it uses the loop index rIdx for all rule-<n>-* keys. Users can now configure a valid local rule name that has no observable effect and still gets index-based descriptor churn after reordering. Please either use the name in local descriptor generation or disallow/document the field for local rate limits.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9466 +/- ##
==========================================
- Coverage 75.59% 75.55% -0.04%
==========================================
Files 252 252
Lines 41713 41715 +2
==========================================
- Hits 31531 31519 -12
- Misses 8057 8067 +10
- Partials 2125 2129 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
9c984e1 to
793215e
Compare
…lity This enhancement allows users to provide meaningful names for rate limit rules, making rate limit keys more identifiable in metrics and dashboards. API Changes: - Add optional 'name' field to RateLimitRule with validation - Pattern: [a-zA-Z0-9._-]+ (alphanumeric, dots, underscores, hyphens) - Max length: 64 characters - Cannot be purely numeric (CEL validation at field level) - Must be unique within a policy (CEL validation at Rules array level) IR Translation: - When name is set: <policy-ns>/<policy-name>/rule/<rule-name> - When name is not set: <policy-ns>/<policy-name>/rule/<rule-index> (existing behavior) Testing: - Add test case for named rules (including shared rules) - Add CEL validation tests for valid names, duplicates, and numeric names - All existing tests continue to pass Benefits: - Improved observability: meaningful keys in metrics/dashboards - Stability: keys unchanged when rule ordering changes - Backward compatible: optional field preserves existing behavior Resolves: envoyproxy#9151 Signed-off-by: liuhy <liuhongyu@apache.org> Co-Authored-By: Claude <noreply@anthropic.com>
The release note for envoyproxy#9151 previously mentioned only the global rate-limit metric key, but the change to irRuleName affects both the local and global rate-limit rule keys. Update the note to reflect the actual scope. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: liuhy <liuhongyu@apache.org>
The CEL validation test used ptr.To from k8s.io/utils/ptr, which is forbidden by forbidigo (use new() builtin or a typed helper instead). Replace the 7 ptr.To(...) call sites with a small file-local ptrString helper that matches the existing SectionNamePtr/ObjectNamePtr convention, and drop the now-unused ptr import. The new ratelimit-rule-names testdata YAML was missing a trailing newline, which fails yamllint's new-line-at-end-of-file rule (part of make lint / gen-check). Add the trailing newline. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: liuhy <liuhongyu@apache.org>
Regenerate the gateway-crds-helm golden snapshots so they reflect the new optional RateLimitRule.name schema and its CEL validations (non-numeric name, unique names within global/local rule slices) added in api/v1alpha1/ratelimit_types.go. The charts CRD sources were already regenerated; only the test/helm/gateway-crds-helm/*.out.yaml snapshots were stale, causing gen-check (helm-template) to fail. Signed-off-by: liuhy <liuhongyu@apache.org> Co-Authored-By: Claude <noreply@anthropic.com>
f3a6953 to
5d5c7d3
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d5c7d36ae
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Set the Name field as <policy-ns>/<policy-name>/rule/<rule-index> | ||
| irRule.Name = irRuleName(policy.Namespace, policy.Name, i) | ||
| // Set the Name field as <policy-ns>/<policy-name>/rule/<rule-name-or-index> | ||
| irRule.Name = irRuleName(policy.Namespace, policy.Name, i, rule.Name) |
There was a problem hiding this comment.
When a local rule has spec.rateLimit.local.rules[].name, this line copies the name into IR, but the local xDS path never consumes ir.RateLimitRule.Name: buildRouteLocalRateLimits still passes the loop index rIdx into every build*LocalRateLimit* helper, which emits rule-<index>-... descriptor keys (internal/xds/translator/local_ratelimit.go:209-233,269-287). As a result accepted local names do not change local rate-limit keys/metrics, despite the API and release note promising both local and global rule keys use the provided name; either wire the name into those descriptors or reject/document local names as unsupported.
Useful? React with 👍 / 👎.
| // Set the Name field as <policy-ns>/<policy-name>/rule/<rule-index> | ||
| irRules[i].Name = irRuleName(policy.Namespace, policy.Name, i) | ||
| // Set the Name field as <policy-ns>/<policy-name>/rule/<rule-name-or-index> | ||
| irRules[i].Name = irRuleName(policy.Namespace, policy.Name, i, rule.Name) |
There was a problem hiding this comment.
For a global rule without shared: true, setting this IR name still has no effect on the generated RLS keys: buildRouteRateLimits and addRateLimitDescriptor choose getRouteDescriptor(route.Name) for non-shared rules and then derive match keys from getRouteRuleDescriptor(domainRuleIdx, ...) (internal/xds/translator/ratelimit.go:260-298,759-764,856-866). Thus a normal named rule such as by-org still produces route/rule-1-match-* descriptors and loses the stable user name in Envoy/RLS metrics; please use rule.Name in the non-shared descriptor/action path or narrow the API/docs to shared rules.
Useful? React with 👍 / 👎.
Summary
This PR implements the enhancement requested in #9151, adding an optional
namefield toRateLimitRulefor improved observability and debugging.Problem: Current global rate limit keys use array indices (e.g.,
envoy-gateway-system/gateway/rule/0), which are:Solution: Allow users to provide meaningful names for rate limit rules.
Changes
API Changes
namefield toRateLimitRulestruct[a-zA-Z0-9._-]+(alphanumeric, dots, underscores, hyphens)IR Translation
<policy-ns>/<policy-name>/rule/<rule-name><policy-ns>/<policy-name>/rule/<rule-index>(existing behavior)Testing
Example Usage
Generated keys:
default/policy-for-route/rule/basic-limit(uses provided name)default/policy-for-route/rule/premium-limit(uses provided name)default/policy-for-route/rule/2(uses index, no name provided)Benefits
Test Results
✅ All rate limit related tests pass
✅ CEL validation tests pass
✅ Gateway API translation tests pass
✅ XDS translator tests pass
Resolves: #9151