-
Notifications
You must be signed in to change notification settings - Fork 90
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
base: main
Are you sure you want to change the base?
Conversation
nice! |
WDYT @t0rr3sp3dr0 ? |
Yes it is ;) I really tried to add a test for websocket and UPGRADE header but without success (lack of real comprehension of current testing). |
I think that we will need a special test case here, something like:
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 |
There was a problem hiding this comment.
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
// implements http.hijacker | |
var _ http.Hijacker = (*loggingResponseWriter)(nil) | |
There was a problem hiding this 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.
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 ? |
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. |
We prefer to manage all the e2e suite using go for orchestation. Could this be feasible for you? |
Any update about this? |
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. |
@tghastings , e2e test are pending yet. as the PR seems abandoned, are you willing to open another PR from here adding the e2e tests? |
I fully agree with @t0rr3sp3dr0 comment: #835 (review) We have to cover those questions with tests (unit + e2e) |
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 |
This is a much needed feature. Any update on this PR? |
I did a rebase yesterday. |
Did you able to rebase and push |
http.Hijacker is mandatory to forward websocket connexions Signed-off-by: Guilhem Lettron <guilhem@barpilot.io>
http.Hijacker is mandatory to forward websocket connexions
Provide a description of what has been changed
Checklist
README.md
docs/
directoryFixes #618 #654