Skip to content

Conversation

@hawkw
Copy link
Contributor

@hawkw hawkw commented Jun 12, 2023

This emits an unused_mut warning on the latest nigthly, fixing the fuzzing and codecov CI builds. This commit removes it.

This emits an `unused_mut` warning on the latest nigthly, fixing the
fuzzing and codecov CI builds. This commit removes it.
@hawkw hawkw requested a review from a team as a code owner June 12, 2023 14:03
@hawkw hawkw merged commit 864a5db into main Jun 12, 2023
@hawkw hawkw deleted the eliza/fix-warning branch June 12, 2023 18:45
risingspiral pushed a commit to linkerd/linkerd2 that referenced this pull request Jun 15, 2023
PRs #2418 and #2419 add per-route and per-backend request timeouts
configured by the `OutboundPolicies` API to the `MatchedRoute` and
`MatchedBackend` layers in the outbound `ClientPolicy` stack,
respectively. This means that — unlike in the `ServiceProfile` stack —
two separate request timeouts can be configured in `ClientPolicy`
stacks. However, because both the `MatchedRoute` and `MatchedBackend`
layers are in the HTTP logical stack, the errors emitted by both
timeouts will have a `LogicalError` as their most specific error
metadata, meaning that the log messages and `l5d-proxy-error` headers
recorded for these timeouts do not indicate whether the timeout that
failed the request was the route request timeout or the backend request
timeout.

In order to ensure this information is recorded and exposed to the user,
this branch adds two new error wrapper types, one of which enriches an
error with a `RouteRef`'s metadata, and one of which enriches an error
with a `BackendRef`'s metadata. The `MatchedRoute` stack now wraps all
errors with `RouteRef` metadata, and the `MatchedBackend` stack wraps
errors with `BackendRef` metadata. This way, when the route timeout
fails a request, the error will include the route metadata, while when
the backend request timeout fails a request, the error will include both
the route and backend metadata.

Adding these new error wrappers also has the additional side benefit of
adding this metadata to errors returned by filters, allowing users to
distinguish between errors emitted by a filter on a route rule and
errors emitted by a per-backend filter. Also, any other errors emitted
lower in the stack for requests that are handled by a client policy
stack will now also include this metadata, which seems generally useful.

Example errors, taken from a proxy unit test:

backend request:
```
logical service logical.test.svc.cluster.local:666: route httproute.test.timeout-route: backend service.test.test-svc:666: HTTP response timeout after 1s
```
route request:
```
logical service logical.test.svc.cluster.local:666: route httproute.test.timeout-route: HTTP response timeout after 2s
```

---

* recover: remove unused `mut` (linkerd/linkerd2-proxy#2425)
* outbound: implement `OutboundPolicies` route request timeouts (linkerd/linkerd2-proxy#2418)
* build(deps): bump tj-actions/changed-files from 35.7.7 to 36.2.1 (linkerd/linkerd2-proxy#2427)
* outbound: implement `OutboundPolicies` backend request timeouts (linkerd/linkerd2-proxy#2419)
* outbound: add backend and route metadata to errors (linkerd/linkerd2-proxy#2428)

Signed-off-by: Eric Anderson <[email protected]>
risingspiral pushed a commit to linkerd/linkerd2 that referenced this pull request Jun 15, 2023
Updating proxy version on main

PRs #2418 and #2419 add per-route and per-backend request timeouts
configured by the `OutboundPolicies` API to the `MatchedRoute` and
`MatchedBackend` layers in the outbound `ClientPolicy` stack,
respectively. This means that — unlike in the `ServiceProfile` stack —
two separate request timeouts can be configured in `ClientPolicy`
stacks. However, because both the `MatchedRoute` and `MatchedBackend`
layers are in the HTTP logical stack, the errors emitted by both
timeouts will have a `LogicalError` as their most specific error
metadata, meaning that the log messages and `l5d-proxy-error` headers
recorded for these timeouts do not indicate whether the timeout that
failed the request was the route request timeout or the backend request
timeout.

In order to ensure this information is recorded and exposed to the user,
this branch adds two new error wrapper types, one of which enriches an
error with a `RouteRef`'s metadata, and one of which enriches an error
with a `BackendRef`'s metadata. The `MatchedRoute` stack now wraps all
errors with `RouteRef` metadata, and the `MatchedBackend` stack wraps
errors with `BackendRef` metadata. This way, when the route timeout
fails a request, the error will include the route metadata, while when
the backend request timeout fails a request, the error will include both
the route and backend metadata.

Adding these new error wrappers also has the additional side benefit of
adding this metadata to errors returned by filters, allowing users to
distinguish between errors emitted by a filter on a route rule and
errors emitted by a per-backend filter. Also, any other errors emitted
lower in the stack for requests that are handled by a client policy
stack will now also include this metadata, which seems generally useful.

Example errors, taken from a proxy unit test:

backend request:
```
logical service logical.test.svc.cluster.local:666: route httproute.test.timeout-route: backend service.test.test-svc:666: HTTP response timeout after 1s
```
route request:
```
logical service logical.test.svc.cluster.local:666: route httproute.test.timeout-route: HTTP response timeout after 2s
```

---

* recover: remove unused `mut` (linkerd/linkerd2-proxy#2425)
* outbound: implement `OutboundPolicies` route request timeouts (linkerd/linkerd2-proxy#2418)
* build(deps): bump tj-actions/changed-files from 35.7.7 to 36.2.1 (linkerd/linkerd2-proxy#2427)
* outbound: implement `OutboundPolicies` backend request timeouts (linkerd/linkerd2-proxy#2419)
* outbound: add backend and route metadata to errors (linkerd/linkerd2-proxy#2428)

Signed-off-by: Eric Anderson <[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