Add http client and server implementation#16
Conversation
ti-mo
left a comment
There was a problem hiding this comment.
Looks good! Please split the move into package tcp into a separate commit to keep the Git history clean and ease reviewing.
| internal.ErrExit("http server shutdown", server.Shutdown(waitCtx)) | ||
| }() | ||
|
|
||
| common.MarkServerReady() |
There was a problem hiding this comment.
I'm not 100% sure what these are used for (k8s ready check?), but technically this marks the server ready before the listening socket has been opened, and given sufficient sample size and noisy neighbours in CI this could become a tricky problem to debug.
We could reimplement ListenAndServe to call MarkServerReady() between net.Listen() and server.Serve(). We'd need to verify whether net.Listen() is enough to let the client connect and send its http request; hopefully the socket would buffer the request until server.Serve() is called. I'm not familiar enough with the http package to know for sure.
There was a problem hiding this comment.
I'm not 100% sure what these are used for (k8s ready check?), but technically this marks the server ready before the listening socket has been opened, and given sufficient sample size and noisy neighbours in CI this could become a tricky problem to debug.
Yes, its used in k8s readiness checks.
Thats's a good point. I have made the required changes.
We could reimplement ListenAndServe to call MarkServerReady() between net.Listen() and server.Serve(). We'd need to verify whether net.Listen() is enough to let the client connect and send its http request; hopefully the socket would buffer the request until server.Serve() is called. I'm not familiar enough with the http package to know for sure.
Yes, validated that server will buffer the requests until it starts serving.
There was a problem hiding this comment.
On a side note, there is another issue with how we are using signal context in the client implementations.
Client won't react to an OS signal if its slepping(we have some os specific sleeps where we don't take context into account), so reaction to context cancellation will be delayed by the max request interval.
If it sounds relevant, I can do another round of cleanup in a follow up PR.
Signed-off-by: Deepesh Pathak <deepesh.pathak@isovalent.com>
Signed-off-by: Deepesh Pathak <deepesh.pathak@isovalent.com>
* Update go module version to 1.25.x to match Dockerfile base image version. Signed-off-by: Deepesh Pathak <deepesh.pathak@isovalent.com>
8aff06c to
49bc8ee
Compare
ti-mo
left a comment
There was a problem hiding this comment.
Looks good! Does the client use the websocket echo feature of the server?
Signed-off-by: Deepesh Pathak <deepesh.pathak@isovalent.com> Signed-off-by: Deepesh Pathak <deepeshpathak09@gmail.com>
49bc8ee to
0c473ff
Compare
No, not yet. I will add websocket support in a follow up PR, removed the dead code for now. |
Oh, makes sense, thanks! |
--protocol. Defaults totcpto keep existing command behavior.Required for extending cilium cli connection disruption connectivity tests for L7 policy.
Test Docker Image:
docker.io/fristonio/test-connection-disruption:v0.0.17Cilium PR: cilium/cilium#42150
CI validation: https://github.com/cilium/cilium/actions/runs/18918259228