-
Notifications
You must be signed in to change notification settings - Fork 39.4k
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
port-forward listen on address #46517
port-forward listen on address #46517
Conversation
@goblain Are you still working on this? If so, please rebase. I can help review this. |
/assign |
d90d95c
to
d43f8d5
Compare
d90d95c
to
93d7a96
Compare
@mengqiy please take a look at this |
/ok-to-test |
66f0c8c
to
187b337
Compare
/retest |
1 similar comment
/retest |
@goblain Any update on the failing test? |
187b337
to
a27924e
Compare
a27924e
to
d90d95c
Compare
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.
/lgtm
/approve
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.
Actually I was wondering whether we could use 10.19.21.23:8888:5000
to indicate port 8888 open on 10.19.21.23 and forward to 5000 in the pod.
@@ -50,6 +50,7 @@ type PortForwardOptions struct { | |||
RESTClient *restclient.RESTClient | |||
Config *restclient.Config | |||
PodClient corev1client.PodsGetter | |||
Address []string |
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.
s/Address/Addresses
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.
LocalAddresses?
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.
(but see my suggestion to only serve one local address)
@@ -113,6 +120,7 @@ func NewCmdPortForward(f cmdutil.Factory, streams genericclioptions.IOStreams) * | |||
}, | |||
} | |||
cmdutil.AddPodRunningTimeoutFlag(cmd, defaultPodPortForwardWaitTimeout) | |||
cmd.Flags().StringSliceVar(&opts.Address, "address", []string{"localhost"}, "Addresses to listen on (comma separated)") |
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.
I suggest "local-addresses"?
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.
address
was selected as it's the same as for ie. kubectl proxy
, so makes sense to stay coherent
var addresses []listenAddress | ||
parsed := make(map[string]listenAddress) | ||
for _, address := range addressesToParse { | ||
if address == "localhost" { |
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.
This looks wrong. I think you need to resolve the name rather than hard coding this stuff.
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.
Likely you should attempt resolving any name that isn't an IP address.
func New(dialer httpstream.Dialer, ports []string, stopChan <-chan struct{}, readyChan chan struct{}, out, errOut io.Writer) (*PortForwarder, error) { | ||
return NewOnAddresses(dialer, []string{"localhost"}, ports, stopChan, readyChan, out, errOut) |
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.
Perhaps explicitly say 127.0.0.1 & ::1 here, then you can remove the special case in the parse function?
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.
In a followup, I think we should do this after confirm that the address exists, if possible.
return NewOnAddresses(dialer, []string{"localhost"}, ports, stopChan, readyChan, out, errOut) | ||
} | ||
|
||
// NewOnAddresses creates a new PortForwarder with custom listen addresses. |
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.
Document that addresses must all be valid IPv4 or IPv6 addresses, no hostnames?
/approve I think other changes can be made in a followup. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: goblain, lavalamp, soltysh 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 |
Wow. This is awesome. |
This is great!
|
@Dimagog it should be in this prerelease: https://github.com/kubernetes/kubernetes/releases/tag/v1.13.0-alpha.1 |
@mengqiy Yes, latest binary from below has the |
…elease-1.12 Partial cherry-pick of #46517 to fix flaky e2e skew tests
What this PR does / why we need it:
Implements #43962 proposal. Adds
--address
flag to port-forward command that allows listening on addresses other then localhost, so that port-forward can ie. be opened to consumers other then residing in local host like running in docker or different machine/vmWhich issue this PR fixes:
fixes #43962, fixes #36152, fixes #29678
Release note: