flenter commented 1 year ago

This is mostly required to make service workers work for /explorer. The static assets can't be hosted on a CDN anymore, so this PR now proxies the files. It's also possible to override this target URL to point to something different then the default explorer.autometrics.dev. This way it's possible for developers working on the explorer front-end to test things without needing to deploy it.

Checklist

[ x] Changelog updated

Resolves https://github.com/autometrics-dev/am/issues/142

flenter commented 1 year ago

There's an issue with this proxy functionality that I've not been able to resolve and that's related to http/2. Right now the proxying works however in the terminal the following is logged:

Connection header illegal in HTTP/2: connection

The Connection: keep-alive header that my browser sends is proxied, but isn't actually valid in an http/2 context. Fixing this goes a bit further than my current rust knowledge

gagbo commented 1 year ago

There's an issue with this proxy functionality that I've not been able to resolve and that's related to http/2. Right now the proxying works however in the terminal the following is logged:
Connection header illegal in HTTP/2: connection
The Connection: keep-alive header that my browser sends is proxied, but isn't actually valid in an http/2 context. Fixing this goes a bit further than my current rust knowledge

The way I understand the issue, this patch should work

diff --git a/src/bin/am/server/assets.rs b/src/bin/am/server/assets.rs
index 5dd3165..9caf438 100644
--- a/src/bin/am/server/assets.rs
+++ b/src/bin/am/server/assets.rs
@@ -1,6 +1,7 @@
 use crate::server::util::proxy_handler;
 use axum::body::Body;
 use axum::response::IntoResponse;
+use http::header::CONNECTION;
 use url::Url;

 pub async fn handler(mut req: http::Request<Body>, upstream_base: Url) -> impl IntoResponse {
@@ -12,5 +13,6 @@ pub async fn handler(mut req: http::Request<Body>, upstream_base: Url) -> impl I
         .replace("/explorer/static", "/static")
         .parse()
         .unwrap();
+    req.headers_mut().remove(CONNECTION);
     proxy_handler(req, upstream_base.clone()).await
 }

I don't think there's a way to tell the request to "remove all HTTP/2 illegal headers", so we might have to do this on a case by case basis :(

EDIT: also, the request is eventually handled by reqwest::Client, and I don't know whether there's a way to know at runtime whether we're using http1 or 2 (or 3, if using the unstable support)

flenter commented 1 year ago

req.headers_mut().remove(CONNECTION);

Thank you for this suggestion! I've modified the code matching your suggestion and the complaints no longer show up in the terminal. 🙏

autometrics-dev / am

Proxy static files #141

Checklist