Don't default RoleAssignmentName on machine templates

[fiunchinho]

/kind bug

What this PR does / why we need it:

While using SystemAssigned identity, I can't use more than 1 replica for my control plane. Or more than 1 replica on my MachineDeployments.

The problem is that AzureMachineTemplate has a RoleAssignmentName field for the name of the role assignment, which is then used when creating AzureMachines. But if more than one AzureMachine or VM is created out of this AzureMachineTemplate (i.e having more than 1 VM for the control plane, or even more than 1 replica in a MachineDeployment) the VM creation will fail with Azure API error RoleAssignmentUpdateNotPermitted saying Tenant ID, application ID, principal ID, and scope are not allowed to be updated.

Special notes for your reviewer:

I see that the templates used for e2e tests normally use only 1 replica for tests, that may explain why this hasn't been spotted so far.

TODOs:

• squashed commits
• includes documentation
• adds unit tests

Release note:

Fix 'SystemAssigned' identity by removing the defaulting of 'RoleAssignmentName' on 'AzureMachineTemplate' so that every 'AzureMachine'  defaults to a random 'RoleAssignmentName'.

[k8s-ci-robot]

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

[CecileRobertMichon]

/cc @shysank

[CecileRobertMichon]

/ok-to-test

[shysank]

Ideally, we want to remove RoleAssignmentName from AzureMachineTemplate which needs a broader refactore, but for now, can we atleast add a validation to make sure no value gets set for that field?

[fiunchinho]

I gave it a shot, please take a look to see if that's what you meant.

[shysank]

/lgtm
/assign @CecileRobertMichon
can you squash commits please?

[jackfrancis]

Rather than changing this here, shouldn't we update the func (s *AzureMachineSpec) SetDefaults() method directly in api/v1beta1/azuremachine_default.go? That SetDefaults() method is also invoked in api/v1beta1/azuremachine_webhook.go (in the func (m *AzureMachine) Default() method)

[fiunchinho]

I think it makes sense to default the RoleAssignmentName to a random UUID in the AzureMachine object, that's fine because all VMs would have a different one. It only doesn't make sense at the AzureMachineTemplate level.

[jackfrancis]

Thanks for the explanation, makes sense

[jackfrancis]

So the core issue here seems to be that a static value in a MachineTemplate will render as-is on each Machine resource using it. Surely there are other such examples of machine-unique configuration, how is that handled?

There seems to be something fundamentally broken in the type spec here. What is the ideal way to define machine-specific configuration as distinct from MachineTemplate configuration? Should such properties even be present in the MachineSpec type definition at all?

cc @CecileRobertMichon @shysank

[fiunchinho]

Can you squash commits please?

done

[jackfrancis]

/lgtm

[CecileRobertMichon]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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

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