feat: circuit breakers #12957
feat: circuit breakers #12957
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 circuit breaker support to the BackendConfigPolicy CRD, allowing users to configure Envoy circuit breaker thresholds (maxConnections, maxPendingRequests, maxRequests, maxRetries) for backend services. The implementation follows established patterns in the codebase for similar backend configuration options like outlier detection.
Key Changes:
- Added
CircuitBreakerstype to the BackendConfigPolicy API with CEL validation requiring at least one threshold field - Implemented translation from API types to Envoy CircuitBreakers configuration
- Added comprehensive unit tests and integration test cases for minimal and full configurations
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| api/v1alpha1/backend_config_policy_types.go | Defines CircuitBreakers struct with four optional int32 fields and XValidation rule |
| api/v1alpha1/zz_generated.deepcopy.go | Auto-generated DeepCopy methods for CircuitBreakers type |
| install/helm/kgateway-crds/templates/gateway.kgateway.dev_backendconfigpolicies.yaml | CRD schema with circuit breaker field definitions and validation rules |
| internal/kgateway/extensions2/plugins/backendconfigpolicy/circuitbreakers.go | Translation logic converting API types to Envoy protobuf messages |
| internal/kgateway/extensions2/plugins/backendconfigpolicy/plugin.go | Integration of circuit breakers into the plugin lifecycle with validation |
| internal/kgateway/extensions2/plugins/backendconfigpolicy/plugin_test.go | Unit tests for minimal and full circuit breaker configurations |
| internal/kgateway/translator/gateway/gateway_translator_test.go | Integration test cases for circuit breaker scenarios |
| internal/kgateway/translator/gateway/testutils/inputs/backendconfigpolicy/circuitbreakers-*.yaml | Test input manifests for minimal and full circuit breaker configurations |
| internal/kgateway/translator/gateway/testutils/outputs/backendconfigpolicy/circuitbreakers-*.yaml | Expected Envoy configuration output for circuit breaker test cases |
| examples/example-backendconfigpolicy-circuit-breakers.yaml | Example manifest demonstrating circuit breaker usage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| minimum: 0 | ||
| type: integer | ||
| maxPendingRequests: | ||
| description: |- | ||
| MaxPendingRequests is the maximum number of pending requests that are | ||
| allowed to the upstream cluster. If not specified, defaults to 1024. | ||
| format: int32 | ||
| minimum: 0 | ||
| type: integer | ||
| maxRequests: | ||
| description: |- | ||
| MaxRequests is the maximum number of parallel requests that are allowed | ||
| to the upstream cluster. If not specified, defaults to 1024. | ||
| format: int32 | ||
| minimum: 0 |
There was a problem hiding this comment.
The validation rules are inconsistent between the API types and the generated CRD. The API types specify Minimum=1 for MaxConnections, MaxPendingRequests, and MaxRequests (lines 114, 120, 126 in backend_config_policy_types.go), but the CRD has minimum: 0 for these same fields (lines 66, 73, 80 in the CRD file).
This inconsistency means the CRD will accept values of 0 for these fields, which would bypass the intended validation. The CRD should be regenerated to match the API types, or the API types should be updated to use Minimum=0 if that's the intended behavior. Based on Envoy's circuit breaker semantics, a minimum of 1 makes more sense for connections, pending requests, and parallel requests.
| minimum: 0 | |
| type: integer | |
| maxPendingRequests: | |
| description: |- | |
| MaxPendingRequests is the maximum number of pending requests that are | |
| allowed to the upstream cluster. If not specified, defaults to 1024. | |
| format: int32 | |
| minimum: 0 | |
| type: integer | |
| maxRequests: | |
| description: |- | |
| MaxRequests is the maximum number of parallel requests that are allowed | |
| to the upstream cluster. If not specified, defaults to 1024. | |
| format: int32 | |
| minimum: 0 | |
| minimum: 1 | |
| type: integer | |
| maxPendingRequests: | |
| description: |- | |
| MaxPendingRequests is the maximum number of pending requests that are | |
| allowed to the upstream cluster. If not specified, defaults to 1024. | |
| format: int32 | |
| minimum: 1 | |
| type: integer | |
| maxRequests: | |
| description: |- | |
| MaxRequests is the maximum number of parallel requests that are allowed | |
| to the upstream cluster. If not specified, defaults to 1024. | |
| format: int32 | |
| minimum: 1 |
|
|
||
| // CircuitBreakers contains the options to configure circuit breaker thresholds for the default priority. | ||
| // See [Envoy documentation](https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/cluster/v3/circuit_breaker.proto) for more details. | ||
| // +kubebuilder:validation:XValidation:rule="has(self.maxConnections) || has(self.maxPendingRequests) || has(self.maxRequests) || has(self.maxRetries)",message="At least one circuit breaker threshold must be specified" |
There was a problem hiding this comment.
| // +kubebuilder:validation:XValidation:rule="has(self.maxConnections) || has(self.maxPendingRequests) || has(self.maxRequests) || has(self.maxRetries)",message="At least one circuit breaker threshold must be specified" | |
| // +kubebuilder:validation:AtLeastOneOf=maxConnections,maxPendingRequests,maxRequests,maxRetries |
|
mostly LGTM, once CI passing |
Signed-off-by: Yuval Kohavi <[email protected]>
|
IMO we should disable the default circuit breakers by default too |
Description
Add circuit breakers to BackendConfigPolicy. claude did most of the work.
Change Type
Changelog