From 4ab997724807123b210a0fe0681d27c4cbb2e7f2 Mon Sep 17 00:00:00 2001 From: Patrick Seidensal Date: Thu, 24 Oct 2024 16:04:54 +0200 Subject: [PATCH 1/3] Use three way merge for unstructured objects Based on work of wxdao in the same issue, supersedes #9938. Basically just adds tests. closes #9937 Signed-off-by: Patrick Seidensal --- cmd/helm/root.go | 2 + pkg/action/action.go | 6 ++ pkg/cli/environment.go | 6 ++ pkg/kube/client.go | 26 ++++++- pkg/kube/client_test.go | 153 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 190 insertions(+), 3 deletions(-) diff --git a/cmd/helm/root.go b/cmd/helm/root.go index e2656ae90..45879b839 100644 --- a/cmd/helm/root.go +++ b/cmd/helm/root.go @@ -161,6 +161,8 @@ func newRootCmd(actionConfig *action.Configuration, out io.Writer, args []string } actionConfig.RegistryClient = registryClient + actionConfig.ThreeWayMergeForUnstructured = settings.ThreeWayMergeForUnstructured + // Add subcommands cmd.AddCommand( // chart commands diff --git a/pkg/action/action.go b/pkg/action/action.go index 8418a4c27..638f96d30 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -94,6 +94,11 @@ type Configuration struct { // Capabilities describes the capabilities of the Kubernetes cluster. Capabilities *chartutil.Capabilities + // ThreeWayMergeForUnstructured controls whether to use three way merge + // patch for unstructured objects (custom resource, custom definitions, + // etc). + ThreeWayMergeForUnstructured bool + Log func(string, ...interface{}) } @@ -373,6 +378,7 @@ func (cfg *Configuration) recordRelease(r *release.Release) { func (cfg *Configuration) Init(getter genericclioptions.RESTClientGetter, namespace, helmDriver string, log DebugLog) error { kc := kube.New(getter) kc.Log = log + kc.ThreeWayMergeForUnstructured = cfg.ThreeWayMergeForUnstructured lazyClient := &lazyClient{ namespace: namespace, diff --git a/pkg/cli/environment.go b/pkg/cli/environment.go index 3f2dc00b2..ef0e8d53d 100644 --- a/pkg/cli/environment.go +++ b/pkg/cli/environment.go @@ -89,6 +89,10 @@ type EnvSettings struct { BurstLimit int // QPS is queries per second which may be used to avoid throttling. QPS float32 + // ThreeWayMergeForUnstructured controls whether to use three way merge + // patch for unstructured objects (custom resource, custom definitions, + // etc). + ThreeWayMergeForUnstructured bool } func New() *EnvSettings { @@ -111,6 +115,7 @@ func New() *EnvSettings { QPS: envFloat32Or("HELM_QPS", defaultQPS), } env.Debug, _ = strconv.ParseBool(os.Getenv("HELM_DEBUG")) + env.ThreeWayMergeForUnstructured, _ = strconv.ParseBool(os.Getenv("HELM_THREE_WAY_MERGE_FOR_UNSTRUCTURED")) // bind to kubernetes config flags config := &genericclioptions.ConfigFlags{ @@ -160,6 +165,7 @@ func (s *EnvSettings) AddFlags(fs *pflag.FlagSet) { fs.StringVar(&s.RepositoryCache, "repository-cache", s.RepositoryCache, "path to the directory containing cached repository indexes") fs.IntVar(&s.BurstLimit, "burst-limit", s.BurstLimit, "client-side default throttling limit") fs.Float32Var(&s.QPS, "qps", s.QPS, "queries per second used when communicating with the Kubernetes API, not including bursting") + fs.BoolVar(&s.ThreeWayMergeForUnstructured, "three-way-merge-for-unstructured", s.ThreeWayMergeForUnstructured, "use a three way merge patch for unstructured objects (custom resources, custom resource definitions, etc.)") } func envOr(name, def string) string { diff --git a/pkg/kube/client.go b/pkg/kube/client.go index 79aa98535..178473289 100644 --- a/pkg/kube/client.go +++ b/pkg/kube/client.go @@ -46,6 +46,8 @@ 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/mergepatch" "k8s.io/apimachinery/pkg/util/strategicpatch" "k8s.io/apimachinery/pkg/watch" "k8s.io/cli-runtime/pkg/genericclioptions" @@ -83,6 +85,11 @@ type Client struct { // Namespace allows to bypass the kubeconfig file for the choice of the namespace Namespace string + // ThreeWayMergeForUnstructured controls whether to use three way merge + // patch for unstructured objects (custom resource, custom definitions, + // etc). + ThreeWayMergeForUnstructured bool + kubeClient *kubernetes.Clientset } @@ -617,7 +624,7 @@ func deleteResource(info *resource.Info, policy metav1.DeletionPropagation) erro }) } -func createPatch(target *resource.Info, current runtime.Object) ([]byte, types.PatchType, error) { +func createPatch(target *resource.Info, current runtime.Object, threeWayMergeForUnstructured bool) ([]byte, types.PatchType, error) { oldData, err := json.Marshal(current) if err != nil { return nil, types.StrategicMergePatchType, errors.Wrap(err, "serializing current configuration") @@ -645,7 +652,7 @@ func createPatch(target *resource.Info, current runtime.Object) ([]byte, types.P // Unstructured objects, such as CRDs, may not have a not registered error // returned from ConvertToVersion. Anything that's unstructured should - // use the jsonpatch.CreateMergePatch. Strategic Merge Patch is not supported + // use generic JSON merge patch. Strategic Merge Patch is not supported // on objects like CRDs. _, isUnstructured := versionedObject.(runtime.Unstructured) @@ -653,6 +660,19 @@ func createPatch(target *resource.Info, current runtime.Object) ([]byte, types.P _, isCRD := versionedObject.(*apiextv1beta1.CustomResourceDefinition) if isUnstructured || isCRD { + if threeWayMergeForUnstructured { + // from https://github.com/kubernetes/kubectl/blob/b83b2ec7d15f286720bccf7872b5c72372cb8e80/pkg/cmd/apply/patcher.go#L129 + preconditions := []mergepatch.PreconditionFunc{ + mergepatch.RequireKeyUnchanged("apiVersion"), + mergepatch.RequireKeyUnchanged("kind"), + mergepatch.RequireMetadataKeyUnchanged("name"), + } + patch, err := jsonmergepatch.CreateThreeWayJSONMergePatch(oldData, newData, currentData, preconditions...) + if err != nil && mergepatch.IsPreconditionFailed(err) { + err = fmt.Errorf("%w: at least one field was changed: apiVersion, kind or name", err) + } + return patch, types.MergePatchType, err + } // fall back to generic JSON merge patch patch, err := jsonpatch.CreateMergePatch(oldData, newData) return patch, types.MergePatchType, err @@ -683,7 +703,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, c.ThreeWayMergeForUnstructured) 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 f2d6bcb59..fb851947a 100644 --- a/pkg/kube/client_test.go +++ b/pkg/kube/client_test.go @@ -24,9 +24,15 @@ import ( "testing" "time" + corev1 "k8s.io/api/core/v1" v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + jsonserializer "k8s.io/apimachinery/pkg/runtime/serializer/json" + "k8s.io/apimachinery/pkg/types" "k8s.io/cli-runtime/pkg/resource" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest/fake" @@ -682,6 +688,153 @@ func TestReal(t *testing.T) { } } +type createPatchTestCase struct { + name string + + // The target state. + target *unstructured.Unstructured + // The current state as it exists in the release. + current *unstructured.Unstructured + // The actual state as it exists in the cluster. + actual *unstructured.Unstructured + + threeWayMergeForUnstructured bool + // The patch is supposed to transfer the current state to the target state, + // thereby preserving the actual state, wherever possible. + expectedPatch string + expectedPatchType types.PatchType +} + +func (c createPatchTestCase) run(t *testing.T) { + scheme := runtime.NewScheme() + corev1.AddToScheme(scheme) + encoder := jsonserializer.NewSerializerWithOptions( + jsonserializer.DefaultMetaFactory, scheme, scheme, jsonserializer.SerializerOptions{ + Yaml: false, Pretty: false, Strict: true, + }, + ) + objBody := func(obj runtime.Object) io.ReadCloser { + return io.NopCloser(bytes.NewReader([]byte(runtime.EncodeOrDie(encoder, obj)))) + } + header := make(http.Header) + header.Set("Content-Type", runtime.ContentTypeJSON) + restClient := &fake.RESTClient{ + NegotiatedSerializer: unstructuredSerializer, + Resp: &http.Response{ + StatusCode: 200, + Body: objBody(c.actual), + Header: header, + }, + } + + targetInfo := &resource.Info{ + Client: restClient, + Namespace: "default", + Name: "test-obj", + Object: c.target, + Mapping: &meta.RESTMapping{ + Resource: schema.GroupVersionResource{ + Group: "crd.com", + Version: "v1", + Resource: "datas", + }, + Scope: meta.RESTScopeNamespace, + }, + } + + patch, patchType, err := createPatch(targetInfo, c.current, c.threeWayMergeForUnstructured) + if err != nil { + t.Fatalf("Failed to create patch: %v", err) + } + + if c.expectedPatch != string(patch) { + t.Errorf("Unexpected patch.\nTarget:\n%s\nCurrent:\n%s\nActual:\n%s\n\nExpected:\n%s\nGot:\n%s", + c.target, + c.current, + c.actual, + c.expectedPatch, + string(patch), + ) + } + + if patchType != types.MergePatchType { + t.Errorf("Expected patch type %s, got %s", types.MergePatchType, patchType) + } +} + +func newTestCustomResourceData(metadata map[string]string, spec map[string]interface{}) *unstructured.Unstructured { + if metadata == nil { + metadata = make(map[string]string) + } + if _, ok := metadata["name"]; !ok { + metadata["name"] = "test-obj" + } + if _, ok := metadata["namespace"]; !ok { + metadata["namespace"] = "default" + } + o := map[string]interface{}{ + "apiVersion": "crd.com/v1", + "kind": "Data", + "metadata": metadata, + } + if len(spec) > 0 { + o["spec"] = spec + } + return &unstructured.Unstructured{ + Object: o, + } +} + +func TestCreatePatchCustomResourceMetadata(t *testing.T) { + target := newTestCustomResourceData(map[string]string{ + "meta.helm.sh/release-name": "foo-simple", + "meta.helm.sh/release-namespace": "default", + "objectset.rio.cattle.io/id": "default-foo-simple", + }, nil) + testCase := createPatchTestCase{ + name: "take ownership of resource", + target: target, + current: target, + actual: newTestCustomResourceData(nil, map[string]interface{}{ + "color": "red", + }), + threeWayMergeForUnstructured: true, + expectedPatch: `{"metadata":{"meta.helm.sh/release-name":"foo-simple","meta.helm.sh/release-namespace":"default","objectset.rio.cattle.io/id":"default-foo-simple"}}`, + expectedPatchType: types.MergePatchType, + } + t.Run(testCase.name, testCase.run) + + // Previous behavior. + testCase.threeWayMergeForUnstructured = false + testCase.expectedPatch = `{}` + t.Run(testCase.name, testCase.run) +} + +func TestCreatePatchCustomResourceSpec(t *testing.T) { + target := newTestCustomResourceData(nil, map[string]interface{}{ + "color": "red", + "size": "large", + }) + testCase := createPatchTestCase{ + name: "merge with spec of existing custom resource", + target: target, + current: target, + actual: newTestCustomResourceData(nil, map[string]interface{}{ + "color": "red", + "weight": "heavy", + }), + threeWayMergeForUnstructured: true, + expectedPatch: `{"spec":{"size":"large"}}`, + expectedPatchType: types.MergePatchType, + } + t.Run(testCase.name, testCase.run) + + // Previous behavior. + testCase.threeWayMergeForUnstructured = false + testCase.expectedPatch = `{}` + t.Run(testCase.name, testCase.run) +} + const testServiceManifest = ` kind: Service apiVersion: v1 From 26f843ce80604324fff23241dc92fd6d03a6b322 Mon Sep 17 00:00:00 2001 From: Patrick Seidensal Date: Wed, 6 Nov 2024 10:26:15 +0100 Subject: [PATCH 2/3] Don't use environment and shorten CLI flag closes #9937 Signed-off-by: Patrick Seidensal --- cmd/helm/install.go | 1 + cmd/helm/rollback.go | 1 + cmd/helm/root.go | 2 -- cmd/helm/upgrade.go | 1 + pkg/action/action.go | 6 ------ pkg/action/install.go | 6 ++++-- pkg/action/rollback.go | 4 +++- pkg/action/upgrade.go | 4 +++- pkg/cli/environment.go | 6 ------ pkg/kube/client.go | 17 ++++++----------- pkg/kube/client_test.go | 2 +- pkg/kube/fake/fake.go | 4 ++-- pkg/kube/fake/printer.go | 2 +- pkg/kube/interface.go | 2 +- 14 files changed, 24 insertions(+), 34 deletions(-) diff --git a/cmd/helm/install.go b/cmd/helm/install.go index ec651140c..c1530db0f 100644 --- a/cmd/helm/install.go +++ b/cmd/helm/install.go @@ -207,6 +207,7 @@ func addInstallFlags(cmd *cobra.Command, f *pflag.FlagSet, client *action.Instal f.BoolVar(&client.EnableDNS, "enable-dns", false, "enable DNS lookups when rendering templates") f.BoolVar(&client.HideNotes, "hide-notes", false, "if set, do not show notes in install output. Does not affect presence in chart metadata") f.BoolVar(&client.TakeOwnership, "take-ownership", false, "if set, install will ignore the check for helm annotations and take ownership of the existing resources") + f.BoolVar(&client.ThreeWayMergeForCustomResources, "three-way-merge-for-custom-resources", false, "also considers the state of custom resources and custom resource definitions in the cluster when adopting resources") addValueOptionsFlags(f, valueOpts) addChartPathOptionsFlags(f, &client.ChartPathOptions) diff --git a/cmd/helm/rollback.go b/cmd/helm/rollback.go index a65f30a1f..507fc9328 100644 --- a/cmd/helm/rollback.go +++ b/cmd/helm/rollback.go @@ -85,6 +85,7 @@ func newRollbackCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { f.BoolVar(&client.WaitForJobs, "wait-for-jobs", false, "if set and --wait enabled, will wait until all Jobs have been completed before marking the release as successful. It will wait for as long as --timeout") f.BoolVar(&client.CleanupOnFail, "cleanup-on-fail", false, "allow deletion of new resources created in this rollback when rollback fails") f.IntVar(&client.MaxHistory, "history-max", settings.MaxHistory, "limit the maximum number of revisions saved per release. Use 0 for no limit") + f.BoolVar(&client.ThreeWayMergeForCustomResources, "three-way-merge-for-custom-resources", false, "also considers the state of custom resources and custom resource definitions in the cluster when upgrading (or adopting) resources") return cmd } diff --git a/cmd/helm/root.go b/cmd/helm/root.go index 45879b839..e2656ae90 100644 --- a/cmd/helm/root.go +++ b/cmd/helm/root.go @@ -161,8 +161,6 @@ func newRootCmd(actionConfig *action.Configuration, out io.Writer, args []string } actionConfig.RegistryClient = registryClient - actionConfig.ThreeWayMergeForUnstructured = settings.ThreeWayMergeForUnstructured - // Add subcommands cmd.AddCommand( // chart commands diff --git a/cmd/helm/upgrade.go b/cmd/helm/upgrade.go index 7b4267894..41625274d 100644 --- a/cmd/helm/upgrade.go +++ b/cmd/helm/upgrade.go @@ -291,6 +291,7 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { f.BoolVar(&client.DependencyUpdate, "dependency-update", false, "update dependencies if they are missing before installing the chart") f.BoolVar(&client.EnableDNS, "enable-dns", false, "enable DNS lookups when rendering templates") f.BoolVar(&client.TakeOwnership, "take-ownership", false, "if set, upgrade will ignore the check for helm annotations and take ownership of the existing resources") + f.BoolVar(&client.ThreeWayMergeForCustomResources, "three-way-merge-for-custom-resources", false, "also considers the state of custom resources in the cluster when upgrading (or adopting) resources") addChartPathOptionsFlags(f, &client.ChartPathOptions) addValueOptionsFlags(f, valueOpts) bindOutputFlag(cmd, &outfmt) diff --git a/pkg/action/action.go b/pkg/action/action.go index 638f96d30..8418a4c27 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -94,11 +94,6 @@ type Configuration struct { // Capabilities describes the capabilities of the Kubernetes cluster. Capabilities *chartutil.Capabilities - // ThreeWayMergeForUnstructured controls whether to use three way merge - // patch for unstructured objects (custom resource, custom definitions, - // etc). - ThreeWayMergeForUnstructured bool - Log func(string, ...interface{}) } @@ -378,7 +373,6 @@ func (cfg *Configuration) recordRelease(r *release.Release) { func (cfg *Configuration) Init(getter genericclioptions.RESTClientGetter, namespace, helmDriver string, log DebugLog) error { kc := kube.New(getter) kc.Log = log - kc.ThreeWayMergeForUnstructured = cfg.ThreeWayMergeForUnstructured lazyClient := &lazyClient{ namespace: namespace, diff --git a/pkg/action/install.go b/pkg/action/install.go index 490e18963..e1c957ca7 100644 --- a/pkg/action/install.go +++ b/pkg/action/install.go @@ -112,7 +112,9 @@ type Install struct { UseReleaseName bool // TakeOwnership will ignore the check for helm annotations and take ownership of the resources. TakeOwnership bool - PostRenderer postrender.PostRenderer + // Also considers the state of custom resources and custom resource definitions in the cluster when adopting resources. + ThreeWayMergeForCustomResources bool + PostRenderer postrender.PostRenderer // Lock to control raceconditions when the process receives a SIGTERM Lock sync.Mutex } @@ -455,7 +457,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.ThreeWayMergeForCustomResources) } if err != nil { return rel, err diff --git a/pkg/action/rollback.go b/pkg/action/rollback.go index 12dee35ce..a7d6944f7 100644 --- a/pkg/action/rollback.go +++ b/pkg/action/rollback.go @@ -45,6 +45,8 @@ type Rollback struct { 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 + // Also considers the state of custom resources and custom resource definitions in the cluster when upgrading (or adopting) resources. + ThreeWayMergeForCustomResources bool } // NewRollback creates a new Rollback object with the given configuration. @@ -188,7 +190,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.ThreeWayMergeForCustomResources) 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 f93bf29b1..a50c007de 100644 --- a/pkg/action/upgrade.go +++ b/pkg/action/upgrade.go @@ -119,6 +119,8 @@ type Upgrade struct { EnableDNS bool // TakeOwnership will skip the check for helm annotations and adopt all existing resources. TakeOwnership bool + // Also considers the state of custom resources and custom resource definitions in the cluster when upgrading (or adopting) resources. + ThreeWayMergeForCustomResources bool } type resultMessage struct { @@ -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.ThreeWayMergeForCustomResources) if err != nil { u.cfg.recordRelease(originalRelease) u.reportToPerformUpgrade(c, upgradedRelease, results.Created, err) diff --git a/pkg/cli/environment.go b/pkg/cli/environment.go index ef0e8d53d..3f2dc00b2 100644 --- a/pkg/cli/environment.go +++ b/pkg/cli/environment.go @@ -89,10 +89,6 @@ type EnvSettings struct { BurstLimit int // QPS is queries per second which may be used to avoid throttling. QPS float32 - // ThreeWayMergeForUnstructured controls whether to use three way merge - // patch for unstructured objects (custom resource, custom definitions, - // etc). - ThreeWayMergeForUnstructured bool } func New() *EnvSettings { @@ -115,7 +111,6 @@ func New() *EnvSettings { QPS: envFloat32Or("HELM_QPS", defaultQPS), } env.Debug, _ = strconv.ParseBool(os.Getenv("HELM_DEBUG")) - env.ThreeWayMergeForUnstructured, _ = strconv.ParseBool(os.Getenv("HELM_THREE_WAY_MERGE_FOR_UNSTRUCTURED")) // bind to kubernetes config flags config := &genericclioptions.ConfigFlags{ @@ -165,7 +160,6 @@ func (s *EnvSettings) AddFlags(fs *pflag.FlagSet) { fs.StringVar(&s.RepositoryCache, "repository-cache", s.RepositoryCache, "path to the directory containing cached repository indexes") fs.IntVar(&s.BurstLimit, "burst-limit", s.BurstLimit, "client-side default throttling limit") fs.Float32Var(&s.QPS, "qps", s.QPS, "queries per second used when communicating with the Kubernetes API, not including bursting") - fs.BoolVar(&s.ThreeWayMergeForUnstructured, "three-way-merge-for-unstructured", s.ThreeWayMergeForUnstructured, "use a three way merge patch for unstructured objects (custom resources, custom resource definitions, etc.)") } func envOr(name, def string) string { diff --git a/pkg/kube/client.go b/pkg/kube/client.go index 178473289..12dea58c8 100644 --- a/pkg/kube/client.go +++ b/pkg/kube/client.go @@ -85,11 +85,6 @@ type Client struct { // Namespace allows to bypass the kubeconfig file for the choice of the namespace Namespace string - // ThreeWayMergeForUnstructured controls whether to use three way merge - // patch for unstructured objects (custom resource, custom definitions, - // etc). - ThreeWayMergeForUnstructured bool - kubeClient *kubernetes.Clientset } @@ -393,7 +388,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 bool, threeWayMergeForCRs bool) (*Result, error) { updateErrors := []string{} res := &Result{} @@ -428,7 +423,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, threeWayMergeForCRs); err != nil { c.Log("error updating the resource %q:\n\t %v", info.Name, err) updateErrors = append(updateErrors, err.Error()) } @@ -624,7 +619,7 @@ func deleteResource(info *resource.Info, policy metav1.DeletionPropagation) erro }) } -func createPatch(target *resource.Info, current runtime.Object, threeWayMergeForUnstructured bool) ([]byte, types.PatchType, error) { +func createPatch(target *resource.Info, current runtime.Object, threeWayMergeForCRs bool) ([]byte, types.PatchType, error) { oldData, err := json.Marshal(current) if err != nil { return nil, types.StrategicMergePatchType, errors.Wrap(err, "serializing current configuration") @@ -660,7 +655,7 @@ func createPatch(target *resource.Info, current runtime.Object, threeWayMergeFor _, isCRD := versionedObject.(*apiextv1beta1.CustomResourceDefinition) if isUnstructured || isCRD { - if threeWayMergeForUnstructured { + if threeWayMergeForCRs { // from https://github.com/kubernetes/kubectl/blob/b83b2ec7d15f286720bccf7872b5c72372cb8e80/pkg/cmd/apply/patcher.go#L129 preconditions := []mergepatch.PreconditionFunc{ mergepatch.RequireKeyUnchanged("apiVersion"), @@ -687,7 +682,7 @@ func createPatch(target *resource.Info, current runtime.Object, threeWayMergeFor 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 bool, threeWayMergeForCRs bool) error { var ( obj runtime.Object helper = resource.NewHelper(target.Client, target.Mapping).WithFieldManager(getManagedFieldsManager()) @@ -703,7 +698,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, c.ThreeWayMergeForUnstructured) + patch, patchType, err := createPatch(target, currentObj, threeWayMergeForCRs) 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 fb851947a..94a78f77e 100644 --- a/pkg/kube/client_test.go +++ b/pkg/kube/client_test.go @@ -282,7 +282,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 a60d9f34e..5808ff9b9 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, ignoreMe bool, threeWayMergeForCRs 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, ignoreMe, threeWayMergeForCRs) } // Build returns the configured error if set or prints diff --git a/pkg/kube/fake/printer.go b/pkg/kube/fake/printer.go index b38e8c084..01f0ab616 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, _ 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..25e3bdc3f 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 bool, threeWayMergeForCRs bool) (*Result, error) // Build creates a resource list from a Reader. // From 03fa004b285ee63e3a34d6ec52e574f79ca6cd34 Mon Sep 17 00:00:00 2001 From: Patrick Seidensal Date: Tue, 26 Nov 2024 15:44:16 +0100 Subject: [PATCH 3/3] Add comments to remove functionality in Helm 4 closes #9937 Signed-off-by: Patrick Seidensal --- pkg/action/install.go | 1 + pkg/action/rollback.go | 3 ++- pkg/action/upgrade.go | 1 + 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/action/install.go b/pkg/action/install.go index e1c957ca7..15ed0a055 100644 --- a/pkg/action/install.go +++ b/pkg/action/install.go @@ -113,6 +113,7 @@ type Install struct { // TakeOwnership will ignore the check for helm annotations and take ownership of the resources. TakeOwnership bool // Also considers the state of custom resources and custom resource definitions in the cluster when adopting resources. + // TODO Helm 4: Remove this config and always merge custom resources ThreeWayMergeForCustomResources bool PostRenderer postrender.PostRenderer // Lock to control raceconditions when the process receives a SIGTERM diff --git a/pkg/action/rollback.go b/pkg/action/rollback.go index a7d6944f7..8e2e37fed 100644 --- a/pkg/action/rollback.go +++ b/pkg/action/rollback.go @@ -45,7 +45,8 @@ type Rollback struct { 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 - // Also considers the state of custom resources and custom resource definitions in the cluster when upgrading (or adopting) resources. + // Also considers the state of custom resources and custom resource definitions in the cluster when downgrading resources. + // TODO Helm 4: Remove this config and always merge custom resources ThreeWayMergeForCustomResources bool } diff --git a/pkg/action/upgrade.go b/pkg/action/upgrade.go index a50c007de..a2d9b84ea 100644 --- a/pkg/action/upgrade.go +++ b/pkg/action/upgrade.go @@ -120,6 +120,7 @@ type Upgrade struct { // TakeOwnership will skip the check for helm annotations and adopt all existing resources. TakeOwnership bool // Also considers the state of custom resources and custom resource definitions in the cluster when upgrading (or adopting) resources. + // TODO Helm 4: Remove this config and always merge custom resources ThreeWayMergeForCustomResources bool }