Skip to content

controller: expose remote rate-limit limitOverride - #2839

Merged
howardjohn merged 1 commit into
agentgateway:mainfrom
FenjuFu:agent/expose-rate-limit-override
Aug 5, 2026
Merged

controller: expose remote rate-limit limitOverride#2839
howardjohn merged 1 commit into
agentgateway:mainfrom
FenjuFu:agent/expose-rate-limit-override

Conversation

@FenjuFu

@FenjuFu FenjuFu commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

What changed

  • Expose optional limitOverride on Kubernetes RateLimitDescriptor.
  • Validate the value as CEL and forward it to the existing xDS LimitOverride field.
  • Add positive translation coverage and invalid-CEL status coverage.

Why

Standalone configuration, the internal API, and the data plane already support descriptor-level dynamic limit overrides, but AgentgatewayPolicy currently rejects or prunes the field and cannot translate it.

Validation performed

  • Go files formatted with Go 1.26.5 gofmt.
  • git diff --check passes.
  • Focused package test compilation was attempted twice on Windows, but the first cold build of the Kubernetes/Istio dependency graph exceeded the local 4-minute command limit without returning a test failure.

Draft follow-up

The repository API generator is a Bash pipeline and this Windows environment has no Bash/Make. Generated deepcopy/CRD/client/reference artifacts are therefore not included yet. This draft is intentionally not ready for review until the Linux generator is run and focused tests pass; CI output can also identify the exact generated diff required.

Fixes #2807.

@FenjuFu
FenjuFu marked this pull request as ready for review August 4, 2026 12:06
@FenjuFu
FenjuFu requested a review from a team as a code owner August 4, 2026 12:06
Copilot AI lite review requested due to automatic review settings August 4, 2026 12:06
Copilot AI reviewed Aug 4, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR exposes an optional limitOverride CEL expression on Kubernetes RateLimitDescriptor, validates it as CEL, and forwards it into the existing xDS LimitOverride field during traffic policy translation so descriptor-level dynamic overrides can be used from AgentgatewayPolicy.

Changes:

  • Add limitOverride to the RateLimitDescriptor API type and document its expected evaluated shape.
  • Extend processRateLimitDescriptor translation to validate and map limitOverride into the generated xDS descriptor.
  • Update translation testdata to cover successful mapping and invalid-CEL status messaging.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
controller/pkg/agentgateway/plugins/traffic_plugin.go Adds LimitOverride validation/mapping in remote rate-limit descriptor translation.
controller/pkg/agentgateway/plugins/testdata/trafficpolicy/global-rl.yaml Adds a positive golden case showing limitOverride is forwarded into output.
controller/pkg/agentgateway/plugins/testdata/trafficpolicy/global-rl-invalid-cel.yaml Adds invalid CEL coverage for limitOverride and updates expected status message/output.
controller/api/v1alpha1/agentgateway/agentgateway_policy_types.go Adds the limitOverride field to the Kubernetes API type with documentation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread controller/pkg/agentgateway/plugins/traffic_plugin.go
if !isCEL(*descriptor.LimitOverride) {
errs = append(errs, fmt.Errorf("rate limit descriptor limit override is not a valid CEL expression: %s", *descriptor.LimitOverride))
}
limitOverride = new(string(*descriptor.LimitOverride))
@danehans

danehans commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

@FenjuFu PTAL and resolve the CI failures and copilot review comment.

@FenjuFu
controller: expose rate limit override
91d3bf0 
Regenerate the CRD schema and deepcopy for the new limitOverride field
(controller-gen was not re-run), and correct the global-rl-invalid-cel
golden to the plugin's actual multi-line validation message. Resolves the
controller-test failure (unknown field spec.traffic.rateLimit.global
.descriptors[].limitOverride).

Signed-off-by: FenjuFu <[email protected]>
@FenjuFu
FenjuFu force-pushed the agent/expose-rate-limit-override branch from 3e46ee1 to 91d3bf0 Compare August 5, 2026 06:56
@FenjuFu

FenjuFu commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

@danehans thanks for the nudge — pushed a fix. Summary of what was failing and why:

controller-test — The new limitOverride field was added to RateLimitDescriptor in the API types, but the generated artifacts weren't regenerated, so the CRD schema didn't know the field and the validator rejected it (unknown field spec.traffic.rateLimit.global.descriptors[].limitOverride). Ran the codegen and committed the results:

  • regenerated the CRD (go run ./controller/hack/crdgen) — adds limitOverride to the schema
  • regenerated the deepcopy block for the new *CELExpression pointer in zz_generated.deepcopy.go (it was missing, so the field was shallow-copied)
  • refreshed the global-rl-invalid-cel golden to the plugin's actual multi-line validation message (it was hand-written with the two CEL errors joined by ; ; the plugin emits them newline-separated)

TestTrafficPolicies/global-rl and /global-rl-invalid-cel pass locally after this.

DCO — added the Signed-off-by trailer (the commit had none).

Copilot comment on new(string(...)) — I believe this one is a false positive: new(expr) with a value is valid as of the Go 1.26 language (new(string(*descriptor.LimitOverride)) builds cleanly under go 1.26, which is what controller/go.mod declares), and it matches the existing idiom already used a few times in traffic_plugin.go on main (e.g. new(string(*descriptor.Cost))). So I left it as-is for consistency — happy to switch both to ptr.To(...) in a separate cleanup if you'd prefer that idiom repo-wide.

PTAL when you get a chance.

@howardjohn
howardjohn enabled auto-merge (squash) August 5, 2026 16:17
@howardjohn
howardjohn merged commit 375951f into agentgateway:main Aug 5, 2026
10 checks passed
@artberger artberger mentioned this pull request Aug 27, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@howardjohn howardjohn howardjohn approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AgentgatewayPolicy: expose remote rate-limit limitOverride

4 participants

@FenjuFu @danehans @howardjohn