Merge 03fa004b28 into a780fa60ed

cmd/helm/install.go

@@ -210,6 +210,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)
 

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
 }

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)

pkg/action/install.go

@@ -112,7 +112,10 @@ 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.
+	// 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
 	Lock sync.Mutex
 }
@@ -459,7 +462,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

pkg/action/rollback.go

@@ -45,6 +45,9 @@ 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 downgrading resources.
+	// TODO Helm 4: Remove this config and always merge custom resources
+	ThreeWayMergeForCustomResources bool
 }
 
 // NewRollback creates a new Rollback object with the given configuration.
@@ -188,7 +191,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)

pkg/action/upgrade.go

@@ -119,6 +119,9 @@ 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.
+	// TODO Helm 4: Remove this config and always merge custom resources
+	ThreeWayMergeForCustomResources bool
 }
 
 type resultMessage struct {
@@ -426,7 +429,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)

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"
@@ -386,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{}
 
@@ -421,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())
 		}
@@ -617,7 +619,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, threeWayMergeForCRs 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 +647,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 +655,19 @@ func createPatch(target *resource.Info, current runtime.Object) ([]byte, types.P
 	_, isCRD := versionedObject.(*apiextv1beta1.CustomResourceDefinition)
 
 	if isUnstructured || isCRD {
+		if threeWayMergeForCRs {
+			// 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
@@ -667,7 +682,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 bool, threeWayMergeForCRs bool) error {
 	var (
 		obj    runtime.Object
 		helper = resource.NewHelper(target.Client, target.Mapping).WithFieldManager(getManagedFieldsManager())
@@ -683,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)
+		patch, patchType, err := createPatch(target, currentObj, threeWayMergeForCRs)
 		if err != nil {
 			return errors.Wrap(err, "failed to create patch")
 		}

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"
@@ -276,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)
 	}
@@ -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

pkg/kube/fake/fake.go

@@ -105,11 +105,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

pkg/kube/fake/printer.go

@@ -89,7 +89,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

pkg/kube/interface.go

@@ -53,7 +53,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.
 	//