kubectl port-forward should resolve service port to target port #59809

phsiao · 2018-02-13T15:20:40Z

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:

`kubectl port-forward` now supports specifying a service to port forward to: `kubectl port-forward svc/myservice 8443:443`

phsiao · 2018-02-13T15:21:07Z

/sig cli

phsiao · 2018-02-13T15:22:34Z

@liggitt PTAL. This one is built on the commit in PR #59705 so if this one is reasonable we can close the other one. But if we still need to iterate on the interface of targetPort resolution, I'd prefer #59705 be approved and merged first.

liggitt · 2018-02-13T17:35:36Z

pkg/kubectl/cmd/portforward.go

 	if err != nil {
 		return err
 	}

+	o.PodName = forwardablePod.Name
+
+	if cmdutil.GetFlagBool(cmd, "disable-target-port-translation") == false {


I'd avoid adding the translation flag until we have a clear need for it

Was thinking to have an escape hatch with this flag, thus default to false, in case there are use cases that I can't conceive. I have no problem removing it and added it back later if needed. I will do that also.

port-forwarding to service is new function, so let's start small and iterate as needed... someone can always go look the pod up themselves if they want to target a specific port on the pod

liggitt · 2018-02-13T17:36:08Z

pkg/kubectl/cmd/portforward.go

+			pod := &v1.Pod{}
+			svc := &v1.Service{}
+			apiv1.Convert_core_Pod_To_v1_Pod(forwardablePod, pod, nil)
+			apiv1.Convert_core_Service_To_v1_Service(t, svc, nil)


I think importing and manually converting is worse that just writing the port mapping to work on internal service and pod objects

Yep, I will give that a shot.

liggitt · 2018-02-13T18:04:51Z

pkg/kubectl/util/service_port.go

+		if svc.Spec.ClusterIP == "None" {
+			mapping[svcportspec.Port] = svcportspec.Port
+		} else {
+			if svcportspec.TargetPort.Type == intstr.Int {


TargetPort can be omitted, and in that case, mapping[svcportspec.Port] = svcportspec.Port should be used:

If this is not specified, the value of the 'port' field is used (an identity map).

Will fix that and add a test case for that.

I think the only safe way to detect omitted is to assume the IntValue() is 0, but if there is a better way please let me know.

hmm, looks like defaulting takes care of this for you and sets targetPort to match port when omitted (https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/core/v1/defaults.go#L120). Can you verify that is the case?

I think it does. I can delete targetPort from a service, and it gets regenerated. I think that is why I did not notice this case before. However, since this code now support internal object, and may be reused by others, it is probably safer to assume defaulting may not happen and leave it as is.

liggitt · 2018-02-13T20:55:17Z

pkg/kubectl/util/service_port.go

+					mapping[svcportspec.Port] = int32(svcportspec.TargetPort.IntValue())
+				}
+			} else {
+				portnum, err := LookupContainerPortNumberByName(pod, svcportspec.TargetPort.String())


this has the potential to return an error and short-circuit on a port that we're not even going to use... you could just omit the target from the mapping (or map to zero), and handle missing cases in translateServicePortToTargetPort if we actually try to make use of ports that don't have corresponding ports in the pod

Would it be better if we just look up as we process the arguments, and the error would be much clear? I did the gathering first because it was easier for me to implement at the time.

Probably so

Realized that lookup failure can't be treated as failures, because service/pod is not required to declare every port a pod listens on, so the point of surfacing clear error is irrelevant.

Actually, I think it would work by surfacing only the named port lookup error.

services are required to declare the ports they surface, and pods must declare a named port for named mappings to work

I think it would work by surfacing only the named port lookup error

that sounds right

services are required to declare the ports they surface

So in that expectation, if a Service does not declare a port, but the user still wants to port-forward to that port, should that be treated as error? Right now I am pass it as-is to match the use case for other types of resources.

liggitt · 2018-02-14T14:38:54Z

pkg/kubectl/util/service_port.go

+// It returns an error when a named port can't find a match (with -1 returned), or when the service does not
+// specify such port (with the input port number returned).
+func LookupContainerPortNumberByServicePort(svc api.Service, pod api.Pod, port int32) (int32, error) {
+	if svc.Spec.ClusterIP == "None" {


nit: use the api constant

kubernetes/staging/src/k8s.io/api/core/v1/types.go

Lines 3580 to 3584 in 0dda5c8

const (

// ClusterIPNone - do not assign a cluster IP

// no proxying required and no environment variables should be created for pods

ClusterIPNone = "None"

)

liggitt · 2018-02-14T14:40:37Z

pkg/kubectl/util/service_port.go

+	for _, ctr := range pod.Spec.Containers {
+		for _, ctrportspec := range ctr.Ports {
+			if ctrportspec.Name == name {
+				namedport = ctrportspec.ContainerPort


just return here

liggitt · 2018-02-14T14:41:06Z

pkg/kubectl/util/service_port.go

+		}
+	}
+
+	if matchfound == false {


unconditional error here

liggitt · 2018-02-14T14:41:24Z

pkg/kubectl/util/service_port.go

+				return int32(svcportspec.TargetPort.IntValue()), nil
+			}
+		} else {
+			ret, err := LookupContainerPortNumberByName(pod, svcportspec.TargetPort.String())


just return LookupContainerPortNumberByName(pod, svcportspec.TargetPort.String())

liggitt · 2018-02-14T14:43:13Z

pkg/kubectl/cmd/portforward.go

+		}
+		containerPort, err := util.LookupContainerPortNumberByServicePort(svc, pod, int32(portnum))
+		if err != nil {
+			if containerPort <= 0 {


error in all cases here... if the service didn't declare the port, I don't think it's valid to try to port-forward to it

I think we went back to the original behavior (using gathered mapping), but it is much more clear how and why we reached the error.

Found another corner case. If ClusterIP: None, the current behavior would return the port as-is, even if the port is not declared by the service. Let me fix that also.

If ClusterIP=None, then no proxying occurs today, right? does it make sense to support port-forward on such a service?

I think it makes sense to support port-forward even for ClusterIP=None. A common use case for me is for pods with NetworkPolicy that restrict general access, so port-forward (not proxy) is a way for reaching it for testing/verification without changing the NetworkPolicy.

ok, with the change to still require the service to declare the port, that seems reasonable

liggitt · 2018-02-14T15:34:12Z

thanks, go ahead and squash and we'll kick off CI

liggitt · 2018-02-14T15:34:27Z

@kubernetes/sig-network-pr-reviews
@kubernetes/sig-cli-pr-reviews

phsiao · 2018-02-14T15:40:44Z

Cool, thanks for the help.

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 13, 2018

k8s-ci-robot requested review from ericchiang and smarterclayton February 13, 2018 15:20

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 13, 2018

k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label Feb 13, 2018

This was referenced Feb 13, 2018

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

Merged

kubectl port-forward should support re-attach #59816

Closed

liggitt reviewed Feb 13, 2018

View reviewed changes

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 14, 2018

liggitt reviewed Feb 14, 2018

View reviewed changes

pkg/kubectl/util/service_port.go Outdated

}

}

if matchfound == false {

Copy link

Member

liggitt Feb 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unconditional error here

liggitt reviewed Feb 14, 2018

View reviewed changes

k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Feb 14, 2018

mxxk mentioned this pull request Aug 5, 2018

kubectl port-forward should allow forwarding to a Service #15180

Closed

montmanu mentioned this pull request Oct 8, 2020

Update the example for kubectl port-forward #95383

Merged

timbonicus mentioned this pull request Apr 15, 2021

Move deployment documentation to its own section backstage/backstage#5337

Merged

5 tasks

	const (
	// ClusterIPNone - do not assign a cluster IP
	// no proxying required and no environment variables should be created for pods
	ClusterIPNone = "None"
	)

kubectl port-forward should resolve service port to target port #59809

kubectl port-forward should resolve service port to target port #59809

Conversation

phsiao commented Feb 13, 2018 • edited by liggitt Loading

phsiao commented Feb 13, 2018

phsiao commented Feb 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Feb 14, 2018

liggitt commented Feb 14, 2018

phsiao commented Feb 14, 2018

phsiao commented Feb 13, 2018 •

edited by liggitt

Loading

liggitt Feb 13, 2018 •

edited

Loading