feat: add timeout to extauth in agentgw #13202
Conversation
Signed-off-by: Yuval Kohavi <[email protected]>
gateway-bot
added
kind/feature
There was a problem hiding this comment.
Pull request overview
This PR adds a timeout field to the ExtAuth policy in the agent gateway, allowing users to configure the timeout for external authorization requests from the gateway to the external auth server.
- Added
Timeoutfield toExtAuthstruct with validation rules - Implemented timeout processing in the traffic plugin to convert the timeout to protobuf duration
- Updated CRD templates with the new timeout field schema and validation
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| api/v1alpha1/agentgateway/agentgateway_policy_types.go | Added Timeout field to ExtAuth struct with validation rules; also modified existing Timeouts.Request minimum validation from 100ms to 1ms |
| api/v1alpha1/agentgateway/zz_generated.deepcopy.go | Added DeepCopyInto implementation for the new Timeout field in ExtAuth |
| pkg/agentgateway/plugins/traffic_plugin.go | Added logic to process and set the timeout in the ExtAuth policy spec |
| install/helm/agentgateway-crds/templates/agentgateway.dev_agentgatewaypolicies.yaml | Added timeout field schema with validation rules to the CRD; also updated existing request timeout minimum validation |
| pkg/agentgateway/plugins/testdata/trafficpolicy/http-extauth-with-timeout.yaml | Added comprehensive test case demonstrating the timeout functionality with expected output |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rule: matches(self, '^([0-9]{1,5}(h|m|s|ms)){1,4}$') | ||
| - message: request must be at least 1ms | ||
| rule: duration(self) >= duration('100ms') | ||
| rule: duration(self) >= duration('1ms') |
There was a problem hiding this comment.
This change modifies the minimum timeout validation from 100ms to 1ms for the existing Timeouts.Request field. While this aligns with the new ExtAuth.Timeout validation rules, this appears to be a breaking change that is not mentioned in the PR description. If this is intentional, it should be documented in the PR description and changelog. If this is a bug fix for an incorrect previous validation, it should be clearly stated.
| // | ||
| // +kubebuilder:validation:XValidation:rule="matches(self, '^([0-9]{1,5}(h|m|s|ms)){1,4}$')",message="invalid duration value" | ||
| // +kubebuilder:validation:XValidation:rule="duration(self) >= duration('100ms')",message="request must be at least 1ms" | ||
| // +kubebuilder:validation:XValidation:rule="duration(self) >= duration('1ms')",message="request must be at least 1ms" |
There was a problem hiding this comment.
This change modifies the minimum timeout validation from 100ms to 1ms for the existing Timeouts.Request field. While this aligns with the new ExtAuth.Timeout validation rules, this appears to be a breaking change that is not mentioned in the PR description. If this is intentional, it should be documented in the PR description and changelog. If this is a bug fix for an incorrect previous validation, it should be clearly stated.
| // +kubebuilder:validation:XValidation:rule="duration(self) >= duration('1ms')",message="request must be at least 1ms" | |
| // +kubebuilder:validation:XValidation:rule="duration(self) >= duration('100ms')",message="request must be at least 100ms" |
There was a problem hiding this comment.
it's not a breaking change - all past configs will still work. it might an unrelated change.
| // | ||
| // +kubebuilder:validation:XValidation:rule="matches(self, '^([0-9]{1,5}(h|m|s|ms)){1,4}$')",message="invalid duration value" | ||
| // +kubebuilder:validation:XValidation:rule="duration(self) >= duration('100ms')",message="request must be at least 1ms" | ||
| // +kubebuilder:validation:XValidation:rule="duration(self) >= duration('1ms')",message="request must be at least 1ms" |
There was a problem hiding this comment.
note: also fixed the validation here
|
This is not the approach we want (imo). Backend newly has a timeout field (I think needs kgateway implementation) which will control this on the ext authz backend. envoy needs to replicate a bunch of fields like retry, timeout, etc since all of these are route level or listener level configs, so they end up haphazardly copied on each filter. Instead in agentgateway we will allow these to natively be on the backend. |
ymesika
left a comment
ymesika
left a comment
There was a problem hiding this comment.
Reading John's comment I think it should be revisited
ymesika
dismissed
their stale review
Reading John's comment I think it should be revisited
@howardjohn i noticed the extauth proto does have a timeout field already in it - is the idea that the backend timeout will the default and will be overridable? |
|
i think i see what you mean in the code. i can add timeout to backend policy (it will be sibling of http version). but whould this timeout in the extauth policy remain? both are configurable in the data plane |
Signed-off-by: Yuval Kohavi <[email protected]>
|
The ext auth timeout field should be removed from the proto (it was never used) |
This reverts commit 3c6647e. Signed-off-by: Yuval Kohavi <[email protected]>
|
ok changed the impl and added request timeout to backend policy |
Description
Add timout field to extauth policy in agent gateway
Change Type
/kind feature
Changelog