feat(middleware-logging): implement http.Hijacker

[guilhem]

http.Hijacker is mandatory to forward websocket connexions

Provide a description of what has been changed

Checklist

• Commits are signed with Developer Certificate of Origin (DCO)
• Changelog has been updated and is aligned with our changelog requirements
• Any necessary documentation is added, such as:
  • README.md
  • The docs/ directory
  • The docs repo

Fixes #618 #654

[JorTurFer]

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

[JorTurFer]

WDYT @t0rr3sp3dr0 ?

[guilhem]

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]

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

[t0rr3sp3dr0]

Small change to force loggingResponseWriter to implement http.Hijacker

Suggested change

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

[t0rr3sp3dr0]

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]

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

[JorTurFer]

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]

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]

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]

Any update about this?

[tghastings]

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]

@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]

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

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

[wadeperrigo]

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

[JorTurFer]

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]

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

[guilhem]

I did a rebase yesterday.
I will push it tomorrow

[twij2332]

I did a rebase yesterday. I will push it tomorrow

Did you able to rebase and push

[JorTurFer]

could you include e2e test coverage for this?

[twij2332]

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