AWS: handle multiple IPs when using more than 1 network interface per ec2 instance #50112

jlzhao27 · 2017-08-03T22:50:57Z

What this PR does / why we need it:
Adds support for kubelets running with the AWS cloud provider on ec2 instances with multiple network interfaces. If the active interface is not eth0, the AWS cloud provider currently reports the wrong node IP.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #44686

Special notes for your reviewer:
There is also some work necessary for handling multiple DNS names and such but I didn't fix them in this PR.

Release note:

Fixed bug in AWS provider to handle multiple IPs when using more than 1 network interface per ec2 instance.

k8s-ci-robot · 2017-08-03T22:50:59Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2017-08-03T22:51:04Z

Hi @jlz27. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

jlzhao27 · 2017-08-03T23:02:49Z

/assign @justinsb

dims · 2017-08-17T11:05:50Z

/ok-to-test

justinsb · 2017-08-17T11:59:59Z

A few nits, but mostly about making the code more defensive, and generally looks good as long as it passes tests!

justinsb · 2017-08-17T11:51:23Z

pkg/cloudprovider/providers/aws/aws.go

+				return nil, fmt.Errorf("error querying AWS metadata for %q: %q", macPath, err)
+			}
+			for _, internalIP := range strings.Split(internalIPs, "\n") {
+				addresses = append(addresses, v1.NodeAddress{Type: v1.NodeInternalIP, Address: internalIP})


And probably best to skip empty strings here also, but again not needed

justinsb · 2017-08-17T11:51:51Z

pkg/cloudprovider/providers/aws/aws.go

+		}
+
+		for _, macID := range strings.Split(macs, "\n") {
+			macPath := path.Join("network/interfaces/macs/", macID, "local-ipv4s")


Probably good to skip empty strings here just in case, but I don't think it is needed for the EC2 metadata service.

justinsb · 2017-08-17T11:54:18Z

pkg/cloudprovider/providers/aws/aws.go

+	// handle internal network interfaces
+	for _, networkInterface := range instance.NetworkInterfaces {
+		// skip network interfaces that are not currently in use
+		if isNilOrEmpty(networkInterface.Status) || *networkInterface.Status != ec2.NetworkInterfaceStatusInUse {


Nit: you can probably use if aws.StringValue(networkInterface.Status) != ec2.NetworkInterfaceStatusInUse

justinsb · 2017-08-17T11:54:56Z

pkg/cloudprovider/providers/aws/aws.go

+				ipAddress := *internalIP.PrivateIpAddress
+				ip := net.ParseIP(ipAddress)
+				if ip == nil {
+					return nil, fmt.Errorf("EC2 instance had invalid private address: %s (%s)", orEmpty(instance.InstanceId), ipAddress)


In general, we're trying to use aws.StringValue instead of orEmpty. orEmpty predates aws.StringValue!

Also something I've learned from bitter experience - always use %q when printing an unparseable value, because you want to see the whitespace / invalid characters!

(Though I realize this is existing code, so no need to change it in this PR!)

fixed instance of orEmpty in the changed code, good call on %q

jlzhao27 · 2017-08-30T18:54:54Z

@justinsb ping on this, I thought all the test passed last week but now it looks like the bazel builds are waiting again?

Let me know if there is anything else I can do to push this through.

justinsb · 2017-08-31T15:01:01Z

I think it's just hung. Going to close and reopen as that often kicks things off again.

Can I ask though - what is the use case here? Because I've realized that we may also want to use multiple ENIs for pods with dedicated ENIs in future. It's a lot of work and the limits are currently a bit low, so I'm not sure that will ever actually come to fruition, but I just want to check that what we're doing here is worth it! (i.e. why are you giving your instances multiple IPs / ENIs?)

justinsb · 2017-08-31T15:01:39Z

/retest

jlzhao27 · 2017-08-31T16:13:11Z

Yep, so the concrete thing that this blocks right now if you want to run a pod with hostNetwork on an instance with multiple ENIs. The problem i ran into was that we disabled eth0 and attached a separate ENI as eth1, when using the AWS provider, specifying a kubelet IP for eth1 did not work because this parsing wasn't returning all possible IP addrs.

jlzhao27 · 2017-08-31T16:14:04Z

/test pull-kubernetes-federation-e2e-gce

jlzhao27 · 2017-08-31T16:14:12Z

/test pull-kubernetes-e2e-gce-bazel

jlzhao27 · 2017-08-31T16:21:35Z

Cool, using ENIs for pods sounds pretty interesting. I think that implementation will need to be aware of the implications of users swapping out the base ENI for the instance since kubelets shouldn't care about how the user set up the AWS cluster. For example, I could attach 2 ENIs to my instance to handle public/private traffic and would like the kubelet to bind correctly to the private ENI.

jlzhao27 · 2017-08-31T17:36:34Z

/test pull-kubernetes-e2e-kops-aws

jlzhao27 · 2017-08-31T19:37:28Z

/test pull-kubernetes-e2e-kops-aws

justinsb · 2017-09-01T02:14:25Z

/lgtm

I suspect you're right, that if we ever do ENI-per-pod (or similar) that we'll need special handling anyway.

justinsb · 2017-09-01T02:14:47Z

/approve

k8s-github-robot · 2017-09-01T02:15:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlz27, justinsb

Associated issue: 44686

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/cloudprovider/providers/aws/OWNERS~~ [justinsb]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-09-03T06:50:02Z

Automatic merge from submit-queue (batch tested with PRs 51301, 50497, 50112, 48184, 50993)

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Aug 3, 2017

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 Aug 3, 2017

k8s-github-robot assigned gnufied and jsafrane Aug 3, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Aug 3, 2017

support multiple ec2 ips in aws provider

43e606a

jlzhao27 force-pushed the multiple-ips branch from cae666b to 43e606a Compare August 3, 2017 22:58

k8s-ci-robot assigned justinsb Aug 3, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 3, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 17, 2017

justinsb reviewed Aug 17, 2017

View reviewed changes

jlzhao27 added 2 commits August 21, 2017 14:21

addressed comments

8b16c3d

run go fmt

35777b2

justinsb changed the title ~~support multiple ec2 ips in aws provider~~ AWS: handle multiple IPs when using more than 1 network interface per ec2 instance Aug 31, 2017

justinsb removed the release-note-label-needed label Aug 31, 2017

justinsb closed this Aug 31, 2017

justinsb reopened this Aug 31, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 1, 2017

justinsb added this to the v1.8 milestone Sep 1, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 1, 2017

k8s-github-robot merged commit 9341f22 into kubernetes:master Sep 3, 2017

jlzhao27 deleted the multiple-ips branch September 4, 2017 05:20

rtripat mentioned this pull request Apr 11, 2018

Kubelet reports secondary InternalIP in AWS with multiple ENIs #61921

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AWS: handle multiple IPs when using more than 1 network interface per ec2 instance #50112

AWS: handle multiple IPs when using more than 1 network interface per ec2 instance #50112

jlzhao27 commented Aug 3, 2017 •

edited

Loading

k8s-ci-robot commented Aug 3, 2017

k8s-ci-robot commented Aug 3, 2017

jlzhao27 commented Aug 3, 2017 •

edited

Loading

dims commented Aug 17, 2017

justinsb commented Aug 17, 2017

justinsb Aug 17, 2017

jlzhao27 Aug 21, 2017

justinsb Aug 17, 2017

jlzhao27 Aug 21, 2017

justinsb Aug 17, 2017

justinsb Aug 17, 2017

justinsb Aug 17, 2017

justinsb Aug 17, 2017

jlzhao27 Aug 21, 2017

jlzhao27 commented Aug 30, 2017

justinsb commented Aug 31, 2017 •

edited

Loading

justinsb commented Aug 31, 2017

jlzhao27 commented Aug 31, 2017

jlzhao27 commented Aug 31, 2017

jlzhao27 commented Aug 31, 2017

jlzhao27 commented Aug 31, 2017 •

edited

Loading

jlzhao27 commented Aug 31, 2017

jlzhao27 commented Aug 31, 2017

justinsb commented Sep 1, 2017

justinsb commented Sep 1, 2017

k8s-github-robot commented Sep 1, 2017

k8s-github-robot commented Sep 3, 2017

AWS: handle multiple IPs when using more than 1 network interface per ec2 instance #50112

AWS: handle multiple IPs when using more than 1 network interface per ec2 instance #50112

Conversation

jlzhao27 commented Aug 3, 2017 • edited Loading

k8s-ci-robot commented Aug 3, 2017

k8s-ci-robot commented Aug 3, 2017

jlzhao27 commented Aug 3, 2017 • edited Loading

dims commented Aug 17, 2017

justinsb commented Aug 17, 2017

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

jlzhao27 commented Aug 30, 2017

justinsb commented Aug 31, 2017 • edited Loading

justinsb commented Aug 31, 2017

jlzhao27 commented Aug 31, 2017

jlzhao27 commented Aug 31, 2017

jlzhao27 commented Aug 31, 2017

jlzhao27 commented Aug 31, 2017 • edited Loading

jlzhao27 commented Aug 31, 2017

jlzhao27 commented Aug 31, 2017

justinsb commented Sep 1, 2017

justinsb commented Sep 1, 2017

k8s-github-robot commented Sep 1, 2017

k8s-github-robot commented Sep 3, 2017

jlzhao27 commented Aug 3, 2017 •

edited

Loading

jlzhao27 commented Aug 3, 2017 •

edited

Loading

justinsb commented Aug 31, 2017 •

edited

Loading

jlzhao27 commented Aug 31, 2017 •

edited

Loading