Skip to content

Commit

Permalink
outbound: Log and remove l5d-proxy-error from responses
Browse files Browse the repository at this point in the history
When an outbound client reads a response from a remote peer that
includes the l5d-proxy-error header, it doesn't communicate this error
state in logs, and it (incorrectly, according to the comments) leaves
the l5d-proxy-error header on the response.

Furthermore, we currently _always_ emit a DEBUG log message "Remote proxy
error", even when no error header is present.

This change updates the error handler to:

1. Emit 'Remote proxy error' messages at INFO level when the header is
   validly set;
2. Strip these headers when they are logged (i.e. to eliminate ambiguity
   and potential for double-logging).

Tests are updated to exercise the header removal logic.
  • Loading branch information
olix0r committed Feb 2, 2024
1 parent fc9ae35 commit fe211c5
Showing 1 changed file with 14 additions and 13 deletions.
27 changes: 14 additions & 13 deletions linkerd/app/outbound/src/http/handle_proxy_error_headers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,17 +127,16 @@ impl<B, F: TryFuture<Ok = http::Response<B>>> Future for ResponseFuture<F> {
}
}

/// Clears the l5d-proxy-error and l5d-proxy-connection headers, logging any
/// error messages. Returns true if the connection should be closed.
fn update_response<B>(rsp: &mut http::Response<B>, closable: bool) -> bool {
// Clear the headers.
let hdr = rsp.headers_mut().remove(L5D_PROXY_CONNECTION);
let err = rsp.headers_mut().get(L5D_PROXY_ERROR);
debug!(
error = err
.and_then(|v| v.to_str().ok())
.map(tracing::field::display),
"Remote proxy error"
);
if let Some(e) = rsp.headers_mut().remove(L5D_PROXY_ERROR) {
if let Ok(error) = e.to_str() {
tracing::info!(error, "Remote proxy error");

Check warning on line 135 in linkerd/app/outbound/src/http/handle_proxy_error_headers.rs

View check run for this annotation

Codecov / codecov/patch

linkerd/app/outbound/src/http/handle_proxy_error_headers.rs#L134-L135

Added lines #L134 - L135 were not covered by tests
}
}

let hdr = rsp.headers_mut().remove(L5D_PROXY_CONNECTION);
if !closable {
return false;
}
Expand All @@ -150,10 +149,8 @@ fn update_response<B>(rsp: &mut http::Response<B>, closable: bool) -> bool {
if rsp.version() == http::Version::HTTP_11 {
// If the response is HTTP/1.1, we need to send a Connection: close
// header to tell the application this connection is being closed.
rsp.headers_mut().insert(
http::header::CONNECTION,
http::HeaderValue::from_static("close"),
);
rsp.headers_mut()
.insert(http::header::CONNECTION, CLOSE.clone());
}
true
}
Expand Down Expand Up @@ -203,6 +200,8 @@ mod test {

let rsp = svc.oneshot(req).await.expect("request must succeed");
assert_eq!(rsp.status(), http::StatusCode::BAD_GATEWAY);
assert!(!rsp.headers().contains_key("l5d-proxy-error"));
assert!(!rsp.headers().contains_key("l5d-proxy-connection"));

// The client handle close future should fire.
time::timeout(time::Duration::from_secs(10), closed)
Expand Down Expand Up @@ -238,6 +237,8 @@ mod test {

let rsp = svc.oneshot(req).await.expect("request must succeed");
assert_eq!(rsp.status(), http::StatusCode::BAD_GATEWAY);
assert!(!rsp.headers().contains_key("l5d-proxy-error"));
assert!(!rsp.headers().contains_key("l5d-proxy-connection"));

// The client handle close future should fire.
tokio::select! {
Expand Down

0 comments on commit fe211c5

Please sign in to comment.