Fix diff logic for server-side apply mode (#1679)

CHANGELOG.md

@@ -1,6 +1,7 @@
 ## HEAD (Unreleased)
 
 - [sdk/python] Fix wait for metadata in `yaml._parse_yaml_object`. (https://github.com/pulumi/pulumi-kubernetes/pull/1675)
+- Fix diff logic for server-side apply mode (https://github.com/pulumi/pulumi-kubernetes/pull/1679)
 
 ## 3.6.0 (Auguest 4, 2021)
 

provider/go.mod

@@ -7,7 +7,7 @@ require (
 	github.com/evanphx/json-patch v4.11.0+incompatible
 	github.com/golang/protobuf v1.5.2
 	github.com/googleapis/gnostic v0.5.1
-	github.com/imdario/mergo v0.3.11
+	github.com/imdario/mergo v0.3.12
 	github.com/onsi/ginkgo v1.12.0 // indirect
 	github.com/onsi/gomega v1.9.0 // indirect
 	github.com/pkg/errors v0.9.1

provider/go.sum

@@ -634,8 +634,9 @@ github.com/ianlancetaylor/demangle v0.0.0-20181102032728-5e5cf60278f6/go.mod h1:
 github.com/ianlancetaylor/demangle v0.0.0-20200824232613-28f6c0f3b639/go.mod h1:aSSvb/t6k1mPoxDqO4vJh6VOCGPwU4O0C2/Eqndh1Sc=
 github.com/ijc/Gotty v0.0.0-20170406111628-a8b993ba6abd/go.mod h1:3LVOLeyx9XVvwPgrt2be44XgSqndprz1G18rSk8KD84=
 github.com/imdario/mergo v0.3.5/go.mod h1:2EnlNZ0deacrJVfApfmtdGgDfMuh/nq6Ok1EcJh5FfA=
-github.com/imdario/mergo v0.3.11 h1:3tnifQM4i+fbajXKBHXWEH+KvNHqojZ778UH75j3bGA=
 github.com/imdario/mergo v0.3.11/go.mod h1:jmQim1M+e3UYxmgPu/WyfjB3N3VflVyUjjjwH0dnCYA=
+github.com/imdario/mergo v0.3.12 h1:b6R2BslTbIEToALKP7LxUvijTsNI9TAe80pLWN2g/HU=
+github.com/imdario/mergo v0.3.12/go.mod h1:jmQim1M+e3UYxmgPu/WyfjB3N3VflVyUjjjwH0dnCYA=
 github.com/inconshreveable/mousetrap v1.0.0 h1:Z8tu5sraLXCXIcARxBp/8cbvlwVa7Z1NHg9XEKhtSvM=
 github.com/inconshreveable/mousetrap v1.0.0/go.mod h1:PxqpIevigyE2G7u3NXJIT2ANytuPF1OarO4DADm73n8=
 github.com/influxdata/influxdb1-client v0.0.0-20191209144304-8bf82d3c094d/go.mod h1:qj24IKcXYK6Iy9ceXlo3Tc+vtHo9lIhSX5JddghvEPo=

provider/pkg/provider/provider.go

@@ -33,6 +33,7 @@ import (
 	jsonpatch "github.com/evanphx/json-patch"
 	pbempty "github.com/golang/protobuf/ptypes/empty"
 	structpb "github.com/golang/protobuf/ptypes/struct"
+	"github.com/imdario/mergo"
 	pkgerrors "github.com/pkg/errors"
 	"github.com/pulumi/pulumi-kubernetes/provider/v3/pkg/await"
 	"github.com/pulumi/pulumi-kubernetes/provider/v3/pkg/await/states"
@@ -1283,59 +1284,16 @@ func (k *kubeProvider) Diff(
 	}
 
 	var patch []byte
-	var isClientSidePatch bool
-	var patchBase *unstructured.Unstructured
+	var patchBase map[string]interface{}
 
-	// Try to compute a server-side patch. Returns true iff the operation succeeded.
-	tryServerSidePatch := func() bool {
-		// If the resource's GVK changed, so compute patch using inputs.
-		if oldInputs.GroupVersionKind().String() != gvk.String() {
-			return false
-		}
-		// If we can't dry-run the new GVK, computed the patch using inputs.
-		if !k.supportsDryRun(gvk) {
-			return false
-		}
-		// TODO: Skipping server-side diff for resources with computed values is a hack. We will want to address this
-		// more granularly so that previews are as accurate as possible, but this is an easy workaround for a critical
-		// bug.
-		if hasComputedValue(newInputs) || hasComputedValue(oldInputs) {
-			return false
-		}
-
-		patch, patchBase, err = k.serverSidePatch(oldInputs, newInputs)
-		if k.isDryRunDisabledError(err) {
-			return false
-		}
-		if se, isStatusError := err.(*errors.StatusError); isStatusError {
-			// If the resource field is immutable.
-			if se.Status().Code == http.StatusUnprocessableEntity ||
-				strings.Contains(se.ErrStatus.Message, "field is immutable") {
-				return false
-			}
-		}
-
-		// The server-side patch succeeded.
-		return true
-	}
-	if !tryServerSidePatch() {
-		isClientSidePatch = true
-		patch, err = k.inputPatch(oldInputs, newInputs)
-		patchBase = oldInputs
-	}
-
-	if isClientSidePatch {
-		logger.V(1).Infof("calculated diffs for %s/%s using inputs only", newInputs.GetNamespace(), newInputs.GetName())
-
-	} else {
-		logger.V(1).Infof("calculated diffs for %s/%s using dry-run", newInputs.GetNamespace(), newInputs.GetName())
-	}
+	// Always compute a client-side patch.
+	patch, err = k.inputPatch(oldInputs, newInputs)
 	if err != nil {
 		return nil, pkgerrors.Wrapf(
-			err, "Failed to check for changes in resource %s/%s because of an error computing the JSON patch "+
-				"describing the resource changes",
-			newInputs.GetNamespace(), newInputs.GetName())
+			err, "Failed to check for changes in resource %s/%s", newInputs.GetNamespace(), newInputs.GetName())
 	}
+	patchBase = oldInputs.Object
+
 	patchObj := map[string]interface{}{}
 	if err = json.Unmarshal(patch, &patchObj); err != nil {
 		return nil, pkgerrors.Wrapf(
@@ -1344,6 +1302,33 @@ func (k *kubeProvider) Diff(
 			newInputs.GetNamespace(), newInputs.GetName())
 	}
 
+	// Try to compute a server-side patch.
+	ssPatch, ssPatchBase, ssPatchOk := k.tryServerSidePatch(oldInputs, newInputs, gvk)
+
+	// If the server-side patch succeeded, then merge that patch into the client-side patch and override any conflicts
+	// with the server-side values.
+	if ssPatchOk {
+		logger.V(1).Infof("calculated diffs for %s/%s using dry-run and inputs", newInputs.GetNamespace(), newInputs.GetName())
+		err = mergo.Merge(&patchBase, ssPatchBase, mergo.WithOverride)
+		if err != nil {
+			return nil, err
+		}
+
+		ssPatchObj := map[string]interface{}{}
+		if err = json.Unmarshal(ssPatch, &ssPatchObj); err != nil {
+			return nil, pkgerrors.Wrapf(
+				err, "Failed to check for changes in resource %s/%s because of an error serializing "+
+					"the JSON patch describing resource changes",
+				newInputs.GetNamespace(), newInputs.GetName())
+		}
+		err = mergo.Merge(&patchObj, ssPatchObj, mergo.WithOverride)
+		if err != nil {
+			return nil, err
+		}
+	} else {
+		logger.V(1).Infof("calculated diffs for %s/%s using inputs only", newInputs.GetNamespace(), newInputs.GetName())
+	}
+
 	// Pack up PB, ship response back.
 	hasChanges := pulumirpc.DiffResponse_DIFF_NONE
 
@@ -1356,16 +1341,14 @@ func (k *kubeProvider) Diff(
 			changes = append(changes, k)
 		}
 
-		if detailedDiff, err = convertPatchToDiff(patchObj, patchBase.Object, newInputs.Object, oldInputs.Object, gvk); err != nil {
+		if detailedDiff, err = convertPatchToDiff(patchObj, patchBase, newInputs.Object, oldInputs.Object, gvk); err != nil {
 			return nil, pkgerrors.Wrapf(
 				err, "Failed to check for changes in resource %s/%s because of an error "+
 					"converting JSON patch describing resource changes to a diff",
 				newInputs.GetNamespace(), newInputs.GetName())
 		}
-		if isClientSidePatch {
-			for _, v := range detailedDiff {
-				v.InputDiff = true
-			}
+		for _, v := range detailedDiff {
+			v.InputDiff = true
 		}
 
 		for k, v := range detailedDiff {
@@ -2188,9 +2171,8 @@ func (k *kubeProvider) readLiveObject(obj *unstructured.Unstructured) (*unstruct
 	return rc.Get(context.TODO(), obj.GetName(), metav1.GetOptions{})
 }
 
-func (k *kubeProvider) serverSidePatch(
-	oldInputs, newInputs *unstructured.Unstructured,
-) ([]byte, *unstructured.Unstructured, error) {
+func (k *kubeProvider) serverSidePatch(oldInputs, newInputs *unstructured.Unstructured,
+) ([]byte, map[string]interface{}, error) {
 
 	client, err := k.clientSet.ResourceClient(oldInputs.GroupVersionKind(), oldInputs.GetNamespace())
 	if err != nil {
@@ -2256,7 +2238,7 @@ func (k *kubeProvider) serverSidePatch(
 		return nil, nil, err
 	}
 
-	return patch, liveObject, nil
+	return patch, liveObject.Object, nil
 }
 
 // inputPatch calculates a patch on the client-side by comparing old inputs to the current inputs.
@@ -2301,6 +2283,40 @@ func (k *kubeProvider) isDryRunDisabledError(err error) bool {
 			strings.Contains(se.Status().Message, "does not support dry run"))
 }
 
+// tryServerSidePatch attempts to compute a server-side patch. Returns true iff the operation succeeded.
+func (k *kubeProvider) tryServerSidePatch(oldInputs, newInputs *unstructured.Unstructured, gvk schema.GroupVersionKind,
+) ([]byte, map[string]interface{}, bool) {
+	// If the resource's GVK changed, so compute patch using inputs.
+	if oldInputs.GroupVersionKind().String() != gvk.String() {
+		return nil, nil, false
+	}
+	// If we can't dry-run the new GVK, computed the patch using inputs.
+	if !k.supportsDryRun(gvk) {
+		return nil, nil, false
+	}
+	// TODO: Skipping server-side diff for resources with computed values is a hack. We will want to address this
+	// more granularly so that previews are as accurate as possible, but this is an easy workaround for a critical
+	// bug.
+	if hasComputedValue(newInputs) || hasComputedValue(oldInputs) {
+		return nil, nil, false
+	}
+
+	ssPatch, ssPatchBase, err := k.serverSidePatch(oldInputs, newInputs)
+	if k.isDryRunDisabledError(err) {
+		return nil, nil, false
+	}
+	if se, isStatusError := err.(*errors.StatusError); isStatusError {
+		// If the resource field is immutable.
+		if se.Status().Code == http.StatusUnprocessableEntity ||
+			strings.Contains(se.ErrStatus.Message, "field is immutable") {
+			return nil, nil, false
+		}
+	}
+
+	// The server-side patch succeeded.
+	return ssPatch, ssPatchBase, true
+}
+
 func mapReplStripSecrets(v resource.PropertyValue) (interface{}, bool) {
 	if v.IsSecret() {
 		return v.SecretValue().Element.MapRepl(nil, mapReplStripSecrets), true

tests/go.sum

@@ -617,8 +617,9 @@ github.com/ianlancetaylor/demangle v0.0.0-20200824232613-28f6c0f3b639/go.mod h1:
 github.com/ijc/Gotty v0.0.0-20170406111628-a8b993ba6abd h1:anPrsicrIi2ColgWTVPk+TrN42hJIWlfPHSBP9S0ZkM=
 github.com/ijc/Gotty v0.0.0-20170406111628-a8b993ba6abd/go.mod h1:3LVOLeyx9XVvwPgrt2be44XgSqndprz1G18rSk8KD84=
 github.com/imdario/mergo v0.3.5/go.mod h1:2EnlNZ0deacrJVfApfmtdGgDfMuh/nq6Ok1EcJh5FfA=
-github.com/imdario/mergo v0.3.11 h1:3tnifQM4i+fbajXKBHXWEH+KvNHqojZ778UH75j3bGA=
 github.com/imdario/mergo v0.3.11/go.mod h1:jmQim1M+e3UYxmgPu/WyfjB3N3VflVyUjjjwH0dnCYA=
+github.com/imdario/mergo v0.3.12 h1:b6R2BslTbIEToALKP7LxUvijTsNI9TAe80pLWN2g/HU=
+github.com/imdario/mergo v0.3.12/go.mod h1:jmQim1M+e3UYxmgPu/WyfjB3N3VflVyUjjjwH0dnCYA=
 github.com/inconshreveable/mousetrap v1.0.0 h1:Z8tu5sraLXCXIcARxBp/8cbvlwVa7Z1NHg9XEKhtSvM=
 github.com/inconshreveable/mousetrap v1.0.0/go.mod h1:PxqpIevigyE2G7u3NXJIT2ANytuPF1OarO4DADm73n8=
 github.com/influxdata/influxdb1-client v0.0.0-20191209144304-8bf82d3c094d/go.mod h1:qj24IKcXYK6Iy9ceXlo3Tc+vtHo9lIhSX5JddghvEPo=