Cluster-api-provider-azure: Add validations for AzureMachine Updates

[CecileRobertMichon]

/kind feature

Describe the solution you'd like
[A clear and concise description of what you want to happen.]

Some fields are immutable and we should fail when they are updated.

We should add to the already implemented validation for AzureMachines here:

https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/master/api/v1alpha3/azuremachine_webhook.go#L56

You can also look how CAPA achieves this here https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/master/api/v1alpha3/awsmachine_webhook.go#L55.

Anything else you would like to add:
[Miscellaneous information that will assist in solving the issue.]

Environment:

• cluster-api-provider-azure version:
• Kubernetes version: (use kubectl version):
• OS (e.g. from /etc/os-release):

[CecileRobertMichon]

/help
/good-first-issue

[srm09]

@CecileRobertMichon Where can I find docs listing the immutable fields?

[CecileRobertMichon]

@srm09 this hasn't been documented yet, one the CAPZ maintainers needs to do a bit of work to list those in this issue before it can really be picked up by a newcomer, going to remove the good first issue label for now.

/remove good-first-issue

[CecileRobertMichon]

/remove-good-first-issue

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[CecileRobertMichon]

/remove-lifecycle stale

[alexeldeib]

/remove-lifecycle rotten

[CecileRobertMichon]

Let's allow updates for:

• AdditionalTags
• ProviderID

And disallow updates for everything else in AzureMachine spec for now. Did an audit of spec and these are the only two I could see as being updatable. Tags get reconciled separately by the tags service. Everything else that is a VM property won't be updated after create since we don't update existing VMs. Users should update the AzureMachineTemplate instead to create new machines that have the desired properties.

ProviderID should technically not change but since right now the provider sets it on every iteration I think it's okay to leave it as updatable.

@devigned wdyt?

/good-first-issue

[k8s-ci-robot]

@CecileRobertMichon:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

Let's allow updates for:

• AdditionalTags
• ProviderID

And disallow updates for everything else in AzureMachine spec for now. Did an audit of spec and these are the only two I could see as being updatable. Tags get reconciled separately by the tags service. Everything else that is a VM property won't be updated after create since we don't update existing VMs. Users should update the AzureMachineTemplate instead to create new machines that have the desired properties.

ProviderID should technically not change but since right now the provider sets it on every iteration I think it's okay to leave it as updatable.

@devigned wdyt?

/good-first-issue

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.

[srm09]

/assign

[CecileRobertMichon]

@srm09 are you still interested in working on this one?

[srm09]

Whoops, @CecileRobertMichon I totally forgot about this one. Gimme a day or two, otherwise I will unassign myself.

[srm09]

@CecileRobertMichon The current implementation allows for updates of a bunch of other values:

• OSDisk
• SSHPublicKey
• Identity ...
  Do we wanna keep those or is the plan to disallow these and just allow AdditionalTags and ProviderID ?

[CecileRobertMichon]

@srm09 see my comment above. Only AdditionalTags and ProviderID should be mutable as VMs are immutable and any other spec that is updated won't actually result in the VM being updated.

[shivi28]

Hello @CecileRobertMichon, Is this issue still required?
@srm09 Are you planning to work on this? If not, then please let me know as I would like to pick this up. :)

[CecileRobertMichon]

@shivi28 please go for it, I don't think there is an active PR. Please comment /assign below if you do decide to work on it.

/unassign @srm09

[shysank]

/assign @shivi28