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

feat(middleware-logging): implement http.Hijacker #835

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

Conversation

guilhem
Copy link

@guilhem guilhem commented Nov 8, 2023

http.Hijacker is mandatory to forward websocket connexions

Provide a description of what has been changed

Checklist

Fixes #618 #654

@JorTurFer
Copy link
Member

nice!
Do this add support to websockets? Could you add a record on changelog and an e2e test to cover the websockets scenario?

@JorTurFer
Copy link
Member

WDYT @t0rr3sp3dr0 ?

@guilhem
Copy link
Author

guilhem commented Nov 21, 2023

nice!
Do this add support to websockets? Could you add a record on changelog and an e2e test to cover the websockets scenario?

Yes it is ;)
I'm using it in production right now.

I really tried to add a test for websocket and UPGRADE header but without success (lack of real comprehension of current testing).
Maybe someone can help or push on my branch ^^

@JorTurFer
Copy link
Member

JorTurFer commented Nov 21, 2023

I think that we will need a special test case here, something like:

  • Custom server exposing websocket
  • A client connecting to the server through the interceptor
  • We trigger a message in the server (calling to other endpoint for example)
  • We validate that the client has received the value via webhook

Could this approach work? If yes, I'll try to include a test like it soon

@@ -46,3 +49,12 @@ func (lrw *loggingResponseWriter) WriteHeader(statusCode int) {

lrw.statusCode = statusCode
}

// implements http.hijacker
Copy link
Contributor

Choose a reason for hiding this comment

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

Small change to force loggingResponseWriter to implement http.Hijacker

Suggested change
// implements http.hijacker
var _ http.Hijacker = (*loggingResponseWriter)(nil)

Copy link
Contributor

@t0rr3sp3dr0 t0rr3sp3dr0 left a comment

Choose a reason for hiding this comment

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

Please implement a unit test to make sure we always call the underlying Hijacker method when downstreamResponseWriter implements it and another one to check the error returned when downstreamResponseWriter is not an http.Hijacker.

Also, is it safe to implement Hijack and return an error when downstreamResponseWriter is not an http.Hijacker? I'm reading the documentation and we may have problems when using HTTP/2:

The default ResponseWriter for HTTP/1.x connections supports Hijacker, but HTTP/2 connections intentionally do not. ResponseWriter wrappers may also not support Hijacker. Handlers should always test for this ability at runtime.

We need to add some HTTP/2 tests to ensure implementing this method and returning an error doesn't break anything. A code that checks whether or not HTTP.Hijacker is implemented may expect Hijack always to succeed, and we now return an error.

@hergi2004
Copy link

hergi2004 commented Jan 16, 2024

Nice!! i validate this with our app and it fixed websocket....

@JorTurFer
Copy link
Member

Nice!! i validate this with our app and it fixed websocket....

Nice to know it! Even that, we do need to include some test that ensures this will continue working during the time. @guilhem do you have any update? If the OP doesn't continue the PR, are you willing to do it @hergi2004 ?

@leblancmeneses
Copy link

leblancmeneses commented Feb 7, 2024

If you are willing to add Playwright to run the e2e test suite for this feature, I could implement a node server and have Playwright verify bidirectional communication is possible over WebSockets when HTTPScaledObject is in play.

Perhaps we can skip the middleware if Upgrade: websocket header is not present.

@JorTurFer
Copy link
Member

JorTurFer commented Feb 7, 2024

We prefer to manage all the e2e suite using go for orchestation.
Given that, client/server can be written in the language that you prefer and used as docker images and that's quite common. I mean, you can open a PR to test-tools repo generating the required docker images for client/server (or both in one, it doesn't matter) and then deploy them into the cluster and checking how it works using a test written in Go.

Could this be feasible for you?

@JorTurFer
Copy link
Member

Any update about this?

@tghastings
Copy link

I too am very interested in this merge request. Please let us know if there is anything we can do to assist moving it forward.

@JorTurFer
Copy link
Member

@tghastings , e2e test are pending yet. as the PR seems abandoned, are you willing to open another PR from here adding the e2e tests?

@JorTurFer
Copy link
Member

I fully agree with @t0rr3sp3dr0 comment: #835 (review)

We have to cover those questions with tests (unit + e2e)

@wadeperrigo
Copy link

My company has a need for this as well, any updates?

@JorTurFer
Copy link
Member

My company has a need for this as well, any updates?

Sadly the OP hasn't answered :( Are you willing to continue from this PR? The code looks well, but we need to cover it with tests to ensure that we don't break anything

@twij2332
Copy link

twij2332 commented Aug 1, 2024

This is a much needed feature. Any update on this PR?

@guilhem
Copy link
Author

guilhem commented Aug 1, 2024

I did a rebase yesterday.
I will push it tomorrow

@twij2332
Copy link

I did a rebase yesterday. I will push it tomorrow

Did you able to rebase and push

http.Hijacker is mandatory to forward websocket connexions

Signed-off-by: Guilhem Lettron <[email protected]>
@JorTurFer
Copy link
Member

could you include e2e test coverage for this?

@twij2332
Copy link

twij2332 commented Sep 2, 2024

Seema like new changes are breaking the external-scaler deployment.

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.

8 participants