diff --git a/internal/controllers/topology/cluster/patches/api/interface.go b/internal/controllers/topology/cluster/patches/api/interface.go index 8a50a5e336f0..5fd5aef3b091 100644 --- a/internal/controllers/topology/cluster/patches/api/interface.go +++ b/internal/controllers/topology/cluster/patches/api/interface.go @@ -23,128 +23,14 @@ package api import ( "context" - "fmt" - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1" ) // Generator defines a component that can generate patches for ClusterClass templates. type Generator interface { // Generate generates patches for templates. - // GenerateRequest contains templates and the corresponding variables. - // GenerateResponse contains the patches which should be applied to the templates of the GenerateRequest. - Generate(ctx context.Context, request *GenerateRequest) (*GenerateResponse, error) -} - -// GenerateRequest defines the input for a Generate request. -type GenerateRequest struct { - // Variables is a name/value map containing variables. - Variables map[string]apiextensionsv1.JSON - - // Items contains the list of templates to generate patches for. - Items []*GenerateRequestTemplate -} - -// GenerateRequestTemplate defines one of the ClusterClass templates to generate patches for. -type GenerateRequestTemplate struct { - // TemplateRef identifies a template to generate patches for; - // the same TemplateRef must be used when specifying to which template a generated patch should be applied to. - TemplateRef TemplateRef - - // Variables is a name/value map containing variables specifically for the current template. - // For example some builtin variables like MachineDeployment replicas and version are context-sensitive - // and thus are only added to templates for MachineDeployments and with values which correspond to the - // current MachineDeployment. - Variables map[string]apiextensionsv1.JSON - - // Template contains the template. - Template apiextensionsv1.JSON -} - -// TemplateRef identifies one of the ClusterClass templates to generate patches for; -// the same TemplateRef must be used when specifying where a generated patch should apply to. -type TemplateRef struct { - // APIVersion of the current template. - APIVersion string - - // Kind of the current template. - Kind string - - // TemplateType defines where the template is used. - TemplateType TemplateType - - // MachineDeployment specifies the MachineDeployment in which the template is used. - // This field is only set if the template is used in the context of a MachineDeployment. - MachineDeploymentRef MachineDeploymentRef -} - -func (t TemplateRef) String() string { - ret := fmt.Sprintf("%s %s/%s", t.TemplateType, t.APIVersion, t.Kind) - if t.MachineDeploymentRef.TopologyName != "" { - ret = fmt.Sprintf("%s, MachineDeployment topology %s", ret, t.MachineDeploymentRef.TopologyName) - } - if t.MachineDeploymentRef.Class != "" { - ret = fmt.Sprintf("%s, MachineDeployment class %s", ret, t.MachineDeploymentRef.Class) - } - return ret -} - -// MachineDeploymentRef specifies the MachineDeployment in which the template is used. -type MachineDeploymentRef struct { - // TopologyName is the name of the MachineDeploymentTopology. - TopologyName string - - // Class is the name of the MachineDeploymentClass. - Class string -} - -// TemplateType define the type for target types enum. -type TemplateType string - -const ( - // InfrastructureClusterTemplateType identifies a template for the InfrastructureCluster object. - InfrastructureClusterTemplateType TemplateType = "InfrastructureClusterTemplate" - - // ControlPlaneTemplateType identifies a template for the ControlPlane object. - ControlPlaneTemplateType TemplateType = "ControlPlaneTemplate" - - // ControlPlaneInfrastructureMachineTemplateType identifies a template for the InfrastructureMachines to be used for the ControlPlane object. - ControlPlaneInfrastructureMachineTemplateType TemplateType = "ControlPlane/InfrastructureMachineTemplate" - - // MachineDeploymentBootstrapConfigTemplateType identifies a template for the BootstrapConfig to be used for a MachineDeployment object. - MachineDeploymentBootstrapConfigTemplateType TemplateType = "MachineDeployment/BootstrapConfigTemplate" - - // MachineDeploymentInfrastructureMachineTemplateType identifies a template for the InfrastructureMachines to be used for a MachineDeployment object. - MachineDeploymentInfrastructureMachineTemplateType TemplateType = "MachineDeployment/InfrastructureMachineTemplate" -) - -// PatchType define the type for patch types enum. -type PatchType string - -const ( - // JSONPatchType identifies a https://datatracker.ietf.org/doc/html/rfc6902 json patch. - JSONPatchType PatchType = "JSONPatch" - - // JSONMergePatchType identifies a https://datatracker.ietf.org/doc/html/rfc7386 json merge patch. - JSONMergePatchType PatchType = "JSONMergePatch" -) - -// GenerateResponse defines the response of a Generate request. -// NOTE: Patches defined in GenerateResponse will be applied in the same order to the original -// GenerateRequest object, thus adding changes on templates across all the subsequent Generate calls. -type GenerateResponse struct { - // Items contains the list of generated patches. - Items []GenerateResponsePatch -} - -// GenerateResponsePatch defines a Patch targeting a specific GenerateRequestTemplate. -type GenerateResponsePatch struct { - // TemplateRef identifies the template the patch should apply to. - TemplateRef TemplateRef - - // Patch contains the patch. - Patch apiextensionsv1.JSON - - // Patch defines the type of the JSON patch. - PatchType PatchType + // GeneratePatchesRequest contains templates and the corresponding variables. + // GeneratePatchesResponse contains the patches which should be applied to the templates of the GenerateRequest. + Generate(context.Context, *runtimehooksv1.GeneratePatchesRequest) *runtimehooksv1.GeneratePatchesResponse } diff --git a/internal/controllers/topology/cluster/patches/engine.go b/internal/controllers/topology/cluster/patches/engine.go index 227a6ce7ea43..cfb6066eaf1e 100644 --- a/internal/controllers/topology/cluster/patches/engine.go +++ b/internal/controllers/topology/cluster/patches/engine.go @@ -19,11 +19,13 @@ package patches import ( "context" + "strings" jsonpatch "github.com/evanphx/json-patch/v5" "github.com/pkg/errors" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1" "sigs.k8s.io/cluster-api/internal/contract" "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/patches/api" "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/patches/inline" @@ -54,9 +56,9 @@ type engine struct { // Apply applies patches to the desired state according to the patches from the ClusterClass, variables from the Cluster // and builtin variables. -// * A GenerateRequest with all templates and global and template-specific variables is created. +// * A GeneratePatchesRequest with all templates and global and template-specific variables is created. // * Then for all ClusterClassPatches of a ClusterClass, JSON or JSON merge patches are generated -// and successively applied to the templates in the GenerateRequest. +// and successively applied to the templates in the GeneratePatchesRequest. // * Eventually the patched templates are used to update the specs of the desired objects. func (e *engine) Apply(ctx context.Context, blueprint *scope.ClusterBlueprint, desired *scope.ClusterState) error { // Return if there are no patches. @@ -90,9 +92,9 @@ func (e *engine) Apply(ctx context.Context, blueprint *scope.ClusterBlueprint, d // NOTE: All the partial patches accumulate on top of the request, so the // patch generator in the next iteration of the loop will get the modified // version of the request (including the patched version of the templates). - resp, err := generator.Generate(ctx, req) - if err != nil { - return errors.Wrapf(err, "failed to generate patches for patch %q", clusterClassPatch.Name) + resp := generator.Generate(ctx, req) + if resp.Status == runtimehooksv1.ResponseStatusFailure { + return errors.Errorf("failed to generate patches for patch %q: %v", clusterClassPatch.Name, resp.Message) } // Apply patches to the request. @@ -110,15 +112,15 @@ func (e *engine) Apply(ctx context.Context, blueprint *scope.ClusterBlueprint, d return nil } -// createRequest creates a GenerateRequest based on the ClusterBlueprint and the desired state. +// 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 GenerateRequest object is used to carry templates state across subsequent Generate calls. -func createRequest(blueprint *scope.ClusterBlueprint, desired *scope.ClusterState) (*api.GenerateRequest, error) { - req := &api.GenerateRequest{} +// NOTE: A single GeneratePatchesRequest object is used to carry templates state across subsequent Generate calls. +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) @@ -128,14 +130,14 @@ func createRequest(blueprint *scope.ClusterBlueprint, desired *scope.ClusterStat req.Variables = globalVariables // Add the InfrastructureClusterTemplate. - t, err := newTemplateBuilder(blueprint.InfrastructureClusterTemplate). - WithType(api.InfrastructureClusterTemplateType). + t, err := newRequestItemBuilder(blueprint.InfrastructureClusterTemplate). + WithHolder(desired.Cluster, "spec.infrastructureRef"). Build() if err != nil { return nil, errors.Wrapf(err, "failed to prepare InfrastructureCluster template %s for patching", tlog.KObj{Obj: blueprint.InfrastructureClusterTemplate}) } - req.Items = append(req.Items, t) + req.Items = append(req.Items, *t) // Calculate controlPlane variables. controlPlaneVariables, err := variables.ControlPlane(&blueprint.Topology.ControlPlane, desired.ControlPlane.Object, desired.ControlPlane.InfrastructureMachineTemplate) @@ -144,28 +146,28 @@ func createRequest(blueprint *scope.ClusterBlueprint, desired *scope.ClusterStat } // Add the ControlPlaneTemplate. - t, err = newTemplateBuilder(blueprint.ControlPlane.Template). - WithType(api.ControlPlaneTemplateType). + t, err = newRequestItemBuilder(blueprint.ControlPlane.Template). + WithHolder(desired.Cluster, "spec.controlPlaneRef"). Build() if err != nil { 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) + req.Items = append(req.Items, *t) // If the clusterClass mandates the controlPlane has infrastructureMachines, // add the InfrastructureMachineTemplate for control plane machines. if blueprint.HasControlPlaneInfrastructureMachine() { - t, err := newTemplateBuilder(blueprint.ControlPlane.InfrastructureMachineTemplate). - WithType(api.ControlPlaneInfrastructureMachineTemplateType). + t, err := newRequestItemBuilder(blueprint.ControlPlane.InfrastructureMachineTemplate). + WithHolder(desired.ControlPlane.Object, strings.Join(contract.ControlPlane().MachineTemplate().InfrastructureRef().Path(), ".")). Build() if err != nil { 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) + req.Items = append(req.Items, *t) } // Add BootstrapConfigTemplate and InfrastructureMachine template for all MachineDeploymentTopologies @@ -194,28 +196,26 @@ func createRequest(blueprint *scope.ClusterBlueprint, desired *scope.ClusterStat } // Add the BootstrapTemplate. - t, err := newTemplateBuilder(mdClass.BootstrapTemplate). - WithType(api.MachineDeploymentBootstrapConfigTemplateType). - WithMachineDeploymentRef(mdTopology). + t, err := newRequestItemBuilder(mdClass.BootstrapTemplate). + WithHolder(md.Object, "spec.template.spec.bootstrap.configRef"). Build() if err != nil { 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) + req.Items = append(req.Items, *t) // Add the InfrastructureMachineTemplate. - t, err = newTemplateBuilder(mdClass.InfrastructureMachineTemplate). - WithType(api.MachineDeploymentInfrastructureMachineTemplateType). - WithMachineDeploymentRef(mdTopology). + t, err = newRequestItemBuilder(mdClass.InfrastructureMachineTemplate). + WithHolder(md.Object, "spec.template.spec.infrastructureRef"). Build() if err != nil { 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) + req.Items = append(req.Items, *t) } return req, nil @@ -243,20 +243,20 @@ func createPatchGenerator(patch *clusterv1.ClusterClassPatch) (api.Generator, er return nil, errors.Errorf("failed to create patch generator for patch %q", patch.Name) } -// applyPatchesToRequest updates the templates of a GenerateRequest by applying the patches -// of a GenerateResponse. -func applyPatchesToRequest(ctx context.Context, req *api.GenerateRequest, resp *api.GenerateResponse) error { +// applyPatchesToRequest updates the templates of a GeneratePatchesRequest by applying the patches +// of a GeneratePatchesResponse. +func applyPatchesToRequest(ctx context.Context, req *runtimehooksv1.GeneratePatchesRequest, resp *runtimehooksv1.GeneratePatchesResponse) error { log := tlog.LoggerFrom(ctx) for _, patch := range resp.Items { - log = log.WithValues("templateRef", patch.TemplateRef.String()) + log = log.WithValues("uid", patch.UID) - // Get the template the patch should be applied to. - template := getTemplate(req, patch.TemplateRef) + // Get the request item the patch belongs to. + requestItem := getRequestItemByUID(req, patch.UID) - // If a patch doesn't apply to any template, this is a misconfiguration. - if template == nil { - return errors.Errorf("generated patch is targeted at the template with ID %q that does not exists", patch.TemplateRef) + // If a patch doesn't have a corresponding request item, the patch is invalid. + if requestItem == nil { + return errors.Errorf("unable to find corresponding request item with uid %q for the patch", patch.UID) } // Use the patch to create a patched copy of the template. @@ -264,49 +264,45 @@ func applyPatchesToRequest(ctx context.Context, req *api.GenerateRequest, resp * var err error switch patch.PatchType { - case api.JSONPatchType: - log.V(5).Infof("Accumulating JSON patch: %s", string(patch.Patch.Raw)) - jsonPatch, err := jsonpatch.DecodePatch(patch.Patch.Raw) + case runtimehooksv1.JSONPatchType: + log.V(5).Infof("Accumulating JSON patch: %s", string(patch.Patch)) + jsonPatch, err := jsonpatch.DecodePatch(patch.Patch) if err != nil { - return errors.Wrapf(err, "failed to apply patch to template %s: error decoding json patch (RFC6902): %s", - template.TemplateRef, string(patch.Patch.Raw)) + return errors.Wrapf(err, "failed to apply patch with uid %q: error decoding json patch (RFC6902): %s", + requestItem.UID, string(patch.Patch)) } - patchedTemplate, err = jsonPatch.Apply(template.Template.Raw) + patchedTemplate, err = jsonPatch.Apply(requestItem.Object.Raw) if err != nil { - return errors.Wrapf(err, "failed to apply patch to template %s: error applying json patch (RFC6902): %s", - template.TemplateRef, string(patch.Patch.Raw)) + return errors.Wrapf(err, "failed to apply patch with uid %q: error applying json patch (RFC6902): %s", + requestItem.UID, string(patch.Patch)) } - case api.JSONMergePatchType: - log.V(5).Infof("Accumulating JSON merge patch: %s", string(patch.Patch.Raw)) - patchedTemplate, err = jsonpatch.MergePatch(template.Template.Raw, patch.Patch.Raw) + case runtimehooksv1.JSONMergePatchType: + log.V(5).Infof("Accumulating JSON merge patch: %s", string(patch.Patch)) + patchedTemplate, err = jsonpatch.MergePatch(requestItem.Object.Raw, patch.Patch) if err != nil { - return errors.Wrapf(err, "failed to apply patch to template %s: error applying json merge patch (RFC7386): %s", - template.TemplateRef, string(patch.Patch.Raw)) + return errors.Wrapf(err, "failed to apply patch with uid %q: error applying json merge patch (RFC7386): %s", + requestItem.UID, string(patch.Patch)) } } // Overwrite the spec of template.Template with the spec of the patchedTemplate, // to ensure that we only pick up changes to the spec. - if err := patchTemplateSpec(&template.Template, patchedTemplate); err != nil { + if err := patchTemplateSpec(&requestItem.Object, patchedTemplate); err != nil { return errors.Wrapf(err, "failed to apply patch to template %s", - template.TemplateRef) + requestItem.UID) } } return nil } -// updateDesiredState uses the patched templates of a GenerateRequest to update the desired state. -// NOTE: This func should be called after all the patches have been applied to the GenerateRequest. -func updateDesiredState(ctx context.Context, req *api.GenerateRequest, blueprint *scope.ClusterBlueprint, desired *scope.ClusterState) error { +// updateDesiredState uses the patched templates of a GeneratePatchesRequest to update the desired state. +// NOTE: This func should be called after all the patches have been applied to the GeneratePatchesRequest. +func updateDesiredState(ctx context.Context, req *runtimehooksv1.GeneratePatchesRequest, blueprint *scope.ClusterBlueprint, desired *scope.ClusterState) error { var err error // Update the InfrastructureCluster. - infrastructureClusterTemplate, err := getTemplateAsUnstructured(req, - newTemplateBuilder(blueprint.InfrastructureClusterTemplate). - WithType(api.InfrastructureClusterTemplateType). - BuildTemplateRef(), - ) + infrastructureClusterTemplate, err := getTemplateAsUnstructured(req, "Cluster", "spec.infrastructureRef", "") if err != nil { return err } @@ -315,11 +311,7 @@ func updateDesiredState(ctx context.Context, req *api.GenerateRequest, blueprint } // Update the ControlPlane. - controlPlaneTemplate, err := getTemplateAsUnstructured(req, - newTemplateBuilder(blueprint.ControlPlane.Template). - WithType(api.ControlPlaneTemplateType). - BuildTemplateRef(), - ) + controlPlaneTemplate, err := getTemplateAsUnstructured(req, "Cluster", "spec.controlPlaneRef", "") if err != nil { return err } @@ -336,11 +328,7 @@ func updateDesiredState(ctx context.Context, req *api.GenerateRequest, blueprint // If the ClusterClass mandates the ControlPlane has InfrastructureMachines, // update the InfrastructureMachineTemplate for ControlPlane machines. if blueprint.HasControlPlaneInfrastructureMachine() { - infrastructureMachineTemplate, err := getTemplateAsUnstructured(req, - newTemplateBuilder(blueprint.ControlPlane.InfrastructureMachineTemplate). - WithType(api.ControlPlaneInfrastructureMachineTemplateType). - BuildTemplateRef(), - ) + infrastructureMachineTemplate, err := getTemplateAsUnstructured(req, desired.ControlPlane.Object.GetKind(), strings.Join(contract.ControlPlane().MachineTemplate().InfrastructureRef().Path(), "."), "") if err != nil { return err } @@ -351,19 +339,8 @@ func updateDesiredState(ctx context.Context, req *api.GenerateRequest, blueprint // Update the templates for all MachineDeployments. for mdTopologyName, md := range desired.MachineDeployments { - // Lookup MachineDeploymentTopology. - mdTopology, err := lookupMDTopology(blueprint.Topology, mdTopologyName) - if err != nil { - return err - } - // Update the BootstrapConfigTemplate. - bootstrapTemplate, err := getTemplateAsUnstructured(req, - newTemplateBuilder(md.BootstrapTemplate). - WithType(api.MachineDeploymentBootstrapConfigTemplateType). - WithMachineDeploymentRef(mdTopology). - BuildTemplateRef(), - ) + bootstrapTemplate, err := getTemplateAsUnstructured(req, "MachineDeployment", "spec.template.spec.bootstrap.configRef", mdTopologyName) if err != nil { return err } @@ -372,12 +349,7 @@ func updateDesiredState(ctx context.Context, req *api.GenerateRequest, blueprint } // Update the InfrastructureMachineTemplate. - infrastructureMachineTemplate, err := getTemplateAsUnstructured(req, - newTemplateBuilder(md.InfrastructureMachineTemplate). - WithType(api.MachineDeploymentInfrastructureMachineTemplateType). - WithMachineDeploymentRef(mdTopology). - BuildTemplateRef(), - ) + infrastructureMachineTemplate, err := getTemplateAsUnstructured(req, "MachineDeployment", "spec.template.spec.infrastructureRef", mdTopologyName) if err != nil { return err } diff --git a/internal/controllers/topology/cluster/patches/engine_test.go b/internal/controllers/topology/cluster/patches/engine_test.go index 3903d2a48298..f64d0f7204bf 100644 --- a/internal/controllers/topology/cluster/patches/engine_test.go +++ b/internal/controllers/topology/cluster/patches/engine_test.go @@ -23,24 +23,18 @@ import ( "testing" . "github.com/onsi/gomega" - "github.com/pkg/errors" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/utils/pointer" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" - "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/patches/api" "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/scope" "sigs.k8s.io/cluster-api/internal/test/builder" . "sigs.k8s.io/cluster-api/internal/test/matchers" ) func TestApply(t *testing.T) { - type patch struct { - name string - patches []api.GenerateResponsePatch - } type expectedFields struct { infrastructureCluster map[string]interface{} controlPlane map[string]interface{} @@ -51,7 +45,7 @@ func TestApply(t *testing.T) { tests := []struct { name string - patches []patch + patches []clusterv1.ClusterClassPatch expectedFields expectedFields }{ { @@ -61,45 +55,61 @@ func TestApply(t *testing.T) { }, { name: "Should apply JSON patches to InfraCluster, ControlPlane and ControlPlaneInfrastructureMachineTemplate", - patches: []patch{ + patches: []clusterv1.ClusterClassPatch{ { - name: "fake-patch1", - patches: []api.GenerateResponsePatch{ + Name: "fake-patch1", + Definitions: []clusterv1.PatchDefinition{ { - TemplateRef: api.TemplateRef{ - APIVersion: builder.InfrastructureGroupVersion.String(), - Kind: builder.GenericInfrastructureClusterTemplateKind, - TemplateType: api.InfrastructureClusterTemplateType, + 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", + Value: &apiextensionsv1.JSON{Raw: []byte(`"infraCluster"`)}, + }, }, - Patch: apiextensionsv1.JSON{Raw: []byte(` -[{"op":"add","path":"/spec/template/spec/resource","value":"infraCluster"}] - `)}, - PatchType: api.JSONPatchType, }, { - TemplateRef: api.TemplateRef{ - APIVersion: builder.ControlPlaneGroupVersion.String(), - Kind: builder.GenericControlPlaneTemplateKind, - TemplateType: api.ControlPlaneTemplateType, + Selector: clusterv1.PatchSelector{ + APIVersion: builder.ControlPlaneGroupVersion.String(), + Kind: builder.GenericControlPlaneTemplateKind, + MatchResources: clusterv1.PatchSelectorMatch{ + ControlPlane: true, + }, + }, + JSONPatches: []clusterv1.JSONPatch{ + { + Op: "add", + Path: "/spec/template/spec/resource", + Value: &apiextensionsv1.JSON{Raw: []byte(`"controlPlane"`)}, + }, }, - Patch: apiextensionsv1.JSON{Raw: []byte(` -[{"op":"add","path":"/spec/template/spec/resource","value":"controlPlane"}] - `)}, - PatchType: api.JSONPatchType, }, { - TemplateRef: api.TemplateRef{ - APIVersion: builder.InfrastructureGroupVersion.String(), - Kind: builder.GenericInfrastructureMachineTemplateKind, - TemplateType: api.ControlPlaneInfrastructureMachineTemplateType, + Selector: clusterv1.PatchSelector{ + APIVersion: builder.InfrastructureGroupVersion.String(), + Kind: builder.GenericInfrastructureMachineTemplateKind, + MatchResources: clusterv1.PatchSelectorMatch{ + ControlPlane: true, + }, + }, + JSONPatches: []clusterv1.JSONPatch{ + { + Op: "add", + Path: "/spec/template/spec/resource", + Value: &apiextensionsv1.JSON{Raw: []byte(`"controlPlaneInfrastructureMachineTemplate"`)}, + }, }, - Patch: apiextensionsv1.JSON{Raw: []byte(` -[{"op":"add","path":"/spec/template/spec/resource","value":"controlPlaneInfrastructureMachineTemplate"}] - `)}, - PatchType: api.JSONPatchType, }, }, - }}, + }, + }, expectedFields: expectedFields{ infrastructureCluster: map[string]interface{}{ "spec.resource": "infraCluster", @@ -114,163 +124,107 @@ func TestApply(t *testing.T) { }, { name: "Should apply JSON patches to MachineDeployment templates", - patches: []patch{ + patches: []clusterv1.ClusterClassPatch{ { - name: "fake-patch1", - patches: []api.GenerateResponsePatch{ - { // Set /spec/template/spec/resource=topo1-infra in InfrastructureMachineTemplate of default-worker-topo1. - TemplateRef: api.TemplateRef{ - APIVersion: builder.InfrastructureGroupVersion.String(), - Kind: builder.GenericInfrastructureMachineTemplateKind, - TemplateType: api.MachineDeploymentInfrastructureMachineTemplateType, - MachineDeploymentRef: api.MachineDeploymentRef{ - TopologyName: "default-worker-topo1", - Class: "default-worker", + Name: "fake-patch1", + Definitions: []clusterv1.PatchDefinition{ + { + Selector: clusterv1.PatchSelector{ + APIVersion: builder.InfrastructureGroupVersion.String(), + Kind: builder.GenericInfrastructureMachineTemplateKind, + MatchResources: clusterv1.PatchSelectorMatch{ + MachineDeploymentClass: &clusterv1.PatchSelectorMatchMachineDeploymentClass{ + Names: []string{"default-worker"}, + }, }, }, - Patch: apiextensionsv1.JSON{Raw: []byte(` -[{"op":"add","path":"/spec/template/spec/resource","value":"topo1-infra"}] - `)}, - PatchType: api.JSONPatchType, - }, - { // Set /spec/template/spec/resource=topo1-bootstrap in BootstrapTemplate of default-worker-topo1. - TemplateRef: api.TemplateRef{ - APIVersion: builder.BootstrapGroupVersion.String(), - Kind: builder.GenericBootstrapConfigTemplateKind, - TemplateType: api.MachineDeploymentBootstrapConfigTemplateType, - MachineDeploymentRef: api.MachineDeploymentRef{ - TopologyName: "default-worker-topo1", - Class: "default-worker", + JSONPatches: []clusterv1.JSONPatch{ + { + Op: "add", + Path: "/spec/template/spec/resource", + Value: &apiextensionsv1.JSON{Raw: []byte(`"default-worker-infra"`)}, }, }, - Patch: apiextensionsv1.JSON{Raw: []byte(` -[{"op":"add","path":"/spec/template/spec/resource","value":"topo1-bootstrap"}] - `)}, - PatchType: api.JSONPatchType, }, - { // Set /spec/template/spec/resource=topo2-infra in InfrastructureMachineTemplate of default-worker-topo2. - TemplateRef: api.TemplateRef{ - APIVersion: builder.InfrastructureGroupVersion.String(), - Kind: builder.GenericInfrastructureMachineTemplateKind, - TemplateType: api.MachineDeploymentInfrastructureMachineTemplateType, - MachineDeploymentRef: api.MachineDeploymentRef{ - TopologyName: "default-worker-topo2", - Class: "default-worker", + { + Selector: clusterv1.PatchSelector{ + APIVersion: builder.BootstrapGroupVersion.String(), + Kind: builder.GenericBootstrapConfigTemplateKind, + MatchResources: clusterv1.PatchSelectorMatch{ + MachineDeploymentClass: &clusterv1.PatchSelectorMatchMachineDeploymentClass{ + Names: []string{"default-worker"}, + }, }, }, - Patch: apiextensionsv1.JSON{Raw: []byte(` -[{"op":"add","path":"/spec/template/spec/resource","value":"topo2-infra"}] - `)}, - PatchType: api.JSONPatchType, - }, - { // Set /spec/template/spec/resource=topo2-bootstrap in BootstrapTemplate of default-worker-topo2. - TemplateRef: api.TemplateRef{ - APIVersion: builder.BootstrapGroupVersion.String(), - Kind: builder.GenericBootstrapConfigTemplateKind, - TemplateType: api.MachineDeploymentBootstrapConfigTemplateType, - MachineDeploymentRef: api.MachineDeploymentRef{ - TopologyName: "default-worker-topo2", - Class: "default-worker", + JSONPatches: []clusterv1.JSONPatch{ + { + Op: "add", + Path: "/spec/template/spec/resource", + Value: &apiextensionsv1.JSON{Raw: []byte(`"default-worker-bootstrap"`)}, }, }, - Patch: apiextensionsv1.JSON{Raw: []byte(` -[{"op":"add","path":"/spec/template/spec/resource","value":"topo2-bootstrap"}] - `)}, - PatchType: api.JSONPatchType, }, }, }, }, expectedFields: expectedFields{ machineDeploymentBootstrapTemplate: map[string]map[string]interface{}{ - "default-worker-topo1": {"spec.template.spec.resource": "topo1-bootstrap"}, - "default-worker-topo2": {"spec.template.spec.resource": "topo2-bootstrap"}, + "default-worker-topo1": {"spec.template.spec.resource": "default-worker-bootstrap"}, + "default-worker-topo2": {"spec.template.spec.resource": "default-worker-bootstrap"}, }, machineDeploymentInfrastructureMachineTemplate: map[string]map[string]interface{}{ - "default-worker-topo1": {"spec.template.spec.resource": "topo1-infra"}, - "default-worker-topo2": {"spec.template.spec.resource": "topo2-infra"}, + "default-worker-topo1": {"spec.template.spec.resource": "default-worker-infra"}, + "default-worker-topo2": {"spec.template.spec.resource": "default-worker-infra"}, }, }, }, { - name: "Should apply same JSON patches to MachineDeployment templates", - patches: []patch{ + name: "Should apply JSON patches in the correct order", + patches: []clusterv1.ClusterClassPatch{ { - name: "fake-patch1", - patches: []api.GenerateResponsePatch{ - { // Set /spec/template/spec/resource=infra in InfrastructureMachineTemplate of default-worker-topo1. - TemplateRef: api.TemplateRef{ - APIVersion: builder.InfrastructureGroupVersion.String(), - Kind: builder.GenericInfrastructureMachineTemplateKind, - TemplateType: api.MachineDeploymentInfrastructureMachineTemplateType, - MachineDeploymentRef: api.MachineDeploymentRef{ - TopologyName: "default-worker-topo1", - Class: "default-worker", + Name: "fake-patch1", + Definitions: []clusterv1.PatchDefinition{ + { + Selector: clusterv1.PatchSelector{ + APIVersion: builder.ControlPlaneGroupVersion.String(), + Kind: builder.GenericControlPlaneTemplateKind, + MatchResources: clusterv1.PatchSelectorMatch{ + ControlPlane: true, }, }, - Patch: apiextensionsv1.JSON{Raw: []byte(` -[{"op":"add","path":"/spec/template/spec/resource","value":"infra"}] - `)}, - PatchType: api.JSONPatchType, - }, - { // Set /spec/template/spec/resource=infra in InfrastructureMachineTemplate of default-worker-topo2. - TemplateRef: api.TemplateRef{ - APIVersion: builder.InfrastructureGroupVersion.String(), - Kind: builder.GenericInfrastructureMachineTemplateKind, - TemplateType: api.MachineDeploymentInfrastructureMachineTemplateType, - MachineDeploymentRef: api.MachineDeploymentRef{ - TopologyName: "default-worker-topo2", - Class: "default-worker", + JSONPatches: []clusterv1.JSONPatch{ + { + Op: "add", + Path: "/spec/template/spec/clusterName", + Value: &apiextensionsv1.JSON{Raw: []byte(`"cluster1"`)}, + }, + { + Op: "add", + Path: "/spec/template/spec/files", + Value: &apiextensionsv1.JSON{Raw: []byte(`[{"key1":"value1"}]`)}, }, }, - Patch: apiextensionsv1.JSON{Raw: []byte(` -[{"op":"add","path":"/spec/template/spec/resource","value":"infra"}] - `)}, - PatchType: api.JSONPatchType, }, }, }, - }, - expectedFields: expectedFields{ - machineDeploymentInfrastructureMachineTemplate: map[string]map[string]interface{}{ - "default-worker-topo1": {"spec.template.spec.resource": "infra"}, - "default-worker-topo2": {"spec.template.spec.resource": "infra"}, - }, - }, - }, - { - name: "Should apply JSON patches in the correct order", - patches: []patch{ { - name: "fake-patch1", - patches: []api.GenerateResponsePatch{ + Name: "fake-patch2", + Definitions: []clusterv1.PatchDefinition{ { - TemplateRef: api.TemplateRef{ - APIVersion: builder.ControlPlaneGroupVersion.String(), - Kind: builder.GenericControlPlaneTemplateKind, - TemplateType: api.ControlPlaneTemplateType, + Selector: clusterv1.PatchSelector{ + APIVersion: builder.ControlPlaneGroupVersion.String(), + Kind: builder.GenericControlPlaneTemplateKind, + MatchResources: clusterv1.PatchSelectorMatch{ + ControlPlane: true, + }, }, - Patch: apiextensionsv1.JSON{Raw: []byte(` -[{"op":"add","path":"/spec/template/spec/clusterName","value":"cluster1"}, -{"op":"add","path":"/spec/template/spec/files","value":[{"key1":"value1"}]}] - `)}, - PatchType: api.JSONPatchType, - }, - }, - }, - { - name: "fake-patch2", - patches: []api.GenerateResponsePatch{ - { - TemplateRef: api.TemplateRef{ - APIVersion: builder.ControlPlaneGroupVersion.String(), - Kind: builder.GenericControlPlaneTemplateKind, - TemplateType: api.ControlPlaneTemplateType, + JSONPatches: []clusterv1.JSONPatch{ + { + Op: "replace", + Path: "/spec/template/spec/clusterName", + Value: &apiextensionsv1.JSON{Raw: []byte(`"cluster1-overwritten"`)}, + }, }, - Patch: apiextensionsv1.JSON{Raw: []byte(` -[{"op":"replace","path":"/spec/template/spec/clusterName","value":"cluster1-overwritten"}] -`)}, - PatchType: api.JSONPatchType, }, }, }, @@ -288,22 +242,35 @@ func TestApply(t *testing.T) { }, { name: "Should apply JSON patches and preserve ControlPlane fields", - patches: []patch{ + patches: []clusterv1.ClusterClassPatch{ { - name: "fake-patch1", - patches: []api.GenerateResponsePatch{ + Name: "fake-patch1", + Definitions: []clusterv1.PatchDefinition{ { - TemplateRef: api.TemplateRef{ - APIVersion: builder.ControlPlaneGroupVersion.String(), - Kind: builder.GenericControlPlaneTemplateKind, - TemplateType: api.ControlPlaneTemplateType, + Selector: clusterv1.PatchSelector{ + APIVersion: builder.ControlPlaneGroupVersion.String(), + Kind: builder.GenericControlPlaneTemplateKind, + MatchResources: clusterv1.PatchSelectorMatch{ + ControlPlane: true, + }, + }, + JSONPatches: []clusterv1.JSONPatch{ + { + Op: "add", + Path: "/spec/template/spec/replicas", + Value: &apiextensionsv1.JSON{Raw: []byte(`1`)}, + }, + { + Op: "add", + Path: "/spec/template/spec/version", + Value: &apiextensionsv1.JSON{Raw: []byte(`"v1.15.0"`)}, + }, + { + Op: "add", + Path: "/spec/template/spec/machineTemplate/infrastructureRef", + Value: &apiextensionsv1.JSON{Raw: []byte(`{"apiVersion":"invalid","kind":"invalid","namespace":"invalid","name":"invalid"}`)}, + }, }, - Patch: apiextensionsv1.JSON{Raw: []byte(` -[{"op":"add","path":"/spec/template/spec/replicas","value":1}, -{"op":"add","path":"/spec/template/spec/version","value":"v1.15.0"}, -{"op":"add","path":"/spec/template/spec/machineTemplate/infrastructureRef","value":{"apiVersion":"invalid","kind":"invalid","namespace":"invalid","name":"invalid"}}] - `)}, - PatchType: api.JSONPatchType, }, }, }, @@ -311,21 +278,30 @@ func TestApply(t *testing.T) { }, { name: "Should apply JSON patches without metadata", - patches: []patch{ + patches: []clusterv1.ClusterClassPatch{ { - name: "fake-patch1", - patches: []api.GenerateResponsePatch{ + Name: "fake-patch1", + Definitions: []clusterv1.PatchDefinition{ { - TemplateRef: api.TemplateRef{ - APIVersion: builder.InfrastructureGroupVersion.String(), - Kind: builder.GenericInfrastructureClusterTemplateKind, - TemplateType: api.InfrastructureClusterTemplateType, + Selector: clusterv1.PatchSelector{ + APIVersion: builder.InfrastructureGroupVersion.String(), + Kind: builder.GenericInfrastructureClusterTemplateKind, + MatchResources: clusterv1.PatchSelectorMatch{ + InfrastructureCluster: true, + }, + }, + JSONPatches: []clusterv1.JSONPatch{ + { + Op: "add", + Path: "/spec/template/spec/clusterName", + Value: &apiextensionsv1.JSON{Raw: []byte(`"cluster1"`)}, + }, + { + Op: "replace", + Path: "/metadata/name", + Value: &apiextensionsv1.JSON{Raw: []byte(`"overwrittenName"`)}, + }, }, - Patch: apiextensionsv1.JSON{Raw: []byte(` -[{"op":"add","path":"/spec/template/spec/clusterName","value":"cluster1"}, -{"op":"replace","path":"/metadata/name","value": "overwrittenName"}] - `)}, - PatchType: api.JSONPatchType, }, }, }, @@ -338,20 +314,25 @@ func TestApply(t *testing.T) { }, { name: "Should apply JSON merge patches", - patches: []patch{ + patches: []clusterv1.ClusterClassPatch{ { - name: "fake-patch1", - patches: []api.GenerateResponsePatch{ + Name: "fake-patch1", + Definitions: []clusterv1.PatchDefinition{ { - TemplateRef: api.TemplateRef{ - APIVersion: builder.InfrastructureGroupVersion.String(), - Kind: builder.GenericInfrastructureClusterTemplateKind, - TemplateType: api.InfrastructureClusterTemplateType, + 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", + Value: &apiextensionsv1.JSON{Raw: []byte(`"infraCluster"`)}, + }, }, - Patch: apiextensionsv1.JSON{Raw: []byte(` -{"spec": {"template": {"spec": {"resource": "infraCluster"}}}} - `)}, - PatchType: api.JSONMergePatchType, }, }, }, @@ -380,21 +361,10 @@ func TestApply(t *testing.T) { blueprint, desired := setupTestObjects() // If there are patches, set up patch generators. - patchEngine := &engine{} + patchEngine := NewEngine() if len(tt.patches) > 0 { - for _, patch := range tt.patches { - // Add the patches to ensure the patch generator is called. - blueprint.ClusterClass.Spec.Patches = append(blueprint.ClusterClass.Spec.Patches, clusterv1.ClusterClassPatch{Name: patch.name}) - } - - patchEngine.createPatchGenerator = func(patch *clusterv1.ClusterClassPatch) (api.Generator, error) { - for _, p := range tt.patches { - if p.name == patch.Name { - return &fakePatchGenerator{patches: p.patches}, nil - } - } - return nil, errors.Errorf("could not find patch generator for patch %q", patch.Name) - } + // Add the patches. + blueprint.ClusterClass.Spec.Patches = tt.patches } // Copy the desired objects before applying patches. @@ -470,7 +440,13 @@ func setupTestObjects() (*scope.ClusterBlueprint, *scope.ClusterState) { WithWorkerMachineDeploymentClasses(*mdClass1). Build() + // Note: we depend on TypeMeta being set to calculate HolderReferences correctly. + // We also set TypeMeta explicitly in the topology/cluster/cluster_controller.go. cluster := &clusterv1.Cluster{ + TypeMeta: metav1.TypeMeta{ + APIVersion: clusterv1.GroupVersion.String(), + Kind: "Cluster", + }, ObjectMeta: metav1.ObjectMeta{ Name: "cluster1", Namespace: metav1.NamespaceDefault, @@ -581,17 +557,6 @@ func setupTestObjects() (*scope.ClusterBlueprint, *scope.ClusterState) { return blueprint, desired } -// fakePatchGenerator is an api.Generator which just returns the provided patches. -type fakePatchGenerator struct { - patches []api.GenerateResponsePatch -} - -func (g *fakePatchGenerator) Generate(_ context.Context, _ *api.GenerateRequest) (*api.GenerateResponse, error) { - return &api.GenerateResponse{ - Items: g.patches, - }, nil -} - // setSpecFields sets fields on an unstructured object from a map. func setSpecFields(obj *unstructured.Unstructured, fields map[string]interface{}) { for k, v := range fields { diff --git a/internal/controllers/topology/cluster/patches/inline/json_patch_generator.go b/internal/controllers/topology/cluster/patches/inline/json_patch_generator.go index aeb983397357..cef9c063d2c5 100644 --- a/internal/controllers/topology/cluster/patches/inline/json_patch_generator.go +++ b/internal/controllers/topology/cluster/patches/inline/json_patch_generator.go @@ -27,18 +27,18 @@ import ( sprig "github.com/Masterminds/sprig/v3" "github.com/pkg/errors" - "github.com/valyala/fastjson" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" kerrors "k8s.io/apimachinery/pkg/util/errors" - "k8s.io/utils/pointer" "sigs.k8s.io/yaml" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1" + "sigs.k8s.io/cluster-api/internal/contract" "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/patches/api" patchvariables "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/patches/variables" ) -// jsonPatchGenerator generates JSON patches for a GenerateRequest based on a ClusterClassPatch. +// jsonPatchGenerator generates JSON patches for a GeneratePatchesRequest based on a ClusterClassPatch. type jsonPatchGenerator struct { patch *clusterv1.ClusterClassPatch } @@ -50,18 +50,24 @@ func New(patch *clusterv1.ClusterClassPatch) api.Generator { } } -// Generate generates JSON patches for the given GenerateRequest based on a ClusterClassPatch. -func (j *jsonPatchGenerator) Generate(_ context.Context, req *api.GenerateRequest) (*api.GenerateResponse, error) { - resp := &api.GenerateResponse{} +// Generate generates JSON patches for the given GeneratePatchesRequest based on a ClusterClassPatch. +func (j *jsonPatchGenerator) Generate(_ context.Context, req *runtimehooksv1.GeneratePatchesRequest) *runtimehooksv1.GeneratePatchesResponse { + resp := &runtimehooksv1.GeneratePatchesResponse{} + + globalVariables := toMap(req.Variables) // Loop over all templates. errs := []error{} - for _, template := range req.Items { + for i := range req.Items { + item := &req.Items[i] + + templateVariables := toMap(item.Variables) + // Calculate the list of patches which match the current template. matchingPatches := []clusterv1.PatchDefinition{} for _, patch := range j.patch.Definitions { // Add the patch to the list, if it matches the template. - if templateMatchesSelector(&template.TemplateRef, patch.Selector) { + if matchesSelector(item, templateVariables, patch.Selector) { matchingPatches = append(matchingPatches, patch) } } @@ -72,15 +78,15 @@ func (j *jsonPatchGenerator) Generate(_ context.Context, req *api.GenerateReques } // Merge template-specific and global variables. - variables, err := mergeVariableMaps(req.Variables, template.Variables) + variables, err := mergeVariableMaps(globalVariables, templateVariables) if err != nil { - errs = append(errs, errors.Wrapf(err, "failed to merge global and template-specific variables for template %s", template.TemplateRef)) + errs = append(errs, errors.Wrapf(err, "failed to merge global and template-specific variables for item with uid %q", item.UID)) continue } enabled, err := patchIsEnabled(j.patch.EnabledIf, variables) if err != nil { - errs = append(errs, errors.Wrapf(err, "failed to calculate if patch %s is enabled for template %s", j.patch.Name, template.TemplateRef)) + errs = append(errs, errors.Wrapf(err, "failed to calculate if patch %s is enabled for item with uid %q", j.patch.Name, item.UID)) continue } if !enabled { @@ -93,56 +99,97 @@ func (j *jsonPatchGenerator) Generate(_ context.Context, req *api.GenerateReques // Generate JSON patches. jsonPatches, err := generateJSONPatches(patch.JSONPatches, variables) if err != nil { - errs = append(errs, errors.Wrapf(err, "failed to generate JSON patches for template %s", template.TemplateRef)) + errs = append(errs, errors.Wrapf(err, "failed to generate JSON patches for item with uid %q", item.UID)) continue } // Add jsonPatches to the response. - resp.Items = append(resp.Items, api.GenerateResponsePatch{ - TemplateRef: template.TemplateRef, - Patch: *jsonPatches, - PatchType: api.JSONPatchType, + resp.Items = append(resp.Items, runtimehooksv1.GeneratePatchesResponseItem{ + UID: item.UID, + Patch: jsonPatches, + PatchType: runtimehooksv1.JSONPatchType, }) } } - return resp, kerrors.NewAggregate(errs) + if err := kerrors.NewAggregate(errs); err != nil { + return &runtimehooksv1.GeneratePatchesResponse{ + Status: runtimehooksv1.ResponseStatusFailure, + Message: err.Error(), + } + } + + return resp } -// templateMatchesSelector returns true if the template matches the selector. -func templateMatchesSelector(templateRef *api.TemplateRef, selector clusterv1.PatchSelector) bool { +// toMap converts a list of Variables to a map of JSON (name is the map key). +func toMap(variables []runtimehooksv1.Variable) map[string]apiextensionsv1.JSON { + variablesMap := map[string]apiextensionsv1.JSON{} + for i := range variables { + variablesMap[variables[i].Name] = variables[i].Value + } + return variablesMap +} + +// matchesSelector returns true if the GeneratePatchesRequestItem matches the selector. +func matchesSelector(req *runtimehooksv1.GeneratePatchesRequestItem, templateVariables map[string]apiextensionsv1.JSON, selector clusterv1.PatchSelector) bool { + gvk := req.Object.Object.GetObjectKind().GroupVersionKind() + // Check if the apiVersion and kind are matching. - if templateRef.APIVersion != selector.APIVersion { + if gvk.GroupVersion().String() != selector.APIVersion { return false } - if templateRef.Kind != selector.Kind { + if gvk.Kind != selector.Kind { return false } - // Check if target matches. - switch templateRef.TemplateType { - case api.InfrastructureClusterTemplateType: - // Check if matchSelector.infrastructureCluster is true. - return selector.MatchResources.InfrastructureCluster - case api.ControlPlaneTemplateType, api.ControlPlaneInfrastructureMachineTemplateType: - // Check if matchSelector.controlPlane is true. - return selector.MatchResources.ControlPlane - case api.MachineDeploymentBootstrapConfigTemplateType, api.MachineDeploymentInfrastructureMachineTemplateType: - if selector.MatchResources.MachineDeploymentClass == nil { - return false + // Check if the request is for an InfrastructureCluster. + if selector.MatchResources.InfrastructureCluster { + // Cluster.spec.infrastructureRef holds the InfrastructureCluster. + if req.HolderReference.Kind == "Cluster" && req.HolderReference.FieldPath == "spec.infrastructureRef" { + return true } - // Check if matchSelector.machineDeploymentClass.names contains the - // MachineDeployment.Class of the template. - for _, name := range selector.MatchResources.MachineDeploymentClass.Names { - if name == templateRef.MachineDeploymentRef.Class { - return true + } + + // Check if the request is for a ControlPlane or the InfrastructureMachineTemplate of a ControlPlane. + if selector.MatchResources.ControlPlane { + // Cluster.spec.controlPlaneRef holds the ControlPlane. + if req.HolderReference.Kind == "Cluster" && req.HolderReference.FieldPath == "spec.controlPlaneRef" { + return true + } + // *.spec.machineTemplate.infrastructureRef holds the InfrastructureMachineTemplate of a ControlPlane. + // Note: this field path is only used in this context. + if req.HolderReference.FieldPath == strings.Join(contract.ControlPlane().MachineTemplate().InfrastructureRef().Path(), ".") { + return true + } + } + + // Check if the request is for a BootstrapConfigTemplate or an InfrastructureMachineTemplate + // of one of the configured MachineDeploymentClasses. + if selector.MatchResources.MachineDeploymentClass != nil { + // MachineDeployment.spec.template.spec.bootstrap.configRef or + // MachineDeployment.spec.template.spec.infrastructureRef holds the BootstrapConfigTemplate or + // InfrastructureMachineTemplate. + if req.HolderReference.Kind == "MachineDeployment" && + (req.HolderReference.FieldPath == "spec.template.spec.bootstrap.configRef" || + req.HolderReference.FieldPath == "spec.template.spec.infrastructureRef") { + // Read the builtin.machineDeployment.class variable. + templateMDClassJSON, err := patchvariables.GetVariableValue(templateVariables, "builtin.machineDeployment.class") + + // If the builtin variable could be read. + if err == nil { + // If templateMDClass matches one of the configured MachineDeploymentClasses. + for _, mdClass := range selector.MatchResources.MachineDeploymentClass.Names { + // We have to quote mdClass as templateMDClassJSON is a JSON string (e.g. "default-worker"). + if string(templateMDClassJSON.Raw) == strconv.Quote(mdClass) { + return true + } + } } } - return false - default: - // Return false if the TargetType is unknown. - return false } + + return false } func patchIsEnabled(enabledIf *string, variables map[string]apiextensionsv1.JSON) (bool, error) { @@ -169,7 +216,7 @@ type jsonPatchRFC6902 struct { } // generateJSONPatches generates JSON patches based on the given JSONPatches and variables. -func generateJSONPatches(jsonPatches []clusterv1.JSONPatch, variables map[string]apiextensionsv1.JSON) (*apiextensionsv1.JSON, error) { +func generateJSONPatches(jsonPatches []clusterv1.JSONPatch, variables map[string]apiextensionsv1.JSON) ([]byte, error) { res := []jsonPatchRFC6902{} for _, jsonPatch := range jsonPatches { @@ -195,7 +242,7 @@ func generateJSONPatches(jsonPatches []clusterv1.JSONPatch, variables map[string return nil, errors.Wrapf(err, "failed to marshal JSON Patch %v", jsonPatches) } - return &apiextensionsv1.JSON{Raw: resJSON}, nil + return resJSON, nil } // calculateValue calculates a value for a JSON patch. @@ -221,7 +268,7 @@ func calculateValue(patch clusterv1.JSONPatch, variables map[string]apiextension // Return variable. if patch.ValueFrom.Variable != nil { - value, err := getVariableValue(variables, *patch.ValueFrom.Variable) + value, err := patchvariables.GetVariableValue(variables, *patch.ValueFrom.Variable) if err != nil { return nil, errors.Wrapf(err, "failed to calculate value") } @@ -236,142 +283,6 @@ func calculateValue(patch clusterv1.JSONPatch, variables map[string]apiextension return value, nil } -const ( - leftArrayDelim = "[" - rightArrayDelim = "]" -) - -// getVariableValue returns a variable from the variables map. -func getVariableValue(variables map[string]apiextensionsv1.JSON, variablePath string) (*apiextensionsv1.JSON, error) { - // Split the variablePath (format: "."). - variableSplit := strings.Split(variablePath, ".") - variableName, relativePath := variableSplit[0], variableSplit[1:] - - // Parse the path segment. - variableNameSegment, err := parsePathSegment(variableName) - if err != nil { - return nil, errors.Wrapf(err, "variable %q is invalid", variablePath) - } - - // Get the variable. - value, ok := variables[variableNameSegment.path] - if !ok { - return nil, errors.Errorf("variable %q does not exist", variableName) - } - - // Return the value, if variablePath points to a top-level variable, i.e. hos no relativePath and no - // array index (i.e. ""). - if len(relativePath) == 0 && !variableNameSegment.HasIndex() { - return &value, nil - } - - // Parse the variable object. - variable, err := fastjson.ParseBytes(value.Raw) - if err != nil { - return nil, errors.Wrapf(err, "cannot parse variable %q: %s", variableName, string(value.Raw)) - } - - // If variableName contains an array index, get the array element (i.e. starts with "[i]"). - // Then return it, if there is no relative path (i.e. "[i]") - if variableNameSegment.HasIndex() { - variable, err = getVariableArrayElement(variable, variableNameSegment, variablePath) - if err != nil { - return nil, err - } - - if len(relativePath) == 0 { - return &apiextensionsv1.JSON{ - Raw: variable.MarshalTo([]byte{}), - }, nil - } - } - - // If the variablePath points to a nested variable, i.e. has a relativePath, inspect the variable object. - - // Retrieve each segment of the relativePath incrementally, taking care of resolving array indexes. - for _, p := range relativePath { - // Parse the path segment. - pathSegment, err := parsePathSegment(p) - if err != nil { - return nil, errors.Wrapf(err, "variable %q has invalid syntax", variablePath) - } - - // Return if the variable does not exist. - if !variable.Exists(pathSegment.path) { - return nil, errors.Errorf("variable %q does not exist: failed to lookup segment %q", variablePath, pathSegment.path) - } - - // Get the variable from the variable object. - variable = variable.Get(pathSegment.path) - - // Continue if the path doesn't contain an index. - if !pathSegment.HasIndex() { - continue - } - - variable, err = getVariableArrayElement(variable, pathSegment, variablePath) - if err != nil { - return nil, err - } - } - - // Return the marshalled value of the variable. - return &apiextensionsv1.JSON{ - Raw: variable.MarshalTo([]byte{}), - }, nil -} - -type pathSegment struct { - path string - index *int -} - -func (p pathSegment) HasIndex() bool { - return p.index != nil -} - -func parsePathSegment(segment string) (*pathSegment, error) { - if (strings.Contains(segment, leftArrayDelim) && !strings.Contains(segment, rightArrayDelim)) || - (!strings.Contains(segment, leftArrayDelim) && strings.Contains(segment, rightArrayDelim)) { - return nil, errors.Errorf("failed to parse path segment %q", segment) - } - - if !strings.Contains(segment, leftArrayDelim) && !strings.Contains(segment, rightArrayDelim) { - return &pathSegment{ - path: segment, - }, nil - } - - arrayIndexStr := segment[strings.Index(segment, leftArrayDelim)+1 : strings.Index(segment, rightArrayDelim)] - index, err := strconv.Atoi(arrayIndexStr) - if err != nil { - return nil, errors.Wrapf(err, "failed to parse array index in path segment %q", segment) - } - if index < 0 { - return nil, errors.Errorf("invalid array index %d in path segment %q", index, segment) - } - - return &pathSegment{ - path: segment[:strings.Index(segment, leftArrayDelim)], //nolint:gocritic // We already check above that segment contains leftArrayDelim, - index: pointer.Int(index), - }, nil -} - -// getVariableArrayElement gets the array element of a given array. -func getVariableArrayElement(array *fastjson.Value, arrayPathSegment *pathSegment, fullVariablePath string) (*fastjson.Value, error) { - // Retrieve the array element, handling index out of range. - arr, err := array.Array() - if err != nil { - return nil, errors.Wrapf(err, "variable %q is invalid: failed to get array %q", fullVariablePath, arrayPathSegment.path) - } - - if len(arr) < *arrayPathSegment.index+1 { - return nil, errors.Errorf("variable %q is invalid: array does not have index %d", fullVariablePath, arrayPathSegment.index) - } - - return arr[*arrayPathSegment.index], nil -} - // renderValueTemplate renders a template with the given variables as data. func renderValueTemplate(valueTemplate string, variables map[string]apiextensionsv1.JSON) (*apiextensionsv1.JSON, error) { // Parse the template. diff --git a/internal/controllers/topology/cluster/patches/inline/json_patch_generator_test.go b/internal/controllers/topology/cluster/patches/inline/json_patch_generator_test.go index 8371ee8fa0b7..3f31ee4856bc 100644 --- a/internal/controllers/topology/cluster/patches/inline/json_patch_generator_test.go +++ b/internal/controllers/topology/cluster/patches/inline/json_patch_generator_test.go @@ -24,23 +24,24 @@ import ( . "github.com/onsi/gomega" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/utils/pointer" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" - "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/patches/api" + runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1" patchvariables "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/patches/variables" ) func TestGenerate(t *testing.T) { tests := []struct { - name string - patch *clusterv1.ClusterClassPatch - req *api.GenerateRequest - want *api.GenerateResponse - wantErr bool + name string + patch *clusterv1.ClusterClassPatch + req *runtimehooksv1.GeneratePatchesRequest + want *runtimehooksv1.GeneratePatchesResponse }{ { - name: "Should generate JSON Patches with correct variable values", + name: "Should generate JSON Results with correct variable values", patch: &clusterv1.ClusterClassPatch{ Name: "clusterName", Definitions: []clusterv1.PatchDefinition{ @@ -114,35 +115,60 @@ func TestGenerate(t *testing.T) { }, }, }, - req: &api.GenerateRequest{ - Variables: map[string]apiextensionsv1.JSON{ - "builtin": {Raw: []byte(`{"cluster":{"name":"cluster-name","namespace":"default","topology":{"class":"clusterClass1","version":"v1.21.1"}}}`)}, - "variableA": {Raw: []byte(`"A"`)}, - "variableB": {Raw: []byte(`"B"`)}, - "variableC": {Raw: []byte(`"C"`)}, + req: &runtimehooksv1.GeneratePatchesRequest{ + Variables: []runtimehooksv1.Variable{ + { + Name: "builtin", + Value: apiextensionsv1.JSON{Raw: []byte(`{"cluster":{"name":"cluster-name","namespace":"default","topology":{"class":"clusterClass1","version":"v1.21.1"}}}`)}, + }, + { + Name: "variableA", + Value: apiextensionsv1.JSON{Raw: []byte(`"A"`)}, + }, + { + Name: "variableB", + Value: apiextensionsv1.JSON{Raw: []byte(`"B"`)}, + }, + { + Name: "variableC", + Value: apiextensionsv1.JSON{Raw: []byte(`"C"`)}, + }, }, - Items: []*api.GenerateRequestTemplate{ + Items: []runtimehooksv1.GeneratePatchesRequestItem{ { - TemplateRef: api.TemplateRef{ - APIVersion: "controlplane.cluster.x-k8s.io/v1beta1", - Kind: "ControlPlaneTemplate", - TemplateType: api.ControlPlaneTemplateType, + UID: "1", + HolderReference: runtimehooksv1.HolderReference{ + APIVersion: clusterv1.GroupVersion.String(), + Kind: "Cluster", + Name: "my-cluster", + Namespace: "default", + FieldPath: "spec.controlPlaneRef", }, - Variables: map[string]apiextensionsv1.JSON{ - "builtin": {Raw: []byte(`{"controlPlane":{"replicas":3,"machineTemplate":{"infrastructureRef":{"name":"controlPlaneInfrastructureMachineTemplate1"}}}}`)}, - "variableC": {Raw: []byte(`"C-template"`)}, + Variables: []runtimehooksv1.Variable{ + { + Name: "builtin", + Value: apiextensionsv1.JSON{Raw: []byte(`{"controlPlane":{"replicas":3,"machineTemplate":{"infrastructureRef":{"name":"controlPlaneInfrastructureMachineTemplate1"}}}}`)}, + }, + { + Name: "variableC", + Value: apiextensionsv1.JSON{Raw: []byte(`"C-template"`)}, + }, + }, + Object: runtime.RawExtension{ + Object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "controlplane.cluster.x-k8s.io/v1beta1", + "kind": "ControlPlaneTemplate", + }, + }, }, }, }, }, - want: &api.GenerateResponse{ - Items: []api.GenerateResponsePatch{ + want: &runtimehooksv1.GeneratePatchesResponse{ + Items: []runtimehooksv1.GeneratePatchesResponseItem{ { - TemplateRef: api.TemplateRef{ - APIVersion: "controlplane.cluster.x-k8s.io/v1beta1", - Kind: "ControlPlaneTemplate", - TemplateType: api.ControlPlaneTemplateType, - }, + UID: "1", Patch: toJSONCompact(`[ {"op":"replace","path":"/spec/value","value":1}, {"op":"replace","path":"/spec/valueFrom/variable","value":"A"}, @@ -158,13 +184,13 @@ func TestGenerate(t *testing.T) { } } }]}]`), - PatchType: api.JSONPatchType, + PatchType: runtimehooksv1.JSONPatchType, }, }, }, }, { - name: "Should generate JSON Patches (multiple PatchDefinitions)", + name: "Should generate JSON Results (multiple PatchDefinitions)", patch: &clusterv1.ClusterClassPatch{ Name: "clusterName", Definitions: []clusterv1.PatchDefinition{ @@ -234,58 +260,75 @@ func TestGenerate(t *testing.T) { }, }, }, - req: &api.GenerateRequest{ - Variables: map[string]apiextensionsv1.JSON{ - "builtin": {Raw: []byte(`{"cluster":{"name":"cluster-name","namespace":"default","topology":{"class":"clusterClass1","version":"v1.21.1"}}}`)}, + req: &runtimehooksv1.GeneratePatchesRequest{ + Variables: []runtimehooksv1.Variable{ + { + Name: "builtin", + Value: apiextensionsv1.JSON{Raw: []byte(`{"cluster":{"name":"cluster-name","namespace":"default","topology":{"class":"clusterClass1","version":"v1.21.1"}}}`)}, + }, }, - Items: []*api.GenerateRequestTemplate{ + Items: []runtimehooksv1.GeneratePatchesRequestItem{ { - TemplateRef: api.TemplateRef{ - APIVersion: "controlplane.cluster.x-k8s.io/v1beta1", - Kind: "ControlPlaneTemplate", - TemplateType: api.ControlPlaneTemplateType, + UID: "1", + HolderReference: runtimehooksv1.HolderReference{ + APIVersion: clusterv1.GroupVersion.String(), + Kind: "Cluster", + Name: "my-cluster", + Namespace: "default", + FieldPath: "spec.controlPlaneRef", + }, + Object: runtime.RawExtension{ + Object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "controlplane.cluster.x-k8s.io/v1beta1", + "kind": "ControlPlaneTemplate", + }, + }, }, }, { - TemplateRef: api.TemplateRef{ - APIVersion: "bootstrap.cluster.x-k8s.io/v1beta1", - Kind: "BootstrapTemplate", - TemplateType: api.MachineDeploymentBootstrapConfigTemplateType, - MachineDeploymentRef: api.MachineDeploymentRef{ - Class: "default-worker", + UID: "2", + HolderReference: runtimehooksv1.HolderReference{ + APIVersion: clusterv1.GroupVersion.String(), + Kind: "MachineDeployment", + Name: "my-md-0", + Namespace: "default", + FieldPath: "spec.template.spec.bootstrap.configRef", + }, + Variables: []runtimehooksv1.Variable{ + { + Name: "builtin", + Value: apiextensionsv1.JSON{Raw: []byte(`{"machineDeployment":{"class":"default-worker"}}`)}, + }, + }, + Object: runtime.RawExtension{ + Object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "bootstrap.cluster.x-k8s.io/v1beta1", + "kind": "BootstrapTemplate", + }, }, }, }, }, }, - want: &api.GenerateResponse{ - Items: []api.GenerateResponsePatch{ + want: &runtimehooksv1.GeneratePatchesResponse{ + Items: []runtimehooksv1.GeneratePatchesResponseItem{ { - TemplateRef: api.TemplateRef{ - APIVersion: "controlplane.cluster.x-k8s.io/v1beta1", - Kind: "ControlPlaneTemplate", - TemplateType: api.ControlPlaneTemplateType, - }, + UID: "1", Patch: toJSONCompact(`[ {"op":"replace","path":"/spec/template/spec/kubeadmConfigSpec/clusterConfiguration/controllerManager/extraArgs/cluster-name","value":"cluster-name"}, {"op":"replace","path":"/spec/template/spec/kubeadmConfigSpec/files","value":[{"contentFrom":{"secret":{"key":"control-plane-azure.json","name":"cluster-name-control-plane-azure-json"}},"owner":"root:root"}]}, {"op":"remove","path":"/spec/template/spec/kubeadmConfigSpec/clusterConfiguration/apiServer/extraArgs"} ]`), - PatchType: api.JSONPatchType, + PatchType: runtimehooksv1.JSONPatchType, }, { - TemplateRef: api.TemplateRef{ - APIVersion: "bootstrap.cluster.x-k8s.io/v1beta1", - Kind: "BootstrapTemplate", - TemplateType: api.MachineDeploymentBootstrapConfigTemplateType, - MachineDeploymentRef: api.MachineDeploymentRef{ - Class: "default-worker", - }, - }, + UID: "2", Patch: toJSONCompact(`[ {"op":"replace","path":"/spec/template/spec/kubeadmConfigSpec/clusterConfiguration/controllerManager/extraArgs/cluster-name","value":[{"contentFrom":{"secret":{"key":"worker-node-azure.json","name":"cluster-name-md-0-azure-json"}},"owner":"root:root"}]} ]`), - PatchType: api.JSONPatchType, + PatchType: runtimehooksv1.JSONPatchType, }, }, }, @@ -296,30 +339,32 @@ func TestGenerate(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - got, err := New(tt.patch).Generate(context.Background(), tt.req) - if tt.wantErr { - g.Expect(err).To(HaveOccurred()) - return - } - g.Expect(err).ToNot(HaveOccurred()) + got := New(tt.patch).Generate(context.Background(), tt.req) g.Expect(got).To(Equal(tt.want)) }) } } -func TestTemplateMatchesSelector(t *testing.T) { +func TestMatchesSelector(t *testing.T) { tests := []struct { - name string - templateRef *api.TemplateRef - selector clusterv1.PatchSelector - match bool + name string + req *runtimehooksv1.GeneratePatchesRequestItem + templateVariables map[string]apiextensionsv1.JSON + selector clusterv1.PatchSelector + match bool }{ { name: "Don't match: apiVersion mismatch", - templateRef: &api.TemplateRef{ - APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1", - Kind: "AzureMachineTemplate", + req: &runtimehooksv1.GeneratePatchesRequestItem{ + Object: runtime.RawExtension{ + Object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "infrastructure.cluster.x-k8s.io/v1beta1", + "kind": "AzureMachineTemplate", + }, + }, + }, }, selector: clusterv1.PatchSelector{ APIVersion: "infrastructure.cluster.x-k8s.io/v1alpha4", @@ -329,9 +374,15 @@ func TestTemplateMatchesSelector(t *testing.T) { }, { name: "Don't match: kind mismatch", - templateRef: &api.TemplateRef{ - APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1", - Kind: "AzureMachineTemplate", + req: &runtimehooksv1.GeneratePatchesRequestItem{ + Object: runtime.RawExtension{ + Object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "infrastructure.cluster.x-k8s.io/v1beta1", + "kind": "AzureMachineTemplate", + }, + }, + }, }, selector: clusterv1.PatchSelector{ APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1", @@ -341,10 +392,22 @@ func TestTemplateMatchesSelector(t *testing.T) { }, { name: "Match InfrastructureClusterTemplate", - templateRef: &api.TemplateRef{ - APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1", - Kind: "AzureClusterTemplate", - TemplateType: api.InfrastructureClusterTemplateType, + req: &runtimehooksv1.GeneratePatchesRequestItem{ + Object: runtime.RawExtension{ + Object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "infrastructure.cluster.x-k8s.io/v1beta1", + "kind": "AzureClusterTemplate", + }, + }, + }, + HolderReference: runtimehooksv1.HolderReference{ + APIVersion: clusterv1.GroupVersion.String(), + Kind: "Cluster", + Name: "my-cluster", + Namespace: "default", + FieldPath: "spec.infrastructureRef", + }, }, selector: clusterv1.PatchSelector{ APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1", @@ -357,10 +420,22 @@ func TestTemplateMatchesSelector(t *testing.T) { }, { name: "Don't match InfrastructureClusterTemplate, .matchResources.infrastructureCluster not set", - templateRef: &api.TemplateRef{ - APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1", - Kind: "AzureClusterTemplate", - TemplateType: api.InfrastructureClusterTemplateType, + req: &runtimehooksv1.GeneratePatchesRequestItem{ + Object: runtime.RawExtension{ + Object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "infrastructure.cluster.x-k8s.io/v1beta1", + "kind": "AzureClusterTemplate", + }, + }, + }, + HolderReference: runtimehooksv1.HolderReference{ + APIVersion: clusterv1.GroupVersion.String(), + Kind: "Cluster", + Name: "my-cluster", + Namespace: "default", + FieldPath: "spec.infrastructureRef", + }, }, selector: clusterv1.PatchSelector{ APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1", @@ -371,10 +446,22 @@ func TestTemplateMatchesSelector(t *testing.T) { }, { name: "Don't match InfrastructureClusterTemplate, .matchResources.infrastructureCluster false", - templateRef: &api.TemplateRef{ - APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1", - Kind: "AzureClusterTemplate", - TemplateType: api.InfrastructureClusterTemplateType, + req: &runtimehooksv1.GeneratePatchesRequestItem{ + Object: runtime.RawExtension{ + Object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "infrastructure.cluster.x-k8s.io/v1beta1", + "kind": "AzureClusterTemplate", + }, + }, + }, + HolderReference: runtimehooksv1.HolderReference{ + APIVersion: clusterv1.GroupVersion.String(), + Kind: "Cluster", + Name: "my-cluster", + Namespace: "default", + FieldPath: "spec.infrastructureRef", + }, }, selector: clusterv1.PatchSelector{ APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1", @@ -387,10 +474,22 @@ func TestTemplateMatchesSelector(t *testing.T) { }, { name: "Match ControlPlaneTemplate", - templateRef: &api.TemplateRef{ - APIVersion: "controlplane.cluster.x-k8s.io/v1beta1", - Kind: "ControlPlaneTemplate", - TemplateType: api.ControlPlaneTemplateType, + req: &runtimehooksv1.GeneratePatchesRequestItem{ + Object: runtime.RawExtension{ + Object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "controlplane.cluster.x-k8s.io/v1beta1", + "kind": "ControlPlaneTemplate", + }, + }, + }, + HolderReference: runtimehooksv1.HolderReference{ + APIVersion: clusterv1.GroupVersion.String(), + Kind: "Cluster", + Name: "my-cluster", + Namespace: "default", + FieldPath: "spec.controlPlaneRef", + }, }, selector: clusterv1.PatchSelector{ APIVersion: "controlplane.cluster.x-k8s.io/v1beta1", @@ -403,10 +502,22 @@ func TestTemplateMatchesSelector(t *testing.T) { }, { name: "Don't match ControlPlaneTemplate, .matchResources.controlPlane not set", - templateRef: &api.TemplateRef{ - APIVersion: "controlplane.cluster.x-k8s.io/v1beta1", - Kind: "ControlPlaneTemplate", - TemplateType: api.ControlPlaneTemplateType, + req: &runtimehooksv1.GeneratePatchesRequestItem{ + Object: runtime.RawExtension{ + Object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "controlplane.cluster.x-k8s.io/v1beta1", + "kind": "ControlPlaneTemplate", + }, + }, + }, + HolderReference: runtimehooksv1.HolderReference{ + APIVersion: clusterv1.GroupVersion.String(), + Kind: "Cluster", + Name: "my-cluster", + Namespace: "default", + FieldPath: "spec.controlPlaneRef", + }, }, selector: clusterv1.PatchSelector{ APIVersion: "controlplane.cluster.x-k8s.io/v1beta1", @@ -417,10 +528,22 @@ func TestTemplateMatchesSelector(t *testing.T) { }, { name: "Don't match ControlPlaneTemplate, .matchResources.controlPlane false", - templateRef: &api.TemplateRef{ - APIVersion: "controlplane.cluster.x-k8s.io/v1beta1", - Kind: "ControlPlaneTemplate", - TemplateType: api.ControlPlaneTemplateType, + req: &runtimehooksv1.GeneratePatchesRequestItem{ + Object: runtime.RawExtension{ + Object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "controlplane.cluster.x-k8s.io/v1beta1", + "kind": "ControlPlaneTemplate", + }, + }, + }, + HolderReference: runtimehooksv1.HolderReference{ + APIVersion: clusterv1.GroupVersion.String(), + Kind: "Cluster", + Name: "my-cluster", + Namespace: "default", + FieldPath: "spec.controlPlaneRef", + }, }, selector: clusterv1.PatchSelector{ APIVersion: "controlplane.cluster.x-k8s.io/v1beta1", @@ -433,10 +556,22 @@ func TestTemplateMatchesSelector(t *testing.T) { }, { name: "Match ControlPlane InfrastructureMachineTemplate", - templateRef: &api.TemplateRef{ - APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1", - Kind: "AzureMachineTemplate", - TemplateType: api.ControlPlaneInfrastructureMachineTemplateType, + req: &runtimehooksv1.GeneratePatchesRequestItem{ + Object: runtime.RawExtension{ + Object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "infrastructure.cluster.x-k8s.io/v1beta1", + "kind": "AzureMachineTemplate", + }, + }, + }, + HolderReference: runtimehooksv1.HolderReference{ + APIVersion: "controlplane.cluster.x-k8s.io/v1beta1", + Kind: "KubeadmControlPlane", + Name: "my-controlplane", + Namespace: "default", + FieldPath: "spec.machineTemplate.infrastructureRef", + }, }, selector: clusterv1.PatchSelector{ APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1", @@ -449,14 +584,26 @@ func TestTemplateMatchesSelector(t *testing.T) { }, { name: "Match MD BootstrapTemplate", - templateRef: &api.TemplateRef{ - APIVersion: "bootstrap.cluster.x-k8s.io/v1beta1", - Kind: "BootstrapTemplate", - TemplateType: api.MachineDeploymentBootstrapConfigTemplateType, - MachineDeploymentRef: api.MachineDeploymentRef{ - Class: "classA", + req: &runtimehooksv1.GeneratePatchesRequestItem{ + Object: runtime.RawExtension{ + Object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "bootstrap.cluster.x-k8s.io/v1beta1", + "kind": "BootstrapTemplate", + }, + }, + }, + HolderReference: runtimehooksv1.HolderReference{ + APIVersion: clusterv1.GroupVersion.String(), + Kind: "MachineDeployment", + Name: "my-md-0", + Namespace: "default", + FieldPath: "spec.template.spec.bootstrap.configRef", }, }, + templateVariables: map[string]apiextensionsv1.JSON{ + "builtin": {Raw: []byte(`{"machineDeployment":{"class":"classA"}}`)}, + }, selector: clusterv1.PatchSelector{ APIVersion: "bootstrap.cluster.x-k8s.io/v1beta1", Kind: "BootstrapTemplate", @@ -470,14 +617,26 @@ func TestTemplateMatchesSelector(t *testing.T) { }, { name: "Don't match BootstrapTemplate, .matchResources.machineDeploymentClass not set", - templateRef: &api.TemplateRef{ - APIVersion: "bootstrap.cluster.x-k8s.io/v1beta1", - Kind: "BootstrapTemplate", - TemplateType: api.MachineDeploymentBootstrapConfigTemplateType, - MachineDeploymentRef: api.MachineDeploymentRef{ - Class: "classA", + req: &runtimehooksv1.GeneratePatchesRequestItem{ + Object: runtime.RawExtension{ + Object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "bootstrap.cluster.x-k8s.io/v1beta1", + "kind": "BootstrapTemplate", + }, + }, + }, + HolderReference: runtimehooksv1.HolderReference{ + APIVersion: clusterv1.GroupVersion.String(), + Kind: "MachineDeployment", + Name: "my-md-0", + Namespace: "default", + FieldPath: "spec.template.spec.bootstrap.configRef", }, }, + templateVariables: map[string]apiextensionsv1.JSON{ + "builtin": {Raw: []byte(`{"machineDeployment":{"class":"classA"}}`)}, + }, selector: clusterv1.PatchSelector{ APIVersion: "bootstrap.cluster.x-k8s.io/v1beta1", Kind: "BootstrapTemplate", @@ -487,14 +646,26 @@ func TestTemplateMatchesSelector(t *testing.T) { }, { name: "Don't match BootstrapTemplate, .matchResources.machineDeploymentClass does not match", - templateRef: &api.TemplateRef{ - APIVersion: "bootstrap.cluster.x-k8s.io/v1beta1", - Kind: "BootstrapTemplate", - TemplateType: api.MachineDeploymentBootstrapConfigTemplateType, - MachineDeploymentRef: api.MachineDeploymentRef{ - Class: "classA", + req: &runtimehooksv1.GeneratePatchesRequestItem{ + Object: runtime.RawExtension{ + Object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "bootstrap.cluster.x-k8s.io/v1beta1", + "kind": "BootstrapTemplate", + }, + }, + }, + HolderReference: runtimehooksv1.HolderReference{ + APIVersion: clusterv1.GroupVersion.String(), + Kind: "MachineDeployment", + Name: "my-md-0", + Namespace: "default", + FieldPath: "spec.template.spec.bootstrap.configRef", }, }, + templateVariables: map[string]apiextensionsv1.JSON{ + "builtin": {Raw: []byte(`{"machineDeployment":{"class":"classA"}}`)}, + }, selector: clusterv1.PatchSelector{ APIVersion: "bootstrap.cluster.x-k8s.io/v1beta1", Kind: "BootstrapTemplate", @@ -508,14 +679,26 @@ func TestTemplateMatchesSelector(t *testing.T) { }, { name: "Match MD InfrastructureMachineTemplate", - templateRef: &api.TemplateRef{ - APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1", - Kind: "AzureMachineTemplate", - TemplateType: api.MachineDeploymentInfrastructureMachineTemplateType, - MachineDeploymentRef: api.MachineDeploymentRef{ - Class: "classA", + req: &runtimehooksv1.GeneratePatchesRequestItem{ + Object: runtime.RawExtension{ + Object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "infrastructure.cluster.x-k8s.io/v1beta1", + "kind": "AzureMachineTemplate", + }, + }, + }, + HolderReference: runtimehooksv1.HolderReference{ + APIVersion: clusterv1.GroupVersion.String(), + Kind: "MachineDeployment", + Name: "my-md-0", + Namespace: "default", + FieldPath: "spec.template.spec.infrastructureRef", }, }, + templateVariables: map[string]apiextensionsv1.JSON{ + "builtin": {Raw: []byte(`{"machineDeployment":{"class":"classA"}}`)}, + }, selector: clusterv1.PatchSelector{ APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1", Kind: "AzureMachineTemplate", @@ -528,12 +711,23 @@ func TestTemplateMatchesSelector(t *testing.T) { match: true, }, { - name: "Don't match: unknown target type", - templateRef: &api.TemplateRef{ - APIVersion: "controlplane.cluster.x-k8s.io/v1beta1", - Kind: "ControlPlaneTemplate", - // invalid is an invalid TemplateType. - TemplateType: "invalid", + name: "Don't match: unknown field path", + req: &runtimehooksv1.GeneratePatchesRequestItem{ + Object: runtime.RawExtension{ + Object: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": "controlplane.cluster.x-k8s.io/v1beta1", + "kind": "ControlPlaneTemplate", + }, + }, + }, + HolderReference: runtimehooksv1.HolderReference{ + APIVersion: clusterv1.GroupVersion.String(), + Kind: "Custom", + Name: "my-md-0", + Namespace: "default", + FieldPath: "spec.machineTemplate.unknown.infrastructureRef", + }, }, selector: clusterv1.PatchSelector{ APIVersion: "controlplane.cluster.x-k8s.io/v1beta1", @@ -549,7 +743,7 @@ func TestTemplateMatchesSelector(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - g.Expect(templateMatchesSelector(tt.templateRef, tt.selector)).To(Equal(tt.match)) + g.Expect(matchesSelector(tt.req, tt.templateVariables, tt.selector)).To(Equal(tt.match)) }) } } @@ -1161,66 +1355,6 @@ func TestCalculateValue(t *testing.T) { } } -func TestParsePathSegment(t *testing.T) { - tests := []struct { - name string - segment string - wantPathSegment *pathSegment - wantErr bool - }{ - { - name: "parse basic segment", - segment: "propertyName", - wantPathSegment: &pathSegment{ - path: "propertyName", - index: nil, - }, - }, - { - name: "parse segment with index", - segment: "arrayProperty[5]", - wantPathSegment: &pathSegment{ - path: "arrayProperty", - index: pointer.Int(5), - }, - }, - { - name: "fail invalid syntax: only left delimiter", - segment: "arrayProperty[", - wantErr: true, - }, - { - name: "fail invalid syntax: only right delimiter", - segment: "arrayProperty]", - wantErr: true, - }, - { - name: "fail invalid syntax: both delimiter but no index", - segment: "arrayProperty[]", - wantErr: true, - }, - { - name: "fail invalid syntax: negative index", - segment: "arrayProperty[-1]", - wantErr: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - got, err := parsePathSegment(tt.segment) - if tt.wantErr { - g.Expect(err).To(HaveOccurred()) - return - } - g.Expect(err).ToNot(HaveOccurred()) - - g.Expect(got).To(Equal(tt.wantPathSegment)) - }) - } -} - func TestRenderValueTemplate(t *testing.T) { tests := []struct { name string @@ -1653,10 +1787,10 @@ func TestMergeVariables(t *testing.T) { } // toJSONCompact is used to be able to write JSON values in a readable manner. -func toJSONCompact(value string) apiextensionsv1.JSON { +func toJSONCompact(value string) []byte { var compactValue bytes.Buffer if err := json.Compact(&compactValue, []byte(value)); err != nil { panic(err) } - return apiextensionsv1.JSON{Raw: compactValue.Bytes()} + return compactValue.Bytes() } diff --git a/internal/controllers/topology/cluster/patches/patch.go b/internal/controllers/topology/cluster/patches/patch.go index f880bc4651a4..29ce0fd56a65 100644 --- a/internal/controllers/topology/cluster/patches/patch.go +++ b/internal/controllers/topology/cluster/patches/patch.go @@ -24,8 +24,8 @@ import ( jsonpatch "github.com/evanphx/json-patch/v5" "github.com/pkg/errors" - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/cluster-api/internal/contract" tlog "sigs.k8s.io/cluster-api/internal/log" @@ -135,7 +135,7 @@ func calculateDiff(original, patched *unstructured.Unstructured) ([]byte, error) } // patchTemplateSpec overwrites spec in templateJSON with spec of patchedTemplateBytes. -func patchTemplateSpec(templateJSON *apiextensionsv1.JSON, patchedTemplateBytes []byte) error { +func patchTemplateSpec(templateJSON *runtime.RawExtension, patchedTemplateBytes []byte) error { // Convert templates to Unstructured. template, err := bytesToUnstructured(templateJSON.Raw) if err != nil { @@ -161,6 +161,7 @@ func patchTemplateSpec(templateJSON *apiextensionsv1.JSON, patchedTemplateBytes if err != nil { return errors.Wrapf(err, "failed to marshal patched template") } + templateJSON.Object = template templateJSON.Raw = templateBytes return nil } diff --git a/internal/controllers/topology/cluster/patches/template.go b/internal/controllers/topology/cluster/patches/template.go index 01b9f616eb53..d8f8a2227e49 100644 --- a/internal/controllers/topology/cluster/patches/template.go +++ b/internal/controllers/topology/cluster/patches/template.go @@ -18,123 +18,115 @@ package patches import ( "encoding/json" + "strconv" "github.com/pkg/errors" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/uuid" + "sigs.k8s.io/controller-runtime/pkg/client" - clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" - "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/patches/api" + runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1" + "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/patches/variables" ) -// templateBuilder builds templates. -type templateBuilder struct { - template *unstructured.Unstructured - templateType api.TemplateType - mdTopology *clusterv1.MachineDeploymentTopology +// requestItemBuilder builds a GeneratePatchesRequestItem. +type requestItemBuilder struct { + template *unstructured.Unstructured + holder runtimehooksv1.HolderReference } -// newTemplateBuilder returns a new templateBuilder. -func newTemplateBuilder(template *unstructured.Unstructured) *templateBuilder { - return &templateBuilder{ +// newRequestItemBuilder returns a new requestItemBuilder. +func newRequestItemBuilder(template *unstructured.Unstructured) *requestItemBuilder { + return &requestItemBuilder{ template: template, } } -// WithType adds templateType to the templateBuilder. -func (t *templateBuilder) WithType(templateType api.TemplateType) *templateBuilder { - t.templateType = templateType - return t -} - -// WithMachineDeploymentRef adds a MachineDeploymentTopology to the templateBuilder, -// which is used to add a MachineDeploymentRef to the TemplateRef during BuildTemplateRef. -func (t *templateBuilder) WithMachineDeploymentRef(mdTopology *clusterv1.MachineDeploymentTopology) *templateBuilder { - t.mdTopology = mdTopology - return t -} - -// BuildTemplateRef builds an api.TemplateRef. -func (t *templateBuilder) BuildTemplateRef() api.TemplateRef { - templateRef := api.TemplateRef{ - APIVersion: t.template.GetAPIVersion(), - Kind: t.template.GetKind(), - TemplateType: t.templateType, - } - - if t.mdTopology != nil { - templateRef.MachineDeploymentRef = api.MachineDeploymentRef{ - TopologyName: t.mdTopology.Name, - Class: t.mdTopology.Class, - } +// WithHolder adds holder to the requestItemBuilder. +func (t *requestItemBuilder) WithHolder(object client.Object, fieldPath string) *requestItemBuilder { + t.holder = runtimehooksv1.HolderReference{ + APIVersion: object.GetObjectKind().GroupVersionKind().GroupVersion().String(), + Kind: object.GetObjectKind().GroupVersionKind().Kind, + Namespace: object.GetNamespace(), + Name: object.GetName(), + FieldPath: fieldPath, } - - return templateRef + return t } -// Build builds an api.GenerateRequestTemplate. -func (t *templateBuilder) Build() (*api.GenerateRequestTemplate, error) { - tpl := &api.GenerateRequestTemplate{ - TemplateRef: t.BuildTemplateRef(), +// Build builds a GeneratePatchesRequestItem. +func (t *requestItemBuilder) Build() (*runtimehooksv1.GeneratePatchesRequestItem, error) { + tpl := &runtimehooksv1.GeneratePatchesRequestItem{ + HolderReference: t.holder, + UID: uuid.NewUUID(), } jsonObj, err := json.Marshal(t.template) if err != nil { - return nil, errors.Wrap(err, "failed to marshal object to json") + return nil, errors.Wrap(err, "failed to marshal template to JSON") + } + + tpl.Object = runtime.RawExtension{ + Raw: jsonObj, + Object: t.template, } - tpl.Template = apiextensionsv1.JSON{Raw: jsonObj} return tpl, nil } -// getTemplateAsUnstructured is a utility func that returns a template matching the templateRef from a GenerateRequest. -func getTemplateAsUnstructured(req *api.GenerateRequest, templateRef api.TemplateRef) (*unstructured.Unstructured, error) { - // Find the template the patch should be applied to. - template := getTemplate(req, templateRef) +// getTemplateAsUnstructured is a utility func that returns a template matching the holderKind, holderFieldPath +// and mdTopologyName from a GeneratePatchesRequest. +func getTemplateAsUnstructured(req *runtimehooksv1.GeneratePatchesRequest, holderKind, holderFieldPath, mdTopologyName string) (*unstructured.Unstructured, error) { + // Find the requestItem. + requestItem := getRequestItem(req, holderKind, holderFieldPath, mdTopologyName) - // If a patch doesn't apply to any object, this is a misconfiguration. - if template == nil { - return nil, errors.Errorf("failed to get template %s", templateRef) + if requestItem == nil { + return nil, errors.Errorf("failed to get request item with holder kind %q, holder field path %q and MD topology name %q", holderKind, holderFieldPath, mdTopologyName) + } + + // Unmarshal the template. + template, err := bytesToUnstructured(requestItem.Object.Raw) + if err != nil { + return nil, errors.Wrapf(err, "failed to convert template to Unstructured") } - return templateToUnstructured(template) + return template, nil } -// getTemplate is a utility func that returns a template matching the templateRef from a GenerateRequest. -func getTemplate(req *api.GenerateRequest, templateRef api.TemplateRef) *api.GenerateRequestTemplate { - for _, template := range req.Items { - if templateRefsAreEqual(templateRef, template.TemplateRef) { - return template +// getRequestItemByUID is a utility func that returns a template matching the uid from a GeneratePatchesRequest. +func getRequestItemByUID(req *runtimehooksv1.GeneratePatchesRequest, uid types.UID) *runtimehooksv1.GeneratePatchesRequestItem { + for i := range req.Items { + if req.Items[i].UID == uid { + return &req.Items[i] } } return nil } -// templateRefsAreEqual returns true if the TemplateRefs are equal. -func templateRefsAreEqual(a, b api.TemplateRef) bool { - return a.APIVersion == b.APIVersion && - a.Kind == b.Kind && - a.TemplateType == b.TemplateType && - a.MachineDeploymentRef.TopologyName == b.MachineDeploymentRef.TopologyName && - a.MachineDeploymentRef.Class == b.MachineDeploymentRef.Class -} +// getRequestItem is a utility func that returns a template matching the holderKind, holderFiledPath and mdTopologyName from a GeneratePatchesRequest. +func getRequestItem(req *runtimehooksv1.GeneratePatchesRequest, holderKind, holderFieldPath, mdTopologyName string) *runtimehooksv1.GeneratePatchesRequestItem { + for _, template := range req.Items { + if holderKind != "" && template.HolderReference.Kind != holderKind { + continue + } + if holderFieldPath != "" && template.HolderReference.FieldPath != holderFieldPath { + continue + } + if mdTopologyName != "" { + templateVariables := toMap(template.Variables) -// templateToUnstructured converts a GenerateRequestTemplate into an Unstructured object. -func templateToUnstructured(t *api.GenerateRequestTemplate) (*unstructured.Unstructured, error) { - // Unmarshal the template. - u, err := bytesToUnstructured(t.Template.Raw) - if err != nil { - return nil, errors.Wrapf(err, "failed to convert template to Unstructured") - } + v, err := variables.GetVariableValue(templateVariables, "builtin.machineDeployment.topologyName") + if err != nil || string(v.Raw) != strconv.Quote(mdTopologyName) { + continue + } + } - // Set the GVK. - gv, err := schema.ParseGroupVersion(t.TemplateRef.APIVersion) - if err != nil { - return nil, errors.Wrap(err, "failed to convert template to Unstructured: failed to parse group version") + return &template } - u.SetGroupVersionKind(gv.WithKind(t.TemplateRef.Kind)) - return u, nil + return nil } // bytesToUnstructured provides a utility method that converts a (JSON) byte array into an Unstructured object. @@ -151,3 +143,13 @@ func bytesToUnstructured(b []byte) (*unstructured.Unstructured, error) { return u, nil } + +// toMap converts a list of Variables to a map of JSON (name is the map key). +func toMap(variables []runtimehooksv1.Variable) map[string]apiextensionsv1.JSON { + variablesMap := map[string]apiextensionsv1.JSON{} + + for i := range variables { + variablesMap[variables[i].Name] = variables[i].Value + } + return variablesMap +} diff --git a/internal/controllers/topology/cluster/patches/variables/value.go b/internal/controllers/topology/cluster/patches/variables/value.go new file mode 100644 index 000000000000..eaf171151fa4 --- /dev/null +++ b/internal/controllers/topology/cluster/patches/variables/value.go @@ -0,0 +1,163 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package variables + +import ( + "strconv" + "strings" + + "github.com/pkg/errors" + "github.com/valyala/fastjson" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + "k8s.io/utils/pointer" +) + +const ( + leftArrayDelim = "[" + rightArrayDelim = "]" +) + +// GetVariableValue returns a variable from the variables map. +func GetVariableValue(variables map[string]apiextensionsv1.JSON, variablePath string) (*apiextensionsv1.JSON, error) { + // Split the variablePath (format: "."). + variableSplit := strings.Split(variablePath, ".") + variableName, relativePath := variableSplit[0], variableSplit[1:] + + // Parse the path segment. + variableNameSegment, err := parsePathSegment(variableName) + if err != nil { + return nil, errors.Wrapf(err, "variable %q is invalid", variablePath) + } + + // Get the variable. + value, ok := variables[variableNameSegment.path] + if !ok { + return nil, errors.Errorf("variable %q does not exist", variableName) + } + + // Return the value, if variablePath points to a top-level variable, i.e. hos no relativePath and no + // array index (i.e. ""). + if len(relativePath) == 0 && !variableNameSegment.HasIndex() { + return &value, nil + } + + // Parse the variable object. + variable, err := fastjson.ParseBytes(value.Raw) + if err != nil { + return nil, errors.Wrapf(err, "cannot parse variable %q: %s", variableName, string(value.Raw)) + } + + // If variableName contains an array index, get the array element (i.e. starts with "[i]"). + // Then return it, if there is no relative path (i.e. "[i]") + if variableNameSegment.HasIndex() { + variable, err = getVariableArrayElement(variable, variableNameSegment, variablePath) + if err != nil { + return nil, err + } + + if len(relativePath) == 0 { + return &apiextensionsv1.JSON{ + Raw: variable.MarshalTo([]byte{}), + }, nil + } + } + + // If the variablePath points to a nested variable, i.e. has a relativePath, inspect the variable object. + + // Retrieve each segment of the relativePath incrementally, taking care of resolving array indexes. + for _, p := range relativePath { + // Parse the path segment. + pathSegment, err := parsePathSegment(p) + if err != nil { + return nil, errors.Wrapf(err, "variable %q has invalid syntax", variablePath) + } + + // Return if the variable does not exist. + if !variable.Exists(pathSegment.path) { + return nil, errors.Errorf("variable %q does not exist: failed to lookup segment %q", variablePath, pathSegment.path) + } + + // Get the variable from the variable object. + variable = variable.Get(pathSegment.path) + + // Continue if the path doesn't contain an index. + if !pathSegment.HasIndex() { + continue + } + + variable, err = getVariableArrayElement(variable, pathSegment, variablePath) + if err != nil { + return nil, err + } + } + + // Return the marshalled value of the variable. + return &apiextensionsv1.JSON{ + Raw: variable.MarshalTo([]byte{}), + }, nil +} + +type pathSegment struct { + path string + index *int +} + +func (p pathSegment) HasIndex() bool { + return p.index != nil +} + +func parsePathSegment(segment string) (*pathSegment, error) { + if (strings.Contains(segment, leftArrayDelim) && !strings.Contains(segment, rightArrayDelim)) || + (!strings.Contains(segment, leftArrayDelim) && strings.Contains(segment, rightArrayDelim)) { + return nil, errors.Errorf("failed to parse path segment %q", segment) + } + + if !strings.Contains(segment, leftArrayDelim) && !strings.Contains(segment, rightArrayDelim) { + return &pathSegment{ + path: segment, + }, nil + } + + arrayIndexStr := segment[strings.Index(segment, leftArrayDelim)+1 : strings.Index(segment, rightArrayDelim)] + index, err := strconv.Atoi(arrayIndexStr) + if err != nil { + return nil, errors.Wrapf(err, "failed to parse array index in path segment %q", segment) + } + if index < 0 { + return nil, errors.Errorf("invalid array index %d in path segment %q", index, segment) + } + + return &pathSegment{ + path: segment[:strings.Index(segment, leftArrayDelim)], //nolint:gocritic // We already check above that segment contains leftArrayDelim, + index: pointer.Int(index), + }, nil +} + +// getVariableArrayElement gets the array element of a given array. +func getVariableArrayElement(array *fastjson.Value, arrayPathSegment *pathSegment, fullVariablePath string) (*fastjson.Value, error) { + // Retrieve the array element, handling index out of range. + arr, err := array.Array() + if err != nil { + return nil, errors.Wrapf(err, "variable %q is invalid: failed to get array %q", fullVariablePath, arrayPathSegment.path) + } + + if len(arr) < *arrayPathSegment.index+1 { + return nil, errors.Errorf("variable %q is invalid: array does not have index %d", fullVariablePath, arrayPathSegment.index) + } + + return arr[*arrayPathSegment.index], nil +} diff --git a/internal/controllers/topology/cluster/patches/variables/value_test.go b/internal/controllers/topology/cluster/patches/variables/value_test.go new file mode 100644 index 000000000000..d1abb2004273 --- /dev/null +++ b/internal/controllers/topology/cluster/patches/variables/value_test.go @@ -0,0 +1,84 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package variables + +import ( + "testing" + + . "github.com/onsi/gomega" + "k8s.io/utils/pointer" +) + +func TestParsePathSegment(t *testing.T) { + tests := []struct { + name string + segment string + wantPathSegment *pathSegment + wantErr bool + }{ + { + name: "parse basic segment", + segment: "propertyName", + wantPathSegment: &pathSegment{ + path: "propertyName", + index: nil, + }, + }, + { + name: "parse segment with index", + segment: "arrayProperty[5]", + wantPathSegment: &pathSegment{ + path: "arrayProperty", + index: pointer.Int(5), + }, + }, + { + name: "fail invalid syntax: only left delimiter", + segment: "arrayProperty[", + wantErr: true, + }, + { + name: "fail invalid syntax: only right delimiter", + segment: "arrayProperty]", + wantErr: true, + }, + { + name: "fail invalid syntax: both delimiter but no index", + segment: "arrayProperty[]", + wantErr: true, + }, + { + name: "fail invalid syntax: negative index", + segment: "arrayProperty[-1]", + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + got, err := parsePathSegment(tt.segment) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + return + } + g.Expect(err).ToNot(HaveOccurred()) + + g.Expect(got).To(Equal(tt.wantPathSegment)) + }) + } +} diff --git a/internal/controllers/topology/cluster/patches/variables/variables.go b/internal/controllers/topology/cluster/patches/variables/variables.go index 2140163e8a8e..4090bc88dae4 100644 --- a/internal/controllers/topology/cluster/patches/variables/variables.go +++ b/internal/controllers/topology/cluster/patches/variables/variables.go @@ -26,6 +26,7 @@ import ( "k8s.io/utils/pointer" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1" "sigs.k8s.io/cluster-api/internal/contract" ) @@ -166,18 +167,22 @@ type MachineDeploymentInfrastructureRefBuiltins struct { Name string `json:"name,omitempty"` } -// VariableMap is a name/value map of variables. -// Values are marshalled as JSON. -type VariableMap map[string]apiextensionsv1.JSON - // Global returns variables that apply to all the templates, including user provided variables // and builtin variables for the Cluster object. -func Global(clusterTopology *clusterv1.Topology, cluster *clusterv1.Cluster) (VariableMap, error) { - variables := VariableMap{} +func Global(clusterTopology *clusterv1.Topology, cluster *clusterv1.Cluster) ([]runtimehooksv1.Variable, error) { + variables := []runtimehooksv1.Variable{} // Add user defined variables from Cluster.spec.topology.variables. for _, variable := range clusterTopology.Variables { - variables[variable.Name] = variable.Value + // Don't add user-defined "builtin" variable. + if variable.Name == BuiltinsName { + continue + } + + variables = append(variables, runtimehooksv1.Variable{ + Name: variable.Name, + Value: variable.Value, + }) } // Construct builtin variable. @@ -211,16 +216,18 @@ func Global(clusterTopology *clusterv1.Topology, cluster *clusterv1.Cluster) (Va } // Add builtin variables derived from the cluster object. - if err := setVariable(variables, BuiltinsName, builtin); err != nil { + variable, err := toVariable(BuiltinsName, builtin) + if err != nil { return nil, err } + variables = append(variables, *variable) return variables, nil } // ControlPlane returns variables that apply to templates belonging to the ControlPlane. -func ControlPlane(cpTopology *clusterv1.ControlPlaneTopology, cp, cpInfrastructureMachineTemplate *unstructured.Unstructured) (VariableMap, error) { - variables := VariableMap{} +func ControlPlane(cpTopology *clusterv1.ControlPlaneTopology, cp, cpInfrastructureMachineTemplate *unstructured.Unstructured) ([]runtimehooksv1.Variable, error) { + variables := []runtimehooksv1.Variable{} // Construct builtin variable. builtin := Builtins{ @@ -255,21 +262,26 @@ func ControlPlane(cpTopology *clusterv1.ControlPlaneTopology, cp, cpInfrastructu } } - if err := setVariable(variables, BuiltinsName, builtin); err != nil { + variable, err := toVariable(BuiltinsName, builtin) + if err != nil { return nil, err } + variables = append(variables, *variable) return variables, nil } // MachineDeployment returns variables that apply to templates belonging to a MachineDeployment. -func MachineDeployment(mdTopology *clusterv1.MachineDeploymentTopology, md *clusterv1.MachineDeployment, mdBootstrapTemplate, mdInfrastructureMachineTemplate *unstructured.Unstructured) (VariableMap, error) { - variables := VariableMap{} +func MachineDeployment(mdTopology *clusterv1.MachineDeploymentTopology, md *clusterv1.MachineDeployment, mdBootstrapTemplate, mdInfrastructureMachineTemplate *unstructured.Unstructured) ([]runtimehooksv1.Variable, error) { + variables := []runtimehooksv1.Variable{} // Add variables overrides for the MachineDeployment. if mdTopology.Variables != nil { for _, variable := range mdTopology.Variables.Overrides { - variables[variable.Name] = variable.Value + variables = append(variables, runtimehooksv1.Variable{ + Name: variable.Name, + Value: variable.Value, + }) } } @@ -300,22 +312,26 @@ func MachineDeployment(mdTopology *clusterv1.MachineDeploymentTopology, md *clus } } - if err := setVariable(variables, BuiltinsName, builtin); err != nil { + variable, err := toVariable(BuiltinsName, builtin) + if err != nil { return nil, err } + variables = append(variables, *variable) return variables, nil } -// setVariable converts value to JSON and adds the variable to the variables map. -func setVariable(variables VariableMap, name string, value interface{}) error { +// toVariable converts name and value to a variable. +func toVariable(name string, value interface{}) (*runtimehooksv1.Variable, error) { marshalledValue, err := json.Marshal(value) if err != nil { - return errors.Wrapf(err, "failed to set variable %q: error marshalling", name) + return nil, errors.Wrapf(err, "failed to set variable %q: error marshalling", name) } - variables[name] = apiextensionsv1.JSON{Raw: marshalledValue} - return nil + return &runtimehooksv1.Variable{ + Name: name, + Value: apiextensionsv1.JSON{Raw: marshalledValue}, + }, nil } func ipFamilyToString(ipFamily clusterv1.ClusterIPFamily) string { diff --git a/internal/controllers/topology/cluster/patches/variables/variables_test.go b/internal/controllers/topology/cluster/patches/variables/variables_test.go index b375276499e4..f20f40c454c8 100644 --- a/internal/controllers/topology/cluster/patches/variables/variables_test.go +++ b/internal/controllers/topology/cluster/patches/variables/variables_test.go @@ -28,6 +28,7 @@ import ( "k8s.io/utils/pointer" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1" "sigs.k8s.io/cluster-api/internal/test/builder" ) @@ -36,7 +37,7 @@ func TestGlobal(t *testing.T) { name string clusterTopology *clusterv1.Topology cluster *clusterv1.Cluster - want VariableMap + want []runtimehooksv1.Variable }{ { name: "Should calculate global variables", @@ -79,10 +80,18 @@ func TestGlobal(t *testing.T) { }, }, }, - want: VariableMap{ - "location": toJSON("\"us-central\""), - "cpu": toJSON("8"), - BuiltinsName: toJSONCompact(`{ + want: []runtimehooksv1.Variable{ + { + Name: "location", + Value: toJSON("\"us-central\""), + }, + { + Name: "cpu", + Value: toJSON("8"), + }, + { + Name: BuiltinsName, + Value: toJSONCompact(`{ "cluster":{ "name": "cluster1", "namespace": "default", @@ -95,8 +104,9 @@ func TestGlobal(t *testing.T) { "services":["10.10.10.1/24"], "pods":["11.10.10.1/24"], "ipFamily": "IPv4" - }}}`, - ), + } + }}`), + }, }, }, { @@ -139,10 +149,18 @@ func TestGlobal(t *testing.T) { }, }, }, - want: VariableMap{ - "location": toJSON("\"us-central\""), - "cpu": toJSON("8"), - BuiltinsName: toJSONCompact(`{ + want: []runtimehooksv1.Variable{ + { + Name: "location", + Value: toJSON("\"us-central\""), + }, + { + Name: "cpu", + Value: toJSON("8"), + }, + { + Name: BuiltinsName, + Value: toJSONCompact(`{ "cluster":{ "name": "cluster1", "namespace": "default", @@ -154,8 +172,9 @@ func TestGlobal(t *testing.T) { "services":["10.10.10.1/24"], "pods":["11.10.10.1/24"], "ipFamily": "IPv4" - }}}`, - ), + } + }}`), + }, }, }, { @@ -195,10 +214,18 @@ func TestGlobal(t *testing.T) { }, }, }, - want: VariableMap{ - "location": toJSON("\"us-central\""), - "cpu": toJSON("8"), - BuiltinsName: toJSONCompact(`{ + want: []runtimehooksv1.Variable{ + { + Name: "location", + Value: toJSON("\"us-central\""), + }, + { + Name: "cpu", + Value: toJSON("8"), + }, + { + Name: BuiltinsName, + Value: toJSONCompact(`{ "cluster":{ "name": "cluster1", "namespace": "default", @@ -209,7 +236,9 @@ func TestGlobal(t *testing.T) { "network":{ "serviceDomain":"cluster.local", "ipFamily": "IPv4" - }}}`), + } + }}`), + }, }, }, { @@ -245,18 +274,27 @@ func TestGlobal(t *testing.T) { ClusterNetwork: nil, }, }, - want: VariableMap{ - "location": toJSON("\"us-central\""), - "cpu": toJSON("8"), - BuiltinsName: toJSONCompact(`{ + want: []runtimehooksv1.Variable{ + { + Name: "location", + Value: toJSON("\"us-central\""), + }, + { + Name: "cpu", + Value: toJSON("8"), + }, + { + Name: BuiltinsName, + Value: toJSONCompact(`{ "cluster":{ "name": "cluster1", "namespace": "default", "topology":{ - "version": "v1.21.1", - "class": "clusterClass1" - } + "version": "v1.21.1", + "class": "clusterClass1" + } }}`), + }, }, }, } @@ -277,7 +315,7 @@ func TestControlPlane(t *testing.T) { controlPlaneTopology *clusterv1.ControlPlaneTopology controlPlane *unstructured.Unstructured controlPlaneInfrastructureMachineTemplate *unstructured.Unstructured - want VariableMap + want []runtimehooksv1.Variable }{ { name: "Should calculate ControlPlane variables", @@ -288,13 +326,16 @@ func TestControlPlane(t *testing.T) { WithReplicas(3). WithVersion("v1.21.1"). Build(), - want: VariableMap{ - BuiltinsName: toJSONCompact(`{ + want: []runtimehooksv1.Variable{ + { + Name: BuiltinsName, + Value: toJSONCompact(`{ "controlPlane":{ "version": "v1.21.1", "name":"controlPlane1", "replicas":3 }}`), + }, }, }, { @@ -303,12 +344,15 @@ func TestControlPlane(t *testing.T) { controlPlane: builder.ControlPlane(metav1.NamespaceDefault, "controlPlane1"). WithVersion("v1.21.1"). Build(), - want: VariableMap{ - BuiltinsName: toJSONCompact(`{ + want: []runtimehooksv1.Variable{ + { + Name: BuiltinsName, + Value: toJSONCompact(`{ "controlPlane":{ "version": "v1.21.1", "name":"controlPlane1" }}`), + }, }, }, { @@ -322,8 +366,10 @@ func TestControlPlane(t *testing.T) { Build(), controlPlaneInfrastructureMachineTemplate: builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "controlPlaneInfrastructureMachineTemplate1"). Build(), - want: VariableMap{ - BuiltinsName: toJSONCompact(`{ + want: []runtimehooksv1.Variable{ + { + Name: BuiltinsName, + Value: toJSONCompact(`{ "controlPlane":{ "version": "v1.21.1", "name":"controlPlane1", @@ -334,6 +380,7 @@ func TestControlPlane(t *testing.T) { } } }}`), + }, }, }, } @@ -355,7 +402,7 @@ func TestMachineDeployment(t *testing.T) { md *clusterv1.MachineDeployment mdBootstrapTemplate *unstructured.Unstructured mdInfrastructureMachineTemplate *unstructured.Unstructured - want VariableMap + want []runtimehooksv1.Variable }{ { name: "Should calculate MachineDeployment variables", @@ -380,10 +427,18 @@ func TestMachineDeployment(t *testing.T) { WithReplicas(3). WithVersion("v1.21.1"). Build(), - want: VariableMap{ - "location": toJSON("\"us-central\""), - "cpu": toJSON("8"), - BuiltinsName: toJSONCompact(`{ + want: []runtimehooksv1.Variable{ + { + Name: "location", + Value: toJSON("\"us-central\""), + }, + { + Name: "cpu", + Value: toJSON("8"), + }, + { + Name: BuiltinsName, + Value: toJSONCompact(`{ "machineDeployment":{ "version": "v1.21.1", "class": "md-class", @@ -391,6 +446,7 @@ func TestMachineDeployment(t *testing.T) { "topologyName": "md-topology", "replicas":3 }}`), + }, }, }, { @@ -404,8 +460,10 @@ func TestMachineDeployment(t *testing.T) { WithReplicas(3). WithVersion("v1.21.1"). Build(), - want: VariableMap{ - BuiltinsName: toJSONCompact(`{ + want: []runtimehooksv1.Variable{ + { + Name: BuiltinsName, + Value: toJSONCompact(`{ "machineDeployment":{ "version": "v1.21.1", "class": "md-class", @@ -413,6 +471,7 @@ func TestMachineDeployment(t *testing.T) { "topologyName": "md-topology", "replicas":3 }}`), + }, }, }, { @@ -436,16 +495,25 @@ func TestMachineDeployment(t *testing.T) { md: builder.MachineDeployment(metav1.NamespaceDefault, "md1"). WithVersion("v1.21.1"). Build(), - want: VariableMap{ - "location": toJSON("\"us-central\""), - "cpu": toJSON("8"), - BuiltinsName: toJSONCompact(`{ + want: []runtimehooksv1.Variable{ + { + Name: "location", + Value: toJSON("\"us-central\""), + }, + { + Name: "cpu", + Value: toJSON("8"), + }, + { + Name: BuiltinsName, + Value: toJSONCompact(`{ "machineDeployment":{ "version": "v1.21.1", "class": "md-class", "name": "md1", "topologyName": "md-topology" }}`), + }, }, }, { @@ -472,10 +540,18 @@ func TestMachineDeployment(t *testing.T) { WithVersion("v1.21.1"). Build(), mdBootstrapTemplate: builder.BootstrapTemplate(metav1.NamespaceDefault, "mdBT1").Build(), - want: VariableMap{ - "location": toJSON("\"us-central\""), - "cpu": toJSON("8"), - BuiltinsName: toJSONCompact(`{ + want: []runtimehooksv1.Variable{ + { + Name: "location", + Value: toJSON("\"us-central\""), + }, + { + Name: "cpu", + Value: toJSON("8"), + }, + { + Name: BuiltinsName, + Value: toJSONCompact(`{ "machineDeployment":{ "version": "v1.21.1", "class": "md-class", @@ -488,6 +564,7 @@ func TestMachineDeployment(t *testing.T) { } } }}`), + }, }, }, { @@ -514,10 +591,18 @@ func TestMachineDeployment(t *testing.T) { WithVersion("v1.21.1"). Build(), mdInfrastructureMachineTemplate: builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "mdIMT1").Build(), - want: VariableMap{ - "location": toJSON("\"us-central\""), - "cpu": toJSON("8"), - BuiltinsName: toJSONCompact(`{ + want: []runtimehooksv1.Variable{ + { + Name: "location", + Value: toJSON("\"us-central\""), + }, + { + Name: "cpu", + Value: toJSON("8"), + }, + { + Name: BuiltinsName, + Value: toJSONCompact(`{ "machineDeployment":{ "version": "v1.21.1", "class": "md-class", @@ -528,6 +613,7 @@ func TestMachineDeployment(t *testing.T) { "name": "mdIMT1" } }}`), + }, }, }, { @@ -555,10 +641,18 @@ func TestMachineDeployment(t *testing.T) { Build(), mdBootstrapTemplate: builder.BootstrapTemplate(metav1.NamespaceDefault, "mdBT1").Build(), mdInfrastructureMachineTemplate: builder.InfrastructureMachineTemplate(metav1.NamespaceDefault, "mdIMT1").Build(), - want: VariableMap{ - "location": toJSON("\"us-central\""), - "cpu": toJSON("8"), - BuiltinsName: toJSONCompact(`{ + want: []runtimehooksv1.Variable{ + { + Name: "location", + Value: toJSON("\"us-central\""), + }, + { + Name: "cpu", + Value: toJSON("8"), + }, + { + Name: BuiltinsName, + Value: toJSONCompact(`{ "machineDeployment":{ "version": "v1.21.1", "class": "md-class", @@ -574,6 +668,7 @@ func TestMachineDeployment(t *testing.T) { "name": "mdIMT1" } }}`), + }, }, }, } diff --git a/test/e2e/data/infrastructure-docker/v1beta1/clusterclass-quick-start.yaml b/test/e2e/data/infrastructure-docker/v1beta1/clusterclass-quick-start.yaml index 00d12ceaf6dd..69828e5d14b7 100644 --- a/test/e2e/data/infrastructure-docker/v1beta1/clusterclass-quick-start.yaml +++ b/test/e2e/data/infrastructure-docker/v1beta1/clusterclass-quick-start.yaml @@ -33,6 +33,12 @@ spec: kind: DockerMachineTemplate name: quick-start-default-worker-machinetemplate variables: + - name: lbImageRepository + required: true + schema: + openAPIV3Schema: + type: string + default: kindest - name: imageRepository required: true schema: @@ -66,6 +72,19 @@ spec: example: "0" description: "kubeadmControlPlaneMaxSurge is the maximum number of control planes that can be scheduled above or under the desired number of control plane machines." patches: + - name: lbImageRepository + definitions: + - selector: + apiVersion: infrastructure.cluster.x-k8s.io/v1beta1 + kind: DockerClusterTemplate + matchResources: + infrastructureCluster: true + jsonPatches: + - op: add + path: "/spec/template/spec/loadBalancer" + valueFrom: + template: | + imageRepository: {{ .lbImageRepository }} - name: imageRepository description: "Sets the imageRepository used for the KubeadmControlPlane." definitions: