Fix route controller create/delete spam: use instanceIDToNodeName in case node name != private DNS

[wongma7]

What type of PR is this?
/kind bug

What this PR does / why we need it:

The function mapInstanceToNodeName assumes that node names are always equal to private dns names. This assumption does not always hold, specifically in 1.23 kops clusters with CCM enabled where node names are overridden to be instance IDs.

I added instanceIDToNodeName as a replacement. It relies on an indexer in the node informer to find nodeName by instance ID. This solution might be heavy-handed, I don't know how expensive exactly it is to have this indexer. If we assume that node names are going to be limited to either private dns or instance ids (or fargate?) then the solution could be simpler: it could check if a node with name equal to instance id exists, as suggested here kubernetes/kops#13376 (comment), and if not then fall back to assuming that node names are private dns names. Open to suggestions of course...

I tested this in my own cluster, I'll need to work on unit tests. And am considering how we can test this end to end or if such a test is necessary. We only caught this issue in the first place because AWS alerted us that we were making an unusual # of calls.

Test:

1. delete routes with destination 100.96.0.0/24 and 100.96.1.0/24 in VPC console
2. check logging of cloud controller: ~ $ k logs -n kube-system aws-cloud-controller-manager-xnmj5

I0317 21:29:45.399248       1 route_controller.go:194] Creating route for node i-001c99c6268df2574 100.96.1.0/24 with hint bd6fb3d1-708e-45ef-8019-01090827d461, throttled 10.433µs
I0317 21:29:45.399295       1 route_controller.go:194] Creating route for node i-001ab2674ef942145 100.96.0.0/24 with hint 24e777a5-cba7-4079-82c6-57d215eb4865, throttled 5.411µs
I0317 21:29:46.129790       1 route_controller.go:214] Created route for node i-001c99c6268df2574 100.96.1.0/24 with hint bd6fb3d1-708e-45ef-8019-01090827d461 after 730.545279ms
I0317 21:29:46.315747       1 route_controller.go:214] Created route for node i-001ab2674ef942145 100.96.0.0/24 with hint 24e777a5-cba7-4079-82c6-57d215eb4865 after 916.451209ms

3. check logging of cloud controller 5 mins later, there are no unnecessary Deletes or Creates: ~ $ k logs -n kube-system aws-cloud-controller-manager-xnmj5

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

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fix route controller making unnecessary CreateRoute/DeleteRoute AWS API requests in case node names are not private DNS names

[k8s-ci-robot]

@wongma7: This issue is currently awaiting triage.

If cloud-provider-aws contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

[wongma7]

/cc @olemarkus

[olemarkus]

This looks good to me. Like the indexer approach.

[nckturner]

Can you remind me why the .Lister() is tried first, and error ignored and the .Informer() is tried second? I guess my question is when does it error? And why doesn't the Indexer work for all cases?

[wongma7]

indexer should work for all cases, the Lister Get is redundant and I was thinking of it as a fast path but in practice I don't think it's any faster than Informer().GetIndexer().IndexKeys, so we an remove.

The Lister Get would error if the node name is NOT equal to instance ID e.g. if the node name is still set to private dns.

[nckturner]

Do we need this check or can we remove the instances, err := c.getInstancesByIDs(instanceIDs) entirely?

[wongma7]

I don't know, I didn't think too much of it bc I think it's out of scope of this PR.

But I think the check is wrong because ListRoutes should return all routes in the cluster's table and let the route controller decide whether it should be deleted. Because right now if there is a zombie route (say a node gets terminated while route controller is down) and then you hit the warning, the zombie will never get cleaned up?

but again I think it's out of scope so I'd rather fix this later

[nckturner]

the zombie will never get cleaned up?

This is what I was wondering about, and I agree I'm pretty sure that check was only there due to necessity and we can remove it now. The route controller can remove routes if the nodes don't exist: https://github.com/kubernetes/cloud-provider/blob/master/controllers/route/route_controller.go#L236-L257

[nckturner]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nckturner, wongma7

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:

• OWNERS [nckturner,wongma7]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment