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

kubectl port-forward allows using resource name to select a matching pod #59705

Merged
merged 1 commit into from Feb 13, 2018

Conversation

Projects
None yet
7 participants
@phsiao
Contributor

phsiao commented Feb 10, 2018

What this PR does / why we need it:

#15180 describes use cases that port-foward should use resource name for selecting a pod.

Which issue(s) this PR fixes:

Add support so resource/name can be used to select a pod.

Special notes for your reviewer:

I decided to reuse AttachablePodForObject to select a pod using resource name, and extended it to support Service (which it did not). I think that should not be a problem, and may help improve attach's use case. If it makes more sense to fork the function I'd be happy to do so. The practice of waiting for pods to become ready is also copied over.

In keeping the change to minimal, I also decided to resolve pod from resource name in Complete(), following the pattern in attach.

Release note:

kubectl port-forward now allows using resource name (e.g., deployment/www) to select a matching pod, as well as allows the use of --pod-running-timeout to wait till at least one pod is running.
kubectl port-forward no longer support deprecated -p flag
@kargakis

This comment has been minimized.

Member

kargakis commented Feb 11, 2018

@phsiao

This comment has been minimized.

Contributor

phsiao commented Feb 11, 2018

/retest

}
o.PodName = forwardablePod.Name
o.Ports = args[1:]

This comment has been minimized.

@liggitt

liggitt Feb 11, 2018

Member

If we port forward a service, wouldn't the specified port be the service port, and would we need to figure out what port on the resolved pod that mapped to?

This comment has been minimized.

@phsiao

phsiao Feb 11, 2018

Contributor

It is likely the intended port would be the service port, but then the user still need to specify what local port to bind to so it is not clear to me what it can help here. Also it is possible that a service has multiple service ports, and the user may not want to bind all of them. I feel it would end up complicate the UI and not simplify it.

The same mechanism also supports other resource types such as statefulset, deployment, or job, so it is probably cleaner to make less assumptions about the ports.

This comment has been minimized.

@liggitt

liggitt Feb 11, 2018

Member

then the user still need to specify what local port to bind to so it is not clear to me what it can help here

The user already has the ability to specify local/remote mappings. I'm saying the remote port should resolve the service port to determine what pod port to forward to, not assume the port number specified by the user is the port to use on the pod.

The same mechanism also supports other resource types such as statefulset, deployment, or job, so it is probably cleaner to make less assumptions about the ports.

None of those provide mechanisms for port indirections like services do

This comment has been minimized.

@phsiao

phsiao Feb 11, 2018

Contributor

I see, let's me see if I get it right this time. I think you are suggesting that it should also resolve spec.ports.port -> spec.ports.targetPort if resource type is a service.

In that case, I think we overload the design of port-forward to a pod to become port-forward to a service, and I am not sure if it should be in the scope of this PR.

This comment has been minimized.

@liggitt

liggitt Feb 11, 2018

Member

I think you are suggesting that it should also resolve spec.ports.port -> spec.ports.targetPort if resource type is a service.

Yes

I am not sure if it should be in the scope of this PR.

Agree. It could be a follow up. I think it is useful enough to consider doing, and a natural fit for this command.

This comment has been minimized.

@phsiao

phsiao Feb 11, 2018

Contributor

I will enter an issue to track that.

This comment has been minimized.

@liggitt

liggitt Feb 11, 2018

Member

I think there already is one.

@@ -403,6 +403,10 @@ func (f *ring1Factory) AttachablePodForObject(object runtime.Object, timeout tim
return nil, fmt.Errorf("invalid label selector: %v", err)
}
case *api.Service:
namespace = t.Namespace
selector = labels.SelectorFromSet(t.Spec.Selector)

This comment has been minimized.

@liggitt

liggitt Feb 11, 2018

Member

Probably only applies to services with selectors… not all services have them

This comment has been minimized.

@phsiao

phsiao Feb 11, 2018

Contributor

Tested with a service with no selector, and it appears the method selects all pods and returns the first one. That is probably not ideal.

I think the right thing to do here might be to return an error when t.Spec.Selector is empty/nil., to indicate to user that the selection yields no result. Do you see issues with this approach?

This comment has been minimized.

@liggitt

liggitt Feb 11, 2018

Member

I think the right thing to do here might be to return an error when t.Spec.Selector is empty/nil., to indicate to user that the selection yields no result. Do you see issues with this approach?

That sounds right, though it seems like we should defer forwarding to a service at all until we get the right port selection logic

This comment has been minimized.

@phsiao

phsiao Feb 11, 2018

Contributor

though it seems like we should defer forwarding to a service at all until we get the right port selection logic

I disagree, based on my own experience it is already a big benefit to not look up a pod before port-forward.

However, I might be biased because in our deployments service port always equals to targetPort so I may not feel the pain.

@jbeda

Looks like a reasonable start and will definitely make it easier to use port-forward.

My only nits are around deprecation and usage strings.

return err
}
forwardablePod, err := f.AttachablePodForObject(obj, getPodTimeout)

This comment has been minimized.

@jbeda

jbeda Feb 11, 2018

Contributor

This resolves to a Pod at the time that port-forward is started. This may be a little confusing to users as it (a) won't load balance across all the pods in a service and (b) won't re-attach to a good pod if this pod dies. That is especially useful across updates of a Deployment.

Regardless -- this is better than the current situation but it should probably be documented as a limitation in the command help.

This comment has been minimized.

@phsiao

phsiao Feb 12, 2018

Contributor

How about this for the command help

$ kubectl port-forward -h
Forward one or more local ports to a pod. 

Use resource type/resource name such as svc/mysvc to select a pod. Resource type defaults to 'pod' if omitted. 

If there are multiple pods matching the criteria, a pod will be selected automatically. The forwarding session ends when
the selected pod terminates, and rerun of the command is needed to resume forwarding.

Examples:
  # Listen on ports 5000 and 6000 locally, forwarding data to/from ports 5000 and 6000 in the pod
  kubectl port-forward pod/mypod 5000 6000
  
  # Listen on ports 5000 and 6000 locally, forwarding data to/from ports 5000 and 6000 in a pod selected by the service
  kubectl port-forward svc/mysvc 5000 6000
  
  # Listen on port 8888 locally, forwarding to 5000 in the pod
  kubectl port-forward pod/mypod 8888:5000
  
  # Listen on a random port locally, forwarding to 5000 in the pod
  kubectl port-forward pod/mypod :5000

Options:
  -p, --pod='': Pod name
      --pod-running-timeout=1m0s: The length of time (like 5s, 2m, or 3h, higher than zero) to wait until at least one
pod is running

Usage:
  kubectl port-forward TYPE/NAME [LOCAL_PORT:]REMOTE_PORT [...[LOCAL_PORT_N:]REMOTE_PORT_N] [options]

Use "kubectl options" for a list of global command-line options (applies to all commands).

This comment has been minimized.

@phsiao

phsiao Feb 12, 2018

Contributor

Regarding re-attach to a good pod, I see port-forward as a pod operation, and the use of resource/name to select a pod is just for convenience. I can create a separate issue to track that if that is desirable.

This comment has been minimized.

@phsiao

phsiao Feb 12, 2018

Contributor

@jbeda PTAL

@@ -70,7 +76,7 @@ func NewCmdPortForward(f cmdutil.Factory, cmdOut, cmdErr io.Writer) *cobra.Comma
},
}
cmd := &cobra.Command{
Use: "port-forward POD [LOCAL_PORT:]REMOTE_PORT [...[LOCAL_PORT_N:]REMOTE_PORT_N]",
Use: "port-forward POD|RESOURCE/NAME [LOCAL_PORT:]REMOTE_PORT [...[LOCAL_PORT_N:]REMOTE_PORT_N]",

This comment has been minimized.

@jbeda

jbeda Feb 11, 2018

Contributor

if kubectl port-forward <pod> is now deprecated, we should remove it from the usage string. It is also strange that we have a -p flag in addition to listing the resource in the arguments. I'd suggest we hide/deprecate that flag also.

Ideally we'd do some heuristicy stuff so that you could also do kubectl port-forward service foo 8080 (notice no /). That would require us to look to see if things look like a number to determine if it is the single-pod case or the type-space-name case.

This comment has been minimized.

@phsiao

phsiao Feb 12, 2018

Contributor

It is -p flag that is deprecated. The message is

-p POD is DEPRECATED and will be removed in a future version. Use port-forward POD instead.

I am not familiar with the deprecation policy in this case. I can remove the support for -p and add that to release note if that is the right thing to do for this PR.

This comment has been minimized.

@phsiao

phsiao Feb 12, 2018

Contributor

@jbeda how about modifying the message to say the -p flag will be removed in 1.11?

@jbeda

This comment has been minimized.

Contributor

jbeda commented Feb 11, 2018

/cc @ncdc as he has deep history here and might want to be aware this is happening.

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Feb 12, 2018

@jbeda

jbeda approved these changes Feb 12, 2018

@jbeda

This comment has been minimized.

Contributor

jbeda commented Feb 12, 2018

Looks good to me. Would love @liggitt to give it a 👍 too.

As for re-attach -- I think this would be a good feature. I often leave a port-forward in the background as I do a bunch of stuff and have it continue to work regardless of how the cluster gets messed with would be great. (A more subtle scenario is when the labels on a pod are changed such that it is no longer selected but still running.) Feel free to file a new issue on that one. But this is certainly a step in the right direction!

@phsiao

This comment has been minimized.

Contributor

phsiao commented Feb 12, 2018

Any preference on how we proceed with removing -p flag? I propose to modify the deprecation message to indicate that it will be removed in 1.11 (or the next release if this does not meet the timeline for 1.10), and include that in the release note.

@jbeda

This comment has been minimized.

Contributor

jbeda commented Feb 12, 2018

It looks like -p was marked for deprecation ~3 years ago: 90f4c79. I think we are good removing it right now and marking it in the release notes.

pod: execPod(),
name: "pod portforward",
podPath: "/api/" + version + "/namespaces/test/pods/foo",
fetchPodPath: "/namespaces/test/pods/foo",

This comment has been minimized.

@liggitt

liggitt Feb 12, 2018

Member

this looks like a mistake in how the test client was constructed... the fetchPodPatch you added is just podPath with the api prefix missing... we shouldn't add both of those

This comment has been minimized.

@phsiao

phsiao Feb 12, 2018

Contributor

I only spent a few min looking for a better solution than this (which is also used in https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/attach_test.go#L190). I think pkg/kubectl/cmd/util/factory_object_mapping.go is the one making the call without /api/version, but don't know enough yet to resolve that correctly.

This comment has been minimized.

@liggitt

liggitt Feb 12, 2018

Member

this fixes it:

tf.Client = &fake.RESTClient{
  VersionedAPIPath:     "/api/v1",
  ...

This comment has been minimized.

@phsiao

phsiao Feb 12, 2018

Contributor

That fixed it. I will file an issue on that so other similar cases can be fixed too.

@@ -403,6 +403,13 @@ func (f *ring1Factory) AttachablePodForObject(object runtime.Object, timeout tim
return nil, fmt.Errorf("invalid label selector: %v", err)
}
case *api.Service:

This comment has been minimized.

@liggitt

liggitt Feb 12, 2018

Member

can we move this change out of this PR to a follow-up that includes the target port mapping discussed in #59733... just want to make sure we work through those issues before including service as an option

This comment has been minimized.

@phsiao

phsiao Feb 12, 2018

Contributor

I can do that.

@k8s-ci-robot k8s-ci-robot added size/M and removed size/L labels Feb 12, 2018

@phsiao

This comment has been minimized.

Contributor

phsiao commented Feb 12, 2018

@liggitt PTAL. I squashed the commits too.

kubectl port-forward pod/mypod 5000 6000
# Listen on ports 5000 and 6000 locally, forwarding data to/from ports 5000 and 6000 in a pod selected by the service
kubectl port-forward svc/mysvc 5000 6000

This comment has been minimized.

@liggitt

liggitt Feb 13, 2018

Member

oops, didn't notice svc was still in the sample usage

This comment has been minimized.

@phsiao

phsiao Feb 13, 2018

Contributor

I can fix this if #59809 is going to need more iterations.

This comment has been minimized.

@phsiao

phsiao Feb 13, 2018

Contributor

Never mind, simple fix, pushed.

@liggitt

This comment has been minimized.

Member

liggitt commented Feb 13, 2018

/lgtm

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 13, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, phsiao

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 OWNERS Files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 13, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 13, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 1d97b6a into kubernetes:master Feb 13, 2018

12 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
cla/linuxfoundation phsiao authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@phsiao phsiao deleted the phsiao:15180_port_forward_with_resource_name branch Feb 13, 2018

k8s-merge-robot added a commit that referenced this pull request Feb 16, 2018

Merge pull request #59809 from phsiao/59733_port_forward_with_target_…
…port

Automatic merge from submit-queue (batch tested with PRs 59809, 59955). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

kubectl port-forward should resolve service port to target port

**What this PR does / why we need it**:

Continues on the work in #59705, this PR adds additional support for looking up targetPort for a service, as well as enable using svc/name to select a pod.

**Which issue(s) this PR fixes**:
Fixes #15180
Fixes #59733

**Special notes for your reviewer**:

I decided to create pkg/kubectl/util/service_port.go to contain two functions that might be re-usable.

**Release note**:
```release-note
`kubectl port-forward` now supports specifying a service to port forward to: `kubectl port-forward svc/myservice 8443:443`
```
@pragyamehta

This comment has been minimized.

pragyamehta commented Mar 13, 2018

Hi @phsiao , I can see that this feature is available in the beta version right now. When will it be rolled out as a stable version of kubectl?

@phsiao

This comment has been minimized.

Contributor

phsiao commented Mar 13, 2018

@pragyamehta it will be in 1.10 release when that is ready.

@pragyamehta

This comment has been minimized.

pragyamehta commented Mar 13, 2018

@phsiao Thanks for the quick reply. We are looking to take a dependency on this feature, but we have a requirement of using the stable version of kubectl client. Where can I get information on when we can expect 1.10 release to be rolled out?

@phsiao

This comment has been minimized.

Contributor

phsiao commented Mar 13, 2018

Here is the 1.10 release plan and timeline.

@Kuqd Kuqd referenced this pull request Aug 16, 2018

Merged

Features/e2e #315

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment