Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AllowNullValues causes a panic when there's a null value #2622

Closed
martinjt opened this issue Oct 23, 2023 · 2 comments · Fixed by pulumi/pulumi#14498
Closed

AllowNullValues causes a panic when there's a null value #2622

martinjt opened this issue Oct 23, 2023 · 2 comments · Fixed by pulumi/pulumi#14498
Assignees
Labels
area/helm impact/panic This bug represents a panic or unexpected crash kind/bug Some behavior is incorrect or out of spec p1 A bug severe enough to be the next item assigned to an engineer resolution/fixed This issue was fixed
Milestone

Comments

@martinjt
Copy link

martinjt commented Oct 23, 2023

What happened?

When I add this:

config:
  receivers:
    zipkin: null

To a helm values file, with the setting AllowNullValues on the resource, I get an exception when applying.

   panic: fatal: A precondition has failed for vsrc: value must be valid
    goroutine 63 [running]:
    github.com/pulumi/pulumi/sdk/v3/go/common/util/contract.failfast(...)
        /home/runner/go/pkg/mod/github.com/pulumi/pulumi/sdk/v3@v3.81.0/go/common/util/contract/failfast.go:23
    github.com/pulumi/pulumi/sdk/v3/go/common/util/contract.Requiref(0xd0?, {0x2650fa4?, 0x15?}, {0x26707fd, 0x13}, {0x0, 0x0, 0x0})
        /home/runner/go/pkg/mod/github.com/pulumi/pulumi/sdk/v3@v3.81.0/go/common/util/contract/require.go:35 +0x199
    github.com/pulumi/pulumi/sdk/v3/go/common/util/mapper.(*mapper).encodeValue(0x221f680?, {0x0?, 0x0?, 0x26707fd?})
        /home/runner/go/pkg/mod/github.com/pulumi/pulumi/sdk/v3@v3.81.0/go/common/util/mapper/mapper_encode.go:85 +0x97
    github.com/pulumi/pulumi/sdk/v3/go/common/util/mapper.(*mapper).encodeValue(0xc001af0750?, {0x221f680?, 0xc003620370?, 0xc001af0730?})
        /home/runner/go/pkg/mod/github.com/pulumi/pulumi/sdk/v3@v3.81.0/go/common/util/mapper/mapper_encode.go:154 +0x2db
    github.com/pulumi/pulumi/sdk/v3/go/common/util/mapper.(*mapper).encodeValue(0x221f680?, {0x2260800?, 0xc001f62540?, 0x26707fd?})
        /home/runner/go/pkg/mod/github.com/pulumi/pulumi/sdk/v3@v3.81.0/go/common/util/mapper/mapper_encode.go:139 +0x8f0
    github.com/pulumi/pulumi/sdk/v3/go/common/util/mapper.(*mapper).encodeValue(0xc001af0ab0?, {0x221f680?, 0xc003620360?, 0xc001af0a90?})
        /home/runner/go/pkg/mod/github.com/pulumi/pulumi/sdk/v3@v3.81.0/go/common/util/mapper/mapper_encode.go:154 +0x2db
    github.com/pulumi/pulumi/sdk/v3/go/common/util/mapper.(*mapper).encodeValue(0x221f680?, {0x2260800?, 0xc001f62510?, 0x26707fd?})
        /home/runner/go/pkg/mod/github.com/pulumi/pulumi/sdk/v3@v3.81.0/go/common/util/mapper/mapper_encode.go:139 +0x8f0
    github.com/pulumi/pulumi/sdk/v3/go/common/util/mapper.(*mapper).encodeValue(0xc001af0e10?, {0x221f680?, 0xc003620350?, 0x5?})
        /home/runner/go/pkg/mod/github.com/pulumi/pulumi/sdk/v3@v3.81.0/go/common/util/mapper/mapper_encode.go:154 +0x2db
    github.com/pulumi/pulumi/sdk/v3/go/common/util/mapper.(*mapper).encodeValue(0x261ff00?, {0x2260800?, 0xc001f4a7a0?, 0x2057b29?})
        /home/runner/go/pkg/mod/github.com/pulumi/pulumi/sdk/v3@v3.81.0/go/common/util/mapper/mapper_encode.go:139 +0x8f0
    github.com/pulumi/pulumi/sdk/v3/go/common/util/mapper.(*mapper).encode(0xc001af1150, {0x20baca0?, 0xc001f4a700?, 0x20?})
        /home/runner/go/pkg/mod/github.com/pulumi/pulumi/sdk/v3@v3.81.0/go/common/util/mapper/mapper_encode.go:54 +0x407
    github.com/pulumi/pulumi/sdk/v3/go/common/util/mapper.(*mapper).Encode(0x0?, {0x20baca0?, 0xc001f4a700?})
        /home/runner/go/pkg/mod/github.com/pulumi/pulumi/sdk/v3@v3.81.0/go/common/util/mapper/mapper_encode.go:28 +0x65
    github.com/pulumi/pulumi/sdk/v3/go/common/util/mapper.Unmap({0x20baca0?, 0xc001f4a700?})
        /home/runner/go/pkg/mod/github.com/pulumi/pulumi/sdk/v3@v3.81.0/go/common/util/mapper/mapper.go:90 +0x146
    github.com/pulumi/pulumi/sdk/v3/go/common/resource.NewPropertyMapRepl({0x20baca0?, 0xc001f4a700?}, 0x93?, 0xc001af1398?)
        /home/runner/go/pkg/mod/github.com/pulumi/pulumi/sdk/v3@v3.81.0/go/common/resource/properties.go:48 +0x27
    github.com/pulumi/pulumi/sdk/v3/go/common/resource.NewPropertyMap(...)
        /home/runner/go/pkg/mod/github.com/pulumi/pulumi/sdk/v3@v3.81.0/go/common/resource/properties.go:40
    github.com/pulumi/pulumi-kubernetes/provider/v4/pkg/provider.(*helmReleaseProvider).Check(0xc002774700, {0x0?, 0x0?}, 0xc001033f80)
        /home/runner/work/pulumi-kubernetes/pulumi-kubernetes/provider/pkg/provider/helm_release.go:381 +0x5c7
    github.com/pulumi/pulumi-kubernetes/provider/v4/pkg/provider.(*kubeProvider).Check(0xc000588f00, {0x2a99618, 0xc001f3d1d0}, 0xc001033f80)
        /home/runner/work/pulumi-kubernetes/pulumi-kubernetes/provider/pkg/provider/provider.go:1295 +0x13fb
    github.com/pulumi/pulumi/sdk/v3/proto/go._ResourceProvider_Check_Handler.func1({0x2a99618, 0xc001f3d1d0}, {0x248d5c0?, 0xc001033f80})
        /home/runner/go/pkg/mod/github.com/pulumi/pulumi/sdk/v3@v3.81.0/proto/go/provider_grpc.pb.go:537 +0x72
    github.com/grpc-ecosystem/grpc-opentracing/go/otgrpc.OpenTracingServerInterceptor.func1({0x2a99618, 0xc001f3c030}, {0x248d5c0, 0xc001033f80}, 0xc0021a87c0, 0xc0020fb7e8)
        /home/runner/go/pkg/mod/github.com/grpc-ecosystem/grpc-opentracing@v0.0.0-20180507213350-8e809c8a8645/go/otgrpc/server.go:57 +0x3d0
    github.com/pulumi/pulumi/sdk/v3/proto/go._ResourceProvider_Check_Handler({0x25e16e0?, 0xc000588f00}, {0x2a99618, 0xc001f3c030}, 0xc00462bb90, 0xc000051380)
        /home/runner/go/pkg/mod/github.com/pulumi/pulumi/sdk/v3@v3.81.0/proto/go/provider_grpc.pb.go:539 +0x135
    google.golang.org/grpc.(*Server).processUnaryRPC(0xc0005d43c0, {0x2aa7920, 0xc000007d40}, 0xc001f32900, 0xc000643e30, 0x42f4670, 0x0)
        /home/runner/go/pkg/mod/google.golang.org/grpc@v1.58.2/server.go:1376 +0xde7
    google.golang.org/grpc.(*Server).handleStream(0xc0005d43c0, {0x2aa7920, 0xc000007d40}, 0xc001f32900, 0x0)
        /home/runner/go/pkg/mod/google.golang.org/grpc@v1.58.2/server.go:1753 +0x9e7
    google.golang.org/grpc.(*Server).serveStreams.func1.1()
        /home/runner/go/pkg/mod/google.golang.org/grpc@v1.58.2/server.go:998 +0x8d
    created by google.golang.org/grpc.(*Server).serveStreams.func1 in goroutine 73
        /home/runner/go/pkg/mod/google.golang.org/grpc@v1.58.2/server.go:996 +0x165

Example

config:
  receivers:
    zipkin: null
        var otelCollectorClusterMetrics = new Release("otel-collector-cluster-metrics", new ReleaseArgs {
            Chart = "opentelemetry-collector",
            Name = "otel-collector-cluster-metrics",
            Version = "0.69.2",
            Namespace = otelColNamespace.Metadata.Apply(m => m.Name),
            RepositoryOpts = new RepositoryOptsArgs {
                Repo = "https://open-telemetry.github.io/opentelemetry-helm-charts"
            },
            DependencyUpdate = true,
            ValueYamlFiles = new FileAsset("./config-files/collector/values-deployment.yaml"),
            Values = values,
            SkipAwait = true,
            AllowNullValues = true
        }, new CustomResourceOptions
        {
            IgnoreChanges = { "resourceNames" },
            Provider = options?.Provider!
        });

More complete example here: https://github.com/martinjt/aks-otel-demo

Output of pulumi about

CLI          
Version      3.89.0
Go Version   go1.21.1
Go Compiler  gc

Plugins
NAME          VERSION
azure-native  2.0.0
azuread       5.37.0
dotnet        unknown
kubernetes    4.4.0
random        4.13.0
tls           4.10.0

Host     
OS       ubuntu
Version  22.04
Arch     x86_64

This project is written in dotnet: executable='/usr/bin/dotnet' version='8.0.100-rc.2.23502.2'

Backend        
Name           Shellington
URL            azblob://aks-demo-state?storage_account=hcpulumistate
User           martin
Organizations  
Token type     personal

Dependencies:
NAME                VERSION
Azure.Core          1.31.0
Pulumi              3.57.0
Pulumi.AzureAD      5.37.0
Pulumi.AzureNative  2.0.0
Pulumi.Kubernetes   4.4.0
Pulumi.Random       4.13.0
Pulumi.Tls          4.10.0

### Additional context

I've also tried this with .NET 6/7 too, so it's not a .NET issue.

### Contributing

Vote on this issue by adding a 👍 reaction. 
To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already). 
@martinjt martinjt added kind/bug Some behavior is incorrect or out of spec needs-triage Needs attention from the triage team labels Oct 23, 2023
@martinjt martinjt changed the title AllowNullValues throws error when there's a null value AllowNullValues causes a panic when there's a null value Oct 23, 2023
@mikhailshilkov mikhailshilkov added p1 A bug severe enough to be the next item assigned to an engineer impact/panic This bug represents a panic or unexpected crash and removed needs-triage Needs attention from the triage team labels Oct 24, 2023
@mikhailshilkov mikhailshilkov added this to the 0.96 milestone Oct 24, 2023
@EronWright EronWright modified the milestones: 0.96, 0.95 Oct 24, 2023
@mikhailshilkov mikhailshilkov modified the milestones: 0.95, 0.96 Oct 26, 2023
@EronWright
Copy link
Contributor

EronWright commented Nov 2, 2023

I am able to reproduce the issue. Here's some relevant background:

Seems like #2220 may have caused a regression, for at least one variation that I haven't isolated yet.

@EronWright
Copy link
Contributor

EronWright commented Nov 3, 2023

The minimal fix for this issue is here: pulumi/pulumi#14498

In the course of investigating this issue, I do see some fidelity issues related to empty lists and nulls:

  1. the provider doesn't return null chart values to the engine even when AllowNullValues is true, which means that the preview is inaccurate (see Empty array value ignored in Helm Chart #2089).
  2. Seems like the provider doesn't send null values to Helm.
  3. Seems like empty lists are converted to nulls in responses to the engine.

github-merge-queue bot pushed a commit to pulumi/pulumi that referenced this issue Nov 4, 2023
<!--- 
Thanks so much for your contribution! If this is your first time
contributing, please ensure that you have read the
[CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md)
documentation.
-->

# Description
Fixes a panic that occurs when attempting to encode certain values, for
example a map that contains a nil value:
```go
	bag := bagtag{
		String:    "something",
		StringOpt: "ohmv",
		MapOpt: map[string]interface{}{
			"a": "something",
			"b": nil,   // this reflect.Value is of type Interface and IsNil is true.
		},
	}
        _ = resource.NewPropertyMap(bag) // panic here
```

New tests are provided to cover the above scenario and to cover each
type of value, including nil cases where appropriate.

Fixes: pulumi/pulumi-kubernetes#2622
Follow-up to: #9810

## Checklist

- [x] I have run `make tidy` to update any new dependencies
- [x] I have run `make lint` to verify my code passes the lint check
  - [ ] I have formatted my code using `gofumpt`

<!--- Please provide details if the checkbox below is to be left
unchecked. -->
- [x] I have added tests that prove my fix is effective or that my
feature works
<!--- 
User-facing changes require a CHANGELOG entry.
-->
- [x] I have run `make changelog` and committed the
`changelog/pending/<file>` documenting my change
<!--
If the change(s) in this PR is a modification of an existing call to the
Pulumi Cloud,
then the service should honor older versions of the CLI where this
change would not exist.
You must then bump the API version in
/pkg/backend/httpstate/client/api.go, as well as add
it to the service.
-->
- [ ] Yes, there are changes in this PR that warrants bumping the Pulumi
Cloud API version
<!-- @pulumi employees: If yes, you must submit corresponding changes in
the service repo. -->
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label Nov 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm impact/panic This bug represents a panic or unexpected crash kind/bug Some behavior is incorrect or out of spec p1 A bug severe enough to be the next item assigned to an engineer resolution/fixed This issue was fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants