Skip to content

Add timeout to HttpRoute CRD and bindings#10969

Merged
adleong merged 3 commits intomainfrom
alex/timeout-crds
Jun 1, 2023
Merged

Add timeout to HttpRoute CRD and bindings#10969
adleong merged 3 commits intomainfrom
alex/timeout-crds

Conversation

@adleong
Copy link
Member

@adleong adleong commented May 31, 2023

Add a new version to the HttpRoute CRD: v1beta3. This version adds a new timeouts struct to the http route rule. This mirrors a corresponding new field in the Gateway API, as described in GEP-1742. This field is currently unused, but will eventually be read by the policy controller and used to configure timeouts enforced by the proxy.

The diff between v1beta2 and v1beta3 is:

                    timeouts:
                      description: "Timeouts defines the timeouts that can be configured
                        for an HTTP request. \n Support: Core \n <gateway:experimental>"
                      properties:
                        backendRequest:
                          description: "BackendRequest specifies a timeout for an
                            individual request from the gateway to a backend service.
                            Typically used in conjunction with automatic retries,
                            if supported by an implementation. Default is the value
                            of Request timeout. \n Support: Extended"
                          format: duration
                          type: string
                        request:
                          description: "Request specifies a timeout for responding
                            to client HTTP requests, disabled by default. \n For example,
                            the following rule will timeout if a client request is
                            taking longer than 10 seconds to complete: \n ``` rules:
                            - timeouts: request: 10s backendRefs: ... ``` \n Support:
                            Core"
                          format: duration
                          type: string
                      type: object

We update the storage version of HttpRoute to be v1beta3 but continue to serve all versions. Since this new field is optional, the Kubernetes API will be able to automatically convert between versions.

@adleong adleong requested a review from a team as a code owner May 31, 2023 23:09
Signed-off-by: Alex Leong <[email protected]>
Copy link
Contributor

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Looks good to me! Once this has merged, I'll rebase my branch that adds the controller side implementation on top of it.

@hawkw
Copy link
Contributor

hawkw commented Jun 1, 2023

Rebased my branch #10975 onto this.

hawkw added a commit to linkerd/linkerd2-proxy-api that referenced this pull request Jun 1, 2023
This branch adds fields to the `OutboundPolicies` API messages for
configuring request timeouts. The `HttpRoute.Rule` and `GrpcRoute.Rule`
messages now contain a `requestTimeout` field, which contains a
`Duration` for a request timeout for that rule. In addition, the
`RouteBackend` messages also contain a `requestTimeout` field, which
configures a duration for requests dispatched to that specific backend.
All new fields have comments describing their intended semantics.

These fields are primarily intended to support the new timeout fields
added to the HTTPRoute CRD in GEP-1742 (see linkerd/linkerd2#10969). The
HTTPRoute timeout configuration are an additional field on the `rule`
fields in a route that contains optional `request` and `backendRequest`
timeouts. This means that a single request timeout and backend request
timeout is set for all backends on an individual route rule. 

The proxy API additions in this PR are somewhat more flexible than the
current HTTPRoute CRD, as each `RouteBackend` has its *own* field for
the `backendRequest` timeout. This means that in theory, independent
`backendRequest` timeout values could be configured for each individual
backend. HTTPRoute doesn't currently support this, but it felt like a
good idea to add this flexibility in the proxy API regardless, to
support future use cases.
@hawkw hawkw requested a review from a team June 1, 2023 21:53
@adleong adleong merged commit 2303788 into main Jun 1, 2023
@adleong adleong deleted the alex/timeout-crds branch June 1, 2023 22:02
hawkw added a commit that referenced this pull request Jun 2, 2023
PR #10969 adds support for the GEP-1742 `timeouts` field to the
HTTPRoute CRD. This branch implements actual support for these fields in
the policy controller. The timeout fields are now read and used to set
the timeout fields added to the proxy-api in
linkerd/linkerd2-proxy-api#243.

In addition, I've added code to ensure that the timeout fields are
parsed correctly when a JSON manifest is deserialized. The current
implementation represents timeouts in the bindings as a Rust
`std::time::Duration` type. `Duration` does implement
`serde::Deserialize` and `serde::Serialize`, but its serialization
implementation attempts to (de)serialize it as a struct consisting of a
number of seconds and a number of subsecond nanoseconds. The timeout
fields are instead supposed to be represented as strings in the Go
standard library's `time.ParseDuration` format. Therefore, I've added a
newtype which wraps the Rust `std::time::Duration` and implements the
same parsing logic as Go. Eventually, I'd like to upstream the
implementation of this to `kube-rs`; see kube-rs/kube#1222 for details.

Depends on #10969
Depends on linkerd/linkerd2-proxy-api#243

Signed-off-by: Eliza Weisman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants