-
Notifications
You must be signed in to change notification settings - Fork 520
enhancement: support setting the timeout of ext_auth and ext_proc #11295
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…ices Signed-off-by: MayorFaj <[email protected]>
Signed-off-by: MayorFaj <[email protected]>
There was a problem hiding this comment.
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 adds support for configurable timeouts for external authorization and processing by introducing a new Timeout field in API types, Helm chart templates, and internal translation logic.
- Adds a new Timeout field and corresponding WithTimeout methods to ExtAuthProviderApplyConfiguration and ExtProcProviderApplyConfiguration.
- Updates the Helm chart to include timeout defaults and validation patterns.
- Modifies internal translation logic to apply timeout values for ExtAuth and ExtProc services.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
internal/kgateway/extensions2/plugins/trafficpolicy/traffic_policy_plugin.go | Adds timeout parsing and logging logic for ExtAuth and ExtProc services |
install/helm/kgateway-crds/templates/gateway.kgateway.dev_gatewayextensions.yaml | Introduces timeout field in the CRD schema with default value and pattern |
api/v1alpha1/gateway_extensions_types.go | Adds Timeout field to ExtAuthProvider and ExtProcProvider API types |
api/applyconfiguration/internal/internal.go | Updates internal schema to include timeout as a scalar string |
api/applyconfiguration/api/v1alpha1/extprocprovider.go | Adds Timeout field and WithTimeout method for ExtProcProviderApplyConfiguration |
api/applyconfiguration/api/v1alpha1/extauthprovider.go | Adds Timeout field and WithTimeout method for ExtAuthProviderApplyConfiguration |
internal/kgateway/extensions2/plugins/trafficpolicy/traffic_policy_plugin.go
Outdated
Show resolved
Hide resolved
internal/kgateway/extensions2/plugins/trafficpolicy/traffic_policy_plugin.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks -- took a quick peek at the changes and had a question/comment. For testing, I think we can get away with adding some simple setup test cases and don't need e2e coverage.
// Timeout for requests to the external processing service. | ||
// +optional | ||
// +kubebuilder:default="200ms" | ||
Timeout gwv1.Duration `json:"timeout,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does a timeout field belong on the shared ExtGrpcService type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea it does, however, diff service types needs diff default timeouts.
(rate limiting: 20ms, ext_auth and proc : 200ms). won't give room for flexibility
Signed-off-by: MayorFaj <[email protected]>
…ders Signed-off-by: MayorFaj <[email protected]>
Signed-off-by: MayorFaj <[email protected]>
Bumping branch due to ubuntu outage. |
internal/kgateway/extensions2/plugins/trafficpolicy/traffic_policy_plugin.go
Show resolved
Hide resolved
…age processing Signed-off-by: MayorFaj <[email protected]>
…roc listeners Signed-off-by: MayorFaj <[email protected]>
Signed-off-by: MayorFaj <[email protected]>
// +kubebuilder:default="200ms" | ||
// +kubebuilder:validation:XValidation:rule="duration(self) >= duration('0s')",message="timeout must be a valid duration string" | ||
// +kubebuilder:validation:XValidation:rule="duration(self) >= duration('1ms')",message="timeout must be at least 1 millisecond" | ||
Timeout metav1.Duration `json:"timeout,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in line with this comment from @timflannagan
I'm a bit confused as to why we wouldn't the shared ExtGrpcService
type to have Timeout
as a field?
That way all extensions which can reference an ExtGrpcService
can configure the timeout if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @lgadban what should we then set the default value to ?200ms or 20ms, since the rate limit has a diff default and it is referencing same service
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @MayorFaj thanks for the reminder, this fell off my radar, apologies for the delay!
My suggestion here is actually to treat this field as completely optional and not provide a default, relying on the default envoy behavior. If a user needs to opt-in and set a timeout, they can do so contextually based on the usage of the ExtGrpService
i.e. if it's rate limit or extauth.
That being said, it's actually not obvious to me if timeout
is a required field or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is optional, not required.
Description
Change Type
Changelog
This pull request introduces support for configuring timeouts for external authorization and processing services in the Gateway API. The changes include updates to the API types, Helm chart templates, and internal translation logic to handle the new
Timeout
field.This pull request introduces support for configuring timeouts for external services in the Gateway API, including external authorization, processing, and rate-limiting providers. The changes add timeout fields with validation rules, default values, and corresponding tests to ensure proper behavior.
Timeout Configuration for External Services
API Updates
Timeout
field of typemetav1.Duration
toExtAuthProvider
,ExtProcProvider
, andRateLimitProvider
structs, with default values and validation rules ensuring a minimum of 1 millisecond. (api/v1alpha1/gateway_extensions_types.go
) [1] [2]ExtAuthProviderApplyConfiguration
,ExtProcProviderApplyConfiguration
) to include theTimeout
field and correspondingWithTimeout
chaining methods. (api/applyconfiguration/api/v1alpha1/extauthprovider.go
,api/applyconfiguration/api/v1alpha1/extprocprovider.go
) [1] [2] [3] [4]Schema and Validation
timeout
field with validation rules and default values for external services. (install/helm/kgateway-crds/templates/gateway.kgateway.dev_gatewayextensions.yaml
) [1] [2] [3]Timeout
field as ametav1.Duration
. (api/applyconfiguration/internal/internal.go
) [1] [2] [3]DeepCopy and Tests
DeepCopy
functions to handle the newTimeout
field. (api/v1alpha1/zz_generated.deepcopy.go
) [1] [2] [3]Timeout
field, including default values and explicit configurations. (internal/kgateway/extensions2/plugins/trafficpolicy/extauth_policy_test.go
,internal/kgateway/extensions2/plugins/trafficpolicy/extproc_policy_test.go
,internal/kgateway/extensions2/plugins/trafficpolicy/global_rate_limit_plugin_test.go
) [1] [2] [3]Traffic Policy Integration
Timeout
field into traffic policy plugins for external authorization, processing, and rate-limiting services, ensuring that the timeout is correctly propagated to Envoy configurations. (internal/kgateway/extensions2/plugins/trafficpolicy/traffic_policy_plugin.go
)closes #11291