Conversation
Signed-off-by: Alex Leong <[email protected]>
Cargo.toml
Outdated
| webpki = { git = "https://github.com/linkerd/webpki", branch = "cert-dns-names-0.22" } | ||
| boring = { git = "https://github.com/cloudflare/boring" } | ||
| tokio-boring = { git = "https://github.com/cloudflare/boring" } | ||
| linkerd2-proxy-api = { git = "https://github.com/linkerd/linkerd2-proxy-api", rev = "f1598b46c179f5a981bfca7b660dc89903b18ab7" } |
There was a problem hiding this comment.
Why wasn't a proxy API dependency present before?
There was a problem hiding this comment.
This is a dependency override. Specifying this replaces the dependency on linkerd2-proxy-api everywhere in the project with one on a specific proxy-api commit. Once we have the changes we need released in a proxy-api release, we will update the dependency to that version and remove this override.
Cargo.lock
Outdated
| version = "0.10.0" | ||
| source = "registry+https://github.com/rust-lang/crates.io-index" | ||
| checksum = "597facef5c3f12aece4d18a5e3dbba88288837b0b5d8276681d063e4c9b98a14" | ||
| source = "git+https://github.com/linkerd/linkerd2-proxy-api?rev=f1598b46c179f5a981bfca7b660dc89903b18ab7#f1598b46c179f5a981bfca7b660dc89903b18ab7" |
There was a problem hiding this comment.
Why did the source of the package change?
Signed-off-by: Alex Leong <[email protected]>
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
this does look like a workable solution to the problem. i definitely see why you weren't sure about the design of adding apply_response to filters, it does feel somewhat awkward. it certainly seems like it would feel nicer if the filter could just wrap the entire future, mapping either the request or the response, the way Service middleware does. but, thinking it through, I think that would make having an arbitrary list of filters that's determined dynamically at runtime much more difficult, since we would have a response future whose type varies based on what's in the list of filters. which...is hard to represent without adding boxes for type erasure for each filter in the list...
so, i think the design proposed here, though kind of awkward, is probably the best solution without completely redesigning how we model these filters. perhaps @olix0r has other ideas, though?
| let mut out = ready!(this.inner.try_poll(cx)); | ||
| if let Ok(rsp) = &mut out { | ||
| if let Err(e) = this.apply.apply_response(rsp) { | ||
| return Poll::Ready(Err(e)); | ||
| }; | ||
| } | ||
| Poll::Ready(out.map_err(Into::into)) |
There was a problem hiding this comment.
| let mut out = ready!(this.inner.try_poll(cx)); | |
| if let Ok(rsp) = &mut out { | |
| if let Err(e) = this.apply.apply_response(rsp) { | |
| return Poll::Ready(Err(e)); | |
| }; | |
| } | |
| Poll::Ready(out.map_err(Into::into)) | |
| Poll::Ready(ready!(this.inner.try_poll(cx)) | |
| .and_then(|mut rsp| { | |
| this.apply.apply_response(&mut rsp)?; | |
| Ok(rsp) | |
| }) | |
| .map_err(Into::into)) |
| http::Filter::InjectFailure(_) => {} // InjectFailure filter does not apply to responses. | ||
| http::Filter::Redirect(_) => {} // Redirect filter does not apply to responses. | ||
| http::Filter::RequestHeaders(_) => {} // RequestHeaders filter does not apply to responses. | ||
| http::Filter::InternalError(_) => {} // InternalError filter does not apply to responses. |
There was a problem hiding this comment.
this feels...somewhat unfortunate. it would be preferable if we could iterate over all the filters once, and apply them not just to the request but to the future, so that the filter can act on both the request and the response as it sees fit. but, that would require a much more substantial redesign of how we represent filters, essentially turning them into arbitrary middleware, and then we run into issues with the future type being determined from the value of a dynamic list of filters...
| fn to_pairs( | ||
| hs: Option<http_types::Headers>, | ||
| ) -> Result<Vec<(HeaderName, HeaderValue)>, InvalidModifyHeader> { | ||
| hs.into_iter() | ||
| .flat_map(|a| a.headers.into_iter()) | ||
| .map(|h| { | ||
| let name = h.name.parse::<HeaderName>()?; | ||
| let value = HeaderValue::from_bytes(&h.value)?; | ||
| Ok((name, value)) | ||
| }) | ||
| .collect() | ||
| } | ||
|
|
There was a problem hiding this comment.
We already have this boilerplate above. It would be better to extract it and reuse:
diff --git a/linkerd/http-route/src/http/filter/modify_header.rs b/linkerd/http-route/src/http/filter/modify_header.rs
index 530eb9ab0..4f3c2b6b4 100644
--- a/linkerd/http-route/src/http/filter/modify_header.rs
+++ b/linkerd/http-route/src/http/filter/modify_header.rs
@@ -46,19 +46,6 @@ pub mod proto {
type Error = InvalidModifyHeader;
fn try_from(rhm: api::RequestHeaderModifier) -> Result<Self, Self::Error> {
- fn to_pairs(
- hs: Option<http_types::Headers>,
- ) -> Result<Vec<(HeaderName, HeaderValue)>, InvalidModifyHeader> {
- hs.into_iter()
- .flat_map(|a| a.headers.into_iter())
- .map(|h| {
- let name = h.name.parse::<HeaderName>()?;
- let value = HeaderValue::from_bytes(&h.value)?;
- Ok((name, value))
- })
- .collect()
- }
-
let add = to_pairs(rhm.add)?;
let set = to_pairs(rhm.set)?;
let remove = rhm
@@ -76,19 +63,6 @@ pub mod proto {
type Error = InvalidModifyHeader;
fn try_from(rhm: api::ResponseHeaderModifier) -> Result<Self, Self::Error> {
- fn to_pairs(
- hs: Option<http_types::Headers>,
- ) -> Result<Vec<(HeaderName, HeaderValue)>, InvalidModifyHeader> {
- hs.into_iter()
- .flat_map(|a| a.headers.into_iter())
- .map(|h| {
- let name = h.name.parse::<HeaderName>()?;
- let value = HeaderValue::from_bytes(&h.value)?;
- Ok((name, value))
- })
- .collect()
- }
-
let add = to_pairs(rhm.add)?;
let set = to_pairs(rhm.set)?;
let remove = rhm
@@ -99,4 +73,17 @@ pub mod proto {
Ok(ModifyHeader { add, set, remove })
}
}
+
+ fn to_pairs(
+ hs: Option<http_types::Headers>,
+ ) -> Result<Vec<(HeaderName, HeaderValue)>, InvalidModifyHeader> {
+ hs.into_iter()
+ .flat_map(|a| a.headers.into_iter())
+ .map(|h| {
+ let name = h.name.parse::<HeaderName>()?;
+ let value = HeaderValue::from_bytes(&h.value)?;
+ Ok((name, value))
+ })
+ .collect()
+ }
}Signed-off-by: Alex Leong <[email protected]>
Add support for the response header modifier, which was added to the proxy API in linkerd/linkerd2-proxy-api#251 Signed-off-by: Alex Leong <[email protected]> --- * Add suport for response header filter (linkerd/linkerd2-proxy#2439) Signed-off-by: Eliza Weisman <[email protected]>
4 participants
Add support for the response header modifier, which was added to the proxy API in linkerd/linkerd2-proxy-api#251