Merge #11189

11189: Make IsSecret threadsafe r=iwahbe a=iwahbe

This PR ensures that `pulumi.IsSecret` works as expected. 

`IsSecret` presented 3 problems.
1. It wasn't thread safe, which triggered go's `-race` flag. (brought up by #11186)
2. It returned the currently known status of secret, meaning that `[]bool{IsSecret(a), IsSecret(a)}` could evaluate to `[]bool{false, true}` if the underlying value resolved between calls.
3. `IsSecret` may return the wrong value for unknown or erred `Output`s.

This PR fixes (1) and (2) by awaiting the underlying value during the `IsSecret` call. (3) is unfixable, and I explain why in a comment.

Blocking on calls to `IsSecret` is necessary for correctness in the general case. We can add optimizations later if necessary. The two I have in mind are
1. Adding a concept of "known secret". `pulumi.Secret(output)` is always secret, so we don't need to block on `output` to resolve this.
2. Distinguishing between functions that return an `Output` (which might be secret) and functions that return raw values (`string`, `int`, ...) which never change the "secretes" of an `Output`. We don't need to block on functions that return raw values.

I believe we should merge this PR without optimizations, and ensure that `IsSecret` is as correct as we can make it. We can add optimizations as they become necessary. I don't think we have customers who rely heavily on `IsSecret` evaluating instantly on not yet resolved outputs, since that is precisely the scenario where `IsSecret` may return the wrong answer.

Co-authored-by: Ian Wahbe <ian@wahbe.com>

changelog/pending/20221028--sdk-go-yaml--block-issecret-until-secretness-is-known.yaml

@@ -0,0 +1,4 @@
+changes:
+- type: fix
+  scope: sdk/go,yaml
+  description: Block IsSecret until secretness is known

sdk/go/pulumi/types.go

@@ -519,8 +519,33 @@ func (o *OutputState) ApplyTWithContext(ctx context.Context, applier interface{}
 }
 
 // IsSecret returns a bool representing the secretness of the Output
+//
+// IsSecret may return an inaccurate results if the Output is unknowable (during a
+// preview) or contains an error.
 func IsSecret(o Output) bool {
-	return o.getState().secret
+	_, _, secret, _, _ := o.getState().await(context.Background())
+	// We intentionally ignore both the `known` and `error` values returned by `await`:
+	//
+	// If a value is not known, it is possible that we will return the wrong result. This
+	// is unavoidable. Consider the example:
+	//
+	// ```go
+	// bucket, _ := s3.Bucket("bucket", &s3.BucketArgs{})
+	// unknowable := bucket.Bucket.ApplyT(func(b string) OutputString {
+	//   if strings.ContainsRune(b, '9') {
+	//     return ToSecret(String(b))
+	//   else {
+	//     return String(b)
+	//   }
+	// })
+	// ```
+	//
+	// Until we resolve values from the cloud, we can't know the correct value of
+	// `IsSecret(unknowable)`. We have the same problem for outputs with non-nil errors.
+	//
+	// This is tolerable because users will never be able to retrieve values (secret or
+	// otherwise) that are unknown or erred.
+	return secret
 }
 
 // Unsecret will unwrap a secret output as a new output with a resolved value and no secretness

sdk/go/pulumi/types_test.go

@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"reflect"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -618,7 +619,25 @@ func TestSecretApply(t *testing.T) {
 			break
 		}
 	}
+}
+
+// Test that secretness is properly bubbled up with all/apply that delays its execution.
+func TestSecretApplyDelayed(t *testing.T) {
+	t.Parallel()
 
+	// We run multiple tests here to increase the likelihood of a hypothetical race
+	// condition triggering. As with all concurrency tests, its not a 100% guarantee.
+	for i := 0; i < 10 && !t.Failed(); i++ {
+		t.Run("", func(t *testing.T) {
+			t.Parallel()
+			s1 := String("foo").ToStringOutput().ApplyT(func(s string) StringOutput {
+				time.Sleep(time.Millisecond * 5)
+				return ToSecret(String("bar")).(StringOutput)
+			})
+			// assert that s1 is secret.
+			assert.True(t, IsSecret(s1))
+		})
+	}
 }
 
 func TestNil(t *testing.T) {