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 should resolve service port to target port #59809

Merged
merged 1 commit into from Feb 16, 2018

Conversation

Projects
None yet
4 participants
@phsiao
Contributor

phsiao commented Feb 13, 2018

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

This comment has been minimized.

Contributor

phsiao commented Feb 13, 2018

/sig cli

@phsiao

This comment has been minimized.

Contributor

phsiao commented Feb 13, 2018

@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.

if err != nil {
return err
}
o.PodName = forwardablePod.Name
if cmdutil.GetFlagBool(cmd, "disable-target-port-translation") == false {

This comment has been minimized.

@liggitt

liggitt Feb 13, 2018

Member

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

This comment has been minimized.

@phsiao

phsiao Feb 13, 2018

Contributor

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.

This comment has been minimized.

@liggitt

liggitt Feb 13, 2018

Member

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

This comment has been minimized.

@phsiao

phsiao Feb 13, 2018

Contributor

Removed.

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)

This comment has been minimized.

@liggitt

liggitt Feb 13, 2018

Member

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

This comment has been minimized.

@phsiao

phsiao Feb 13, 2018

Contributor

Yep, I will give that a shot.

This comment has been minimized.

@phsiao

phsiao Feb 13, 2018

Contributor

Done.

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

This comment has been minimized.

@liggitt

liggitt Feb 13, 2018

Member

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).

This comment has been minimized.

@phsiao

phsiao Feb 13, 2018

Contributor

Will fix that and add a test case for that.

This comment has been minimized.

@phsiao

phsiao Feb 13, 2018

Contributor

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.

This comment has been minimized.

@liggitt

liggitt Feb 13, 2018

Member

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?

This comment has been minimized.

@phsiao

phsiao Feb 13, 2018

Contributor

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.

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

This comment has been minimized.

@liggitt

liggitt Feb 13, 2018

Member

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

This comment has been minimized.

@phsiao

phsiao Feb 13, 2018

Contributor

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.

This comment has been minimized.

@liggitt

liggitt Feb 13, 2018

Member

Probably so

This comment has been minimized.

@phsiao

phsiao Feb 14, 2018

Contributor

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.

This comment has been minimized.

@phsiao

phsiao Feb 14, 2018

Contributor

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

This comment has been minimized.

@liggitt

liggitt Feb 14, 2018

Member

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

This comment has been minimized.

@phsiao

phsiao Feb 14, 2018

Contributor

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.

@k8s-ci-robot k8s-ci-robot added size/XL and removed size/L labels Feb 14, 2018

// 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" {

This comment has been minimized.

@liggitt

liggitt Feb 14, 2018

Member

nit: use the api constant

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

This comment has been minimized.

@phsiao

phsiao Feb 14, 2018

Contributor

Fixed.

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

This comment has been minimized.

@liggitt

liggitt Feb 14, 2018

Member

just return here

}
}
if matchfound == false {

This comment has been minimized.

@liggitt

liggitt Feb 14, 2018

Member

unconditional error here

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

This comment has been minimized.

@liggitt

liggitt Feb 14, 2018

Member

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

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

This comment has been minimized.

@liggitt

liggitt Feb 14, 2018

Member

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

This comment has been minimized.

@phsiao

phsiao Feb 14, 2018

Contributor

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.

This comment has been minimized.

@phsiao

phsiao Feb 14, 2018

Contributor

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.

This comment has been minimized.

@liggitt

liggitt Feb 14, 2018

Member

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

This comment has been minimized.

@phsiao

phsiao Feb 14, 2018

Contributor

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.

This comment has been minimized.

@liggitt

liggitt Feb 14, 2018

Member

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

@liggitt

This comment has been minimized.

Member

liggitt commented Feb 14, 2018

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

@liggitt

This comment has been minimized.

@phsiao

This comment has been minimized.

Contributor

phsiao commented Feb 14, 2018

Cool, thanks for the help.

@k8s-merge-robot k8s-merge-robot merged commit 01ec7a9 into kubernetes:master Feb 16, 2018

9 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
pull-kubernetes-e2e-gce Job triggered.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job triggered.
Details
pull-kubernetes-verify Job triggered.
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-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

@wojtek-t wojtek-t referenced this pull request Mar 21, 2018

Closed

[test flakes] master-scalability suites #60589

3 of 3 tasks complete

This was referenced Aug 6, 2018

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