[release/1.7] Fix fatal concurrency error in port forwarding #11306
Conversation
|
Hi @ningmingxiao. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
There was a problem hiding this comment.
This looks good to me as a stop gap solution; maybe we could also consider updating klog module to pull in JSON encoder changes as a more robust solution but even that is not bullet proof.
Unfortunately I am unable to brainstorm any (unit/integration) test to validate; it is a difficult thing to test. Maybe others have ideas?
If others agree, it would be good to have in release/1.6 as well.
| @@ -279,7 +279,7 @@ func (h *httpStreamHandler) portForward(p *httpStreamPair) { | |||
| // see: https://github.com/kubernetes/kubernetes/issues/74551 for detail | |||
| // | |||
| // In both cases, we should RESET the httpStream. | |||
| klog.ErrorS(err, "forwarding port", "conn", h.conn, "request", p.requestID, "port", portString) | |||
| klog.Errorf("forwarding port conn=%p request=%s port=%s failed:%v", h.conn, p.requestID, portString, err) | |||
There was a problem hiding this comment.
minor nit on formatting; non-blocking though.
| klog.Errorf("forwarding port conn=%p request=%s port=%s failed:%v", h.conn, p.requestID, portString, err) | |
| klog.Errorf("(conn=%p, request=%s) forwarding port %s failed: %v", h.conn, p.requestID, portString, err) |
There was a problem hiding this comment.
done. do we need update klog to v2.130.1
There was a problem hiding this comment.
Thanks @ningmingxiao; I think the proposed fix here is good enough solution. I did a proof of concept and was able to reproduce the issue using the JSON encoder if the connection has a exported map field that is not annotated to be hidden. See #11302 (comment) for more information.
Signed-off-by: ningmingxiao <[email protected]>
ningmingxiao
force-pushed
the
fix_1.7_panic
branch
from
1568c12 to
0fe9f0b
Compare
fuweid
changed the title
fuweid
added
the
cherry-pick/1.6.x
dmcgowan
changed the title
fix:#11302