fix: don't generate nodegroup role name starting with hyphen

[davidspek]

What type of PR is this?
/kind bug

What this PR does / why we need it:

Currently if EnableIAM is set to true and no role is specified for a node group the controller auto generates a role. However, the name formatting seems to be backwards from what it is intended to be, causing the role name to start with a -. What's surprising to me is that this doesn't seem to cause problems, other than that it looks weird. The name for the role that is currently generated looks like: -nodegroup-iam-service-role_<cluster-name>-<nodegroup-name>. This PR swaps the generated role name so it will be: <cluster-name>-<nodegroup-name>_nodegroup-iam-service-role. This is follows the same format as the name generated for Fargate.

Special notes for your reviewer:

Checklist:

• squashed commits
• includes documentation
• adds unit tests
• adds or updates e2e tests

Release note:

Change generated nodegroup IAM role name from `-nodegroup-iam-service-role_<cluster-name>-<nodegroup-name>` to `<cluster-name>-<nodegroup-name>_nodegroup-iam-service-role`

[k8s-ci-robot]

Welcome @davidspek!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-aws 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-aws has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @davidspek. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

[richardcase]

/ok-to-test

[davidspek]

@richardcase Have you had the chance to take a look at this PR?

[Ankitasw]

/test pull-cluster-api-provider-aws-e2e-eks

[Ankitasw]

/lgtm

cc @richardcase for approval

[Skarlso]

This will definitely cause existing clusters to break right? Since it will look for a specific role format?

[Ankitasw]

@Skarlso that's a good point, shall we hold it for next release?

[Skarlso]

Yep, sounds good.

[davidspek]

I'm not sure if it would break existing clusters, since this code is only used to create a new role if none were specified in the node group, which then get placed into the node group CR. Existing node group CRs would have the name of the role already in them.

[Skarlso]

It sets up the role name and then looks for it:

	s.scope.ManagedMachinePool.Spec.RoleName = roleName
	}
	role, err := s.GetIAMRole(s.scope.RoleName())

When reconciling normally, this would also happen and since it wouldn't find it, because it's looking for the set name it would try creating one again.

[Skarlso]

Ah but it only does that if the role is empty. Which would only happen if the role hasn't been created yet.

[Skarlso]

Okay, thinking about this some more, it should be fine. It should only happen for new clusters and old clusters should work. UNLESS, for whatever reason, the role name is wiped and it needs to look for it again.

[davidspek]

Is there even logic to handle having the role name being wiped properly? The field should be immutable and if it gets removed I don't think there's a way to be able to recover what it should be (since it could also be a non-generated name).

[Ankitasw]

cc @richardcase for final approval

[Ankitasw]

/cherry-pick release-2.2

[k8s-infra-cherrypick-robot]

@Ankitasw: once the present PR merges, I will cherry-pick it on top of release-2.2 in a new PR and assign it to you.

In response to this:

/cherry-pick release-2.2

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.

[davidspek]

@AndiDog Maybe you can have a look at this as well.

[davidspek]

@Ankitasw @Skarlso What needs to be done before this PR can get merged?

[Skarlso]

I'll take a look later today. Been busy with work.

[davidspek]

Awesome, thanks!

[Skarlso]

Okay, we did run tests, I thought this through, and I think it shouldn't break anything. "Famous last words".

Here we go:
/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Skarlso

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

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

[k8s-infra-cherrypick-robot]

@Ankitasw: new pull request created: #4516

In response to this:

/cherry-pick release-2.2

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.