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

Fix proxy ws and other behavior #14

Merged
merged 4 commits into from
Mar 16, 2023
Merged

Conversation

Yshayy
Copy link
Contributor

@Yshayy Yshayy commented Mar 15, 2023

Fix #13, Grafana couldn't use websocket and also got weird errors in http requests. (project docker compose with Loki+Grafana)
The proxy didn't forward ws traffic as it require explicit forwarding of the ws socket. (using node-proxy proxy.ws)
Initially, I used the same approach and added websocket support to fastify and try to pass it to node-proxy, but it didn't work well.
Fastify offical proxy library is also incomplete.

End up using raw node.http handlers and rewiring it with fastify server factory.
All request with "preview" hosts (based on baseUrl) are skipping fastify handler pipeline.

@Yshayy Yshayy requested a review from royra March 15, 2023 20:45
@Yshayy Yshayy force-pushed the fix-proxy-ws-and-other-behavior branch from f060d9f to 61d1a48 Compare March 15, 2023 20:48
@Yshayy Yshayy force-pushed the fix-proxy-ws-and-other-behavior branch from f1c60e4 to 63ea419 Compare March 15, 2023 20:56
export const isProxyRequest = (baseUrl: {hostname:string, port:string}) => (req: IncomingMessage)=> {
const host = req.headers["host"]
if (!host) return false
const {hostname: reqHostname, port: reqPort} = new URL(`http://${host}`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

optimization nit creating a URL for each request might be more expensive than doing regex

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Started that way (actually started with splits and joins), but it shouldn't matter that much and and it's nicer to read

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nodejs/node#30334
actually slower then I thought, might replace it later

Copy link
Collaborator

@royra royra left a comment

Choose a reason for hiding this comment

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

LGTM

  • this will easily break without tests, I think it's pretty easy to add a test with the existing base client from the docker-proxy package.
  • looks like it wasn't linted
  • see minor comments below

@Yshayy Yshayy merged commit 3a69407 into master Mar 16, 2023
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.

Cross service communication error with grafana/loki
2 participants