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

Panic on non-string kubeconfig #1032

Closed
lukehoban opened this issue Mar 15, 2020 · 4 comments
Closed

Panic on non-string kubeconfig #1032

lukehoban opened this issue Mar 15, 2020 · 4 comments
Assignees
Labels
impact/panic This bug represents a panic or unexpected crash kind/bug Some behavior is incorrect or out of spec resolution/duplicate This issue is a duplicate of another issue

Comments

@lukehoban
Copy link
Contributor

A user reported the panic below. This appears to be due to the kubeconfig property being set to something other than a string, and a call to .StringValue() panicing.

In general, there are a lot of calls to .StringValue() and similar in this package that are not guarded with .IsString() checks and are thus likely to panic instead of cleanly error on malformed inputs. We likely should audit (and test) these more holistically.

    panic: interface conversion: interface {} is resource.PropertyMap, not string
    goroutine 28 [running]:
    github.com/pulumi/pulumi/pkg/resource.PropertyValue.StringValue(...)
    	/home/travis/gopath/pkg/mod/github.com/pulumi/[email protected]/pkg/resource/properties.go:359
    github.com/pulumi/pulumi-kubernetes/pkg/provider.parseKubeconfigPropertyValue(0x2386280, 0xc0001fea50, 0x2475423, 0xa, 0xc0001b2508)
    	/home/travis/gopath/src/github.com/pulumi/pulumi-kubernetes/pkg/provider/util.go:85 +0x169
    github.com/pulumi/pulumi-kubernetes/pkg/provider.(*kubeProvider).DiffConfig(0xc000014000, 0x26e1260, 0xc0001fe9f0, 0xc0001380e0, 0xc000014000, 0x2275301, 0xc00031a0c0)
    	/home/travis/gopath/src/github.com/pulumi/pulumi-kubernetes/pkg/provider/provider.go:278 +0x61b
    github.com/pulumi/pulumi/sdk/proto/go._ResourceProvider_DiffConfig_Handler.func1(0x26e1260, 0xc0001fe9f0, 0x23a5e80, 0xc0001380e0, 0x23c0a00, 0x33070c0, 0x26e1260, 0xc0001fe9f0)
    	/home/travis/gopath/pkg/mod/github.com/pulumi/[email protected]/sdk/proto/go/provider.pb.go:1504 +0x86
    github.com/grpc-ecosystem/grpc-opentracing/go/otgrpc.OpenTracingServerInterceptor.func1(0x26e1260, 0xc000577200, 0x23a5e80, 0xc0001380e0, 0xc00000cb20, 0xc00000cb40, 0x0, 0x0, 0x26a05e0, 0xc0000cf7b0)
    	/home/travis/gopath/pkg/mod/github.com/grpc-ecosystem/[email protected]/go/otgrpc/server.go:61 +0x36e
    github.com/pulumi/pulumi/sdk/proto/go._ResourceProvider_DiffConfig_Handler(0x2402f60, 0xc000014000, 0x26e1260, 0xc000577200, 0xc0000de3c0, 0xc0004d2040, 0x26e1260, 0xc000577200, 0xc000331300, 0x101f)
    	/home/travis/gopath/pkg/mod/github.com/pulumi/[email protected]/sdk/proto/go/provider.pb.go:1506 +0x14b
    google.golang.org/grpc.(*Server).processUnaryRPC(0xc00034e300, 0x26fdc00, 0xc00045b500, 0xc00015a200, 0xc000436180, 0x32d3258, 0x0, 0x0, 0x0)
    	/home/travis/gopath/pkg/mod/google.golang.org/[email protected]/server.go:998 +0x46a
    google.golang.org/grpc.(*Server).handleStream(0xc00034e300, 0x26fdc00, 0xc00045b500, 0xc00015a200, 0x0)
    	/home/travis/gopath/pkg/mod/google.golang.org/[email protected]/server.go:1278 +0xd97
    google.golang.org/grpc.(*Server).serveStreams.func1.1(0xc00039bd30, 0xc00034e300, 0x26fdc00, 0xc00045b500, 0xc00015a200)
    	/home/travis/gopath/pkg/mod/google.golang.org/[email protected]/server.go:717 +0xbb
    created by google.golang.org/grpc.(*Server).serveStreams.func1
    	/home/travis/gopath/pkg/mod/google.golang.org/[email protected]/server.go:715 +0xa1
@metral
Copy link
Contributor

metral commented Mar 24, 2020

This occurs when the kubeconfig used in the k8s provider is not stringified, and instead is passed as an object of Output<t>.

The workaround is to use a string

const kubeconfig = cluster.kubeconfig.apply(JSON.stringify)
const provider = new k8s.Provider("provider", {kubeconfig: kubeconfig});

Related: pulumi/pulumi-eks#359

@lukehoban
Copy link
Contributor Author

We should do one or both of:

  1. Have a better error message here (type check in our code, not let this get through to serialization code to report the error).
  2. Actually support string | object here, and JSON-stringify an object input. I'm not exactly sure what this looks like in a purely cross-platform compatible form, and it's likely something we want to solve for as a general schema model concept.

@EronWright
Copy link
Contributor

EronWright commented Jan 27, 2024

Closing this as a duplicate of #1231, which was resolved by adding support for an object-typed resource value.

Note that #2771 adds new tests for handling of an object-typed kubeconfig.

@EronWright EronWright closed this as not planned Won't fix, can't repro, duplicate, stale Jan 27, 2024
@pulumi-bot pulumi-bot reopened this Jan 27, 2024
@pulumi-bot

This comment was marked as resolved.

@EronWright EronWright added resolution/fixed This issue was fixed resolution/duplicate This issue is a duplicate of another issue and removed resolution/fixed This issue was fixed labels Jan 27, 2024
@EronWright EronWright closed this as not planned Won't fix, can't repro, duplicate, stale Jan 27, 2024
EronWright added a commit that referenced this issue Jan 27, 2024
…2771)

<!--Thanks for your contribution. See [CONTRIBUTING](CONTRIBUTING.md)
    for Pulumi's contribution guidelines.

    Help us merge your changes more quickly by adding more details such
    as labels, milestones, and reviewers.-->

### Proposed changes

<!--Give us a brief description of what you've done and what it solves.
-->

This PR refactors the kubeconfig loading code to be more consistent
between `DiffConfig` and `Configure`. Previously the `DiffConfig` method
did not support file-based kubeconfigs, and would throw a spurious
parser error.

`DiffConfig` loads the kubeconfig to be able to detect cluster
replacement, by comparing the active cluster info. Unlike `Configure`,
it doesn't have support for ambient kubeconfigs. When an ambient config
is in use, or the config is invalid (e.g. the `context` or `cluster`
properties refer to a non-existent entry), then the replacement logic is
skipped.

This PR also fixes a panic in `DiffConfig` that would occur if `cluster`
or `context` is a computed value; the provider will conservatively
suggest replacement as it does when `kubeconfig` is a computed value.

### Testing
New test cases were added for cluster change detection.

New tests were added for the parsing code that was consolidated into
`utils.go`. The tests now cover file-, string-, and object-based
resource property values for `kubeconfig`.

### Related issues (optional)

<!--Refer to related PRs or issues: #1234, or 'Fixes #1234' or 'Closes
#1234'.
Or link to full URLs to issues or pull requests in other GitHub
repositories. -->
Closes: #2663 
Related: #1032
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/panic This bug represents a panic or unexpected crash kind/bug Some behavior is incorrect or out of spec resolution/duplicate This issue is a duplicate of another issue
Projects
None yet
Development

No branches or pull requests

6 participants