export a method to expose which ports were forwarded #67513
Conversation
k8s-ci-robot
added
release-note-none
k8s-ci-robot
added
the
needs-ok-to-test
|
@novas0x2a /ok-to-test |
k8s-ci-robot
added
sig/api-machinery
|
Hm, maybe that one's flaky? |
|
/assign @yliaog |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
@liggitt / @caesarxuchao anything else this needs? |
| // GetPorts will return the ports that were forwarded; this can be used to | ||
| // retrieve the locally-bound port in cases where the input was port 0. | ||
| func (pf *PortForwarder) GetPorts() []ForwardedPort { | ||
| return pf.ports |
There was a problem hiding this comment.
calls to this for purposes of retrieving auto-assigned port numbers are necessarily made from a separate goroutine than the one that called ForwardPorts() (since that method blocks until the operation is complete). That means that anything using the value returned from this method is subject to data races with the port.Local = uint16(localPortUInt) assignment.
At the very least, we need to protect against that data race.
A bigger question is whether this method should block until the ForwardPorts() call has established listeners and the actual local ports are available.
There was a problem hiding this comment.
Originally I was going to leave it up to the caller to avoid the race, since any user of this would also have to know about Ready, and once Ready is signaled, the local ports are available, too. I perhaps should have documented that in the func doc.
(This code is meant specifically to close races like this.)
If you prefer, though, I can move the exclusion to here, no big deal. My first inclination would perhaps to be something like
close(pf.listenersReady)
if pf.Ready != nil {
close(pf.Ready)
}
...
func (pf *PortForwarder) GetPorts() ([]ForwardedPort, error) {
select {
case <-pf.listenersReady:
return pf.ports, nil
default:
return nil, fmt.Errorf("listeners not ready")
}
}A caller would already be doing its own blocking on the Ready channel (if they cared to) so leaving GetPorts nonblocking seems fine, but I'll defer toYourJudgement() on that (obviously, not a huge code change either way). :)
There was a problem hiding this comment.
or, could require a Ready channel be specified in order to use GetPorts() (which they have to provide if they want to call GetPorts() safely)... what do you think of something like this:
func (pf *PortForwarder) GetPorts() ([]ForwardedPort, error) {
if pf.Ready == nil {
return nil, fmt.Errorf("no Ready channel provided")
}
select {
case <-pf.Ready:
return pf.ports, nil
default:
return nil, fmt.Errorf("listeners not ready")
}
}There was a problem hiding this comment.
SGTM. Updated the PR.
Without this change, the only method to discover what local port was bound (if port 0 was requested) is to parse it out of the "out" stream, which isn't the most reliable method.
k8s-ci-robot
added
size/M
|
/lgtm |
k8s-ci-robot
added
lgtm
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, novas0x2a, yliaog 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 |
|
I guess the next step here is a milestone assignment? I'm not sure what the process is for that (this is my first contribution, obviously). Is there something I need to do? |
Master is currently limited to features and bug fixes specifically targeted at 1.12. Once it opens again, this will merge without any further action. |
|
/retest Review the full test history for this PR. Silence the bot with an |
|
/retest Review the full test history for this PR. Silence the bot with an |
1 similar comment
|
/retest Review the full test history for this PR. Silence the bot with an |
What this PR does / why we need it:
The port-forwarding mechanism in client-go currently doesn't provide any
convenient method to retrieve the locally-bound port in cases where the
port requested was "0" (use any port). (It's only possible by parsing it
out of the output log stream).
Special notes for your reviewer:
Because this is a non-breaking client API change with no end-user-visible
changes (just end-developer changes :) ). I've marked this "no release
note." Let me know if you disagree and I'll add one.
Release note: