Skip to content
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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions api/v1beta1/common_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,11 @@ const (
// update that disallows a pre-existing Cluster to be populated with Topology information and Class.
ClusterTopologyUnsafeUpdateClassNameAnnotation = "unsafe.topology.cluster.x-k8s.io/disable-update-class-name-check"

// FallbackMachineLabel indicates that a Machine belongs to a MachinePool that does not support MachinePool Machines.
// As such, these Machines exist to create a consistent user experience and will not have an infrastructure reference. The user will
// also be prevented from deleting these Machines.
FallbackMachineLabel = "machinepool.cluster.x-k8s.io/fallback-machine"
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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?

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

Copy link
Member

@sbueringer sbueringer May 10, 2023

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, ...).

Copy link
Contributor

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?


// ProviderNameLabel is the label set on components in the provider manifest.
// This label allows to easily identify all the components belonging to a provider; the clusterctl
// tool uses this label for implementing provider's lifecycle operations.
Expand Down
4 changes: 4 additions & 0 deletions api/v1beta1/machine_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ const (
// MachineDeploymentNameLabel is the label set on machines if they're controlled by MachineDeployment.
MachineDeploymentNameLabel = "cluster.x-k8s.io/deployment-name"

// MachinePoolNameLabel is the label indicating the name of the MachinePool a Machine is controlled by.
// Note: The value of this label may be a hash if the MachinePool name is longer than 63 characters.
MachinePoolNameLabel = "cluster.x-k8s.io/pool-name"

// MachineControlPlaneNameLabel is the label set on machines if they're controlled by a ControlPlane.
// Note: The value of this label may be a hash if the control plane name is longer than 63 characters.
MachineControlPlaneNameLabel = "cluster.x-k8s.io/control-plane-name"
Expand Down
135 changes: 108 additions & 27 deletions api/v1beta1/machine_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ limitations under the License.
package v1beta1

import (
"context"
"fmt"
"os"
"strings"
"time"

Expand All @@ -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.
Expand All @@ -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") {
Expand All @@ -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 {
Expand All @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

The 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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But maybe there's a reason why it works anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 {
Expand All @@ -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
}
Loading