Skip to content

Commit

Permalink
Merge #12303
Browse files Browse the repository at this point in the history
12303: sdk/go: Fix panic from uninitialized parent resources r=abhinav a=abhinav

# Description

The Go SDK panics if RegisterResource is called with a parent resource
that is uninitialized
(hasn't had its RegisterResource or RegisterComponentResource call yet).

The panic is during alias collapsing:
the system fails to read the unintialized parent resource's URN
from a zero-valued URNOutput.

Guard against this panic by verifying that the value passed to `Parent`
has been initialized.
We do this by checking the name of the resource--names are not allowed
to be empty when a resource is registered.

Resolves #12138

## Checklist

- [x] I have added tests that prove my fix is effective or that my feature works
- [x] I have run `make changelog` and committed the `changelog/pending/<file>` documenting my change


Co-authored-by: Abhinav Gupta <[email protected]>
  • Loading branch information
bors[bot] and abhinav authored Mar 2, 2023
2 parents b62356e + 043707f commit bba7a23
Show file tree
Hide file tree
Showing 3 changed files with 127 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
changes:
- type: fix
scope: sdk/go
description: Fix panic from attempting to create a resource with an uninitialized parent resource.
20 changes: 20 additions & 0 deletions sdk/go/pulumi/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -803,6 +803,26 @@ func (ctx *Context) registerResource(
}

options := merge(opts...)

if parent := options.Parent; parent != nil && parent.URN().getState() == nil {
// Guard against uninitialized parent resources to prevent
// panics from invalid state further down the line.
// Uninitialized parent resources won't have a URN.

resourceType := "resource"
registerMethod := "RegisterResource"
if _, parentIsCustom := parent.(CustomResource); !parentIsCustom {
resourceType = "component resource"
registerMethod = "RegisterComponentResource"
}
err := ctx.Log.Warn(fmt.Sprintf(
"Ignoring %v %T (parent of %v :: %v) because it was not registered with %v",
resourceType, parent, name, t, registerMethod), nil /* args */)
contract.IgnoreError(err)

options.Parent = nil
}

parent := options.Parent
if options.Parent == nil {
options.Parent = ctx.stack
Expand Down
103 changes: 103 additions & 0 deletions sdk/go/pulumi/resource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@
package pulumi

import (
"bytes"
"context"
"fmt"
"log"
"reflect"
"sync"
"testing"
Expand Down Expand Up @@ -552,6 +554,107 @@ func TestNewResourceInput(t *testing.T) {
assert.Equal(t, "abracadabra", unpackedRes.foo)
}

// Verifies that a Parent resource that has not been initialized will panic,
// and will instead report a meaningful error message.
func TestUninitializedParentResource(t *testing.T) {
t.Parallel()

type myComponent struct {
ResourceState
}

type myCustomResource struct {
CustomResourceState
}

tests := []struct {
desc string
parent Resource

// additional options to pass to RegisterResource
// besides the Parent.
// The original report of the panic was with an Alias option.
opts []ResourceOption

// Message that should be part of the error message.
wantErr string
}{
{
desc: "component resource",
parent: &myComponent{},
wantErr: "WARNING: Ignoring component resource *pulumi.myComponent " +
"(parent of my-resource :: test:index:MyResource) " +
"because it was not registered with RegisterComponentResource",
},
{
desc: "component resource/alias",
parent: &myComponent{},
opts: []ResourceOption{
Aliases([]Alias{
{Name: String("alias1")},
}),
},
wantErr: "WARNING: Ignoring component resource *pulumi.myComponent " +
"(parent of my-resource :: test:index:MyResource) " +
"because it was not registered with RegisterComponentResource",
},
{
desc: "custom resource",
parent: &myCustomResource{},
wantErr: "WARNING: Ignoring resource *pulumi.myCustomResource " +
"(parent of my-resource :: test:index:MyResource) " +
"because it was not registered with RegisterResource",
},
{
desc: "custom resource/alias",
parent: &myCustomResource{},
opts: []ResourceOption{
Aliases([]Alias{
{Name: String("alias1")},
}),
},
wantErr: "WARNING: Ignoring resource *pulumi.myCustomResource " +
"(parent of my-resource :: test:index:MyResource) " +
"because it was not registered with RegisterResource",
},
}

for _, tt := range tests {
tt := tt
t.Run(tt.desc, func(t *testing.T) {
t.Parallel()

require.NotEmpty(t, tt.wantErr,
"test case must specify an error message")

err := RunErr(func(ctx *Context) error {
// This is a hack.
// We're accesing context mock internals
// because the mock API does not expose a way
// to set the logger.
//
// If this ever becomes a problem,
// add a way to supply a logger to the mock
// and use that here.
var buff bytes.Buffer
ctx.engine.(*mockEngine).logger = log.New(&buff, "", 0)

opts := []ResourceOption{Parent(tt.parent)}
opts = append(opts, tt.opts...)

var res testRes
require.NoError(t, ctx.RegisterResource(
"test:index:MyResource",
"my-resource",
nil /* props */, &res, opts...))
assert.Contains(t, buff.String(), tt.wantErr)
return nil
}, WithMocks("project", "stack", &testMonitor{}))
assert.NoError(t, err)
})
}
}

func TestDependsOnInputs(t *testing.T) {
t.Parallel()

Expand Down

0 comments on commit bba7a23

Please sign in to comment.