fix: Correctly handle variable overrides

Currently the metamutator only passes the override values when they are
present on a machine deployment or control plane config. This commit
fixes that by merging the global variables into the overrides, resulting
in the behaviour that we always expected.

As overrides are not that commonly used yet, we have not seen this issue
but as taints, etc become more commonly used this will become an issue.

Tests added to prove the fix as well.

Author: jimmidyson

common/go.mod

@@ -8,6 +8,7 @@ go 1.23.0
 toolchain go1.24.5
 
 require (
+	dario.cat/mergo v1.0.1
 	github.com/evanphx/json-patch/v5 v5.9.11
 	github.com/go-logr/logr v1.4.3
 	github.com/onsi/ginkgo/v2 v2.23.4

common/go.sum

@@ -1,5 +1,7 @@
 cel.dev/expr v0.18.0 h1:CJ6drgk+Hf96lkLikr4rFf19WrU0BOWEihyZnI2TAzo=
 cel.dev/expr v0.18.0/go.mod h1:MrpN08Q+lEBs+bGYdLxxHkZoUSsCp0nSKTs0nTymJgw=
+dario.cat/mergo v1.0.1 h1:Ra4+bf83h2ztPIQYNP99R6m+Y7KfnARDfID+a+vLl4s=
+dario.cat/mergo v1.0.1/go.mod h1:uNxQE+84aUszobStD9th8a29P2fMDhsBdgRYvZOxGmk=
 github.com/antlr4-go/antlr/v4 v4.13.0 h1:lxCg3LAv+EUK6t1i0y1V6/SLeUi0eKEKdhQAlS8TVTI=
 github.com/antlr4-go/antlr/v4 v4.13.0/go.mod h1:pfChB/xh/Unjila75QW7+VU4TSnWnnk9UTnmpPaOR2g=
 github.com/asaskevich/govalidator v0.0.0-20230301143203-a9d515a09cc2 h1:DklsrG3dyBCFEj5IhUbnKptjxatkF07cF2ak3yi77so=

common/pkg/capi/clustertopology/handlers/mutation/meta.go

@@ -5,9 +5,13 @@ package mutation
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
+	"maps"
 	"sync"
 
+	"dario.cat/mergo"
+	"github.com/samber/lo"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -89,6 +93,21 @@ func (mgp metaGeneratePatches) GeneratePatches(
 ) {
 	clusterKey := handlers.ClusterKeyFromReq(req)
 	clusterGetter := mgp.CreateClusterGetter(clusterKey)
+
+	// Create a map of variables from the request that can be overridden by machine deployment or control plane
+	// configuration.
+	// Filter out the "builtin" variable, which is already present in the vars map and should not be overridden by
+	// the global variables.
+	globalVars := lo.FilterSliceToMap(
+		req.Variables,
+		func(v runtimehooksv1.Variable) (string, apiextensionsv1.JSON, bool) {
+			if v.Name == "builtin" {
+				return "", apiextensionsv1.JSON{}, false
+			}
+			return v.Name, v.Value, true
+		},
+	)
+
 	topologymutation.WalkTemplates(
 		ctx,
 		unstructured.UnstructuredJSONScheme,
@@ -108,20 +127,88 @@ func (mgp metaGeneratePatches) GeneratePatches(
 
 			log.V(3).Info("Starting mutation pipeline", "handlerCount", len(mgp.mutators))
 
-			for i, h := range mgp.mutators {
-				handlerName := fmt.Sprintf("%T", h)
-				log.V(5).Info("Running mutator", "index", i, "handler", handlerName)
+			// Merge the global variables to the current resource vars. This allows the handlers to access
+			// the variables defined in the cluster class or cluster configuration and use these correctly when
+			// overrides are specified on machine deployment or control plane configuration.
+			mergedVars, err := mergeVariableDefinitions(vars, globalVars)
+			if err != nil {
+				log.Error(err, "Failed to merge global variables")
+				return err
+			}
 
-				if err := h.Mutate(ctx, obj.(*unstructured.Unstructured), vars, holderRef, clusterKey, clusterGetter); err != nil {
-					log.Error(err, "Mutator failed", "index", i, "handler", handlerName)
+			for i, h := range mgp.mutators {
+				mutatorType := fmt.Sprintf("%T", h)
+				log.V(5).
+					Info("Running mutator", "index", i, "handler", mutatorType, "vars", vars)
+
+				if err := h.Mutate(
+					ctx,
+					obj.(*unstructured.Unstructured),
+					mergedVars,
+					holderRef,
+					clusterKey,
+					clusterGetter,
+				); err != nil {
+					log.Error(err, "Mutator failed", "index", i, "handler", mutatorType)
 					return err
 				}
 
-				log.V(5).Info("Mutator completed successfully", "index", i, "handler", handlerName)
+				log.V(5).Info("Mutator completed successfully", "index", i, "handler", mutatorType)
 			}
 
 			log.V(3).Info("Mutation pipeline completed successfully", "handlerCount", len(mgp.mutators))
 			return nil
 		},
 	)
 }
+
+func mergeVariableDefinitions(
+	vars, globalVars map[string]apiextensionsv1.JSON,
+) (map[string]apiextensionsv1.JSON, error) {
+	mergedVars := maps.Clone(vars)
+
+	for k, v := range globalVars {
+		// If the value of v is nil, skip it.
+		if v.Raw == nil {
+			continue
+		}
+
+		existingValue, exists := mergedVars[k]
+
+		// If the variable does not exist in the mergedVars or the value is nil, add it and continue.
+		if !exists || existingValue.Raw == nil {
+			mergedVars[k] = v
+			continue
+		}
+
+		// Wrap the value in a temporary key to ensure we can unmarshal to a map.
+		// This is necessary because the values could be scalars.
+		tempValJSON := fmt.Sprintf(`{"value": %s}`, string(existingValue.Raw))
+		tempGlobalValJSON := fmt.Sprintf(`{"value": %s}`, string(v.Raw))
+
+		// Unmarshal the existing value and the global value into maps.
+		var val, globalVal map[string]interface{}
+		if err := json.Unmarshal([]byte(tempValJSON), &val); err != nil {
+			return nil, fmt.Errorf("failed to unmarshal existing value for key %q: %w", k, err)
+		}
+
+		if err := json.Unmarshal([]byte(tempGlobalValJSON), &globalVal); err != nil {
+			return nil, fmt.Errorf("failed to unmarshal global value for key %q: %w", k, err)
+		}
+
+		// Now use mergo to perform a deep merge of the values, retaining the values in `val` if present.
+		if err := mergo.Merge(&val, globalVal); err != nil {
+			return nil, fmt.Errorf("failed to merge values for key %q: %w", k, err)
+		}
+
+		// Marshal the merged value back to JSON.
+		mergedVal, err := json.Marshal(val["value"])
+		if err != nil {
+			return nil, fmt.Errorf("failed to marshal merged value for key %q: %w", k, err)
+		}
+
+		mergedVars[k] = apiextensionsv1.JSON{Raw: mergedVal}
+	}
+
+	return mergedVars, nil
+}