User Story
As a [developer] I would like to populate defaults of MachineDeployment via an admission webhook for consistency in the project and to not have to constantly try and populate default values during the reconcile loop.
Detailed Description
This can be called from a mutating admission webhook
And the following can be removed.
https://github.com/kubernetes-sigs/cluster-api/blob/86c86080fc55577960c83d68f24df385e8e1b476/controllers/machinedeployment_controller.go#L113-L114
Anything else you would like to add:
The function is also called in this place. So the function itself might not need to be removed.
/kind feature
I would prefer not to use a mutating webhook for this. This would cause the controller to be dependent on the webhook which is not ideal. A validating webhook does not inject a dependency like this would.
v1alpha3 will require validating webhooks. Given that, mutating/defaulting webhooks are appropriate if needed.
v1alpha3 will require validating webhooks. Given that, mutating/defaulting webhooks are appropriate if needed.
I think it's a poor UX to require webhooks. They should be a nice-to-have, not required. The biggest problem is life cycling the certificates. We should at least have a release of OPTIONAL webhooks before they become required, and we should have the tooling to lifecycle them before they become required. That's a lot of work for not a lot of result IMO, but we need to do these things if we want webhooks.
I thought that we use cert-manager to handle the issuing and rotation of the certificates to services like Validating/Mutating Webhooks to improve the UX for managing certificates.
It seems that we have been adding in validation and defaulting webhook support in many places already. For example, we currently default kubeadmcontrolplane via mutating webhook. If we want to support a webhook-less option, we could discuss that as well, separately. That way we can track all the things that need to become webhook-less.
/assign
/lifecycle active
As per this https://github.com/kubernetes-sigs/cluster-api/pull/1813#issuecomment-560520198, I'll be adding the following to this PR as well.
Cluster.Spec.InfrastructureRef.Namespace should default to cluster.NamespaceMachine.Spec.InfrastrucutreRef.Namespace should default to machine.NamespaceMachine.Spec.Bootstrap.ConfigRef.Namespace should default to machine.NamespaceRe requiring webhooks, we also need them for conversions between v1alpha2 and v1alpha3. If users have existing v1alpha2 clusters and they want to upgrade their management clusters to v1alpha3, they will have to use webhooks for the version conversions; otherwise, the updated controllers won't be able to see any of the v1alpha2 data.
Most helpful comment
As per this https://github.com/kubernetes-sigs/cluster-api/pull/1813#issuecomment-560520198, I'll be adding the following to this PR as well.
Cluster.Spec.InfrastructureRef.Namespaceshould default tocluster.NamespaceMachine.Spec.InfrastrucutreRef.Namespaceshould default tomachine.NamespaceMachine.Spec.Bootstrap.ConfigRef.Namespaceshould default tomachine.Namespace