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

Modify response to have via and server headers #1033

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aryan9600
Copy link

This PR addresses linkerd/linkerd2#2295.
I am not very confident about this PR, hence the draft PR.
Also, a rather peculiar thing, checking the response via the viz dashboard doesn't show the via or server header, but the debug statements do confirm that the response has been modified. It'd be great if I could get some feedback on how to improve this.

@olix0r
Copy link
Member

olix0r commented Jun 7, 2021

@aryan9600 What do you think about omitting the via header for now? Users may not necessarily want to expose this information on requests so I'd like to be a bit careful about introducing that.

I think adding a server header to Linkerd-terminated requests seems like a good compromise, though; as this information is necessary to effectively debug 5xx responses.

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.

thanks for working on this! i agree with @olix0r's comments about the via header. also, i left a few suggestions about the server header inline.

@@ -218,11 +234,13 @@ impl<RspB: Default + hyper::body::HttpBody> respond::Respond<http::Response<RspB
}

let status = http_status(&*error);
const SERVER_HEADER: &'static str = concat!("linkerd-proxy/", env!("CARGO_PKG_VERSION"));
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 it's really correct to use the cargo package version as the proxy's version. this is going to be the version of the linkerd-app-core crate, which isn't the actual proxy version: https://github.com/aryan9600/linkerd2-proxy/blob/6438d0d19dafc22d6645d198b566085c7c5d0e73/linkerd/app/core/Cargo.toml#L3

instead, we should probably use the GIT_VERSION env var, which is the current proxy version git tag, set by the build script:

set_env(
"GIT_VERSION",
Command::new("git").args(&["describe", "--always", "HEAD"]),
);

Suggested change
const SERVER_HEADER: &'static str = concat!("linkerd-proxy/", env!("CARGO_PKG_VERSION"));
const SERVER_HEADER: &'static str = concat!("linkerd-proxy/", env!("GIT_VERSION"));

Copy link
Contributor

Choose a reason for hiding this comment

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

the linting checks on CI are failing because the clippy linter doesn't like the explicit 'static lifetime here (constants always have the 'static lifetime). to get CI passing, we should probably remove it:

Suggested change
const SERVER_HEADER: &'static str = concat!("linkerd-proxy/", env!("CARGO_PKG_VERSION"));
const SERVER_HEADER: &str = concat!("linkerd-proxy/", env!("CARGO_PKG_VERSION"));

Copy link
Author

Choose a reason for hiding this comment

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

oops, forgot to consult clippy before pushing 😅

@aryan9600
Copy link
Author

Users may not necessarily want to expose this information on requests so I'd like to be a bit careful about introducing that.

What if we keep it disabled by default and add an opt-in annotation to enable this? @olix0r @hawkw

Signed-off-by: Sanskar Jaiswal <[email protected]>
@aryan9600 aryan9600 requested a review from hawkw June 12, 2021 10:46
@aryan9600 aryan9600 marked this pull request as ready for review June 23, 2021 11:25
@aryan9600 aryan9600 requested a review from a team June 23, 2021 11:25
@olix0r olix0r marked this pull request as draft August 15, 2022 21:19
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