Merge pull request #8128 from killianmuldoon/pr-exvars-per-definition-patches

⚠️  Add filter to associate variables with specific patches

Author: k8s-ci-robot

internal/controllers/topology/cluster/patches/engine.go

@@ -26,6 +26,7 @@ import (
 	jsonpatch "github.com/evanphx/json-patch/v5"
 	"github.com/pkg/errors"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
+	"k8s.io/klog/v2"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1"
@@ -83,6 +84,14 @@ func (e *engine) Apply(ctx context.Context, blueprint *scope.ClusterBlueprint, d
 		clusterClassPatch := blueprint.ClusterClass.Spec.Patches[i]
 		ctx, log := log.WithValues("patch", clusterClassPatch.Name).Into(ctx)
 
+		definitionFrom := clusterClassPatch.Name
+		// If this isn't an external patch, use the inline patch name.
+		if clusterClassPatch.External == nil {
+			definitionFrom = clusterv1.VariableDefinitionFromInline
+		}
+		if err := addVariablesForPatch(blueprint, desired, req, definitionFrom); err != nil {
+			return errors.Wrapf(err, "failed to calculate variables for patch %q", clusterClassPatch.Name)
+		}
 		log.V(5).Infof("Applying patch to templates")
 
 		// Create patch generator for the current patch.
@@ -139,23 +148,86 @@ func (e *engine) Apply(ctx context.Context, blueprint *scope.ClusterBlueprint, d
 	return nil
 }
 
+// addVariablesForPatch adds variables for a given ClusterClassPatch to the items in the PatchRequest.
+func addVariablesForPatch(blueprint *scope.ClusterBlueprint, desired *scope.ClusterState, req *runtimehooksv1.GeneratePatchesRequest, definitionFrom string) error {
+	// If there is no definitionFrom return an error.
+	if definitionFrom == "" {
+		return errors.New("failed to calculate variables: no patch name provided")
+	}
+
+	patchVariableDefinitions := definitionsForPatch(blueprint, definitionFrom)
+	// Calculate global variables.
+	globalVariables, err := variables.Global(blueprint.Topology, desired.Cluster, definitionFrom, patchVariableDefinitions)
+	if err != nil {
+		return errors.Wrapf(err, "failed to calculate global variables")
+	}
+	req.Variables = globalVariables
+
+	// Calculate the Control Plane variables.
+	controlPlaneVariables, err := variables.ControlPlane(&blueprint.Topology.ControlPlane, desired.ControlPlane.Object, desired.ControlPlane.InfrastructureMachineTemplate)
+	if err != nil {
+		return errors.Wrapf(err, "failed to calculate ControlPlane variables")
+	}
+
+	mdStateIndex := map[string]*scope.MachineDeploymentState{}
+	for _, md := range desired.MachineDeployments {
+		mdStateIndex[md.Object.Name] = md
+	}
+	for i, item := range req.Items {
+		// If the item is a Control Plane add the Control Plane variables.
+		if item.HolderReference.FieldPath == "spec.controlPlaneRef" {
+			item.Variables = controlPlaneVariables
+		}
+		// If the item holder reference is a Control Plane machine add the Control Plane variables.
+		if blueprint.HasControlPlaneInfrastructureMachine() &&
+			item.HolderReference.FieldPath == strings.Join(contract.ControlPlane().MachineTemplate().InfrastructureRef().Path(), ".") {
+			item.Variables = controlPlaneVariables
+		}
+		// If the item holder reference is a MachineDeployment calculate the variables for each MachineDeploymentTopology
+		// and add them to the variables for the MachineDeployment.
+		if item.HolderReference.Kind == "MachineDeployment" {
+			md, ok := mdStateIndex[item.HolderReference.Name]
+			if !ok {
+				return errors.Errorf("could not find desired state for MachineDeployment %s", klog.KObj(md.Object))
+			}
+			mdTopology, err := getMDTopologyFromMD(blueprint, md.Object)
+			if err != nil {
+				return err
+			}
+
+			// Calculate MachineDeployment variables.
+			mdVariables, err := variables.MachineDeployment(mdTopology, md.Object, md.BootstrapTemplate, md.InfrastructureMachineTemplate, definitionFrom, patchVariableDefinitions)
+			if err != nil {
+				return errors.Wrapf(err, "failed to calculate variables for %s", klog.KObj(md.Object))
+			}
+			item.Variables = mdVariables
+		}
+		req.Items[i] = item
+	}
+	return nil
+}
+
+func getMDTopologyFromMD(blueprint *scope.ClusterBlueprint, md *clusterv1.MachineDeployment) (*clusterv1.MachineDeploymentTopology, error) {
+	topologyName, ok := md.Labels[clusterv1.ClusterTopologyMachineDeploymentNameLabel]
+	if !ok {
+		return nil, errors.Errorf("failed to get topology name for %s", klog.KObj(md))
+	}
+	mdTopology, err := lookupMDTopology(blueprint.Topology, topologyName)
+	if err != nil {
+		return nil, err
+	}
+	return mdTopology, nil
+}
+
 // createRequest creates a GeneratePatchesRequest based on the ClusterBlueprint and the desired state.
-// ClusterBlueprint supplies the templates. Desired state is used to calculate variables which are later used
-// as input for the patch generation.
 // NOTE: GenerateRequestTemplates are created for the templates of each individual MachineDeployment in the desired
 // state. This is necessary because some builtin variables are MachineDeployment specific. For example version and
 // replicas of a MachineDeployment.
 // NOTE: A single GeneratePatchesRequest object is used to carry templates state across subsequent Generate calls.
+// NOTE: This function does not add variables to items for the request, as the variables depend on the specific patch.
 func createRequest(blueprint *scope.ClusterBlueprint, desired *scope.ClusterState) (*runtimehooksv1.GeneratePatchesRequest, error) {
 	req := &runtimehooksv1.GeneratePatchesRequest{}
 
-	// Calculate global variables.
-	globalVariables, err := variables.Global(blueprint.Topology, desired.Cluster)
-	if err != nil {
-		return nil, errors.Wrapf(err, "failed to calculate global variables")
-	}
-	req.Variables = globalVariables
-
 	// Add the InfrastructureClusterTemplate.
 	t, err := newRequestItemBuilder(blueprint.InfrastructureClusterTemplate).
 		WithHolder(desired.Cluster, "spec.infrastructureRef").
@@ -166,12 +238,6 @@ func createRequest(blueprint *scope.ClusterBlueprint, desired *scope.ClusterStat
 	}
 	req.Items = append(req.Items, *t)
 
-	// Calculate controlPlane variables.
-	controlPlaneVariables, err := variables.ControlPlane(&blueprint.Topology.ControlPlane, desired.ControlPlane.Object, desired.ControlPlane.InfrastructureMachineTemplate)
-	if err != nil {
-		return nil, errors.Wrapf(err, "failed to calculate ControlPlane variables")
-	}
-
 	// Add the ControlPlaneTemplate.
 	t, err = newRequestItemBuilder(blueprint.ControlPlane.Template).
 		WithHolder(desired.Cluster, "spec.controlPlaneRef").
@@ -180,7 +246,6 @@ func createRequest(blueprint *scope.ClusterBlueprint, desired *scope.ClusterStat
 		return nil, errors.Wrapf(err, "failed to prepare ControlPlane template %s for patching",
 			tlog.KObj{Obj: blueprint.ControlPlane.Template})
 	}
-	t.Variables = controlPlaneVariables
 	req.Items = append(req.Items, *t)
 
 	// If the clusterClass mandates the controlPlane has infrastructureMachines,
@@ -193,7 +258,6 @@ func createRequest(blueprint *scope.ClusterBlueprint, desired *scope.ClusterStat
 			return nil, errors.Wrapf(err, "failed to prepare ControlPlane's machine template %s for patching",
 				tlog.KObj{Obj: blueprint.ControlPlane.InfrastructureMachineTemplate})
 		}
-		t.Variables = controlPlaneVariables
 		req.Items = append(req.Items, *t)
 	}
 
@@ -216,12 +280,6 @@ func createRequest(blueprint *scope.ClusterBlueprint, desired *scope.ClusterStat
 			return nil, errors.Errorf("failed to lookup MachineDeployment class %q in ClusterClass", mdTopology.Class)
 		}
 
-		// Calculate MachineDeployment variables.
-		mdVariables, err := variables.MachineDeployment(mdTopology, md.Object, md.BootstrapTemplate, md.InfrastructureMachineTemplate)
-		if err != nil {
-			return nil, errors.Wrapf(err, "failed to calculate variables for %s", tlog.KObj{Obj: md.Object})
-		}
-
 		// Add the BootstrapTemplate.
 		t, err := newRequestItemBuilder(mdClass.BootstrapTemplate).
 			WithHolder(md.Object, "spec.template.spec.bootstrap.configRef").
@@ -230,7 +288,6 @@ func createRequest(blueprint *scope.ClusterBlueprint, desired *scope.ClusterStat
 			return nil, errors.Wrapf(err, "failed to prepare BootstrapConfig template %s for MachineDeployment topology %s for patching",
 				tlog.KObj{Obj: mdClass.BootstrapTemplate}, mdTopologyName)
 		}
-		t.Variables = mdVariables
 		req.Items = append(req.Items, *t)
 
 		// Add the InfrastructureMachineTemplate.
@@ -241,7 +298,6 @@ func createRequest(blueprint *scope.ClusterBlueprint, desired *scope.ClusterStat
 			return nil, errors.Wrapf(err, "failed to prepare InfrastructureMachine template %s for MachineDeployment topology %s for patching",
 				tlog.KObj{Obj: mdClass.InfrastructureMachineTemplate}, mdTopologyName)
 		}
-		t.Variables = mdVariables
 		req.Items = append(req.Items, *t)
 	}
 
@@ -431,3 +487,16 @@ func updateDesiredState(ctx context.Context, req *runtimehooksv1.GeneratePatches
 
 	return nil
 }
+
+// definitionsForPatch returns a set which of variables in a ClusterClass defined by "definitionFrom".
+func definitionsForPatch(blueprint *scope.ClusterBlueprint, definitionFrom string) map[string]bool {
+	variableDefinitionsForPatch := make(map[string]bool)
+	for _, definitionsWithName := range blueprint.ClusterClass.Status.Variables {
+		for _, definition := range definitionsWithName.Definitions {
+			if definition.From == definitionFrom {
+				variableDefinitionsForPatch[definitionsWithName.Name] = true
+			}
+		}
+	}
+	return variableDefinitionsForPatch
+}

internal/controllers/topology/cluster/patches/engine_test.go

@@ -54,6 +54,7 @@ func TestApply(t *testing.T) {
 	tests := []struct {
 		name                   string
 		patches                []clusterv1.ClusterClassPatch
+		varDefinitions         []clusterv1.ClusterClassStatusVariable
 		externalPatchResponses map[string]runtimehooksv1.ResponseObject
 		expectedFields         expectedFields
 		wantErr                bool
@@ -479,6 +480,109 @@ func TestApply(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "Should correctly apply variables for a given patch definitionFrom",
+			varDefinitions: []clusterv1.ClusterClassStatusVariable{
+				{
+					Name: "default-worker-infra",
+					Definitions: []clusterv1.ClusterClassStatusVariableDefinition{
+						{
+							From: "inline",
+						},
+					},
+				},
+				{
+					Name:                "infraCluster",
+					DefinitionsConflict: true,
+					Definitions: []clusterv1.ClusterClassStatusVariableDefinition{
+						{
+							From: "inline",
+						},
+						{
+							From: "not-used-patch",
+						},
+					},
+				},
+			},
+			patches: []clusterv1.ClusterClassPatch{
+				{
+					Name: "fake-patch1",
+					Definitions: []clusterv1.PatchDefinition{
+						{
+							Selector: clusterv1.PatchSelector{
+								APIVersion: builder.InfrastructureGroupVersion.String(),
+								Kind:       builder.GenericInfrastructureClusterTemplateKind,
+								MatchResources: clusterv1.PatchSelectorMatch{
+									InfrastructureCluster: true,
+								},
+							},
+							JSONPatches: []clusterv1.JSONPatch{
+								{
+									Op:   "add",
+									Path: "/spec/template/spec/resource",
+									ValueFrom: &clusterv1.JSONPatchValue{
+										Variable: pointer.String("infraCluster"),
+									},
+								},
+							},
+						},
+						{
+							Selector: clusterv1.PatchSelector{
+								APIVersion: builder.BootstrapGroupVersion.String(),
+								Kind:       builder.GenericBootstrapConfigTemplateKind,
+								MatchResources: clusterv1.PatchSelectorMatch{
+									MachineDeploymentClass: &clusterv1.PatchSelectorMatchMachineDeploymentClass{
+										Names: []string{"default-worker"},
+									},
+								},
+							},
+							JSONPatches: []clusterv1.JSONPatch{
+								{
+									Op:   "add",
+									Path: "/spec/template/spec/resource",
+									ValueFrom: &clusterv1.JSONPatchValue{
+										Variable: pointer.String("default-worker-infra"),
+									},
+								},
+							},
+						},
+						{
+							Selector: clusterv1.PatchSelector{
+								APIVersion: builder.InfrastructureGroupVersion.String(),
+								Kind:       builder.GenericInfrastructureMachineTemplateKind,
+								MatchResources: clusterv1.PatchSelectorMatch{
+									MachineDeploymentClass: &clusterv1.PatchSelectorMatchMachineDeploymentClass{
+										Names: []string{"default-worker"},
+									},
+								},
+							},
+							JSONPatches: []clusterv1.JSONPatch{
+								{
+									Op:   "add",
+									Path: "/spec/template/spec/resource",
+									ValueFrom: &clusterv1.JSONPatchValue{
+										Variable: pointer.String("default-worker-infra"),
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			expectedFields: expectedFields{
+				infrastructureCluster: map[string]interface{}{
+					"spec.resource": "value99",
+				},
+				machineDeploymentInfrastructureMachineTemplate: map[string]map[string]interface{}{
+					"default-worker-topo1": {"spec.template.spec.resource": "value1"},
+					"default-worker-topo2": {"spec.template.spec.resource": "default-worker-topo2"},
+				},
+				machineDeploymentBootstrapTemplate: map[string]map[string]interface{}{
+					"default-worker-topo1": {"spec.template.spec.resource": "value1"},
+					"default-worker-topo2": {"spec.template.spec.resource": "default-worker-topo2"},
+				},
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -521,6 +625,10 @@ func TestApply(t *testing.T) {
 				// Add the patches.
 				blueprint.ClusterClass.Spec.Patches = tt.patches
 			}
+			if len(tt.varDefinitions) > 0 {
+				// If there are variable definitions in the test add them to the ClusterClass.
+				blueprint.ClusterClass.Status.Variables = tt.varDefinitions
+			}
 
 			// Copy the desired objects before applying patches.
 			expectedCluster := desired.Cluster.DeepCopy()
@@ -631,12 +739,40 @@ func setupTestObjects() (*scope.ClusterBlueprint, *scope.ClusterState) {
 				ControlPlane: clusterv1.ControlPlaneTopology{
 					Replicas: pointer.Int32(3),
 				},
+				Variables: []clusterv1.ClusterVariable{
+					{
+						Name:           "infraCluster",
+						Value:          apiextensionsv1.JSON{Raw: []byte(`"value99"`)},
+						DefinitionFrom: "inline",
+					},
+					{
+						Name: "infraCluster",
+						// This variable should not be used as it is from "non-used-patch" which is not applied in any test.
+						Value:          apiextensionsv1.JSON{Raw: []byte(`"should-never-be-used"`)},
+						DefinitionFrom: "not-used-patch",
+					},
+					{
+						Name: "default-worker-infra",
+						// This value should be overwritten for the default-worker-topo1 MachineDeployment.
+						Value:          apiextensionsv1.JSON{Raw: []byte(`"default-worker-topo2"`)},
+						DefinitionFrom: "inline",
+					},
+				},
 				Workers: &clusterv1.WorkersTopology{
 					MachineDeployments: []clusterv1.MachineDeploymentTopology{
 						{
 							Metadata: clusterv1.ObjectMeta{},
 							Class:    "default-worker",
 							Name:     "default-worker-topo1",
+							Variables: &clusterv1.MachineDeploymentVariables{
+								Overrides: []clusterv1.ClusterVariable{
+									{
+										Name:           "default-worker-infra",
+										DefinitionFrom: "inline",
+										Value:          apiextensionsv1.JSON{Raw: []byte(`"value1"`)},
+									},
+								},
+							},
 						},
 						{
 							Metadata: clusterv1.ObjectMeta{},
@@ -697,6 +833,7 @@ func setupTestObjects() (*scope.ClusterBlueprint, *scope.ClusterState) {
 		MachineDeployments: map[string]*scope.MachineDeploymentState{
 			"default-worker-topo1": {
 				Object: builder.MachineDeployment(metav1.NamespaceDefault, "md1").
+					WithLabels(map[string]string{clusterv1.ClusterTopologyMachineDeploymentNameLabel: "default-worker-topo1"}).
 					WithVersion("v1.21.2").
 					Build(),
 				// Make sure we're using an independent instance of the template.
@@ -705,6 +842,7 @@ func setupTestObjects() (*scope.ClusterBlueprint, *scope.ClusterState) {
 			},
 			"default-worker-topo2": {
 				Object: builder.MachineDeployment(metav1.NamespaceDefault, "md2").
+					WithLabels(map[string]string{clusterv1.ClusterTopologyMachineDeploymentNameLabel: "default-worker-topo2"}).
 					WithVersion("v1.20.6").
 					WithReplicas(5).
 					Build(),