[dotnet] Unit tests with Helm.V3.Chart throw NullReferenceException

vipentti commented 3 years ago

I'm trying to write some unit tests in C# for my stacks and I'm having trouble with a stack which contains a Helm.V3.Chart. Running unit tests which utilize charts currently throw System.NullReferenceException.

Expected behavior

Unit tests execute without exceptions.

Current behavior

The following exception is thrown when executing a unit test with a stack containing Helm.V3.Chart.

   Pulumi.RunException : Running program '<path>\bin\Debug\net5.0\testhost.dll' failed with an unhandled exception:
System.NullReferenceException: Object reference not set to an instance of an object.
   at System.Collections.Immutable.ImmutableArray.CreateRange[TSource,TResult](ImmutableArray`1 items, Func`2 selector)
   at Pulumi.Extensions.SelectAsArray[TItem,TResult](ImmutableArray`1 items, Func`2 map)
   at Pulumi.InputList`1.op_Implicit(ImmutableArray`1 values)
   at Pulumi.Kubernetes.Helm.V3.Chart.<>c__DisplayClass3_0.<ParseTemplate>b__0(ImmutableArray`1 objs)
   at Pulumi.Output`1.ApplyHelperAsync[U](Task`1 dataTask, Func`2 func)
   at Pulumi.Output`1.ApplyHelperAsync[U](Task`1 dataTask, Func`2 func)
   at Pulumi.Output`1.Pulumi.IOutput.GetDataAsync()
   at Pulumi.Serialization.Serializer.SerializeAsync(String ctx, Object prop, Boolean keepResources)
   at Pulumi.Deployment.SerializeFilteredPropertiesAsync(String label, IDictionary`2 args, Predicate`1 acceptKey, Boolean keepResources)
   at Pulumi.Deployment.SerializeAllPropertiesAsync(String label, IDictionary`2 args, Boolean keepResources)
   at Pulumi.Deployment.RegisterResourceOutputsAsync(Resource resource, Output`1 outputs)
   at Pulumi.Deployment.Runner.<>c__DisplayClass9_0.<<WhileRunningAsync>g__HandleCompletion|0>d.MoveNext()
--- End of stack trace from previous location ---
   at Pulumi.Deployment.Runner.WhileRunningAsync()
    Stack Trace:
       at Pulumi.Deployment.TestAsync(IMocks mocks, Func`2 runAsync, TestOptions options)
    <snip>
--- End of stack trace from previous location ---

I did some digging and found that the trouble is caused by the implicit conversion operator invoked at https://github.com/pulumi/pulumi-kubernetes/blob/80d4c16c2240d77ca575273a9c167c739e7681a3/sdk/dotnet/Helm/V3/Chart.cs#L365

The actual problem seems to be that the ImmutableArray-parameter https://github.com/pulumi/pulumi-kubernetes/blob/80d4c16c2240d77ca575273a9c167c739e7681a3/sdk/dotnet/Helm/V3/Invokes.cs#L46 given to HelmTemplateResult is uninitialized when running the tests. Since the ImmutableArray is uninitialized, the System.NullReferenceException is thrown when trying to do the implicit conversion.

Steps to reproduce

The problem can be produced by having the following stack:

public class FailingHelmStack : Pulumi.Stack
{
    public FailingHelmStack()
    {
        var chart = new Pulumi.Kubernetes.Helm.V3.Chart("chart"
            , new Pulumi.Kubernetes.Helm.ChartArgs()
            {
                Chart = "ingress-nginx",
                Namespace = "kube-system",
                FetchOptions = new Pulumi.Kubernetes.Helm.ChartFetchArgs
                {
                    Repo = "https://kubernetes.github.io/ingress-nginx",
                },
            });
    }
}

And invoking it with Deployment.TestAsync in a C# test project.

Context (Environment)

Trying to write unit tests for stacks which contain Helm-charts.

Proposed fix

Ensure that the HelmTemplateResult.Result array is always initialized.

~~For example by adding the following to HelmTemplateResult constructor https://github.com/pulumi/pulumi-kubernetes/blob/80d4c16c2240d77ca575273a9c167c739e7681a3/sdk/dotnet/Helm/V3/Invokes.cs#L48~~

Result = result;
// Ensure the Result array is always initialized
if (Result.IsDefault)
{
    Result = ImmutableArray<ImmutableDictionary<string, object>>.Empty;
}

~~If this seems like a reasonable solution, I can submit a PR.~~

NOTE I am not sure if similiar problem occurs anywhere else with ImmutableArray

UPDATE The implicit conversion fix is in pulumi/pulumi#6544

vipentti commented 3 years ago

Upon testing, this problem also happens with YamlDecodeResult https://github.com/pulumi/pulumi-kubernetes/blob/80d4c16c2240d77ca575273a9c167c739e7681a3/sdk/dotnet/Yaml/Invokes.cs#L43

t0yv0 commented 3 years ago

Do you have a unit test to repro? I was attempting to repro with pulumi up but it needs a K8S cluster. I will have one later in the day as I'm working on something adjacent and can try that then. But if the unit tests repros without a cluster that's even better.

Per our conversation in linked PR, I'd like to poke around here to see where the invalid ImmutableArray originates from in the SDK and perhaps we can come up with a fix that makes those go away.

vipentti commented 3 years ago

@t0yv0 I've created a small repo which displays the problem https://github.com/vipentti/pulumi-helm-chart-unittest-issue

t0yv0 commented 3 years ago

I have a workaround for your unit test that makes it pass for me:

https://github.com/vipentti/pulumi-helm-chart-unittest-issue/pull/1

At a high level, what I think is happening, the Mock implementation you were using was violating some assumptions of how kubernetes:helm:template resource responds. I could only find this by perusing the provider.go source code in pulumi-kubernetes.. but it seems like a "result" property is populated with output to decodeYaml, which has this:

// decodeYaml parses a YAML string, and then returns a slice of untyped structs that can be marshalled into
// Pulumi RPC calls. If a default namespace is specified, set that on the relevant decoded objects.

So mocking the helm resources deeply and accurately sounds like something off the beaten path. I am a bit new to Pulumi and will ask around if there is a better way. Please let me know if my PR unblocks you to get to something sufficiently useful, or you run into more roadblocks trying to mock the Helm provider?

Considering changes to pulumi-kubernetes. Technically with the original shallow mock, this is what's happening:

V3/Chart.cs calls Invokes.HelmTemplate(..)
which calls Deployement.Instance.InvokeAsync<HelmTemplateResult>("kubernetes:helm:template", .., ..) 
which calls  internal interface IDeployment { Task<T> InvokeAsync(..) } 
which returns null
which gets wrapped in HelmTemplateResult and continues downstream

So we could consider HelmTemplateResult normalizing null to empty ImmutableArray. It is clear though this only affects the scenario of testing with incomplete mocks, and I am wondering if this would actually unblock some useful testing?

vipentti commented 3 years ago

Wow! Thank you @t0yv0!

Your PR indeed does unblock my testing and in addition to that it actually gives me the opportunity to actually mock Kubernetes resources that would be created by the template, which I had to do since rest of the code then extracts the loadBalancer IP from the service that is created.

Basically what I did was:

First in my unit test I define the service that I want the template to return in YAML, which I then deserialize into a custom class, but anything that can be turned to ImmutableDictionary<string, object> probably works.

string ipAddress = "192.0.0.1";
string serviceName = "stack-http-ngx-ingress-nginx-controller";
string serviceYaml = $@"
kind: Service
apiVersion: v1
metadata:
  name: {serviceName}
  namespace: kube-system
  labels:
    app.kubernetes.io/name: ingress-nginx
    app.kubernetes.io/part-of: ingress-nginx
spec:
  externalTrafficPolicy: Local
  type: LoadBalancer
  selector:
    app.kubernetes.io/name: ingress-nginx
    app.kubernetes.io/part-of: ingress-nginx
  ports:
    - name: http
      port: 80
      targetPort: http
    - name: https
      port: 443
      targetPort: https
status:
  loadBalancer:
    ingress:
      - ip: {ipAddress}";

// Read the YAML contents into a custom class
var serviceObject = YamlUtils.ReadYamlToMap(serviceYaml);

The serviceObject is then added to to the template results inside CallAsync

// Inside CallAsync

// Workaround for NRE with helm templates
if (token == "kubernetes:helm:template")
{
    outputs.Add("result", new object[]
    {
        // Turn the serviceObject into ImmutableDictionary<string, object>
        serviceObject.ToImmutable(),
    }.ToImmutableArray());
}

Where the Helm.V3.Chart is created, I am able to get the IP / hostname of the loadbalancer defined by the service.

// nginxIngress is the Helm.V3.Chart

// get the IP or hostname of the LoadBalancer from the Kubernetes Service
var service = nginxIngress.GetResource<Pulumi.Kubernetes.Core.V1.Service>(
    // Name is derived from the nginxIngress chart name
    $"{name}-ingress-nginx-controller"
    , namespaceName: "kube-system"
);

// Output
LoadBalancerEndpoint = service
    .Apply(svc => svc.Status)
    .Apply(sts => sts.LoadBalancer.Ingress[0].GetHostnameOrIp());

Inside my unit test I can then get the provided IP address from the service and verify it is the same

var ingressStack = resources.OfType<IngressStack>().Single();
var loadBalancerEndpoint = await ingressStack.LoadBalancerEndpoint.GetValueAsync();
loadBalancerEndpoint.Should().Be(ipAddress);

t0yv0 commented 3 years ago

Ah, very nice!

My colleague @lblackstone had a pro tip here to consider local charts: https://www.pulumi.com/docs/reference/pkg/kubernetes/helm/v3/chart/#local-chart-directory

That sounds to be another way to get a local yaml in the picture. Would that help at all?

Your code makes sense and I see the value but it worries me a little we are using the magic "result" property that seems to be an implementation detail that can change over time.

vipentti commented 3 years ago

Unfortunately local charts also have the same problem when using them inside unit tests, because the InvokeAsync ends up calling the mocked IMocks.CallAsync instead of actually invoking Helm.

I agree that the result property is not ideal since it is not particularly clear where it comes from and why it is required. But for now it at least gives me the ability to write unit tests for stacks with Helm charts, even if the resources actually created by the chart are not available.

Perhaps at some point some higher level abstractions can be provided for unit testing, which wrap the current IMocks.CallAsync and IMocks.NewResourceAsync since both of them are quite "low-level" in that they require knowledge of Pulumi internals a bit.

For example using a StackReference in a stack which is being unit tested require adding specific properties to the state returned from IMocks.NewResourceAsync. e.g.

// StackReferences need both secretOutputNames and outputs configured
if (type.Contains("StackReference"))
{
    // These names match the names in the Output-attribute for each property
    outputs.Add("secretOutputNames", Array.Empty<string>().ToImmutableArray());
    outputs.Add("outputs", new Dictionary<string, object>()
    {
        { "KubeConfig", "KubeConfig" },
        { "ClusterName", "ClusterName" },
    }.ToImmutableDictionary());
}

t0yv0 commented 3 years ago

Perhaps at some point some higher level abstractions can be provided for unit testing

Right! Let me circle back here on what's available after I sync up with @mikhailshilkov tomorrow.

t0yv0 commented 3 years ago

I don't have too many good news yet around the testing facilities at the moment. It sounds like selective per-resource mocking by delegating to the engine a bit difficult due to interaction with engine state. And there are no helpers currently provided for typed access to what you might want to return or consume in IMocks.

I'll be sure to follow up on this.

For the moment, if you don't mind, I will close the issue as we maybe have worked around sufficiently in https://github.com/t0yv0/pulumi-helm-chart-unittest-issue

pulumi / pulumi-kubernetes