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.

Sign up for GitHub

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

Jump to bottom

[engine] Only record a resource's chosen alias. #9288

Merged
merged 2 commits into from Mar 28, 2022
+80 −46
Merged

[engine] Only record a resource's chosen alias. #9288

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3442463
[engine] Only record a resource's chosen alias.
pgavlin Mar 24, 2022
0890634
PR feedback
pgavlin Mar 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG_PENDING.md
View file
Open in desktop
@@ -1,5 +1,10 @@
### Improvements

- When a resource is aliased to an existing resource with a different URN, only store
the alias of the existing resource in the statefile rather than storing all possible
aliases.
[#9288](https://github.com/pulumi/pulumi/pull/9288)

- Clear pending operations during `pulumi refresh` or `pulumi up -r`.
[#8435](https://github.com/pulumi/pulumi/pull/8435)

Expand Down
11 changes: 8 additions & 3 deletions pkg/engine/journal.go
View file
Open in desktop
Expand Up @@ -28,7 +28,7 @@ type JournalEntry struct {

type JournalEntries []JournalEntry

func (entries JournalEntries) Snap(base *deploy.Snapshot) *deploy.Snapshot {
func (entries JournalEntries) Snap(base *deploy.Snapshot) (*deploy.Snapshot, error) {
// Build up a list of current resources by replaying the journal.
resources, dones := []*resource.State{}, make(map[*resource.State]bool)
ops, doneOps := []resource.Operation{}, make(map[*resource.State]bool)
Expand Down Expand Up @@ -130,8 +130,13 @@ func (entries JournalEntries) Snap(base *deploy.Snapshot) *deploy.Snapshot {

manifest := deploy.Manifest{}
manifest.Magic = manifest.NewMagic()
return deploy.NewSnapshot(manifest, secretsManager, resources, operations)

snap := deploy.NewSnapshot(manifest, secretsManager, resources, operations)
err := snap.NormalizeURNReferences()
if err != nil {
return snap, err
}
return snap, snap.VerifyIntegrity()
}

type Journal struct {
Expand Down Expand Up @@ -189,7 +194,7 @@ func (j *Journal) RecordPlugin(plugin workspace.PluginInfo) error {
return nil
}

func (j *Journal) Snap(base *deploy.Snapshot) *deploy.Snapshot {
func (j *Journal) Snap(base *deploy.Snapshot) (*deploy.Snapshot, error) {
return j.entries.Snap(base)
}

Expand Down
5 changes: 4 additions & 1 deletion pkg/engine/lifeycletest/golang_sdk_test.go
View file
Open in desktop
Expand Up @@ -250,7 +250,10 @@ func TestSingleResourceDefaultProviderGolangTransformations(t *testing.T) {
foundRes3 := false
foundRes4Child := false
// foundRes5Child1 := false
for _, res := range entries.Snap(target.Snapshot).Resources {

snap, err := entries.Snap(target.Snapshot)
require.NoError(t, err)
for _, res := range snap.Resources {
// "res1" has a transformation which adds additionalSecretOutputs
if res.URN.Name() == "res1" {
foundRes1 = true
Expand Down
8 changes: 6 additions & 2 deletions pkg/engine/lifeycletest/provider_test.go
View file
Open in desktop
Expand Up @@ -133,7 +133,9 @@ func TestSingleResourceDefaultProviderUpgrade(t *testing.T) {
t.Fatalf("unexpected resource %v", urn)
}
}
assert.Len(t, entries.Snap(target.Snapshot).Resources, 2)
snap, err := entries.Snap(target.Snapshot)
require.NoError(t, err)
assert.Len(t, snap.Resources, 2)
return res
}

Expand Down Expand Up @@ -166,7 +168,9 @@ func TestSingleResourceDefaultProviderUpgrade(t *testing.T) {
}
}
assert.Len(t, deleted, 2)
assert.Len(t, entries.Snap(target.Snapshot).Resources, 0)
snap, err := entries.Snap(target.Snapshot)
require.NoError(t, err)
assert.Len(t, snap.Resources, 0)
return res
},
}}
Expand Down
58 changes: 27 additions & 31 deletions pkg/engine/lifeycletest/pulumi_test.go
View file
Open in desktop
Expand Up @@ -1302,7 +1302,7 @@ func TestAliases(t *testing.T) {
}

updateProgramWithResource := func(
snap *deploy.Snapshot, resources []Resource, allowedOps []deploy.StepOp) *deploy.Snapshot {
snap *deploy.Snapshot, resources []Resource, allowedOps []deploy.StepOp, expectFailure bool) *deploy.Snapshot {
program := deploytest.NewLanguageRuntime(func(_ plugin.RunInfo, monitor *deploytest.ResourceMonitor) error {
err := registerResources(t, monitor, resources)
return err
Expand All @@ -1312,7 +1312,8 @@ func TestAliases(t *testing.T) {
Options: UpdateOptions{Host: host},
Steps: []TestStep{
{
Op: Update,
Op: Update,
ExpectFailure: expectFailure,
Validate: func(project workspace.Project, target deploy.Target, entries JournalEntries,
events []Event, res result.Result) result.Result {
for _, event := range events {
Expand Down Expand Up @@ -1347,14 +1348,14 @@ func TestAliases(t *testing.T) {
snap := updateProgramWithResource(nil, []Resource{{
t: "pkgA:index:t1",
name: "n1",
}}, []deploy.StepOp{deploy.OpCreate})
}}, []deploy.StepOp{deploy.OpCreate}, false)

// Ensure that rename produces Same
snap = updateProgramWithResource(snap, []Resource{{
t: "pkgA:index:t1",
name: "n2",
aliases: []resource.URN{"urn:pulumi:test::test::pkgA:index:t1::n1"},
}}, []deploy.StepOp{deploy.OpSame})
}}, []deploy.StepOp{deploy.OpSame}, false)

// Ensure that rename produces Same with multiple aliases
snap = updateProgramWithResource(snap, []Resource{{
Expand All @@ -1364,7 +1365,7 @@ func TestAliases(t *testing.T) {
"urn:pulumi:test::test::pkgA:index:t1::n1",
"urn:pulumi:test::test::pkgA:index:t1::n2",
},
}}, []deploy.StepOp{deploy.OpSame})
}}, []deploy.StepOp{deploy.OpSame}, false)

// Ensure that rename produces Same with multiple aliases (reversed)
snap = updateProgramWithResource(snap, []Resource{{
Expand All @@ -1374,7 +1375,7 @@ func TestAliases(t *testing.T) {
"urn:pulumi:test::test::pkgA:index:t1::n2",
"urn:pulumi:test::test::pkgA:index:t1::n1",
},
}}, []deploy.StepOp{deploy.OpSame})
}}, []deploy.StepOp{deploy.OpSame}, false)

// Ensure that aliasing back to original name is okay
snap = updateProgramWithResource(snap, []Resource{{
Expand All @@ -1385,13 +1386,13 @@ func TestAliases(t *testing.T) {
"urn:pulumi:test::test::pkgA:index:t1::n2",
"urn:pulumi:test::test::pkgA:index:t1::n1",
},
}}, []deploy.StepOp{deploy.OpSame})
}}, []deploy.StepOp{deploy.OpSame}, false)

// Ensure that removing aliases is okay (once old names are gone from all snapshots)
snap = updateProgramWithResource(snap, []Resource{{
t: "pkgA:index:t1",
name: "n1",
}}, []deploy.StepOp{deploy.OpSame})
}}, []deploy.StepOp{deploy.OpSame}, false)

// Ensure that changing the type works
snap = updateProgramWithResource(snap, []Resource{{
Expand All @@ -1400,7 +1401,7 @@ func TestAliases(t *testing.T) {
aliases: []resource.URN{
"urn:pulumi:test::test::pkgA:index:t1::n1",
},
}}, []deploy.StepOp{deploy.OpSame})
}}, []deploy.StepOp{deploy.OpSame}, false)

// Ensure that changing the type again works
snap = updateProgramWithResource(snap, []Resource{{
Expand All @@ -1410,7 +1411,7 @@ func TestAliases(t *testing.T) {
"urn:pulumi:test::test::pkgA:index:t1::n1",
"urn:pulumi:test::test::pkgA:index:t2::n1",
},
}}, []deploy.StepOp{deploy.OpSame})
}}, []deploy.StepOp{deploy.OpSame}, false)

// Ensure that order of aliases doesn't matter
snap = updateProgramWithResource(snap, []Resource{{
Expand All @@ -1421,13 +1422,13 @@ func TestAliases(t *testing.T) {
"urn:pulumi:test::test::pkgA:othermod:t3::n1",
"urn:pulumi:test::test::pkgA:index:t2::n1",
},
}}, []deploy.StepOp{deploy.OpSame})
}}, []deploy.StepOp{deploy.OpSame}, false)

// Ensure that removing aliases is okay (once old names are gone from all snapshots)
snap = updateProgramWithResource(snap, []Resource{{
t: "pkgA:othermod:t3",
name: "n1",
}}, []deploy.StepOp{deploy.OpSame})
}}, []deploy.StepOp{deploy.OpSame}, false)

// Ensure that changing everything (including props) leads to update not delete and re-create
snap = updateProgramWithResource(snap, []Resource{{
Expand All @@ -1439,7 +1440,7 @@ func TestAliases(t *testing.T) {
aliases: []resource.URN{
"urn:pulumi:test::test::pkgA:othermod:t3::n1",
},
}}, []deploy.StepOp{deploy.OpUpdate})
}}, []deploy.StepOp{deploy.OpUpdate}, false)

// Ensure that changing everything again (including props) leads to update not delete and re-create
snap = updateProgramWithResource(snap, []Resource{{
Expand All @@ -1451,7 +1452,7 @@ func TestAliases(t *testing.T) {
aliases: []resource.URN{
"urn:pulumi:test::test::pkgA:index:t4::n2",
},
}}, []deploy.StepOp{deploy.OpUpdate})
}}, []deploy.StepOp{deploy.OpUpdate}, false)

// Ensure that changing a forceNew property while also changing type and name leads to replacement not delete+create
snap = updateProgramWithResource(snap, []Resource{{
Expand All @@ -1463,7 +1464,7 @@ func TestAliases(t *testing.T) {
aliases: []resource.URN{
"urn:pulumi:test::test::pkgA:index:t5::n3",
},
}}, []deploy.StepOp{deploy.OpReplace, deploy.OpCreateReplacement, deploy.OpDeleteReplaced})
}}, []deploy.StepOp{deploy.OpReplace, deploy.OpCreateReplacement, deploy.OpDeleteReplaced}, false)

// Ensure that changing a forceNew property and deleteBeforeReplace while also changing type and name leads to
// replacement not delete+create
Expand All @@ -1477,7 +1478,7 @@ func TestAliases(t *testing.T) {
aliases: []resource.URN{
"urn:pulumi:test::test::pkgA:index:t6::n4",
},
}}, []deploy.StepOp{deploy.OpReplace, deploy.OpCreateReplacement, deploy.OpDeleteReplaced})
}}, []deploy.StepOp{deploy.OpReplace, deploy.OpCreateReplacement, deploy.OpDeleteReplaced}, false)

// Start again - this time with two resources with depends on relationship
snap = updateProgramWithResource(nil, []Resource{{
Expand All @@ -1491,7 +1492,7 @@ func TestAliases(t *testing.T) {
t: "pkgA:index:t2",
name: "n2",
dependencies: []resource.URN{"urn:pulumi:test::test::pkgA:index:t1::n1"},
}}, []deploy.StepOp{deploy.OpCreate})
}}, []deploy.StepOp{deploy.OpCreate}, false)

_ = updateProgramWithResource(snap, []Resource{{
t: "pkgA:index:t1-new",
Expand All @@ -1510,7 +1511,7 @@ func TestAliases(t *testing.T) {
aliases: []resource.URN{
"urn:pulumi:test::test::pkgA:index:t2::n2",
},
}}, []deploy.StepOp{deploy.OpSame, deploy.OpReplace, deploy.OpCreateReplacement, deploy.OpDeleteReplaced})
}}, []deploy.StepOp{deploy.OpSame, deploy.OpReplace, deploy.OpCreateReplacement, deploy.OpDeleteReplaced}, false)

// Start again - this time with two resources with parent relationship
snap = updateProgramWithResource(nil, []Resource{{
Expand All @@ -1524,7 +1525,7 @@ func TestAliases(t *testing.T) {
t: "pkgA:index:t2",
name: "n2",
parent: resource.URN("urn:pulumi:test::test::pkgA:index:t1::n1"),
}}, []deploy.StepOp{deploy.OpCreate})
}}, []deploy.StepOp{deploy.OpCreate}, false)

_ = updateProgramWithResource(snap, []Resource{{
t: "pkgA:index:t1-new",
Expand All @@ -1543,27 +1544,22 @@ func TestAliases(t *testing.T) {
aliases: []resource.URN{
"urn:pulumi:test::test::pkgA:index:t1$pkgA:index:t2::n2",
},
}}, []deploy.StepOp{deploy.OpSame, deploy.OpReplace, deploy.OpCreateReplacement, deploy.OpDeleteReplaced})
}}, []deploy.StepOp{deploy.OpSame, deploy.OpReplace, deploy.OpCreateReplacement, deploy.OpDeleteReplaced}, false)

// ensure failure when different resources use duplicate aliases
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should still test ok now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an error we can no longer detect. The setup for this test is... odd. It starts from an empty base state and wants to validate that two resources cannot mention the same alias. The only way to do that from the statefile alone is to always store all of the aliases in the statefile. In order to support this check, we'd need to move the handling of that specific case into the step generator. I did this in a different branch, but I got a little bit more ambitious there and fixed a few other bugs I found along the way: https://github.com/pulumi/pulumi/compare/pgavlin/choose-single-alias

IMO, this specific check is relatively low-value, as it's really only covering the case where resources that have no base state cannot be mention the same alias. If the resources are both aliased to the same base state, then we will still issue an error (just added a test for that).

There is one interesting case that we don't catch in either case: given the base state [A], it is possible to register a resource A and a resource B that is aliased to A. This is bad, and we should fix it, but that requires fixing a nasty bug in provider aliasing. That bug is an easy fix, I think (a possible fix is included in that other branch), but I'd like to separate it out nonetheless.

snap = updateProgramWithResource(nil, []Resource{{
_ = updateProgramWithResource(snap, []Resource{{
t: "pkgA:index:t1",
name: "n1",
name: "n2",
aliases: []resource.URN{
"urn:pulumi:test::test::pkgA:index:t1::n1",
},
}, {
t: "pkgA:index:t2",
name: "n2",
name: "n3",
aliases: []resource.URN{
"urn:pulumi:test::test::pkgA:index:t1::n1",
},
}}, []deploy.StepOp{deploy.OpCreate})

err := snap.NormalizeURNReferences()
assert.Equal(t, err.Error(),
"Two resources ('urn:pulumi:test::test::pkgA:index:t1::n1'"+
" and 'urn:pulumi:test::test::pkgA:index:t2::n2') aliased to the same: 'urn:pulumi:test::test::pkgA:index:t1::n1'")
}}, []deploy.StepOp{deploy.OpCreate}, true)

// ensure different resources can use different aliases
snap = updateProgramWithResource(nil, []Resource{{
Expand All @@ -1578,9 +1574,9 @@ func TestAliases(t *testing.T) {
aliases: []resource.URN{
"urn:pulumi:test::test::pkgA:index:t1::n2",
},
}}, []deploy.StepOp{deploy.OpCreate})
}}, []deploy.StepOp{deploy.OpCreate}, false)

err = snap.NormalizeURNReferences()
err := snap.NormalizeURNReferences()
assert.Nil(t, err)
}

Expand Down
31 changes: 23 additions & 8 deletions pkg/engine/lifeycletest/test_plan.go
View file
Open in desktop
Expand Up @@ -8,6 +8,7 @@ import (

"github.com/mitchellh/copystructure"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

. "github.com/pulumi/pulumi/pkg/v3/engine"
"github.com/pulumi/pulumi/pkg/v3/resource/deploy"
Expand Down Expand Up @@ -121,8 +122,10 @@ func (op TestOp) runWithContext(
return plan, nil, res
}

snap := journal.Snap(target.Snapshot)
if res == nil && snap != nil {
snap, err := journal.Snap(target.Snapshot)
if res == nil && err != nil {
res = result.FromError(err)
} else if res == nil && snap != nil {
res = result.WrapIfNonNil(snap.VerifyIntegrity())
}
return nil, snap, res
Expand Down Expand Up @@ -290,7 +293,9 @@ func MakeBasicLifecycleSteps(t *testing.T, resCount int) []TestStep {
op := entry.Step.Op()
assert.True(t, op == deploy.OpCreate || op == deploy.OpRead)
}
assert.Len(t, entries.Snap(target.Snapshot).Resources, resCount)
snap, err := entries.Snap(target.Snapshot)
require.NoError(t, err)
assert.Len(t, snap.Resources, resCount)
return res
},
},
Expand All @@ -305,7 +310,9 @@ func MakeBasicLifecycleSteps(t *testing.T, resCount int) []TestStep {
assert.Equal(t, deploy.OpRefresh, entry.Step.Op())
assert.Equal(t, deploy.OpSame, entry.Step.(*deploy.RefreshStep).ResultOp())
}
assert.Len(t, entries.Snap(target.Snapshot).Resources, resCount)
snap, err := entries.Snap(target.Snapshot)
require.NoError(t, err)
assert.Len(t, snap.Resources, resCount)
return res
},
},
Expand All @@ -320,7 +327,9 @@ func MakeBasicLifecycleSteps(t *testing.T, resCount int) []TestStep {
op := entry.Step.Op()
assert.True(t, op == deploy.OpSame || op == deploy.OpRead)
}
assert.Len(t, entries.Snap(target.Snapshot).Resources, resCount)
snap, err := entries.Snap(target.Snapshot)
require.NoError(t, err)
assert.Len(t, snap.Resources, resCount)
return res
},
},
Expand All @@ -335,7 +344,9 @@ func MakeBasicLifecycleSteps(t *testing.T, resCount int) []TestStep {
assert.Equal(t, deploy.OpRefresh, entry.Step.Op())
assert.Equal(t, deploy.OpSame, entry.Step.(*deploy.RefreshStep).ResultOp())
}
assert.Len(t, entries.Snap(target.Snapshot).Resources, resCount)
snap, err := entries.Snap(target.Snapshot)
require.NoError(t, err)
assert.Len(t, snap.Resources, resCount)
return res
},
},
Expand All @@ -354,7 +365,9 @@ func MakeBasicLifecycleSteps(t *testing.T, resCount int) []TestStep {
assert.Fail(t, "expected OpDelete or OpReadDiscard")
}
}
assert.Len(t, entries.Snap(target.Snapshot).Resources, 0)
snap, err := entries.Snap(target.Snapshot)
require.NoError(t, err)
assert.Len(t, snap.Resources, 0)
return res
},
},
Expand All @@ -365,7 +378,9 @@ func MakeBasicLifecycleSteps(t *testing.T, resCount int) []TestStep {
_ []Event, res result.Result) result.Result {

assert.Len(t, entries, 0)
assert.Len(t, entries.Snap(target.Snapshot).Resources, 0)
snap, err := entries.Snap(target.Snapshot)
require.NoError(t, err)
assert.Len(t, snap.Resources, 0)
return res
},
},
Expand Down