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
Bump konnectivity-client to 0.0.32 #110731
Conversation
/priority important-soon |
/lgtm |
/assign @thockin |
Fixed merge conflicts (with #110724) |
/retest |
/triage accepted |
/cc @liggitt |
// close any channels remaining for these connections. | ||
t.connsLock.Lock() | ||
for _, conn := range t.conns { | ||
close(conn.readCh) |
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.
hoisting question from #110019 (comment)
do we also need to send "" to
conn.closeCh
andclose(conn.closeCh)
likecase client.PacketType_CLOSE_RSP
does?
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.
From what I see, improving this inconsistency would at most help to unblock any goroutine calling conn.Close (and perhaps allow an error message like "tunnel is closing").
Overall kubernetes-sigs/apiserver-network-proxy#341 is a net improvement because it unblocks goroutines that call conn.Read (which block forever on readCh), and it doesn't seem any worse from conn.Close perspective.
So I think we don't need to block this PR on this question, and can take the code cleanup to the apiserver-network-proxy project.
@cheftako Please chime in if I'm wrong about anything.
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.
sounds good
/approve |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheftako, jkh52, liggitt The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind feature
-->
What this PR does / why we need it:
Tag 0.0.32 includes an API change (kubernetes-sigs/apiserver-network-proxy#360) needed to continue improving tunnel context handling in kube-apiserver.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Partial progress toward kubernetes-sigs/apiserver-network-proxy#357
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: