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.

Sign up for GitHub

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

Jump to bottom

⚠️ Add filter to associate variables with specific patches #8128

Merged
merged 1 commit into from
Feb 23, 2023
Merged

⚠️ Add filter to associate variables with specific patches #8128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
119 changes: 94 additions & 25 deletions internal/controllers/topology/cluster/patches/engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
sbueringer marked this conversation as resolved.
Show resolved Hide resolved
// If there is no definitionFrom return an error.
if definitionFrom == "" {
killianmuldoon marked this conversation as resolved.
Show resolved Hide resolved
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)
fabriziopandini marked this conversation as resolved.
Show resolved Hide resolved
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.
sbueringer marked this conversation as resolved.
Show resolved Hide resolved
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").
Expand All @@ -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").
Expand All @@ -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,
Expand All @@ -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)
}

Expand All @@ -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").
Expand All @@ -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.
Expand All @@ -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)
}

Expand Down Expand Up @@ -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 {
killianmuldoon marked this conversation as resolved.
Show resolved Hide resolved
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
}
138 changes: 138 additions & 0 deletions internal/controllers/topology/cluster/patches/engine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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{},
Expand Down Expand Up @@ -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.
Expand All @@ -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(),
Expand Down
Loading