From f15cba70b378f198a80c39d16265a6c017f673ee Mon Sep 17 00:00:00 2001 From: "Cote, Jason" Date: Fri, 16 Dec 2022 13:51:33 -0500 Subject: [PATCH] Use 3-way diff for JSON merge patches on CRD & unstructured resources - This change will produce more reliable patches when modifying these resource types. - It especially helps when adopting an existing one of these resources into a chart. (fixes #11638) Signed-off-by: Cote, Jason --- cmd/helm/install.go | 1 + cmd/helm/rollback.go | 1 + cmd/helm/upgrade.go | 2 ++ pkg/action/install.go | 13 +++++++------ pkg/action/rollback.go | 23 ++++++++++++----------- pkg/action/upgrade.go | 4 +++- pkg/kube/client.go | 22 ++++++++++++++++------ pkg/kube/client_test.go | 2 +- pkg/kube/fake/fake.go | 4 ++-- pkg/kube/fake/printer.go | 2 +- pkg/kube/interface.go | 2 +- 11 files changed, 47 insertions(+), 29 deletions(-) diff --git a/cmd/helm/install.go b/cmd/helm/install.go index 23ff29d95..5ebe0a65d 100644 --- a/cmd/helm/install.go +++ b/cmd/helm/install.go @@ -183,6 +183,7 @@ func addInstallFlags(cmd *cobra.Command, f *pflag.FlagSet, client *action.Instal f.StringVar(&client.DryRunOption, "dry-run", "", "simulate an install. If --dry-run is set with no option being specified or as '--dry-run=client', it will not attempt cluster connections. Setting '--dry-run=server' allows attempting cluster connections.") f.Lookup("dry-run").NoOptDefVal = "client" f.BoolVar(&client.Force, "force", false, "force resource updates through a replacement strategy") + f.BoolVar(&client.Force3WayMergePatch, "force-3-way-merge-patch", false, "forces use of a 3-way merge patch in cases where a 2-way merge is still the default") f.BoolVar(&client.DisableHooks, "no-hooks", false, "prevent hooks from running during install") f.BoolVar(&client.Replace, "replace", false, "re-use the given name, only if that name is a deleted release which remains in the history. This is unsafe in production") f.DurationVar(&client.Timeout, "timeout", 300*time.Second, "time to wait for any individual Kubernetes operation (like Jobs for hooks)") diff --git a/cmd/helm/rollback.go b/cmd/helm/rollback.go index 3dab4689f..61f3ef9d5 100644 --- a/cmd/helm/rollback.go +++ b/cmd/helm/rollback.go @@ -79,6 +79,7 @@ func newRollbackCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { f.BoolVar(&client.DryRun, "dry-run", false, "simulate a rollback") f.BoolVar(&client.Recreate, "recreate-pods", false, "performs pods restart for the resource if applicable") f.BoolVar(&client.Force, "force", false, "force resource update through delete/recreate if needed") + f.BoolVar(&client.Force3WayMergePatch, "force-3-way-merge-patch", false, "forces use of a 3-way merge patch in cases where a 2-way merge is still the default") f.BoolVar(&client.DisableHooks, "no-hooks", false, "prevent hooks from running during rollback") f.DurationVar(&client.Timeout, "timeout", 300*time.Second, "time to wait for any individual Kubernetes operation (like Jobs for hooks)") f.BoolVar(&client.Wait, "wait", false, "if set, will wait until all Pods, PVCs, Services, and minimum number of Pods of a Deployment, StatefulSet, or ReplicaSet are in a ready state before marking the release as successful. It will wait for as long as --timeout") diff --git a/cmd/helm/upgrade.go b/cmd/helm/upgrade.go index 108550cbf..9fd0f5562 100644 --- a/cmd/helm/upgrade.go +++ b/cmd/helm/upgrade.go @@ -131,6 +131,7 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { instClient.CreateNamespace = createNamespace instClient.ChartPathOptions = client.ChartPathOptions instClient.Force = client.Force + instClient.Force3WayMergePatch = client.Force3WayMergePatch instClient.DryRun = client.DryRun instClient.DryRunOption = client.DryRunOption instClient.DisableHooks = client.DisableHooks @@ -261,6 +262,7 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { f.BoolVar(&client.Recreate, "recreate-pods", false, "performs pods restart for the resource if applicable") f.MarkDeprecated("recreate-pods", "functionality will no longer be updated. Consult the documentation for other methods to recreate pods") f.BoolVar(&client.Force, "force", false, "force resource updates through a replacement strategy") + f.BoolVar(&client.Force3WayMergePatch, "force-3-way-merge-patch", false, "forces use of a 3-way merge patch in cases where a 2-way merge is still the default") f.BoolVar(&client.DisableHooks, "no-hooks", false, "disable pre/post upgrade hooks") f.BoolVar(&client.DisableOpenAPIValidation, "disable-openapi-validation", false, "if set, the upgrade process will not validate rendered templates against the Kubernetes OpenAPI Schema") f.BoolVar(&client.SkipCRDs, "skip-crds", false, "if set, no CRDs will be installed when an upgrade is performed with install flag enabled. By default, CRDs are installed if not already present, when an upgrade is performed with install flag enabled") diff --git a/pkg/action/install.go b/pkg/action/install.go index f0292a0a3..8faf24c82 100644 --- a/pkg/action/install.go +++ b/pkg/action/install.go @@ -69,11 +69,12 @@ type Install struct { ChartPathOptions - ClientOnly bool - Force bool - CreateNamespace bool - DryRun bool - DryRunOption string + ClientOnly bool + Force bool + Force3WayMergePatch bool + CreateNamespace bool + DryRun bool + DryRunOption string // HideSecret can be set to true when DryRun is enabled in order to hide // Kubernetes Secrets in the output. It cannot be used outside of DryRun. HideSecret bool @@ -455,7 +456,7 @@ func (i *Install) performInstall(rel *release.Release, toBeAdopted kube.Resource if len(toBeAdopted) == 0 && len(resources) > 0 { _, err = i.cfg.KubeClient.Create(resources) } else if len(resources) > 0 { - _, err = i.cfg.KubeClient.Update(toBeAdopted, resources, i.Force) + _, err = i.cfg.KubeClient.Update(toBeAdopted, resources, i.Force, i.Force3WayMergePatch) } if err != nil { return rel, err diff --git a/pkg/action/rollback.go b/pkg/action/rollback.go index b0be17d13..922b0d88b 100644 --- a/pkg/action/rollback.go +++ b/pkg/action/rollback.go @@ -35,16 +35,17 @@ import ( type Rollback struct { cfg *Configuration - Version int - Timeout time.Duration - Wait bool - WaitForJobs bool - DisableHooks bool - DryRun bool - Recreate bool // will (if true) recreate pods after a rollback. - Force bool // will (if true) force resource upgrade through uninstall/recreate if needed - CleanupOnFail bool - MaxHistory int // MaxHistory limits the maximum number of revisions saved per release + Version int + Timeout time.Duration + Wait bool + WaitForJobs bool + DisableHooks bool + DryRun bool + Recreate bool // will (if true) recreate pods after a rollback. + Force bool // will (if true) force resource upgrade through uninstall/recreate if needed + Force3WayMergePatch bool + CleanupOnFail bool + MaxHistory int // MaxHistory limits the maximum number of revisions saved per release } // NewRollback creates a new Rollback object with the given configuration. @@ -188,7 +189,7 @@ func (r *Rollback) performRollback(currentRelease, targetRelease *release.Releas if err != nil { return targetRelease, errors.Wrap(err, "unable to set metadata visitor from target release") } - results, err := r.cfg.KubeClient.Update(current, target, r.Force) + results, err := r.cfg.KubeClient.Update(current, target, r.Force, r.Force3WayMergePatch) if err != nil { msg := fmt.Sprintf("Rollback %q failed: %s", targetRelease.Name, err) diff --git a/pkg/action/upgrade.go b/pkg/action/upgrade.go index 15bdae8da..61e751147 100644 --- a/pkg/action/upgrade.go +++ b/pkg/action/upgrade.go @@ -81,6 +81,8 @@ type Upgrade struct { // // This should be used with caution. Force bool + // Forces use of a 3-way merge patch in cases where a 2-way merge is still the default + Force3WayMergePatch bool // ResetValues will reset the values to the chart's built-ins rather than merging with existing. ResetValues bool // ReuseValues will re-use the user's last supplied values. @@ -426,7 +428,7 @@ func (u *Upgrade) releasingUpgrade(c chan<- resultMessage, upgradedRelease *rele u.cfg.Log("upgrade hooks disabled for %s", upgradedRelease.Name) } - results, err := u.cfg.KubeClient.Update(current, target, u.Force) + results, err := u.cfg.KubeClient.Update(current, target, u.Force, u.Force3WayMergePatch) if err != nil { u.cfg.recordRelease(originalRelease) u.reportToPerformUpgrade(c, upgradedRelease, results.Created, err) diff --git a/pkg/kube/client.go b/pkg/kube/client.go index 8fc0958ef..4847f8d9f 100644 --- a/pkg/kube/client.go +++ b/pkg/kube/client.go @@ -46,6 +46,7 @@ import ( "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/jsonmergepatch" "k8s.io/apimachinery/pkg/util/strategicpatch" "k8s.io/apimachinery/pkg/watch" "k8s.io/cli-runtime/pkg/genericclioptions" @@ -385,7 +386,7 @@ func (c *Client) BuildTable(reader io.Reader, validate bool) (ResourceList, erro // occurs, a Result will still be returned with the error, containing all // resource updates, creations, and deletions that were attempted. These can be // used for cleanup or other logging purposes. -func (c *Client) Update(original, target ResourceList, force bool) (*Result, error) { +func (c *Client) Update(original, target ResourceList, force, force3WayMergePatch bool) (*Result, error) { updateErrors := []string{} res := &Result{} @@ -420,7 +421,7 @@ func (c *Client) Update(original, target ResourceList, force bool) (*Result, err return errors.Errorf("no %s with the name %q found", kind, info.Name) } - if err := updateResource(c, info, originalInfo.Object, force); err != nil { + if err := updateResource(c, info, originalInfo.Object, force, force3WayMergePatch); err != nil { c.Log("error updating the resource %q:\n\t %v", info.Name, err) updateErrors = append(updateErrors, err.Error()) } @@ -608,7 +609,7 @@ func deleteResource(info *resource.Info, policy metav1.DeletionPropagation) erro return err } -func createPatch(target *resource.Info, current runtime.Object) ([]byte, types.PatchType, error) { +func createPatch(target *resource.Info, current runtime.Object, force3WayMergePatch bool) ([]byte, types.PatchType, error) { oldData, err := json.Marshal(current) if err != nil { return nil, types.StrategicMergePatchType, errors.Wrap(err, "serializing current configuration") @@ -636,7 +637,7 @@ func createPatch(target *resource.Info, current runtime.Object) ([]byte, types.P // Unstructured objects, such as CRDs, may not have an not registered error // returned from ConvertToVersion. Anything that's unstructured should - // use the jsonpatch.CreateMergePatch. Strategic Merge Patch is not supported + // use the jsonmergepatch.CreateThreeWayJSONMergePatch. Strategic Merge Patch is not supported // on objects like CRDs. _, isUnstructured := versionedObject.(runtime.Unstructured) @@ -645,6 +646,15 @@ func createPatch(target *resource.Info, current runtime.Object) ([]byte, types.P if isUnstructured || isCRD { // fall back to generic JSON merge patch + + // TODO Helm 4: the logic controlled by this flag should become the default. + // The flag can then be removed from all the commands where it is accepted: install, upgrade, & rollback. + if force3WayMergePatch { + patch, err := jsonmergepatch.CreateThreeWayJSONMergePatch(oldData, newData, currentData) + return patch, types.MergePatchType, err + } + + // 2-way merge patch logic that is kept to maintain backwards-compatibility for Helm 3 patch, err := jsonpatch.CreateMergePatch(oldData, newData) return patch, types.MergePatchType, err } @@ -658,7 +668,7 @@ func createPatch(target *resource.Info, current runtime.Object) ([]byte, types.P return patch, types.StrategicMergePatchType, err } -func updateResource(c *Client, target *resource.Info, currentObj runtime.Object, force bool) error { +func updateResource(c *Client, target *resource.Info, currentObj runtime.Object, force, force3WayMergePatch bool) error { var ( obj runtime.Object helper = resource.NewHelper(target.Client, target.Mapping).WithFieldManager(getManagedFieldsManager()) @@ -674,7 +684,7 @@ func updateResource(c *Client, target *resource.Info, currentObj runtime.Object, } c.Log("Replaced %q with kind %s for kind %s", target.Name, currentObj.GetObjectKind().GroupVersionKind().Kind, kind) } else { - patch, patchType, err := createPatch(target, currentObj) + patch, patchType, err := createPatch(target, currentObj, force3WayMergePatch) if err != nil { return errors.Wrap(err, "failed to create patch") } diff --git a/pkg/kube/client_test.go b/pkg/kube/client_test.go index 55aa5d8ed..c83bb0897 100644 --- a/pkg/kube/client_test.go +++ b/pkg/kube/client_test.go @@ -166,7 +166,7 @@ func TestUpdate(t *testing.T) { t.Fatal(err) } - result, err := c.Update(first, second, false) + result, err := c.Update(first, second, false, false) if err != nil { t.Fatal(err) } diff --git a/pkg/kube/fake/fake.go b/pkg/kube/fake/fake.go index 267020d57..3b71c63c3 100644 --- a/pkg/kube/fake/fake.go +++ b/pkg/kube/fake/fake.go @@ -107,11 +107,11 @@ func (f *FailingKubeClient) WatchUntilReady(resources kube.ResourceList, d time. } // Update returns the configured error if set or prints -func (f *FailingKubeClient) Update(r, modified kube.ResourceList, ignoreMe bool) (*kube.Result, error) { +func (f *FailingKubeClient) Update(r, modified kube.ResourceList, force, force3WayMergePatch bool) (*kube.Result, error) { if f.UpdateError != nil { return &kube.Result{}, f.UpdateError } - return f.PrintingKubeClient.Update(r, modified, ignoreMe) + return f.PrintingKubeClient.Update(r, modified, force, force3WayMergePatch) } // Build returns the configured error if set or prints diff --git a/pkg/kube/fake/printer.go b/pkg/kube/fake/printer.go index cc2c84b40..5d87c0d81 100644 --- a/pkg/kube/fake/printer.go +++ b/pkg/kube/fake/printer.go @@ -90,7 +90,7 @@ func (p *PrintingKubeClient) WatchUntilReady(resources kube.ResourceList, _ time } // Update implements KubeClient Update. -func (p *PrintingKubeClient) Update(_, modified kube.ResourceList, _ bool) (*kube.Result, error) { +func (p *PrintingKubeClient) Update(_, modified kube.ResourceList, _, _ bool) (*kube.Result, error) { _, err := io.Copy(p.Out, bufferize(modified)) if err != nil { return nil, err diff --git a/pkg/kube/interface.go b/pkg/kube/interface.go index ce42ed950..a9eb6ca90 100644 --- a/pkg/kube/interface.go +++ b/pkg/kube/interface.go @@ -54,7 +54,7 @@ type Interface interface { // Update updates one or more resources or creates the resource // if it doesn't exist. - Update(original, target ResourceList, force bool) (*Result, error) + Update(original, target ResourceList, force, force3WayMergePatch bool) (*Result, error) // Build creates a resource list from a Reader. //