[sdk/dotnet] Input<T> eagerly converts values to Output<T> leading to undesirable preview diffs

[justinvp]

In the .NET SDK, Input<T> (and InputList<T>, InputMap<V>, InputUnion<T0, T1>, and InputJson) eagerly stores the value of T as an Output<T>, which leads to undesirable preview diffs.

https://github.com/pulumi/pulumi/blob/0a38bc295cfc5ae3c325dcece169da5a5e80fffc/sdk/dotnet/Pulumi/Core/Input.cs#L22-L31

For example, consider this program:

using Pulumi;
using Pulumi.Random;
using Pulumi.Aws.S3;

class MyStack : Stack
{
    public MyStack()
    {
        var randString = new RandomString("str", new RandomStringArgs { Length = 10 });
        var bucket = new Bucket("bucket", new BucketArgs {
            Tags = {
                { "hello", "world" },
                { "hi", "there" },
                { "foo", randString.Id },
            }
        });
    }
}

Expected preview diff:

        tags        : {
            hello     : "world"
            hi        : "there"
            foo       : output<string>
        }

Actual:

        tags        : output<string>

This also means that we have the same issue with programs that create multi-lang components, even with output values added in pulumi/pulumi#8316.

To address this, we need to fix Input<T> et. al. to not eagerly convert values to Output<T>. Instead, internally, it can keep a discriminated union of the plain T and Output<T> and only convert to Output<T> when needed.

[Frassle]

Shouldn't preview diffs print Output's that are known?

[justinvp]

Shouldn't preview diffs print Output's that are known?

The problem is the way Add is implemented for InputMap<V> and InputList<T> (used by the collection initializer syntax):

https://github.com/pulumi/pulumi/blob/013fcdec9d391bb071b0100b6a6c92ac97e03d1f/sdk/dotnet/Pulumi/Core/InputMap.cs#L54-L59

https://github.com/pulumi/pulumi/blob/013fcdec9d391bb071b0100b6a6c92ac97e03d1f/sdk/dotnet/Pulumi/Core/InputList.cs#L58-L63

Internally InputMap<V> and InputList<T> are storing their state as:

• Output<ImmutableDictionary<string, V>>
• Output<ImmutableArray<T>>

So when adding an Input<T> value, these collections add the item inside an Apply and save the new resulting Output<ImmutableDictionary<string, V>> or Output<ImmutableArray<T>>. When doing that, if one of the values was unknown or secret, then the new outer Output is marked as unknown and secret. Which makes the wholecollection value display as unknown or secret in the diff, rather than showing the individual values inside.

One relatively simple solution is to change InputMap<V> and InputList<T> to save their internal state as:

• OneOf<ImmutableDictionary<string, Input<V>>, Output<ImmutableDictionary<string, V>>>
• OneOf<ImmutableArray<Input<T>>, Output<ImmutableArray<T>>>

Then, for the most part, we can keep the added values as-is by storing them in ImmutableDictionary<string, Input<V>> and ImmutableArray<Input<T>>, only converting to Output when calling Merge or Concat with a map or list that's internally using an Output.

[t0yv0]

👍 from the consistency across SDK POV, the other SDKs have the capability to discriminate Input into either T, Promise or Output, so .NET SDK should also have that in the programming model. Fraser raised that perhaps .NET SDK can discriminate Output to find out if it's prompt known T, potentially eventually removing T. It sounds like something that should not block this work, but if we go that way I'd like to figure out how that works with other SDKs, how does imaginary "meta-SDK" code translate into .NET SDK under that API.

[t0yv0]

So still 👍 on the main idea listed.

Now the subtle implementation point on Lists (I think map follows similarly) - to follow up on the design discussion today.

I made a point earlier about Tuple/Map2 interface on Input as a potential implementation path to updated InputList, but it was not very clear. I went ahead and wrote some code that clarified the issue for me at least:

https://gist.github.com/t0yv0/84e4f976e190aabbd1ccd666b89d73bd

In a world where Input discriminates between T and Output<T> the suggested type OneOf<ImmutableArray<Input<T>>, Output<ImmutableArray<T>>> is similar to Input<ImmutableArray<Input<T>>>, and one can bring the "standard Input combinators" into the implementation if this second form is chosen. The primitive combinators (Map, Tuple) guarantee minimal Output/unknown contamination, the rest of the code automatically preserves this property.

The code above works as expected, however there is a problem if InputList<T> is cast into Input<ImmutableArray<T>>, then the Output/unknown contamination does happen anyway through the use of Join and is inevitable if we insist on this type. That is if you want to observe a partially known list like this printout below, then you need to work with Input<ImmutableArray<Input<T>>> form:

  // [plain: [len=3: [plain: 1], [plain: 2], output]]

So in addition to changing internals of InputList, to carry this through previews, we either need to change the base type of InputList which is breaking or else we change the way reflective machinery considers InputList values when it encounters them; either way it seems potentially a lot more invasive change than a simple change to InputList internals. If I am wrong in this prediction you Justin can work out a solution that is a purely internal change to InputList I would be very curious to see how that works.

This may have been exactly the point @Frassle was making. They are reasoning about this much faster than I am. But I do not want to assume.

[Frassle]

@t0yv0 Yeh these types are a bit confusing and hard to reason about, my first thinking was that either this is really just A) Input<ImmutableArray<T>> or B) ImmutableArray<Input<T>> but actually its trying to do both, and @justinvp change is to switch from B to A more clearly, so yes 👍 to the PR.
However I think your description that it's actually an Input<ImmutableArray<Input<T>>> pretending to be an Input<ImmutableArray<T>> is correct. If Input tracks if it is a known input or not then adding an item is an "IsKnown" preserving operation and so would preserve the intended diff behavior. Then once you want to get the value out you just flatten the inner Inputs and the outer Input to return Input<ImmutableArray<T>> (i.e. join . sequenceA if your from Haskell land)

Also all of this is still fine in an SDK where Input<T> is replaced with just Output<T> because that can still track knowns.

[t0yv0]

I'm still struggling to understand how the PR will solve this example at the top, and I'm running out of time to try it out, guess need to wait for the tests:

       var bucket = new Bucket("bucket", new BucketArgs {
            Tags = {
                { "hello", "world" },
                { "hi", "there" },
                { "foo", randString.Id },
            }
        });

What's the type of Tags?

Looking at def of BucketArgs.Tags I see:

        /// <summary>
        /// A mapping of tags to assign to the bucket.
        /// </summary>
        public InputMap<string> Tags
        {
            get => _tags ?? (_tags = new InputMap<string>());
            set => _tags = value;
        }

And this header remains:

public sealed class InputMap<V> : Input<ImmutableDictionary<string, V>>, IEnumerable, IAsyncEnumerable<Input<KeyValuePair<string, V>>>

So then, if our framework casts it to Input<ImmutableDictionary<string, string>> then the entire thing will be unknown because of contamination from randString.Id unknown.

If our framework however casts it to IAsyncEnumerable<Input<KeyValuePair<string, V>>> then there is less loss of information implicit in the type, because Input slots are per k-v pair, so that's great. This can represent known pairs separately from unknown pairs. Is this the case already without changes? That'd be good. Ditto for IAsyncEnumerable<Input<T>> in the List case.

Input<ImmutableArray<Input<T>>> I put forward is slightly more general than IAsyncEnumerable<Input<T>>, but both have power to express what's needed here.

[justinvp]

Love the discussion above and agree with the point on it trying to be Input<ImmutableArray<Input<T>>>. But to answer your questions in the most recent comment (based on the draft PR as it currently is)...

I'm still struggling to understand how the PR will solve this example at the top

I definitely intend to add a bunch more tests, but the draft does include an output value serialization test that indirectly demonstrates the fix (using a secret rather than unknown).

https://github.com/pulumi/pulumi/pull/8419/files#diff-0f8ea07faed5e1d4b818855971eed7002f55f1b2d5f0ced29cf4520faf947875

            new object[]
             {
                 new InputMap<string> { { "foo", Output.CreateSecret("hello") } },
                 ImmutableDictionary<string, object>.Empty.Add("foo", CreateOutputValue("hello", isSecret: true))
             },

Without the changes in the PR, the serialized value is:

{
    "4dabf18193072939515e22adb298388d": "d0e6a833031e9bbcd3f4e8bde6ca49a4",
    "value": { "foo": "hello" },
    "secret": true
}

The expected serialized value (with the changes in the PR):

{
    "foo": {
        "4dabf18193072939515e22adb298388d": "d0e6a833031e9bbcd3f4e8bde6ca49a4",
        "value": "hello",
        "secret": true
    }
}

The serializer isn't casting to Input<ImmutableDictionary<string, string>> or IAsyncEnumerable<Input<KeyValuePair<string, string>>>.

The serializer currently does this:

https://github.com/pulumi/pulumi/blob/e60d6bf248b0804504bfbe55409985114c273063/sdk/dotnet/Pulumi/Serialization/Serializer.cs#L106-L114

The implementation of input.ToOutput() (in a round about way) returns the internal Output<T> value inside Input<T>:

https://github.com/pulumi/pulumi/blob/e60d6bf248b0804504bfbe55409985114c273063/sdk/dotnet/Pulumi/Core/Input.cs#L42-L43

The change in the PR changes the serializer to not use input.ToOutput() but to instead use a new property I added to the IInput interface: input.Value, which returns OneOf<T0, T1>.Value:

https://github.com/pulumi/pulumi/pull/8419/files#diff-142b127b79cc1c863d28b0703711e21d05d47242e83e649b6ab920624829240d

So the value that ends up being serialized will either be ImmutableDictionary<string, Input<V>> or Output<ImmutableDictionary<string, V>>.

For the former, it'll then hit the non-generic IDictionary case in the serializer:

https://github.com/pulumi/pulumi/blob/e60d6bf248b0804504bfbe55409985114c273063/sdk/dotnet/Pulumi/Serialization/Serializer.cs#L266-L269

For the latter, it'll hit the non-generic IOutput case:

https://github.com/pulumi/pulumi/blob/e60d6bf248b0804504bfbe55409985114c273063/sdk/dotnet/Pulumi/Serialization/Serializer.cs#L136-L206

[IaroslavTitov]

Hitting this issue while developing Pulumi Service Provider - really confusing behavior, forced to rewrite a map into an array of custom objects. Would be great to fix and avoid this.