Skip to content

Commit

Permalink
fix review findings add unit tests
Browse files Browse the repository at this point in the history
  • Loading branch information
@sbueringer
sbueringer committed Nov 2, 2021
1 parent 86a9816 commit 23fe99f
Show file tree
Hide file tree
Showing 7 changed files with 1,322 additions and 187 deletions.
4 changes: 2 additions & 2 deletions controllers/topology/internal/extensions/patches/api/interface.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,8 @@ const (
// JSONPatchType identifies a https://datatracker.ietf.org/doc/html/rfc6902 json patch.
JSONPatchType PatchType = 0

// MergePatchType identifies a https://datatracker.ietf.org/doc/html/rfc7386 json merge patch.
MergePatchType PatchType = 1
// JSONMergePatchType identifies a https://datatracker.ietf.org/doc/html/rfc7386 json merge patch.
JSONMergePatchType PatchType = 1
)

// GenerateResponse defines the response of a Generate request.
Expand Down
195 changes: 11 additions & 184 deletions controllers/topology/internal/extensions/patches/engine.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"context"
"encoding/json"
"fmt"
"strings"

jsonpatch "github.com/evanphx/json-patch"
"github.com/pkg/errors"
Expand All @@ -33,7 +32,6 @@ import (
"sigs.k8s.io/cluster-api/controllers/topology/internal/extensions/patches/api"
"sigs.k8s.io/cluster-api/controllers/topology/internal/extensions/patches/variables"
tlog "sigs.k8s.io/cluster-api/controllers/topology/internal/log"
"sigs.k8s.io/cluster-api/controllers/topology/internal/mergepatch"
"sigs.k8s.io/cluster-api/controllers/topology/internal/scope"
"sigs.k8s.io/controller-runtime/pkg/client"
)
Expand Down Expand Up @@ -67,20 +65,20 @@ func Apply(ctx context.Context, blueprint *scope.ClusterBlueprint, desired *scop
}

// Generate patches.
patches, err := generator.Generate(ctx, req)
resp, err := generator.Generate(ctx, req)
if err != nil {
return errors.Errorf("failed to generate patches for patch %q", patch.Name)
}

// Apply patches to the request.
if err := applyPatchesToRequest(ctx, req, patches); err != nil {
if err := applyPatchesToRequest(ctx, req, resp); err != nil {
return err
}
}

// Use patched templates to update the desired state objects.
log.V(5).Infof("Applying patches to desired state")
if err := requestToDesiredState(ctx, req, blueprint, desired); err != nil {
if err := updateDesiredState(ctx, req, blueprint, desired); err != nil {
return errors.Wrapf(err, "failed to apply patches to desired state")
}

Expand Down Expand Up @@ -216,17 +214,17 @@ func request(blueprint *scope.ClusterBlueprint, desired *scope.ClusterState) (*a

// newTemplate returns a GenerateRequestTemplate object based on the client.Object and the templateID.
func newTemplate(obj client.Object, templateID api.TemplateID) (*api.GenerateRequestTemplate, error) {
ret := &api.GenerateRequestTemplate{
t := &api.GenerateRequestTemplate{
TemplateID: templateID,
}

jsonObj, err := json.Marshal(obj)
if err != nil {
return nil, errors.Wrap(err, "failed to marshal object to json")
}
ret.Template = apiextensionsv1.JSON{Raw: jsonObj}
t.Template = apiextensionsv1.JSON{Raw: jsonObj}

return ret, nil
return t, nil
}

// lookupMDTopology looks up the MachineDeploymentTopology based on a mdTopologyName in a topology.
Expand Down Expand Up @@ -270,9 +268,7 @@ func applyPatchesToRequest(ctx context.Context, req *api.GenerateRequest, resp *

// If a patch doesn't apply to any template, this is a misconfiguration.
if template == nil {
log.V(5).Infof("Could not find template in GenerateRequest")
// TODO(TBD) @fabrizio should we return an error here?
continue
return errors.Errorf("could not find template with id %q in GenerateRequest", templateIDRef(patch.TemplateID))
}

// Use the patch to create a patched copy of the template.
Expand All @@ -293,7 +289,7 @@ func applyPatchesToRequest(ctx context.Context, req *api.GenerateRequest, resp *
return errors.Wrapf(err, "failed to apply patch to template %s: error applying json patch (RFC6902): %s",
templateIDRef(template.TemplateID), string(patch.Patch.Raw))
}
case api.MergePatchType:
case api.JSONMergePatchType:
log.V(5).Infof("Accumulating JSON merge patch", "patch", string(patch.Patch.Raw))
patchedTemplate, err = jsonpatch.MergePatch(template.Template.Raw, patch.Patch.Raw)
if err != nil {
Expand All @@ -312,10 +308,10 @@ func applyPatchesToRequest(ctx context.Context, req *api.GenerateRequest, resp *
return nil
}

// requestToDesiredState uses the patched templates of a GenerateRequest to update the desired state.
// 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.
// TODO: Let's only overwrite desired state objects if the corresponding template actually have been patched.
func requestToDesiredState(ctx context.Context, req *api.GenerateRequest, blueprint *scope.ClusterBlueprint, desired *scope.ClusterState) error {
// TODO(sbueringer): Let's only overwrite desired state objects if the corresponding template actually have been patched.
func updateDesiredState(ctx context.Context, req *api.GenerateRequest, blueprint *scope.ClusterBlueprint, desired *scope.ClusterState) error {
var err error

// Update the InfrastructureCluster.
Expand Down Expand Up @@ -497,172 +493,3 @@ func bytesToUnstructured(b []byte) (*unstructured.Unstructured, error) {

return u, nil
}

// PatchOption represents an option for the patchObject and patchTemplate funcs.
type PatchOption interface {
// ApplyToHelper applies configuration to the given options.
ApplyToHelper(*PatchOptions)
}

// PatchOptions contains options for patchObject and patchTemplate.
type PatchOptions struct {
preserveFields []contract.Path
}

// ApplyOptions applies the given patch options
// and then returns itself (for convenient chaining).
func (o *PatchOptions) ApplyOptions(opts []PatchOption) *PatchOptions {
for _, opt := range opts {
opt.ApplyToHelper(o)
}
return o
}

// PreserveFields instructs the patch func to preserve fields.
type PreserveFields []contract.Path

// ApplyToHelper applies this configuration to the given patch options.
func (i PreserveFields) ApplyToHelper(opts *PatchOptions) {
opts.preserveFields = i
}

// patchObject overwrites spec in object with spec.template.spec of patchedTemplate,
// while preserving the configured fields.
func patchObject(ctx context.Context, object, patchedTemplate *unstructured.Unstructured, opts ...PatchOption) error {
log := tlog.LoggerFrom(ctx)

patchOptions := &PatchOptions{}
patchOptions = patchOptions.ApplyOptions(opts)

// Create a copy of the object.
original := object.DeepCopy()

if err := copySpec(copySpecInput{
src: patchedTemplate,
dest: object,
srcSpecPath: "spec.template.spec",
destSpecPath: "spec",
fieldsToPreserve: patchOptions.preserveFields,
}); err != nil {
return errors.Wrapf(err, "failed to patch object %s by appying changes from the patched template",
tlog.KObj{Obj: object})
}

// Log the delta between the object before and after applying the accumulated patches.
helper, err := mergepatch.NewHelper(original, object, nil)
if err != nil {
return err
}
log.V(4).WithObject(object).Infof("Applying accumulated patches", "delta", string(helper.Changes()))

return nil
}

// patchTemplate overwrites spec.template.spec in template with spec.template.spec of patchedTemplate,
// while preserving the configured fields.
func patchTemplate(ctx context.Context, template, patchedTemplate *unstructured.Unstructured, opts ...PatchOption) error {
log := tlog.LoggerFrom(ctx)

patchOptions := &PatchOptions{}
patchOptions = patchOptions.ApplyOptions(opts)

// Create a copy of the template.
original := template.DeepCopy()

if err := copySpec(copySpecInput{
src: patchedTemplate,
dest: template,
srcSpecPath: "spec.template.spec",
destSpecPath: "spec.template.spec",
fieldsToPreserve: patchOptions.preserveFields,
}); err != nil {
return errors.Wrapf(err, "failed to patch template %s by appying changes from the patched template",
tlog.KObj{Obj: template})
}

// Log the delta between the object before and after applying the accumulated patches.
helper, err := mergepatch.NewHelper(original, template, nil)
if err != nil {
return err
}
log.V(4).WithObject(template).Infof("Applying accumulated patches", "delta", string(helper.Changes()))

return nil
}

// patchTemplateSpec overwrites spec in templateJSON with spec of patchedTemplateBytes.
func patchTemplateSpec(templateJSON *apiextensionsv1.JSON, patchedTemplateBytes []byte) error {
// Convert templates to Unstructured.
template, err := bytesToUnstructured(templateJSON.Raw)
if err != nil {
return errors.Wrap(err, "failed to convert template to Unstructured")
}
patchedTemplate, err := bytesToUnstructured(patchedTemplateBytes)
if err != nil {
return errors.Wrap(err, "failed to convert patched template to Unstructured")
}

// Copy spec from patchedTemplate to template.
if err := copySpec(copySpecInput{
src: patchedTemplate,
dest: template,
srcSpecPath: "spec",
destSpecPath: "spec",
}); err != nil {
return errors.Wrap(err, "failed to apply patch to template")
}

// Marshal template and store it in templateJSON.
templateBytes, err := template.MarshalJSON()
if err != nil {
return errors.Wrapf(err, "failed to marshal patched template")
}
templateJSON.Raw = templateBytes
return nil
}

type copySpecInput struct {
src *unstructured.Unstructured
dest *unstructured.Unstructured
srcSpecPath string
destSpecPath string
fieldsToPreserve []contract.Path
}

// copySpec copies a field from a srcSpecPath in src to a destSpecPath in dest,
// while preserving fieldsToPreserve.
func copySpec(in copySpecInput) error {
// Backup fields that should be preserved from dest.
preservedFields := map[string]interface{}{}
for _, field := range in.fieldsToPreserve {
value, found, err := unstructured.NestedFieldNoCopy(in.dest.Object, field...)
if !found {
// Continue if the field does not exist in src. fieldsToPreserve don't have to exist.
continue
} else if err != nil {
return errors.Wrapf(err, "failed to get field %q from %s", strings.Join(field, "."), tlog.KObj{Obj: in.dest})
}
preservedFields[strings.Join(field, ".")] = value
}

// Get spec from src.
srcSpec, found, err := unstructured.NestedFieldNoCopy(in.src.Object, strings.Split(in.srcSpecPath, ".")...)
if !found {
return errors.Errorf("missing field %q in %s", in.srcSpecPath, tlog.KObj{Obj: in.src})
} else if err != nil {
return errors.Wrapf(err, "failed to get field %q from %s", in.srcSpecPath, tlog.KObj{Obj: in.src})
}

// Set spec in dest.
if err := unstructured.SetNestedField(in.dest.Object, srcSpec, strings.Split(in.destSpecPath, ".")...); err != nil {
return errors.Wrapf(err, "failed to set field %q on %s", in.destSpecPath, tlog.KObj{Obj: in.dest})
}

// Restore preserved fields.
for path, value := range preservedFields {
if err := unstructured.SetNestedField(in.dest.Object, value, strings.Split(path, ".")...); err != nil {
return errors.Wrapf(err, "failed to set field %q on %s", path, tlog.KObj{Obj: in.dest})
}
}
return nil
}
Loading

0 comments on commit 23fe99f

Please sign in to comment.