Skip to content
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

apis: add implementation for GEP-1731 HTTPRoute Retries #3301

Merged
Merged
63 changes: 63 additions & 0 deletions apis/applyconfiguration/apis/v1/httprouteretry.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions apis/applyconfiguration/apis/v1/httprouterule.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 18 additions & 0 deletions apis/applyconfiguration/internal/internal.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions apis/applyconfiguration/utils.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

104 changes: 104 additions & 0 deletions apis/v1/httproute_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,14 @@ type HTTPRouteRule struct {
// +optional
Timeouts *HTTPRouteTimeouts `json:"timeouts,omitempty"`

// Retry defines the configuration for when to retry an HTTP request.
//
// Support: Extended
//
// +optional
// <gateway:experimental>
Retry *HTTPRouteRetry `json:"retry,omitempty"`

// SessionPersistence defines and configures session persistence
// for the route rule.
//
Expand Down Expand Up @@ -361,6 +369,102 @@ type HTTPRouteTimeouts struct {
BackendRequest *Duration `json:"backendRequest,omitempty"`
}

// HTTPRouteRetry defines retry configuration for an HTTPRoute.
//
// Implementations SHOULD retry on connection errors (disconnect, reset, timeout,
// TCP failure) if a retry stanza is configured.
Comment on lines +374 to +375
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to make this configurable?

Copy link
Contributor Author

@mikemorris mikemorris Aug 28, 2024

Choose a reason for hiding this comment

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

Primarily because there's way too much variance in how different implementations define, default, bucket or even allow these to be configurable for this to be generally implementable within extended conformance, and secondarily because it's a simpler UX for what is often a good practice.

Copy link
Contributor

Choose a reason for hiding this comment

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

FTR I think that makes sense, @mikemorris.

type HTTPRouteRetry struct {
// Codes defines the HTTP response status codes for which a backend request
// should be retried.
//
// Support: Extended
//
// +optional
Codes []HTTPRouteRetryStatusCode `json:"codes,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

If at some point we want to include strings here, do we add a new Reasons []string field?

Copy link
Contributor Author

@mikemorris mikemorris Aug 28, 2024

Choose a reason for hiding this comment

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

Exactly what I had been thinking if that we end up wanting that in the future! Per above comment on connection error variance between implementations, a Reasons []string field may need to be implementation-specific, and could potentially even enable use cases like unsetting default behavior with a string like google.com/no-retry-connection-errors or something.


// Attempts specifies the maxmimum number of times an individual request
// from the gateway to a backend should be retried.
//
// If the maximum number of retries has been attempted without a successful
// response from the backend, the Gateway MUST return an error.
//
// When this field is unspecified, the number of times to attempt to retry
// a backend request is implementation-specific.
//
// Support: Extended
//
// +optional
Attempts *int `json:"attempts,omitempty"`

// Backoff specifies the minimum duration a Gateway should wait between
// retry attempts and is represented in Gateway API Duration formatting.
//
// For example, setting the `rules[].retry.backoff` field to the value
// `100ms` will cause a backend request to first be retried approximately
// 100 milliseconds after timing out or receiving a response code configured
// to be retryable.
//
// An implementation MAY use an exponential or alternative backoff strategy
// for subsequent retry attempts, MAY cap the maximum backoff duration to
// some amount greater than the specified minimum, and MAY add arbitrary
// jitter to stagger requests, as long as unsuccessful backend requests are
// not retried before the configured minimum duration.
//
// If a Request timeout (`rules[].timeouts.request`) is configured on the
// route, the entire duration of the initial request and any retry attempts
// MUST not exceed the Request timeout duration. If any retry attempts are
// still in progress when the Request timeout duration has been reached,
// these SHOULD be canceled if possible and the Gateway MUST immediately
// return a timeout error.
//
// If a BackendRequest timeout (`rules[].timeouts.backendRequest`) is
// configured on the route, any retry attempts which reach the configured
// BackendRequest timeout duration without a response SHOULD be canceled if
// possible and the Gateway should wait for at least the specified backoff
// duration before attempting to retry the backend request again.
//
// If a BackendRequest timeout is _not_ configured on the route, retry
// attempts MAY time out after an implementation default duration, or MAY
// remain pending until a configured Request timeout or implementation
// default duration for total request time is reached.
//
// When this field is unspecified, the time to wait between retry attempts
// is implementation-specific.
//
// Support: Extended
//
// +optional
Backoff *Duration `json:"backoff,omitempty"`
}

// HTTPRouteRetryStatusCode defines an HTTP response status code for
// which a backend request should be retried.
//
// Implementations MUST support the following status codes as retryable:
//
// * 500
// * 502
// * 503
// * 504
//
// Implementations MAY support specifying additional discrete values in the
// 500-599 range.
//
// Implementations SHOULD NOT support retrying status codes in the 100-399
// range, as these responses are generally not appropriate to retry.
mikemorris marked this conversation as resolved.
Show resolved Hide resolved
//
// Implementations MAY support specifying discrete values in the 400-499 range,
// which are often inadvisable to retry.
//
// Implementations MAY support discrete values in the 600-999 (inclusive)
// range, which are not valid for HTTP clients, but are sometimes used for
// communicating application-specific errors.
mikemorris marked this conversation as resolved.
Show resolved Hide resolved
mikemorris marked this conversation as resolved.
Show resolved Hide resolved
//
// +kubebuilder:validation:Minimum:=400
// +kubebuilder:validation:Maximum:=599
mikemorris marked this conversation as resolved.
Show resolved Hide resolved
// <gateway:experimental>
type HTTPRouteRetryStatusCode int

// PathMatchType specifies the semantics of how HTTP paths should be compared.
// Valid PathMatchType values, along with their support levels, are:
//
Expand Down
35 changes: 35 additions & 0 deletions apis/v1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading