Handle unknowns in Helm Release (#2822)

### Proposed changes

This PR improves support for preview mode in the Helm Release,
specifically the handling of unknown inputs.

The specific improvements are:
1. Ensure that the Helm provider handles the RPC requests (Check, Diff,
Create, Update, and Read); previously, when unknowns were present, the
kube provider itself handled some requests (with undefined behavior, see
#2679).
2. When decoding the inputs into an internal `Release` struct for
further processing, render the unknown values as `null`. Previously,
`decodeRelease` would panic.
3. When encoding the `Release` back to a property map, recover the
unknownness of the properties.
4. In `Check`, perform the roundtrip of inputs-to-Release-to-outputs in
all cases. This has the effect of stabilizing the checked inputs.
5. In `Diff`, detect the edge case of a property becoming a computed
value.
6. In preview mode, emit unknown values for the output-only properties
`status` and `resourceNames`.
 
Note: the handling of unknowns works best with `allowNullValues: true`.

### Related issues (optional)

Closes #2660 
Closes #2679 
```

CHANGELOG.md

@@ -5,7 +5,8 @@
 - Use output properties for await logic (https://github.com/pulumi/pulumi-kubernetes/pull/2790)
 - Support for metadata.generateName (CSA) (https://github.com/pulumi/pulumi-kubernetes/pull/2808)
 - Fix unmarshalling of Helm values yaml file (https://github.com/pulumi/pulumi-kubernetes/issues/2815)
-
+- Handle unknowns in Helm Release resource (https://github.com/pulumi/pulumi-kubernetes/pull/2822)
+
 ## 4.7.1 (January 17, 2024)
 
 - Fix deployment await logic for accurate rollout detection

provider/pkg/provider/diff_test.go

@@ -9,6 +9,7 @@ import (
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 
 	jsonpatch "github.com/evanphx/json-patch"
+	"github.com/pulumi/pulumi/sdk/v3/go/common/resource"
 	pulumirpc "github.com/pulumi/pulumi/sdk/v3/proto/go"
 	"github.com/stretchr/testify/assert"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -191,6 +192,46 @@ func TestPatchToDiff(t *testing.T) {
 				"data.property1": UR,
 			},
 		},
+		{
+			name:  `Changing computed object values results in correct diff`,
+			group: "core", version: "v1", kind: "Pod",
+			old:    object{"spec": object{"containers": list{object{"name": "nginx", "image": "nginx"}}}},
+			new:    object{"spec": object{"containers": list{object{"name": "nginx", "image": nil}}}},
+			inputs: object{"spec": object{"containers": list{object{"name": "nginx", "image": resource.Computed{}}}}},
+			expected: expected{
+				"spec.containers[0].image": UR,
+			},
+		},
+		{
+			name:  `Adding computed object values results in correct diff`,
+			group: "core", version: "v1", kind: "Pod",
+			old:    object{"spec": object{"containers": list{object{"name": "nginx"}}}},
+			new:    object{"spec": object{"containers": list{object{"name": "nginx", "image": nil}}}},
+			inputs: object{"spec": object{"containers": list{object{"name": "nginx", "image": resource.Computed{}}}}},
+			expected: expected{
+				"spec.containers[0].image": AR,
+			},
+		},
+		{
+			name:  `Adding computed array values results in correct diff`,
+			group: "core", version: "v1", kind: "Pod",
+			old:    object{"spec": object{"containers": list{}}},
+			new:    object{"spec": object{"containers": list{nil}}},
+			inputs: object{"spec": object{"containers": list{resource.Computed{}}}},
+			expected: expected{
+				"spec.containers[0]": A,
+			},
+		},
+		{
+			name:  `Changing computed array values results in correct diff`,
+			group: "core", version: "v1", kind: "Pod",
+			old:    object{"spec": object{"containers": list{object{"name": "nginx"}}}},
+			new:    object{"spec": object{"containers": list{nil}}},
+			inputs: object{"spec": object{"containers": list{resource.Computed{}}}},
+			expected: expected{
+				"spec.containers[0]": U,
+			},
+		},
 	}
 
 	for _, tt := range tests {

provider/pkg/provider/helm_release.go

@@ -276,14 +276,18 @@ func (r *helmReleaseProvider) getActionConfig(namespace string) (*action.Configu
 	return conf, nil
 }
 
+var (
+	// mapReplExtractValues extracts pure values from the property map.
+	mapReplExtractValues = combineMapReplv(mapReplStripSecrets, mapReplStripComputed)
+)
+
 func decodeRelease(pm resource.PropertyMap, label string) (*Release, error) {
 	var release Release
 	values := map[string]any{}
-	stripped := pm.MapRepl(nil, mapReplStripSecrets)
+	stripped := pm.MapRepl(nil, mapReplExtractValues)
 	logger.V(9).Infof("[%s] Decoding release: %#v", label, stripped)
 
-	if pm.HasValue("valueYamlFiles") {
-		v := stripped["valueYamlFiles"]
+	if v, ok := stripped["valueYamlFiles"]; ok {
 		switch reflect.TypeOf(v).Kind() {
 		case reflect.Slice, reflect.Array:
 			s := reflect.ValueOf(v)
@@ -344,19 +348,19 @@ func (r *helmReleaseProvider) Check(ctx context.Context, req *pulumirpc.CheckReq
 		return nil, pkgerrors.Wrapf(err, "check failed because malformed resource inputs: %+v", err)
 	}
 
-	if len(olds.Mappable()) > 0 {
+	if len(olds) > 0 {
 		adoptOldNameIfUnnamed(news, olds)
 	}
 	assignNameIfAutonameable(news, urn)
 	r.setDefaults(news)
 
-	if !news.ContainsUnknowns() {
-		logger.V(9).Infof("Decoding new release.")
-		new, err := decodeRelease(news, fmt.Sprintf("%s.news", label))
-		if err != nil {
-			return nil, err
-		}
+	logger.V(9).Infof("Decoding new release.")
+	new, err := decodeRelease(news, fmt.Sprintf("%s.news", label))
+	if err != nil {
+		return nil, err
+	}
 
+	if !news.ContainsUnknowns() {
 		logger.V(9).Infof("Loading Helm chart.")
 		chart, err := r.helmLoad(ctx, urn, new)
 		if err != nil {
@@ -370,11 +374,11 @@ func (r *helmReleaseProvider) Check(ctx context.Context, req *pulumirpc.CheckReq
 			// with this we may determine whether the Helm release needs to be upgraded.
 			new.Version = chart.Metadata.Version
 		}
-
-		logger.V(9).Infof("New: %+v", new)
-		news = resource.NewPropertyMap(new)
 	}
 
+	logger.V(9).Infof("New: %+v", new)
+	news = resource.NewPropertyMap(new)
+
 	// remove deprecated inputs
 	delete(news, "resourceNames")
 
@@ -387,7 +391,8 @@ func (r *helmReleaseProvider) Check(ctx context.Context, req *pulumirpc.CheckReq
 	if err != nil {
 		return nil, pkgerrors.Wrapf(err, "check failed because malformed resource inputs: %+v", err)
 	}
-	// ensure we don't leak secrets into state.
+	// ensure we don't leak secrets into state, and preserve the computedness of inputs.
+	annotateComputed(news, newInputs)
 	annotateSecrets(news, newInputs)
 
 	autonamedInputs, err := plugin.MarshalProperties(news, plugin.MarshalOptions{
@@ -743,17 +748,19 @@ func (r *helmReleaseProvider) Diff(ctx context.Context, req *pulumirpc.DiffReque
 	logger.V(9).Infof("Diff: New release: %#v", newRelease)
 
 	// Generate a patch to apply the new inputs to the old state, including deletions.
-	oldInputsJSON, err := json.Marshal(oldInputs.MapRepl(nil, mapReplStripSecrets))
+	// Computed values are mapped to null, and secrets are mapped to plain values.
+	// Later, we'll use this patch to generate a diff response, with special handling for the computed values.
+	oldInputsJSON, err := json.Marshal(oldInputs.MapRepl(nil, mapReplExtractValues))
 	if err != nil {
 		return nil, pkgerrors.Wrapf(err, "internal error: json.Marshal(oldInputsJson)")
 	}
 	logger.V(9).Infof("oldInputsJSON: %s", string(oldInputsJSON))
-	newInputsJSON, err := json.Marshal(news.MapRepl(nil, mapReplStripSecrets))
+	newInputsJSON, err := json.Marshal(news.MapRepl(nil, mapReplExtractValues))
 	if err != nil {
 		return nil, pkgerrors.Wrapf(err, "internal error: json.Marshal(oldInputsJson)")
 	}
 	logger.V(9).Infof("newInputsJSON: %s", string(newInputsJSON))
-	oldStateJSON, err := json.Marshal(olds.MapRepl(nil, mapReplStripSecrets))
+	oldStateJSON, err := json.Marshal(olds.MapRepl(nil, mapReplExtractValues))
 	if err != nil {
 		return nil, pkgerrors.Wrapf(err, "internal error: json.Marshal(oldStateJson)")
 	}
@@ -768,7 +775,7 @@ func (r *helmReleaseProvider) Diff(ctx context.Context, req *pulumirpc.DiffReque
 		return nil, pkgerrors.Wrapf(
 			err, "Failed to check for changes in Helm release %s/%s because of an error serializing "+
 				"the JSON patch describing resource changes",
-			newRelease.Namespace, newRelease.Name)
+			oldRelease.Namespace, oldRelease.Name)
 	}
 
 	// Pack up PB, ship response back.
@@ -788,12 +795,16 @@ func (r *helmReleaseProvider) Diff(ctx context.Context, req *pulumirpc.DiffReque
 		logger.V(9).Infof("news: %+v", news.Mappable())
 		logger.V(9).Infof("oldInputs: %+v", oldInputs.Mappable())
 
+		strip := func(pm resource.PropertyMap) map[string]interface{} {
+			// strip the secretness but retain computedness (as is understood by convertPatchToDiff)
+			return pm.MapRepl(nil, mapReplStripSecrets)
+		}
 		forceNewFields := []string{".name", ".namespace"}
-		if detailedDiff, err = convertPatchToDiff(patchObj, olds.Mappable(), news.Mappable(), oldInputs.Mappable(), forceNewFields...); err != nil {
+		if detailedDiff, err = convertPatchToDiff(patchObj, strip(olds), strip(news), strip(oldInputs), forceNewFields...); err != nil {
 			return nil, pkgerrors.Wrapf(
 				err, "Failed to check for changes in helm release %s/%s because of an error "+
 					"converting JSON patch describing resource changes to a diff",
-				newRelease.Namespace, newRelease.Name)
+				oldRelease.Namespace, oldRelease.Name)
 		}
 
 		for k, v := range detailedDiff {
@@ -877,7 +888,7 @@ func (r *helmReleaseProvider) Create(ctx context.Context, req *pulumirpc.CreateR
 		}
 	}
 
-	obj := checkpointRelease(news, newRelease, fmt.Sprintf("%s.news", label))
+	obj := checkpointRelease(news, newRelease, fmt.Sprintf("%s.news", label), req.GetPreview())
 	inputsAndComputed, err := plugin.MarshalProperties(
 		obj, plugin.MarshalOptions{
 			Label:        fmt.Sprintf("%s.inputsAndComputed", label),
@@ -926,7 +937,7 @@ func (r *helmReleaseProvider) Read(ctx context.Context, req *pulumirpc.ReadReque
 	logger.V(9).Infof("%s decoded release: %#v", label, existingRelease)
 
 	var namespace, name string
-	if len(oldState.Mappable()) == 0 {
+	if len(oldState) == 0 {
 		namespace, name = parseFqName(req.GetId())
 		logger.V(9).Infof("%s Starting import for %s/%s", label, namespace, name)
 	} else {
@@ -970,7 +981,7 @@ func (r *helmReleaseProvider) Read(ctx context.Context, req *pulumirpc.ReadReque
 
 	// Return a new "checkpoint object".
 	state, err := plugin.MarshalProperties(
-		checkpointRelease(oldInputs, existingRelease, fmt.Sprintf("%s.olds", label)), plugin.MarshalOptions{
+		checkpointRelease(oldInputs, existingRelease, fmt.Sprintf("%s.olds", label), false), plugin.MarshalOptions{
 			Label:        fmt.Sprintf("%s.state", label),
 			KeepUnknowns: true,
 			SkipNulls:    true,
@@ -1051,7 +1062,7 @@ func (r *helmReleaseProvider) Update(ctx context.Context, req *pulumirpc.UpdateR
 		}
 	}
 
-	checkpointed := checkpointRelease(newResInputs, newRelease, fmt.Sprintf("%s.news", label))
+	checkpointed := checkpointRelease(newResInputs, newRelease, fmt.Sprintf("%s.news", label), req.GetPreview())
 	inputsAndComputed, err := plugin.MarshalProperties(
 		checkpointed, plugin.MarshalOptions{
 			Label:        fmt.Sprintf("%s.inputsAndComputed", label),
@@ -1117,15 +1128,23 @@ func (r *helmReleaseProvider) Delete(ctx context.Context, req *pulumirpc.DeleteR
 	return &pbempty.Empty{}, nil
 }
 
-func checkpointRelease(inputs resource.PropertyMap, outputs *Release, label string) resource.PropertyMap {
+func checkpointRelease(inputs resource.PropertyMap, outputs *Release, label string, isPreview bool) resource.PropertyMap {
 	logger.V(9).Infof("[%s] Checkpointing outputs: %#v", label, outputs)
 	logger.V(9).Infof("[%s] Checkpointing inputs: %#v", label, inputs)
 	object := resource.NewPropertyMap(outputs)
 	object["__inputs"] = resource.MakeSecret(resource.NewObjectProperty(inputs))
 
 	// Make sure parts of the inputs which are marked as secrets in the inputs are retained as
-	// secrets in the outputs.
+	// secrets in the outputs. Likewise for computed values.
+	annotateComputed(object, inputs)
 	annotateSecrets(object, inputs)
+
+	// If this is a preview, emit computed placeholders for the pure outputs.
+	if isPreview {
+		object["resourceNames"] = resource.MakeComputed(resource.NewStringProperty(""))
+		object["status"] = resource.MakeComputed(resource.NewStringProperty(""))
+	}
+
 	return object
 }
 

provider/pkg/provider/provider.go

@@ -1532,6 +1532,9 @@ func (k *kubeProvider) Diff(ctx context.Context, req *pulumirpc.DiffRequest) (*p
 	//
 
 	urn := resource.URN(req.GetUrn())
+	if isHelmRelease(urn) {
+		return k.helmReleaseProvider.Diff(ctx, req)
+	}
 
 	label := fmt.Sprintf("%s.Diff(%s)", k.label(), urn)
 	logger.V(9).Infof("%s executing", label)
@@ -1559,10 +1562,7 @@ func (k *kubeProvider) Diff(ctx context.Context, req *pulumirpc.DiffRequest) (*p
 	newInputs := propMapToUnstructured(newResInputs)
 
 	oldInputs, oldLive := parseCheckpointObject(oldState)
-	if !isHelmRelease(urn) {
-		// "isHelmRelease" is due to https://github.com/pulumi/pulumi-kubernetes/issues/2679
-		contract.Assertf(oldLive.GetName() != "", "expected live object name to be nonempty: %v", oldLive)
-	}
+	contract.Assertf(oldLive.GetName() != "", "expected live object name to be nonempty: %v", oldLive)
 
 	oldInputs, err = normalizeInputs(oldInputs)
 	if err != nil {
@@ -1576,10 +1576,6 @@ func (k *kubeProvider) Diff(ctx context.Context, req *pulumirpc.DiffRequest) (*p
 
 	gvk := k.gvkFromUnstructured(newInputs)
 
-	if isHelmRelease(urn) && !hasComputedValue(newInputs) {
-		return k.helmReleaseProvider.Diff(ctx, req)
-	}
-
 	namespacedKind, err := clients.IsNamespacedKind(gvk, k.clientSet)
 	if err != nil {
 		if clients.IsNoNamespaceInfoErr(err) {
@@ -1759,8 +1755,7 @@ func (k *kubeProvider) Create(
 	//   resource. This is important both for `Diff` and for `Update`. See comments in those methods for details.
 	//
 	urn := resource.URN(req.GetUrn())
-
-	if isHelmRelease(urn) && !req.GetPreview() {
+	if isHelmRelease(urn) {
 		return k.helmReleaseProvider.Create(ctx, req)
 	}
 
@@ -2235,6 +2230,10 @@ func (k *kubeProvider) Update(
 	//
 
 	urn := resource.URN(req.GetUrn())
+	if isHelmRelease(urn) {
+		return k.helmReleaseProvider.Update(ctx, req)
+	}
+
 	label := fmt.Sprintf("%s.Update(%s)", k.label(), urn)
 	logger.V(9).Infof("%s executing", label)
 
@@ -2275,9 +2274,6 @@ func (k *kubeProvider) Update(
 		return &pulumirpc.UpdateResponse{Properties: req.News}, nil
 	}
 
-	if isHelmRelease(urn) {
-		return k.helmReleaseProvider.Update(ctx, req)
-	}
 	// Ignore old state; we'll get it from Kubernetes later.
 	oldInputs, oldLive := parseCheckpointObject(oldState)
 
@@ -2790,6 +2786,17 @@ func normalizeInputs(uns *unstructured.Unstructured) (*unstructured.Unstructured
 	return uns, nil
 }
 
+func combineMapReplv(replvs ...func(resource.PropertyValue) (any, bool)) func(resource.PropertyValue) (any, bool) {
+	return func(v resource.PropertyValue) (any, bool) {
+		for _, replv := range replvs {
+			if r, ok := replv(v); ok {
+				return r, true
+			}
+		}
+		return "", false
+	}
+}
+
 func mapReplStripSecrets(v resource.PropertyValue) (any, bool) {
 	if v.IsSecret() {
 		return v.SecretValue().Element.MapRepl(nil, mapReplStripSecrets), true
@@ -2798,6 +2805,14 @@ func mapReplStripSecrets(v resource.PropertyValue) (any, bool) {
 	return nil, false
 }
 
+func mapReplStripComputed(v resource.PropertyValue) (any, bool) {
+	if v.IsComputed() {
+		return nil, true
+	}
+
+	return nil, false
+}
+
 // mapReplUnderscoreToDash is needed to work around cases where SDKs don't allow dashes in variable names, and so the
 // parameter is renamed with an underscore during schema generation. This function normalizes those keys to the format
 // expected by the cluster.
@@ -3015,9 +3030,9 @@ type patchConverter struct {
 func (pc *patchConverter) addPatchValueToDiff(
 	path []any, v, old, newInput, oldInput any, inArray bool,
 ) error {
-	contract.Assertf(v != nil || old != nil || oldInput != nil,
-		"path: %+v  |  v: %+v  | old: %+v  |  oldInput: %+v",
-		path, v, old, oldInput)
+	contract.Assertf(v != nil || old != nil || oldInput != nil || newInput != nil,
+		"path: %+v  |  v: %+v  | old: %+v  |  oldInput: %+v  |  newInput: %+v",
+		path, v, old, oldInput, newInput)
 
 	// If there is no new input, then the only possible diff here is a delete. All other diffs must be diffs between
 	// old and new properties that are populated by the server. If there is also no old input, then there is no diff
@@ -3029,7 +3044,16 @@ func (pc *patchConverter) addPatchValueToDiff(
 	var diffKind pulumirpc.PropertyDiff_Kind
 	inputDiff := false
 	if v == nil {
-		diffKind, inputDiff = pulumirpc.PropertyDiff_DELETE, true
+		// computed values are rendered as null in the patch; handle this special case.
+		if _, ok := newInput.(resource.Computed); ok {
+			if old == nil {
+				diffKind = pulumirpc.PropertyDiff_ADD
+			} else {
+				diffKind = pulumirpc.PropertyDiff_UPDATE
+			}
+		} else {
+			diffKind, inputDiff = pulumirpc.PropertyDiff_DELETE, true
+		}
 	} else if old == nil {
 		diffKind = pulumirpc.PropertyDiff_ADD
 	} else {