🐛 normalize version in managed control plane #818
Conversation
Signed-off-by: Alexander Eldeib <[email protected]>
k8s-ci-robot
added
release-note
k8s-ci-robot
added
area/provider/azure
|
also fixes a panic when owner ref isn't set on managed control plane |
k8s-ci-robot
added
size/S
|
/test pull-cluster-api-provider-azure-test |
| @@ -54,13 +55,20 @@ func newAzureManagedMachinePoolReconciler(scope *scope.ManagedControlPlaneScope) | |||
| // Reconcile reconciles all the services in pre determined order | |||
| func (r *azureManagedMachinePoolReconciler) Reconcile(ctx context.Context, scope *scope.ManagedControlPlaneScope) error { | |||
| scope.Logger.Info("reconciling machine pool") | |||
|
|
|||
| var normalizedVersion *string | |||
| if scope.MachinePool.Spec.Template.Spec.Version != nil { | |||
There was a problem hiding this comment.
can this go in a webhook?
There was a problem hiding this comment.
Are you suggesting a default version?
There was a problem hiding this comment.
yes that way the normalized version appears in the spec and it's not too hidden from the user, same as https://github.com/CecileRobertMichon/cluster-api/blob/master/api/v1alpha3/machine_webhook.go#L60
There was a problem hiding this comment.
Webhooks are not currently setup for AzureManged*. Can we open an issue to track this and pull in these changes?
There was a problem hiding this comment.
That was part of the changes in kubernetes-sigs/cluster-api#3147, the Version isn't part of MachinePool spec, it's part of Machine spec which is embedded in the MachinePool spec as part of the template https://github.com/kubernetes-sigs/cluster-api/blob/master/exp/api/v1alpha3/machinepool_types.go#L44
There was a problem hiding this comment.
So actually do we even need this normalization here? Since the Machine webhook already takes care of it
There was a problem hiding this comment.
For AzureManagedControlPlane Version is part of the object https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/818/files#diff-69a80dafa12ba6b9eeaf233d609e8f5eR29 so we could do the same normalization in a webhook, for AzureMachinePool I believe it's already taken care of by the Machine webhooks
There was a problem hiding this comment.
Ok so after some coffee and thinking through it with @devigned I think we need to:
- keep the AzureMachinePool normalization until we can move it into the capi MachinePool webhook
- Add a webhook normalization for AzureManagedControlPlane, either here or in a separate PR
Let's track whatever we don't get done here in issues so they don't get lost.
I take back what I said above, the Machine webhook doesn't do anything for MachinePool, only the schema validation is shared.
k8s-ci-robot
added
the
lgtm
|
[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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Signed-off-by: Alexander Eldeib [email protected]
What this PR does / why we need it:
remove strict validation and always let prefix normalization occur if necessary on the versions. We added this in #745 after realizing it mismatched with Machine, but Machine and KCP themselves were mismatched which was fixed in kubernetes-sigs/cluster-api#3147
This PR removes the stricter validation we added, and allows the prefix trimming (inside managed control plane reconciler) to always normalize the version string.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):n/a
Release note: