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.

Sign up for GitHub

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

Jump to bottom

Set grpc-status headers on dispatch errors #416

Merged
merged 4 commits into from
Jan 28, 2020
Merged

Set grpc-status headers on dispatch errors #416

merged 4 commits into from
Jan 28, 2020

Conversation

olix0r
Copy link
Member

@olix0r olix0r commented Jan 27, 2020

When the proxy encounters errors, it sets the HTTP response code.

However, gRPC applications do not honor HTTP response codes and,
instead, rely on the grpc-status response header to be set. Since the
proxy has specialized gRPC support in some other places (namely
metrics/classification), we should attempt to behave well with gRPC
applications.

This change extracts the errors module from app-core into a new
error-respond crate, which exposes a simplified trait interface.
app::core::errors now provides an implementation for these traits that
is able to translate proxy-internal errors to gRPC status codes and
messages.

Fixes linkerd/linkerd2#3493

@olix0r
Set grpc-status headers on dispatch errors
062bd59 
When the proxy encounters errors, it sets the HTTP response code.

However, gRPC applications do not honor HTTP response codes and,
instead, rely on the `grpc-status` response header to be set. Since the
proxy has specialized gRPC support in some other places (namely
metrics/classification), we should attempt to behave well with gRPC
applications.

This change extracts the `errors` module from app-core into a new
`error-respond` crate, which exposes a simplified trait interface.
`app::core::errors` now provides an implementation for these traits that
is able to translate proxy-internal errors to gRPC status codes and
messages.

Fixes linkerd/linkerd2#3493
@olix0r olix0r requested a review from a team January 27, 2020 23:17
@olix0r olix0r self-assigned this Jan 27, 2020
@olix0r olix0r mentioned this pull request Jan 28, 2020
Merged
hawkw
hawkw approved these changes Jan 28, 2020
View reviewed changes
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.

lgtm — i don't think any of the minor nits i commented on are critical

linkerd/app/core/src/errors.rs Outdated Show resolved Hide resolved
linkerd/app/core/src/errors.rs Outdated Show resolved Hide resolved
linkerd/app/core/src/errors.rs Outdated Show resolved Hide resolved
kleimkuhler
kleimkuhler approved these changes Jan 28, 2020
View reviewed changes
linkerd/error-respond/src/lib.rs Outdated
@@ -0,0 +1,89 @@
//! Layer to map service errors into responss
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: responses

linkerd/app/core/src/errors.rs
);
code
} else if error.is::<IdentityRequired>() {
let code = Code::FailedPrecondition;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think anything relied on the Forbidden status code, but is this changing the error type for required tap identity? Before in require_identity_on_endpoint, if the condition failed we returned an error::StatusError with a Status = Forbidden. Now it's a gRPC status code FailedPrecondition

Copy link
Member Author

Choose a reason for hiding this comment

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

It's still a FORBIDDEN for HTTP requests; but is now a FailedPrecondition for gRPC requests.. reading through the descriptions i thought this was the closest ft.

@olix0r olix0r merged commit 3567471 into master Jan 28, 2020
olix0r added a commit to linkerd/linkerd2 that referenced this pull request Feb 4, 2020
@olix0r
proxy: v2.85.0
9112d8c 
This release fixes a bug in the proxy's logging subsystem that could
cause the proxy to consume memory until the process is OOMKilled,
especially when the proxy was configured to log diagnostic information.

The proxy also now properly emits `grpc-status` headers when signaling
proxy errors to gRPC clients.

This release upgrades the proxy's Rust version, the `http` crate
dependency to address RUSTSEC-2019-0033 and RUSTSEC-2019-0034, and the
`prost` crate dependency has been patched to address RUSTSEC-2020-02.

---

* internal: Introduce a locking middleware (linkerd/linkerd2-proxy#408)
* Update to Rust 1.40 with new Cargo.lock format (linkerd/linkerd2-proxy#410)
* Update http to v0.1.21 (linkerd/linkerd2-proxy#412)
* internal: Split retry, http-classify, and http-metrics (linkerd/linkerd2-proxy#409)
* Actually update http to v0.1.21 (linkerd/linkerd2-proxy#413)
* patch `prost` 0.5 to pick up security fix (linkerd/linkerd2-proxy#414)
* metrics: Make Counter & Gauge atomic (linkerd/linkerd2-proxy#415)
* Set grpc-status headers on dispatch errors (linkerd/linkerd2-proxy#416)
* trace: update `tracing-subscriber` to 0.2.0-alpha.4 (linkerd/linkerd2-proxy#418)
* discover: Warn on discovery error (linkerd/linkerd2-proxy#422)
* router: Avoid large up-front allocations (linkerd/linkerd2-proxy#421)
* errors: Set correct HTTP version on responses (linkerd/linkerd2-proxy#424)
* app: initialize tracing prior to parsing env vars (linkerd/linkerd2-proxy#425)
* trace: update tracing-subscriber to 0.2.0-alpha.6 (linkerd/linkerd2-proxy#423)
@olix0r olix0r mentioned this pull request Feb 4, 2020
Merged
adleong pushed a commit to linkerd/linkerd2 that referenced this pull request Feb 4, 2020
@olix0r
proxy: v2.85.0 (#4010)
afcbebd 
This release fixes a bug in the proxy's logging subsystem that could
cause the proxy to consume memory until the process is OOMKilled,
especially when the proxy was configured to log diagnostic information.

The proxy also now properly emits `grpc-status` headers when signaling
proxy errors to gRPC clients.

This release upgrades the proxy's Rust version, the `http` crate
dependency to address RUSTSEC-2019-0033 and RUSTSEC-2019-0034, and the
`prost` crate dependency has been patched to address RUSTSEC-2020-02.

---

* internal: Introduce a locking middleware (linkerd/linkerd2-proxy#408)
* Update to Rust 1.40 with new Cargo.lock format (linkerd/linkerd2-proxy#410)
* Update http to v0.1.21 (linkerd/linkerd2-proxy#412)
* internal: Split retry, http-classify, and http-metrics (linkerd/linkerd2-proxy#409)
* Actually update http to v0.1.21 (linkerd/linkerd2-proxy#413)
* patch `prost` 0.5 to pick up security fix (linkerd/linkerd2-proxy#414)
* metrics: Make Counter & Gauge atomic (linkerd/linkerd2-proxy#415)
* Set grpc-status headers on dispatch errors (linkerd/linkerd2-proxy#416)
* trace: update `tracing-subscriber` to 0.2.0-alpha.4 (linkerd/linkerd2-proxy#418)
* discover: Warn on discovery error (linkerd/linkerd2-proxy#422)
* router: Avoid large up-front allocations (linkerd/linkerd2-proxy#421)
* errors: Set correct HTTP version on responses (linkerd/linkerd2-proxy#424)
* app: initialize tracing prior to parsing env vars (linkerd/linkerd2-proxy#425)
* trace: update tracing-subscriber to 0.2.0-alpha.6 (linkerd/linkerd2-proxy#423)
@olix0r olix0r deleted the ver/error-handling branch April 13, 2020 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kleimkuhler kleimkuhler kleimkuhler approved these changes

@hawkw hawkw hawkw approved these changes

Assignees

@olix0r olix0r

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

proxy: Set grpc-status when failing grpc requests
3 participants
@olix0r @hawkw @kleimkuhler