Skip to content
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

Fix localport open with --nodeport-addresses specified #62003

Merged
merged 4 commits into from May 14, 2018

Conversation

@m1093782566
Copy link
Member

m1093782566 commented Apr 2, 2018

What this PR does / why we need it:

Fix localport open with --nodeport-addresses specified.

Which issue(s) this PR fixes:
Fixes #61953

Special notes for your reviewer:

@ephur

Release note:

Services can listen on same host ports on different interfaces with --nodeport-addresses specified
@m1093782566

This comment has been minimized.

Copy link
Member Author

m1093782566 commented Apr 2, 2018

/assign @thockin

@rramkumar1
Copy link
Member

rramkumar1 left a comment

Added some comments. Also, can you please change the release note to something more user-friendly? I don't think including an implementation detail (i.e LocalPort) makes sense for a release note.

Protocol: protocol,
addresses, err := utilproxy.GetNodeAddresses(proxier.nodePortAddresses, proxier.networkInterfacer)
if err != nil {
glog.Errorf("Failed to get node ip address matching nodeport cidr")

This comment has been minimized.

@rramkumar1

rramkumar1 Apr 2, 2018

Member

Change error to read something like: glog.Errorf("Failed to get nodeport ip addresses: %v", err). Basically, include the error from the GetNodeAddresses call in some way. Same comment for ipvs code

This comment has been minimized.

@m1093782566

m1093782566 May 2, 2018

Author Member

Done.

Protocol: protocol,
}
lps = append(lps, lp)
break

This comment has been minimized.

@rramkumar1

rramkumar1 Apr 2, 2018

Member

Can you add a comment here as to why you are breaking? I assume it is because if you encounter a zero CIDR, then there is no point in processing the rest of the addresses? Same comment for ipvs code

This comment has been minimized.

@m1093782566

m1093782566 May 2, 2018

Author Member

Yes, you are right. Added the comment now. THANKS!

lps = append(lps, lp)
}

for _, lp := range lps {

This comment has been minimized.

@rramkumar1

rramkumar1 Apr 2, 2018

Member

Short comment here please. Same for ipvs code.

This comment has been minimized.

@m1093782566

m1093782566 May 2, 2018

Author Member

Fixed.

@ephur

This comment has been minimized.

Copy link

ephur commented Apr 21, 2018

Verified the functionality in our deployments, we're currently running a forked kube-proxy so would love to get back to mainline! Thanks for this work.

root@ip-10-0-38-21:/etc/systemd/system# netstat -anpe | grep LISTEN | grep -E '(kube-proxy|envoy)'                                                            
tcp        0      0 127.0.0.1:30000         0.0.0.0:*               LISTEN      0          334186      26202/kube-proxy                                       
tcp        0      0 10.0.38.21:10256        0.0.0.0:*               LISTEN      0          333634      26202/kube-proxy                                       
tcp        0      0 127.0.0.1:30001         0.0.0.0:*               LISTEN      0          334197      26202/kube-proxy                                       
tcp        0      0 127.0.0.1:31761         0.0.0.0:*               LISTEN      0          334175      26202/kube-proxy                                       
tcp        0      0 10.0.38.21:31761        0.0.0.0:*               LISTEN      0          317246      17597/envoy                                            
tcp        0      0 127.0.0.1:30900         0.0.0.0:*               LISTEN      0          334164      26202/kube-proxy                                       
tcp        0      0 127.0.0.1:30902         0.0.0.0:*               LISTEN      0          334153      26202/kube-proxy                                       
tcp        0      0 0.0.0.0:1080            0.0.0.0:*               LISTEN      0          317240      17597/envoy                                            
tcp        0      0 127.0.0.1:30144         0.0.0.0:*               LISTEN      0          334131      26202/kube-proxy                                       
tcp        0      0 10.0.38.21:30144        0.0.0.0:*               LISTEN      0          317249      17597/envoy                                            
tcp        0      0 127.0.0.1:32258         0.0.0.0:*               LISTEN      0          334142      26202/kube-proxy                                       
tcp        0      0 10.0.38.21:32258        0.0.0.0:*               LISTEN      0          317247      17597/envoy                                            
tcp        0      0 127.0.0.1:30248         0.0.0.0:*               LISTEN      0          334208      26202/kube-proxy                                       
tcp        0      0 10.0.38.21:30248        0.0.0.0:*               LISTEN      0          317248      17597/envoy                                            
tcp        0      0 127.0.0.1:10249         0.0.0.0:*               LISTEN      0          333636      26202/kube-proxy  
@rramkumar1

This comment has been minimized.

Copy link
Member

rramkumar1 commented Apr 23, 2018

@m1093782566

This comment has been minimized.

Copy link
Member Author

m1093782566 commented May 2, 2018

@rramkumar1 @ephur

Sorry for late response, all comments are fixed now. PTAL, THANKS!

I want this fix to get in earlier to meet @ephur's need :)

glog.Errorf("can't open %s, skipping this nodePort: %v", lp.String(), err)
continue

lps := make([]utilproxy.LocalPort, 0)

This comment has been minimized.

@rramkumar1

rramkumar1 May 9, 2018

Member

Can I suggest rewriting this in this way:

lps := make([]utilproxy.LocalPort, 0)
for address := range addresses {
	lp := utilproxy.LocalPort{`
		Description: "nodePort for " + svcNameString,
		IP: address,
		Port: svcInfo.NodePort,
		Protocol: protocol,
	}
	if utilproxy.IsZeroCIDR(address) {
		lp.IP = ""
                lps.append(lps, lp)
		// If we encounter a zero CIDR, then there is no point in processing the rest of the addresses.	
		break
	} 
        lps.append(lps, lp)
}

Saves you 10 lines if you do it here and in the ipvs code :)

This comment has been minimized.

@m1093782566

m1093782566 May 14, 2018

Author Member

Definitely make sense! It's fixed now. PTAL.

THANKS!

@rramkumar1

This comment has been minimized.

Copy link
Member

rramkumar1 commented May 14, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label May 14, 2018

@thockin

This comment has been minimized.

Copy link
Member

thockin commented May 14, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 14, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: m1093782566, rramkumar1, thockin

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 14, 2018

Automatic merge from submit-queue (batch tested with PRs 63787, 62003). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit fc28745 into kubernetes:master May 14, 2018

16 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation m1093782566 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-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.