Use AWS metadata to retrieve local-hostname in nodeup

[bwagner5]

Cloud Provider: AWS

This PR changes nodeups method of looking up an instance private DNS name from a DescribeInstances call to an instance metadata lookup. DescribeInstances is prone to throttling with large clusters and can cause significant delays when a node is trying to join a cluster.

[k8s-ci-robot]

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

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

[rifelpet]

This would break clusters with IMDS disabled. I think it would be reasonable to try IMDS first and fallback to DescribeInstances.

[hakman]

This would break clusters with IMDS disabled. I think it would be reasonable to try IMDS first and fallback to DescribeInstances.

Can / should a kOps cluster work without IMDS?
Previous implementation also used IMDS to get metadata://aws/meta-data/instance-id.

[hakman]

/ok-to-test

[hakman]

Thanks @bwagner5. Seems like a good improvement from my point of view.
@rifelpet Leaving this to you for the final review.

[hakman]

/lgtm

[bwagner5]

This would break clusters with IMDS disabled. I think it would be reasonable to try IMDS first and fallback to DescribeInstances.

Can / should a kOps cluster work without IMDS?
Previous implementation also used IMDS to get metadata://aws/meta-data/instance-id.

There's some other places that use IMDS in nodeup as well so it might be worth going through and figuring out if it's reasonable to perform a fallback if IMDS is disabled everywhere. But it's almost impossible to do really large scale-ups if you're not using IMDS on node bootstrap so I'd lean towards getting rid of the API calls and requiring IMDS.

I have a version of nodeup that does zero API calls and relies only on IMDS and OS calls for node info rather than DescribeInstanceTypes. I may try to PR some of those after they're cleaned up.

[johngmyers]

Don't you have to use IMDS to get the instance role credentials to authenticate to kops-controller? I don't think it's going to be possible to bootstrap without IMDS.

[johngmyers]

/assign @rifelpet

[hakman]

There's some other places that use IMDS in nodeup as well so it might be worth going through and figuring out if it's reasonable to perform a fallback if IMDS is disabled everywhere. But it's almost impossible to do really large scale-ups if you're not using IMDS on node bootstrap so I'd lean towards getting rid of the API calls and requiring IMDS.

In any case one would need instance ID, which is pretty hard to get without IMDS, right?

I have a version of nodeup that does zero API calls and relies only on IMDS and OS calls for node info rather than DescribeInstanceTypes. I may try to PR some of those after they're cleaned up.

That would be very nice.
I think there are also some places where metadata api is called directly instead of using vfs to read it. It may be worth a PR to change all to use vfs.

[olemarkus]

In any case one would need instance ID, which is pretty hard to get without IMDS, right?

Instances with RBN enabled can just get this from os.hostname :)

[rifelpet]

It sounds like IMDS is required at this point then. This lgtm and I agree with proposed follow up PRs.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rifelpet

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 [rifelpet]

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

[rifelpet]

Should we remove eco:DescribeInstances from the node IAM policy? Users may be depending on it for other workloads though, and having them add it back makes the policy size larger since it would become its own statement.

[johngmyers]

Doesn't kubelet require DescribeInstances?

[olemarkus]

We can remove it from the nodeup function (if it is there). Kubelet still needs it if the in-tree CCM is used, but it should be a part of the CCM function too.