-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Implement MachinePool Machines in CAPI, CAPD, and clusterctl #7938
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,7 +17,9 @@ limitations under the License. | |
package v1beta1 | ||
|
||
import ( | ||
"context" | ||
"fmt" | ||
"os" | ||
"strings" | ||
"time" | ||
|
||
|
@@ -36,14 +38,15 @@ const defaultNodeDeletionTimeout = 10 * time.Second | |
|
||
func (m *Machine) SetupWebhookWithManager(mgr ctrl.Manager) error { | ||
return ctrl.NewWebhookManagedBy(mgr). | ||
For(m). | ||
For(&Machine{}). | ||
WithValidator(MachineValidator(mgr.GetScheme())). | ||
Complete() | ||
} | ||
|
||
// +kubebuilder:webhook:verbs=create;update,path=/validate-cluster-x-k8s-io-v1beta1-machine,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=cluster.x-k8s.io,resources=machines,versions=v1beta1,name=validation.machine.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1 | ||
// +kubebuilder:webhook:verbs=create;update;delete,path=/validate-cluster-x-k8s-io-v1beta1-machine,mutating=false,failurePolicy=fail,matchPolicy=Equivalent,groups=cluster.x-k8s.io,resources=machines,versions=v1beta1,name=validation.machine.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1 | ||
// +kubebuilder:webhook:verbs=create;update,path=/mutate-cluster-x-k8s-io-v1beta1-machine,mutating=true,failurePolicy=fail,matchPolicy=Equivalent,groups=cluster.x-k8s.io,resources=machines,versions=v1beta1,name=default.machine.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1 | ||
|
||
var _ webhook.Validator = &Machine{} | ||
var _ webhook.CustomValidator = &machineValidator{} | ||
sbueringer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
var _ webhook.Defaulter = &Machine{} | ||
|
||
// Default implements webhook.Defaulter so a webhook will be registered for the type. | ||
|
@@ -58,7 +61,10 @@ func (m *Machine) Default() { | |
} | ||
|
||
if m.Spec.InfrastructureRef.Namespace == "" { | ||
m.Spec.InfrastructureRef.Namespace = m.Namespace | ||
// Don't autofill namespace for MachinePool Machines since the infraRef will be populated by the MachinePool controller. | ||
if !isMachinePoolMachine(m) { | ||
m.Spec.InfrastructureRef.Namespace = m.Namespace | ||
} | ||
} | ||
|
||
if m.Spec.Version != nil && !strings.HasPrefix(*m.Spec.Version, "v") { | ||
|
@@ -71,36 +77,94 @@ func (m *Machine) Default() { | |
} | ||
} | ||
|
||
// ValidateCreate implements webhook.Validator so a webhook will be registered for the type. | ||
func (m *Machine) ValidateCreate() (admission.Warnings, error) { | ||
// MachineValidator creates a new CustomValidator for Machines. | ||
func MachineValidator(_ *runtime.Scheme) webhook.CustomValidator { | ||
return &machineValidator{} | ||
} | ||
|
||
// machineValidator implements a defaulting webhook for Machine. | ||
type machineValidator struct{} | ||
|
||
// // ValidateCreate implements webhook.Validator so a webhook will be registered for the type. | ||
// func (m *Machine) ValidateCreate() (admission.Warnings, error) { | ||
// return nil, m.validate(nil) | ||
// } | ||
|
||
// // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type. | ||
// func (m *Machine) ValidateUpdate(old runtime.Object) (admission.Warnings, error) { | ||
// oldM, ok := old.(*Machine) | ||
// if !ok { | ||
// return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a Machine but got a %T", old)) | ||
// } | ||
// return nil, m.validate(oldM) | ||
// } | ||
|
||
// // ValidateDelete implements webhook.Validator so a webhook will be registered for the type. | ||
// | ||
// func (m *Machine) ValidateDelete() (admission.Warnings, error) { | ||
// return nil, nil | ||
func (*machineValidator) ValidateCreate(_ context.Context, obj runtime.Object) (admission.Warnings, error) { | ||
m, ok := obj.(*Machine) | ||
if !ok { | ||
return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a Machine but got a %T", obj)) | ||
} | ||
|
||
return nil, m.validate(nil) | ||
} | ||
|
||
// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type. | ||
func (m *Machine) ValidateUpdate(old runtime.Object) (admission.Warnings, error) { | ||
oldM, ok := old.(*Machine) | ||
func (*machineValidator) ValidateUpdate(_ context.Context, oldObj runtime.Object, newObj runtime.Object) (admission.Warnings, error) { | ||
newM, ok := newObj.(*Machine) | ||
if !ok { | ||
return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a Machine but got a %T", newObj)) | ||
} | ||
|
||
oldM, ok := oldObj.(*Machine) | ||
if !ok { | ||
return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a Machine but got a %T", old)) | ||
return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a Machine but got a %T", oldObj)) | ||
} | ||
return nil, m.validate(oldM) | ||
return nil, newM.validate(oldM) | ||
} | ||
|
||
// ValidateDelete implements webhook.Validator so a webhook will be registered for the type. | ||
func (m *Machine) ValidateDelete() (admission.Warnings, error) { | ||
func (*machineValidator) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { | ||
m, ok := obj.(*Machine) | ||
if !ok { | ||
return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a Machine but got a %T", obj)) | ||
} | ||
|
||
req, err := admission.RequestFromContext(ctx) | ||
if err != nil { | ||
return nil, apierrors.NewBadRequest(fmt.Sprintf("expected a admission.Request inside context: %v", err)) | ||
} | ||
|
||
// Fallback machines are placeholders for InfraMachinePools that do not support MachinePool Machines. These have | ||
// no bootstrap or infrastructure data and cannot be deleted by users. They instead exist to provide a consistent | ||
// user experience for MachinePool Machines. | ||
if _, isFallbackMachine := m.Labels[FallbackMachineLabel]; isFallbackMachine { | ||
// Only allow the request if it is coming from the CAPI controller service account. | ||
if req.UserInfo.Username != "system:serviceaccount:"+os.Getenv("POD_NAMESPACE")+":"+os.Getenv("POD_SERVICE_ACCOUNT") { | ||
return nil, apierrors.NewBadRequest("this Machine is a placeholder for InfraMachinePools that do not support MachinePool Machines and cannot be deleted by users, scale down the MachinePool instead to delete") | ||
} | ||
} | ||
|
||
return nil, nil | ||
} | ||
|
||
func (m *Machine) validate(old *Machine) error { | ||
var allErrs field.ErrorList | ||
specPath := field.NewPath("spec") | ||
if m.Spec.Bootstrap.ConfigRef == nil && m.Spec.Bootstrap.DataSecretName == nil { | ||
allErrs = append( | ||
allErrs, | ||
field.Required( | ||
specPath.Child("bootstrap", "data"), | ||
"expected either spec.bootstrap.dataSecretName or spec.bootstrap.configRef to be populated", | ||
), | ||
) | ||
// MachinePool Machines don't have a bootstrap configRef, so don't require it. The bootstrap config is instead owned by the MachinePool. | ||
if !isMachinePoolMachine(m) { | ||
allErrs = append( | ||
allErrs, | ||
field.Required( | ||
specPath.Child("bootstrap", "data"), | ||
"expected either spec.bootstrap.dataSecretName or spec.bootstrap.configRef to be populated", | ||
), | ||
) | ||
} | ||
} | ||
|
||
if m.Spec.Bootstrap.ConfigRef != nil && m.Spec.Bootstrap.ConfigRef.Namespace != m.Namespace { | ||
|
@@ -114,15 +178,18 @@ func (m *Machine) validate(old *Machine) error { | |
) | ||
} | ||
|
||
if m.Spec.InfrastructureRef.Namespace != m.Namespace { | ||
allErrs = append( | ||
allErrs, | ||
field.Invalid( | ||
specPath.Child("infrastructureRef", "namespace"), | ||
m.Spec.InfrastructureRef.Namespace, | ||
"must match metadata.namespace", | ||
), | ||
) | ||
// InfraRef can be empty for MachinePool Machines so skip the check in that case. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How are we getting around the OpenAPI validation run by the kube-apiserver? infrastructureRef is a required field on the Machine and its corresponding OpenAPI schema. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since it's not a pointer we just leave the infraRef uninitialized when we create it for MPMs. Is that what you're asking? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm surprised that it works as in the schema in the CRD infraRef is required. So the apiserver should block There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So for this to work I would expect that we have to change the Machine CRD to make infraRef optional (eg like config ref) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But maybe there's a reason why it works anyway There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can have uninitialized structs even if it's not optional. Should be the same reason a blank label selector works in CAAPH. |
||
if !isMachinePoolMachine(m) { | ||
if m.Spec.InfrastructureRef.Namespace != m.Namespace { | ||
allErrs = append( | ||
allErrs, | ||
field.Invalid( | ||
specPath.Child("infrastructureRef", "namespace"), | ||
m.Spec.InfrastructureRef.Namespace, | ||
"must match metadata.namespace", | ||
), | ||
) | ||
} | ||
} | ||
|
||
if old != nil && old.Spec.ClusterName != m.Spec.ClusterName { | ||
|
@@ -143,3 +210,17 @@ func (m *Machine) validate(old *Machine) error { | |
} | ||
return apierrors.NewInvalid(GroupVersion.WithKind("Machine").GroupKind(), m.Name, allErrs) | ||
} | ||
|
||
func isMachinePoolMachine(m *Machine) bool { | ||
if m.OwnerReferences == nil { | ||
return false | ||
} | ||
|
||
for _, owner := range m.OwnerReferences { | ||
if owner.Kind == "MachinePool" { | ||
return true | ||
} | ||
} | ||
|
||
return false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fear that the concept of a fallback machine would be too opaque to users, I think we should find a better name.
Also wondering if this should be a new field in status (optional, defined by the contract), signaling that the Machine has limited capabilities vs a "standard" machine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm open to coming up with a better name. The MPM proposal described these as "CAPI-only" Machines which seemed a bit confusing so I tried to rename it. Do you have any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should have a different name.
It is just a regular machine without the capability to trigger deletion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify, do you mean that we should keep the name "fallback machines" or rename it to "CAPI-only" in the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should have a different name.
It is just a regular machine without the capability to trigger deletion, and this can be modeled e.g. with a new field in status (optional, defined by the contract), signaling that the Machine has limited capabilities.
cc @vincepri
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think fallback-machine is not a good option. I'm not sure about status.
If we put "not deletable" in there it wouldn't be exactly true as those machines are just not deleteable by the user, right? (but they are deletable by the controller)
I wonder if we basically have the same concept as "mirror pods" that kubelet is using for static pods (kube-apiserver, ...).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of "static machine"
StaticMachineLabel = "machinepool.cluster.x-k8s.io/static-machine"
thoughts?