inbound: Add HTTP route filters by olix0r · Pull Request #1788 · linkerd/linkerd2-proxy

olix0r · 2022-06-24T20:27:35Z

This change adds several route filters:

An HTTP header rewriting;
An HTTP redirection filter;
An HTTP error filter; and
A gRPC error filter.

The header-rewriting and HTTP redirection filters are implemented as
specified by the Kubernetes Gateway API HTTPRoute spec.

These filters apply only on inbound policies are not yet configurable
via the control plane. A followup change will be made to support
configuring these filters. Depends on #1781.

Signed-off-by: Oliver Gould [email protected]

This change adds a standalone utility crate that will support matching routes for the Gateway API's HTTP route types. This crate is not yet used. In followup changes: * filter types will be added to support header-rewriting, HTTP redirection, and error injection filters; * the inbound proxy will be updated to supcort route-oriented authorization policies; and * the inbound policy API client will be updated to configure inbound server routes. Signed-off-by: Oliver Gould <[email protected]>

This change adds per-route authorization policies to the inbound proxy. This change does NOT introduce an API client to configure these policies. That will be added in follow-up changes, as will richer per-route configurations. This change: * Modifies the inbound policy `Protocol` type so that each protocol variant includes policy configuration. TCP protocols now include authorization policies (moved from the `Server` type). * Implements an HTTP policy router that determines the route for each request and applies its authorization policy. * Fails requests for which no route matches with a `404 Not Found` response. A new metric, `inbound_http_route_not_found_total`, is introduced to count occurrences of this error. * Includes a default route for all inbound HTTP communication, using the server's authorization policies. Signed-off-by: Oliver Gould <[email protected]>

This change adds several route filters: * An HTTP header rewriting; * An HTTP redirection filter; * An HTTP error filter; and * A gRPC error filter. The header-rewriting and HTTP redirection filters are implemented as specified by the Kubernetes Gateway API HTTPRoute spec. These filters apply only on inbound policies are not yet configurable via the control plane. A followup change will be made to support configuring these filters. Depends on #1781. Signed-off-by: Oliver Gould <[email protected]>

This change adds per-route authorization policies to the inbound proxy. This change does NOT introduce an API client to configure these policies. That will be added in follow-up changes, as will richer per-route configurations. This change: * Modifies the inbound policy `Protocol` type so that each protocol variant includes policy configuration. TCP protocols now include authorization policies (moved from the `Server` type). * Implements an HTTP policy router that determines the route for each request and applies its authorization policy. * Fails requests for which no route matches with a `404 Not Found` response. A new metric, `inbound_http_route_not_found_total`, is introduced to count occurrences of this error. * Includes a default route for all inbound HTTP communication, using the server's authorization policies. Signed-off-by: Oliver Gould <[email protected]>

Signed-off-by: Oliver Gould <[email protected]>

The Gateway API defines a [header-rewriting filter](gwapi) that may be attached to HTTP routes. This change updates the HTTP route types to support a list of filters. The inbound proxy supports only the header modifier filter. Additional filters will be added in follow-up changes (see #1788 for an example of these additional filters). [gwapi]: https://gateway-api.sigs.k8s.io/v1alpha2/references/spec/#gateway.networking.k8s.io/v1alpha2.HTTPRequestHeaderFilter Signed-off-by: Oliver Gould <[email protected]>

hawkw

i got kind of nerd-sniped by trying to reduce allocation & copying in the RedirectRequest's URI-rewriting code, so i left a bunch of comments there. if you like, i'd be happy to open a PR against this with what i believe would be a more efficient implementation, but it's also something we can come back to later.

hawkw · 2022-06-28T23:01:53Z

linkerd/app/inbound/src/policy/http.rs

+                }
+
+                Ok(None) => {
+                    tracing::debug!("Ignoring irrelvant redirect");


typo:

Suggested change

tracing::debug!("Ignoring irrelvant redirect");

tracing::debug!("Ignoring irrelevant redirect");

hawkw · 2022-06-28T23:07:37Z

linkerd/http-route/src/http/filter/redirect.rs

+                (Some(h), p) => p
+                    .map(|p| format!("{}:{}", h, p).parse())
+                    .unwrap_or_else(|| h.parse())?,


seems like we could potentially avoid doing this for every request, in the case that there's a port configured --- this authority doesn't depend on the request at all, unlike in the case where the redirect is configured without a host...

hawkw · 2022-06-28T23:12:41Z

linkerd/http-route/src/http/filter/redirect.rs

+                    .map(|p| format!("{}:{}", h, p).parse())
+                    .unwrap_or_else(|| h.parse())?,
+                // If a host is NOT configured, use the request's original host and either an
+                // overridden port or the original port.
+                (None, p) => {
+                    let h = orig_uri.host().ok_or(InvalidRedirect::MissingAuthority)?;
+                    p.or_else(|| orig_uri.port_u16())
+                        .map(|p| format!("{}:{}", h, p).parse())
+                        .unwrap_or_else(|| h.parse())?


also, we may want to use the TryFrom<String> impl for Authority rather than FromStr? the TryFrom<String> impl will try to construct an Authority from the String converted into a Bytes while the FromStr impl calls the TryFrom<&'a str> impl, which will copy the &str's bytes into a new Bytes. since we have ownership of the strings returned by format! here, we may want to use the conversion that takes the String by value, as it won't allocate a new Bytes and copy the string into it, but instead uses the same allocation.

hawkw · 2022-06-28T23:17:17Z

linkerd/http-route/src/http/filter/redirect.rs

+                    Some(ModifyPath::ReplacePrefixMatch(new_pfx)) => match rm.route.path() {
+                        PathMatch::Prefix(pfx_len) if *pfx_len <= orig_path.len() => {
+                            let (_, rest) = orig_path.split_at(*pfx_len);
+                            format!("{}{}", new_pfx, rest)


it might be a little bit more efficient to do something like

Suggested change

format!("{}{}", new_pfx, rest)

let mut path = String::with_capacity(new_pfx.len() + rest.len());

path.push_str(new_pfx);

path.push_str(rest);

path

but, that's also several more lines of code...

hawkw · 2022-06-28T23:20:20Z

linkerd/http-route/src/http/filter/redirect.rs

+                let orig_path = orig_uri.path();
+                match &self.path {
+                    None => orig_path.to_string(),
+                    Some(ModifyPath::ReplaceFullPath(p)) => p.clone(),


if we stored the full path as a Bytes rather than a String, the clone here would just be a reference count bump, rather than an alloc + memcpy (and http::uri::Builder::path_and_query will accept a Bytes). we would also have to convert the modified path in that case, though.

hawkw · 2022-06-28T23:23:28Z

linkerd/http-route/src/http/filter/redirect.rs

+            let path = {
+                use crate::http::r#match::PathMatch;
+
+                let orig_path = orig_uri.path();
+                match &self.path {
+                    None => orig_path.to_string(),
+                    Some(ModifyPath::ReplaceFullPath(p)) => p.clone(),
+                    Some(ModifyPath::ReplacePrefixMatch(new_pfx)) => match rm.route.path() {
+                        PathMatch::Prefix(pfx_len) if *pfx_len <= orig_path.len() => {
+                            let (_, rest) = orig_path.split_at(*pfx_len);
+                            format!("{}{}", new_pfx, rest)
+                        }


if we converted the paths in the other match arms to http::uri::PathAndQuery here, we could re-use the original request path without formatting it to a string and then back again:

Suggested change

let path = {

use crate::http::r#match::PathMatch;

let orig_path = orig_uri.path();

match &self.path {

None => orig_path.to_string(),

Some(ModifyPath::ReplaceFullPath(p)) => p.clone(),

Some(ModifyPath::ReplacePrefixMatch(new_pfx)) => match rm.route.path() {

PathMatch::Prefix(pfx_len) if *pfx_len <= orig_path.len() => {

let (_, rest) = orig_path.split_at(*pfx_len);

format!("{}{}", new_pfx, rest)

}

let path: http::uri::PathAndQuery = {

use crate::http::r#match::PathMatch;

let orig_path = orig_uri.path();

match &self.path {

None => orig_path,

Some(ModifyPath::ReplaceFullPath(p)) => p.clone().try_into()?,

Some(ModifyPath::ReplacePrefixMatch(new_pfx)) => match rm.route.path() {

PathMatch::Prefix(pfx_len) if *pfx_len <= orig_path.len() => {

let (_, rest) = orig_path.split_at(*pfx_len);

format!("{}{}", new_pfx, rest).try_into()?

}

we would need to change the error a bit in that case, though, because the String -> PathAndQuery conversion also returns InvalidUrl

Signed-off-by: Oliver Gould <[email protected]>

hawkw · 2022-06-29T17:31:04Z

BTW, I did end up throwing together a quick branch that makes some optimizations to the redirect URI rewriting; happy to open a PR against this branch, or save it for later if you're still making a bunch of changes here.

olix0r · 2022-06-29T19:13:52Z

@hawkw For what it's worth, I don't think it's particularly important to optimize this codepath. I'd bias strongly towards readability for the time being.

Signed-off-by: Oliver Gould <[email protected]>

hawkw · 2022-06-29T22:14:53Z

linkerd/http-route/src/http/filter/redirect.rs

+            let path = {
+                use crate::http::r#match::PathMatch;
+
+                let orig_path = orig_uri.path();
+                match &self.path {
+                    None => orig_path.to_string(),
+                    Some(ModifyPath::ReplaceFullPath(p)) => p.clone(),
+                    Some(ModifyPath::ReplacePrefixMatch(new_pfx)) => match rm.route.path() {
+                        PathMatch::Prefix(pfx_len) if *pfx_len <= orig_path.len() => {
+                            let (_, rest) = orig_path.split_at(*pfx_len);
+                            format!("{}{}", new_pfx, rest)
+                        }
+                        _ => return Err(InvalidRedirect::InvalidReplacePrefix),
+                    },
+                }
+            };


Noticed what I believe is a bug here: in all cases (even when just using the original path), this filter will clobber any query params. This is because we get orig_path by calling orig_uri.path(), which returns just the path component off the URI, and not the query component, but then we pass a value constructed using orig_path to uri::Builder::path_and_query when constructing the new URI. If the original URL had a query component, it's discarded here.

I think we should probably call orig_uri.query() at some point and ensure that the query parameters are added to the new path-and-query part passed to uri::Builder::path_and_query.

(as a side note, it also appears that the redirect filter will currently always throw out any fragment identifier component of the URI...but, see also hyperium/http#127)

olix0r added 4 commits June 24, 2022 19:08

olix0r force-pushed the ver/inbound-routes branch from 1d0df3f to 10cb66e Compare June 25, 2022 01:09

olix0r added 5 commits June 25, 2022 22:09

Add inbound HTTP route authorization tests

0f9f9a0

Signed-off-by: Oliver Gould <[email protected]>

Merge branch 'main' into ver/inbound-route-filters

141219c

Merge branch 'ver/inbound-routes' into ver/inbound-route-filters

e118a03

Add filter tests

2e93a86

Signed-off-by: Oliver Gould <[email protected]>

touchup

9a7a3ae

Signed-off-by: Oliver Gould <[email protected]>

Base automatically changed from ver/inbound-routes to main June 27, 2022 23:46

olix0r added 5 commits June 27, 2022 23:54

Merge branch 'main' into ver/inbound-route-filters

b5f1b97

Remove unused crate

ea33976

Signed-off-by: Oliver Gould <[email protected]>

fixup accidental rename

1f0e4d0

Signed-off-by: Oliver Gould <[email protected]>

fixup

3b7ba40

Signed-off-by: Oliver Gould <[email protected]>

more fixup/backout

5d6bf41

Signed-off-by: Oliver Gould <[email protected]>

olix0r marked this pull request as ready for review June 28, 2022 00:05

olix0r requested a review from a team as a code owner June 28, 2022 00:05

olix0r marked this pull request as draft June 28, 2022 05:46

olix0r mentioned this pull request Jun 28, 2022

inbound: Add a header-modification route filter #1793

Merged

hawkw reviewed Jun 28, 2022

View reviewed changes

olix0r added 2 commits June 29, 2022 02:48

Merge branch 'main' into ver/inbound-route-filters

261dac0

words

283f98c

Signed-off-by: Oliver Gould <[email protected]>

fixup

2b27b6a

Signed-off-by: Oliver Gould <[email protected]>

hawkw reviewed Jun 29, 2022

View reviewed changes

olix0r closed this Jul 6, 2022

olix0r deleted the ver/inbound-route-filters branch March 7, 2023 21:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

inbound: Add HTTP route filters#1788

inbound: Add HTTP route filters#1788
olix0r wants to merge 17 commits intomainfrom
ver/inbound-route-filters

olix0r commented Jun 24, 2022

Uh oh!

hawkw left a comment

Uh oh!

hawkw Jun 28, 2022

Uh oh!

hawkw Jun 28, 2022

Uh oh!

hawkw Jun 28, 2022

Uh oh!

hawkw Jun 28, 2022

Uh oh!

hawkw Jun 28, 2022

Uh oh!

hawkw Jun 28, 2022

Uh oh!

hawkw commented Jun 29, 2022

Uh oh!

olix0r commented Jun 29, 2022

Uh oh!

hawkw Jun 29, 2022

Uh oh!

hawkw Jun 29, 2022 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	tracing::debug!("Ignoring irrelvant redirect");
	tracing::debug!("Ignoring irrelevant redirect");

-                            format!("{}{}", new_pfx, rest)
+                            let mut path = String::with_capacity(new_pfx.len() + rest.len());
+                            path.push_str(new_pfx);
+                            path.push_str(rest);
+                            path

Conversation

olix0r commented Jun 24, 2022

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

hawkw Jun 28, 2022

Choose a reason for hiding this comment

Uh oh!

hawkw Jun 28, 2022

Choose a reason for hiding this comment

Uh oh!

hawkw Jun 28, 2022

Choose a reason for hiding this comment

Uh oh!

hawkw Jun 28, 2022

Choose a reason for hiding this comment

Uh oh!

hawkw Jun 28, 2022

Choose a reason for hiding this comment

Uh oh!

hawkw Jun 28, 2022

Choose a reason for hiding this comment

Uh oh!

hawkw commented Jun 29, 2022

Uh oh!

olix0r commented Jun 29, 2022

Uh oh!

hawkw Jun 29, 2022

Choose a reason for hiding this comment

Uh oh!

hawkw Jun 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hawkw Jun 29, 2022 •

edited

Loading