Skip to content
Merged
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: 3 additions & 2 deletions pkg/controllers/machinemigration/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ import (
"k8s.io/apimachinery/pkg/api/meta"
"k8s.io/client-go/rest"
"k8s.io/klog/v2"
"k8s.io/klog/v2/textlogger"

ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
"sigs.k8s.io/controller-runtime/pkg/envtest"
Expand All @@ -54,7 +54,8 @@ func TestMachineMigration(t *testing.T) {
var _ = BeforeSuite(func() {
klog.SetOutput(GinkgoWriter)

logf.SetLogger(textlogger.NewLogger(textlogger.NewConfig()))
logf.SetLogger(GinkgoLogr)
ctrl.SetLogger(GinkgoLogr)

By("bootstrapping test environment")
var err error
Expand Down
5 changes: 3 additions & 2 deletions pkg/controllers/machinesetmigration/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ import (
"k8s.io/apimachinery/pkg/api/meta"
"k8s.io/client-go/rest"
"k8s.io/klog/v2"
"k8s.io/klog/v2/textlogger"

ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
"sigs.k8s.io/controller-runtime/pkg/envtest"
Expand All @@ -54,7 +54,8 @@ func TestMachineSetMigration(t *testing.T) {
var _ = BeforeSuite(func() {
klog.SetOutput(GinkgoWriter)

logf.SetLogger(textlogger.NewLogger(textlogger.NewConfig()))
logf.SetLogger(GinkgoLogr)
ctrl.SetLogger(GinkgoLogr)

By("bootstrapping test environment")
var err error
Expand Down
44 changes: 44 additions & 0 deletions pkg/controllers/machinesetsync/machineset_sync_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,12 @@ var (

// errMachineAPIMachineSetOwnerReferenceConversionUnsupported.
errMachineAPIMachineSetOwnerReferenceConversionUnsupported = errors.New("could not convert Machine API machine set owner references to Cluster API")

// errInvalidInfraClusterReference is returned when the cluster name is empty in CAPI machineset spec.
errInvalidInfraClusterReference = errors.New("cluster name is empty in Cluster API machine set spec")

// errInvalidInfraMachineTemplateReference is returned when the infrastructure machine template reference is invalid or incomplete.
errInvalidInfraMachineTemplateReference = errors.New("infrastructure machine template reference is invalid or incomplete")
)

const (
Expand All @@ -104,6 +110,8 @@ const (
messageSuccessfullySynchronizedMAPItoCAPI = "Successfully synchronized MAPI MachineSet to CAPI"

controllerName string = "MachineSetSyncController"

terminalConfigurationErrorLog string = "Detected terminal configuration error - cluster name or infrastructure machine template reference is empty, not requeuing"
)

// MachineSetSyncReconciler reconciles CAPI and MAPI MachineSets.
Expand Down Expand Up @@ -234,6 +242,18 @@ func (r *MachineSetSyncReconciler) fetchCAPIInfraResources(ctx context.Context,
Name: infraMachineTemplateRef.Name,
}

// Validate that required references are not empty to avoid nil pointer issues later.
// These are terminal configuration errors that require user intervention.
if capiMachineSet.Spec.ClusterName == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity: is this a real case? The field is required in CAPI AFAIK :-)

(no need to change anything, just thinking about if we need all the "terminal failure handling" if this is something which should never happen)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

return nil, nil, fmt.Errorf("machine set %s/%s: %w",
capiMachineSet.Namespace, capiMachineSet.Name, errInvalidInfraClusterReference)
}

if infraMachineTemplateRef.Name == "" || infraMachineTemplateRef.Namespace == "" {
return nil, nil, fmt.Errorf("machine %s/%s: %w",
capiMachineSet.Namespace, capiMachineSet.Name, errInvalidInfraMachineTemplateReference)
}

infraMachineTemplate, infraCluster, err := controllers.InitInfraMachineTemplateAndInfraClusterFromProvider(r.Platform)
if err != nil {
return nil, nil, fmt.Errorf("unable to devise CAPI infra resources: %w", err)
Expand Down Expand Up @@ -340,6 +360,12 @@ func (r *MachineSetSyncReconciler) reconcileMAPIMachineSetToCAPIMachineSet(ctx c
restoreCAPIFields(existingCAPIMachineSet, convertedCAPIMachineSet, r.CAPINamespace, authoritativeAPI, clusterOwnerRefence)

if err := r.ensureCAPIInfraMachineTemplate(ctx, sourceMAPIMachineSet, convertedCAPIMachineSet, convertedCAPIInfraMachineTemplate, clusterOwnerRefence); err != nil {
// Don't requeue for terminal configuration errors
if isTerminalConfigurationError(err) {
logger.Error(err, terminalConfigurationErrorLog)
return ctrl.Result{}, nil
}

return ctrl.Result{}, fmt.Errorf("unable to ensure CAPI infra machine template: %w", err)
}

Expand Down Expand Up @@ -571,6 +597,14 @@ func (r *MachineSetSyncReconciler) reconcileCAPIMachineSetToMAPIMachineSet(ctx c
if err != nil {
fetchErr := fmt.Errorf("failed to fetch CAPI infra resources: %w", err)

// Don't requeue for terminal configuration errors
if isTerminalConfigurationError(err) {
logger.Error(err, terminalConfigurationErrorLog)
r.Recorder.Event(sourceCAPIMachineSet, corev1.EventTypeWarning, "SynchronizationError", fetchErr.Error())

return ctrl.Result{}, nil
}

if condErr := r.applySynchronizedConditionWithPatch(
ctx, existingMAPIMachineSet, corev1.ConditionFalse, reasonFailedToGetCAPIInfraResources, fetchErr.Error(), nil); condErr != nil {
return ctrl.Result{}, utilerrors.NewAggregate([]error{fetchErr, condErr})
Expand Down Expand Up @@ -1531,3 +1565,13 @@ func hasSpecOrMetadataOrProviderSpecChanges(diff map[string]any) bool {

return ok1 || ok2 || ok3
}

// isTerminalConfigurationError returns true if the provided error is
// errInvalidClusterInfraReference or errInvalidInfraMachineTemplateReference.
func isTerminalConfigurationError(err error) bool {
if errors.Is(err, errInvalidInfraClusterReference) || errors.Is(err, errInvalidInfraMachineTemplateReference) {
return true
}

return false
}
Loading