Ensure that addresses are added in network device index order #909
Conversation
k8s-ci-robot
added
release-note
|
|
k8s-ci-robot
added
cncf-cla: no
|
This issue is currently awaiting triage. If cloud-provider-aws contributors determine this is a relevant issue, they will accept it by applying the The 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. |
|
Welcome @javanthropus! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @javanthropus. 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 Once the patch is verified, the new status will be reflected by the 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. |
k8s-ci-robot
added
the
size/XS
javanthropus
force-pushed
the
ensure-network-address-order
branch
from
8fd8eee
to
4020280
Compare
k8s-ci-robot
added
cncf-cla: yes
pkg/providers/v1/aws.go
Outdated
| @@ -1543,6 +1543,11 @@ func extractIPv4NodeAddresses(instance *ec2.Instance) ([]v1.NodeAddress, error) | |||
|
|
|||
| addresses := []v1.NodeAddress{} | |||
|
|
|||
| // sort by device index so that the first address added to the addresses list is from the first (primary) device | |||
| sort.Slice(instance.NetworkInterfaces, func(i, j int) bool { | |||
| return *instance.NetworkInterfaces[i].Attachment.DeviceIndex < *instance.NetworkInterfaces[j].Attachment.DeviceIndex | |||
There was a problem hiding this comment.
Change makes sense to me. Can you add checks for a nil attachment and use aws.IntValue to dereference the index? The EC2 API should return sane values but we shouldn't crash if it doesn't
There was a problem hiding this comment.
Should I filter out interfaces where the attachment is nil, or should I just sort them to the end of the list? I feel like filtering them out would be the right solution since they should not have addresses associated if the interface isn't actually attached to an instance.
There was a problem hiding this comment.
Should filter those out I think? so you just need to handle the potential nil pointer here. Start or end of the slice doesn't matter to me 😄
There was a problem hiding this comment.
I believe the updated code should just cause them to be sorted to the end of the list. It avoids allocating another slice, and the code that pulls the addresses out should ignore them anyway.
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
What's the process for back porting changes? My understanding is that we need to use the 1.27 release branch since our clusters are still running k8s 1.27. |
k8s-ci-robot
added
size/L
|
Ah I didn't notice you had this open against example here: https://kops.sigs.k8s.io/contributing/proposing-a-cherry-pick/ |
Sorry about that. It wasn't clear what the process is for changes like this. Should I go ahead and squash my commits when I rebase? |
|
Sure you can go ahead and squash! Thx |
javanthropus
force-pushed
the
ensure-network-address-order
branch
from
f4a7f9f
to
826d6f8
Compare
| @@ -928,6 +928,19 @@ func extractIPv4NodeAddresses(instance *ec2.Instance) ([]v1.NodeAddress, error) | |||
|
|
|||
| addresses := []v1.NodeAddress{} | |||
|
|
|||
| // sort by device index so that the first address added to the addresses list is from the first (primary) device | |||
| sort.Slice(instance.NetworkInterfaces, func(i, j int) bool { | |||
There was a problem hiding this comment.
Can we write a unit test for this to confirm that the ordering is what we intend?
There was a problem hiding this comment.
I could give it a try, but I didn't do so already because I couldn't find much in the way of existing tests here. As a result, I don't have any good examples to base my work on. Did I overlook something?
I can report that I'm running this same patch via a forked version of 1.27.6 on several clusters, and it fixed the issue reported here.
There was a problem hiding this comment.
That's good information to have. I don't know that it necessarily needs to be a strict blocker, but I don't think it would be too crazy.
I don't see any asynchronous calls here, so it should act entirely on the inputs. You could separate this out into another method to really narrow down the testing, but shouldn't be much more scope to leave as is. Here's a somewhat similar sort of test.
There was a problem hiding this comment.
Thanks for the link. I was looking in the wrong place for tests. That's not surprising since I'm very green with golang. I'll try to add some tests tomorrow and push them up.
There was a problem hiding this comment.
I finally managed to understand how the fakes work here, but I don't see any tests for this functionality. I see some tests for Fargate instances, but they don't use the NetworkInterfaces list and rely solely on the PrivateDnsName and PrivateIpAddress fields of the instance. Shouldn't that be sufficient for non-Fargate instances as well? In other words, why does this controller attempt to list all addresses for all network interfaces for a node rather than just the primary address and DNS name?
There was a problem hiding this comment.
@mmerkes, do you know if we can simplify this whole thing and just use the primary address and DNS name of the nodes rather than iterate over all attached addresses?
There was a problem hiding this comment.
A test has been added as requested.
k8s-ci-robot
added
size/M
javanthropus
force-pushed
the
ensure-network-address-order
branch
from
94098bc
to
8cc52c5
Compare
|
/retest-required |
|
The last couple of failed test runs don't appear to be related to my change. @mmerkes or @cartermckinnon, can one of you PTAL? |
|
@javanthropus sorry about that, the CI is busted. If we can't get a fix in shortly I'll override. |
|
/test pull-cloud-provider-aws-e2e |
|
@javanthropus Please rebase instead of merge. Thanks! |
javanthropus
force-pushed
the
ensure-network-address-order
branch
from
1229945
to
e637708
Compare
|
/retest |
|
/test pull-cloud-provider-aws-e2e-kubetest2 |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
Thanks for fixing this @javanthropus! |
…09-upstream-release-1.30 Automated cherry pick of #909: Ensure that addresses are added in network device index order
…09-upstream-release-1.29 Automated cherry pick of #909: Ensure that addresses are added in network device index order
…09-upstream-release-1.28 Automated cherry pick of #909: Ensure that addresses are added in network device index order
…09-upstream-release-1.27 Automated cherry pick of #909: Ensure that addresses are added in network device index order
…09-upstream-release-1.26 Automated cherry pick of #909: Ensure that addresses are added in network device index order
What type of PR is this?
/kind bug
What this PR does / why we need it:
This ensures that the addresses associated with network devices attached to the host are added to a Node resource's address list in order of device index. For some unknown reason AWS returns the list of network devices for just some of our EC2 instances where the primary device is not first in the list. Without this code change, the addresses of the secondary devices are listed first in the addresses for a Node, and this breaks the ability to interact with pods on the node, such as fetching logs and creating port forwards, because the apiserver always uses the first address of the Node resource to reach the kubelet but is unable to reach kubelet on any of these other addresses.
Which issue(s) this PR fixes:
Fixes #911.
Special notes for your reviewer:
None
Does this PR introduce a user-facing change?: