Merge #11212

11212: Check for duplicate aliases and plain URNs r=Frassle a=Frassle



<!--- 
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

<!--- Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. -->

We need to check that for example that if a resource X is created that we don't allow another resource Y to be alias against X. Likewise if our old state has X and we then create Y aliased against X we should not then allow X to be created later in the deployment.

Fixes #11173

## Checklist

<!--- 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 Service,
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 Service API version
  <!-- `@Pulumi` employees: If yes, you must submit corresponding changes in the service repo. -->


Co-authored-by: Fraser Waters <fraser@pulumi.com>

changelog/pending/20221101--engine--expand-duplicate-urn-checks-across-direct-urns-and-aliases.yaml

@@ -0,0 +1,4 @@
+changes:
+- type: fix
+  scope: engine
+  description: Expand duplicate URN checks across direct URNs and aliases.

pkg/engine/lifecycletest/pulumi_test.go

@@ -4116,3 +4116,108 @@ func TestPendingDeleteOrder(t *testing.T) {
 	assert.NotNil(t, snap)
 	assert.Len(t, snap.Resources, 3)
 }
+
+func TestDuplicatesDueToAliases(t *testing.T) {
+	t.Parallel()
+
+	// This is a test for https://github.com/pulumi/pulumi/issues/11173
+	// to check that we don't allow resource aliases to refer to other resources.
+	// That is if you have A, then try and add B saying it's alias is A we should error that's a duplicate.
+	// We need to be careful that we handle this regardless of the order we send the RegisterResource requests for A and B.
+
+	loaders := []*deploytest.ProviderLoader{
+		deploytest.NewProviderLoader("pkgA", semver.MustParse("1.0.0"), func() (plugin.Provider, error) {
+			return &deploytest.Provider{
+				CreateF: func(urn resource.URN, news resource.PropertyMap, timeout float64,
+					preview bool) (resource.ID, resource.PropertyMap, resource.Status, error) {
+					return "created-id", news, resource.StatusOK, nil
+				},
+			}, nil
+		}, deploytest.WithoutGrpc),
+	}
+
+	mode := 0
+	program := deploytest.NewLanguageRuntime(func(info plugin.RunInfo, monitor *deploytest.ResourceMonitor) error {
+
+		switch mode {
+		case 0:
+			// Default case, just make resA
+			_, _, _, err := monitor.RegisterResource(
+				"pkgA:m:typA",
+				"resA",
+				true,
+				deploytest.ResourceOptions{})
+			assert.NoError(t, err)
+
+		case 1:
+			// First test case, try and create a new B that aliases to A. First make the A like normal...
+			_, _, _, err := monitor.RegisterResource(
+				"pkgA:m:typA",
+				"resA",
+				true,
+				deploytest.ResourceOptions{})
+			assert.NoError(t, err)
+
+			// ... then make B with an alias, it should error
+			_, _, _, err = monitor.RegisterResource(
+				"pkgA:m:typA",
+				"resB",
+				true,
+				deploytest.ResourceOptions{
+					Aliases: []resource.Alias{{Name: "resA"}},
+				})
+			assert.Error(t, err)
+
+		case 2:
+			// Second test case, try and create a new B that aliases to A. First make the B with an alias...
+			_, _, _, err := monitor.RegisterResource(
+				"pkgA:m:typA",
+				"resB",
+				true,
+				deploytest.ResourceOptions{
+					Aliases: []resource.Alias{{Name: "resA"}},
+				})
+			assert.NoError(t, err)
+
+			// ... then try to make the A like normal. It should error that it's already been aliased away
+			_, _, _, err = monitor.RegisterResource(
+				"pkgA:m:typA",
+				"resA",
+				true,
+				deploytest.ResourceOptions{})
+			assert.Error(t, err)
+		}
+		return nil
+	})
+	host := deploytest.NewPluginHost(nil, nil, program, loaders...)
+
+	p := &TestPlan{
+		Options: UpdateOptions{Host: host},
+	}
+
+	project := p.GetProject()
+
+	// Run an update to create the starting A resource
+	snap, res := TestOp(Update).Run(project, p.GetTarget(t, nil), p.Options, false, p.BackendClient, nil)
+	assert.Nil(t, res)
+	assert.NotNil(t, snap)
+	assert.Len(t, snap.Resources, 2)
+	assert.Equal(t, resource.URN("urn:pulumi:test::test::pkgA:m:typA::resA"), snap.Resources[1].URN)
+
+	// Set mode to try and create A then a B that aliases to it, this should fail
+	mode = 1
+	snap, res = TestOp(Update).Run(project, p.GetTarget(t, snap), p.Options, false, p.BackendClient, nil)
+	assert.NotNil(t, res)
+	assert.NotNil(t, snap)
+	assert.Len(t, snap.Resources, 2)
+	assert.Equal(t, resource.URN("urn:pulumi:test::test::pkgA:m:typA::resA"), snap.Resources[1].URN)
+
+	// Set mode to try and create B first then a A, this should fail
+	mode = 2
+	snap, res = TestOp(Update).Run(project, p.GetTarget(t, snap), p.Options, false, p.BackendClient, nil)
+	assert.NotNil(t, res)
+	assert.NotNil(t, snap)
+	assert.Len(t, snap.Resources, 2)
+	// Because we made the B first that's what should end up in the state file
+	assert.Equal(t, resource.URN("urn:pulumi:test::test::pkgA:m:typA::resB"), snap.Resources[1].URN)
+}

pkg/resource/deploy/step_generator.go

@@ -445,6 +445,12 @@ func (sg *stepGenerator) generateSteps(event RegisterResourceEvent) ([]Step, res
 		}
 	}
 
+	if previousAliasURN, alreadyAliased := sg.aliased[urn]; alreadyAliased {
+		// This resource is claiming to be X but we've already seen another resource claim that via aliases
+		invalid = true
+		sg.deployment.Diag().Errorf(diag.GetDuplicateResourceAliasedError(urn), urn, previousAliasURN)
+	}
+
 	// Check for an old resource so that we can figure out if this is a create, delete, etc., and/or
 	// to diff.  We look up first by URN and then by any provided aliases.  If it is found using an
 	// alias, record that alias so that we do not delete the aliased resource later.
@@ -460,7 +466,13 @@ func (sg *stepGenerator) generateSteps(event RegisterResourceEvent) ([]Step, res
 			oldInputs = old.Inputs
 			oldOutputs = old.Outputs
 			if urnOrAlias != urn {
+				if _, alreadySeen := sg.urns[urnOrAlias]; alreadySeen {
+					// This resource is claiming to X but we've already seen that urn created
+					invalid = true
+					sg.deployment.Diag().Errorf(diag.GetDuplicateResourceAliasError(urn), urnOrAlias, urn, urn)
+				}
 				if previousAliasURN, alreadyAliased := sg.aliased[urnOrAlias]; alreadyAliased {
+					// This resource is claiming to be X but we've already seen another resource claim that
 					invalid = true
 					sg.deployment.Diag().Errorf(diag.GetDuplicateResourceAliasError(urn), urnOrAlias, urn, previousAliasURN)
 				}

sdk/go/common/diag/errors.go

@@ -84,3 +84,9 @@ Either include resource in --target list or pass --target-dependents to proceed.
 func GetDefaultProviderDenied(urn resource.URN) *Diag {
 	return newError(urn, 2015, `Default provider for '%v' disabled. '%v' must use an explicit provider.`)
 }
+
+func GetDuplicateResourceAliasedError(urn resource.URN) *Diag {
+	return newError(urn, 2016,
+		"Duplicate resource URN '%v' conflicting with alias on resource with URN '%v'",
+	)
+}