cluster-api-provider-azure

cluster-api-provider-azure copied to clipboard

What type of PR is this?

/kind bug

What this PR does / why we need it:

While auditing AzureManagedCluster property CRUD operations, I observed that you're able to edit both the azuremanagedcluster resource and the azuremanagedcontrolplane resource and change the values of the spec.controlPlaneEndpoint configuration. The changes are accepted, and then it seems that the controllers are smart enough to revert the changes (or possibly they are calling the AKS API and resetting it to its authoritative value).

Rather than the above, we should fail updates on that configuration via webhook.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged): Fixes #

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

• [ ] squashed commits
• [ ] includes documentation
• [ ] adds unit tests

Release note:

AzureManagedCluster spec.controlPlaneEndpoint is immutable

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Once this PR has been reviewed and has the lgtm label, please ask for approval from jackfrancis by writing /assign @jackfrancis in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

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

@jackfrancis: PR needs rebase.

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.

/assign @nojnhuh

Would love to get one more sign-off from @CecileRobertMichon to make sure merging this is preferable to the current status quo. Thank you!

Any chance we could discuss during office hours tomorrow? I'm not super clear on how this is an improvement. My current understanding:

1. Currently, the AzureManagedCluster controlPlaneEndpoint gets set by the controller and overrides whatever value is already there. This means if a user sets a value it gets ignored and overwritten, and there are no errors (but user gets ignored).
2. With this change, AzureManagedCluster controlPlaneEndpoint becomes immutable so updates get rejected by the webhook. If a user sets a value that is different from the AKS API Server, the controller would run into an error trying to update the field as the update would get rejected by the webhook (user is allowed to shoot themselves in the foot).
3. This is different from how the same validation is handled for AzureCluster. AzureCluster controlPlaneEndpoint is immutable in the webhook so the webhook rejects any updates, but, the controller doesn't try to set it unless the value is unset. This means the controller will never run into a webhook error trying to set an immutable field (but also wrong value could be set by the user and issues would happen).

I think my main questions are: similar to AzureCluster, should we change the managed control plane controller to only try to update the field if it's empty so it doesn't run into its own webhook validation error? If the intent is to never let the user set the value, is there a way to reject the user trying to set the field before the controller has a chance to? Maybe we can use Status and only allow setting the endpoint if the cluster is ready or provisioned? Is there ever a valid scenario for the user wanting to set the endpoint themselves before cluster exists?

@CecileRobertMichon let's discuss during office hours. In fact this PR was my attempt to do what you describe in point 3 above.

/hold

@CecileRobertMichon incorporated discussion points from yesterday's office hours, PTAL

incorporated discussion points from yesterday's office hours, PTAL

I see you made those changes for AzureManagedCluster but not AzureManagedControlPlane, any reason for that? Wouldn't the webhook also fail for the AzureManagedControlPlane if the user sets it before the controller has a chance to?

@CecileRobertMichon fair point, I've applied the change there as well

/retest

/hold cancel /assign @nojnhuh @CecileRobertMichon @nawazkh

/lgtm 🚀

@nojnhuh @CecileRobertMichon I think we should scope this PR down to the updated PR description. Integrating "cluster readiness" and/or "detecting whether an update originates from the AKS API vs a user" is proving to be difficult, and the real-world payoff for this additional webhook enforcement is (IMO) not worth the effort at this time.

Ready for final review.

[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