Implicit importing of SNS Topics

[VenelinMartinov]

What happened?

When deploying SNS topics which already exists, pulumi will implicitly import them.

This can cause confusing behaviour down the line, for example if the topic is defined twice and then one of them is destroyed, both of them will actually disapper.

Example

We tried to address this in #3235 and that also has a regression test. Unfortunately, we had to revert because it introduces a hard failure when replacing parent providers, see #3268

Output of pulumi about

Tested in 6.18

Additional context

No response

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).

[VenelinMartinov]

Related to #2009

[VenelinMartinov]

I think @iwahbe suggested that we reintroduce the logic in #3235 as a warning - this would not have the drawback which caused us to revert the original PR but would still give an indication to the users of the confusing behaviour of implicitly importing the SNS topics

[corymhall]

Looking into this more I think #3235 is the correct solution for this issue. From what I can tell, it was abandoned because SNS topics could no longer be recreated if there is a name conflict. Users would be able to specify that the resource should be DeleteBeforeReplace, and this would match all other Pulumi resources (unless I am missing something).

If there are users who are currently depending on the implicit importing of SNS topics, they would have to switch to explicit importing (which we should enforce).

The only hard failure appeared to be in the case of the provider itself being renamed/replaced (#2009). I would argue that there are no users that would rename a provider and go ahead with an apply which replaces all the resources in their stack. That is what would need to happen in order to hit this hard failure on recreating the SNS topic.

@iwahbe, @VenelinMartinov, @t0yv0 my suggestion is to re-roll #3235, but let me know what you think.

[VenelinMartinov]

The only hard failure appeared to be in the case of the provider itself being renamed/replaced

Not quite true - there'd also be a hard failure upon trying to create a stack with an SNS topic which already exists. This currently works fine but would not work with #3235. I suspect this is the most common path hit by users, so we'd be breaking most programs which are related to this issue.

I believe the reason we originally abandoned the PR is that it wasn't obvious that the benefit outweighs the cost - it doesn't address #2009 and maybe users don't mind the implicit importing behaviour and the hard failures might be a net worse.

I believe @iwahbe suggested reimplementing the hard failure as a warning - that wouldn't break the programs which currently work fine and will warn users of the odd implicit importing. In the case of a renamed provider it'd still get silently deleted but at least there's a warning about the behaviour.

[corymhall]

there'd also be a hard failure upon trying to create a stack with an SNS topic which already exists.

I would argue that this is the behavior that we want. There should be a failure in this case. Users would still be able to import the SNS topic by explicitly importing it, and this would be inline with what they must do for all other resources.

I can see three different types of users, and in all three I think fixing the bug would be the preferred scenario.

• New user that is creating a new stack and has given a hard coded name for the SNS topic that matches an existing topic. They will get a failure and will have to either change the name or explicitly import the topic. I don't think this would surprise them as it would match all other resources. Implicitly importing the topic would surprise them. They now think that they created this topic, but in fact someone else did and may update it in the future without their knowledge.
• Existing users that created a stack previously with an implicitly imported SNS topic and only manage the SNS topic through this stack. These users would see no change since the topic has already been imported.
• Existing users that created a stack previously with an implicitly imported SNS topic and manage the SNS topic across multiple stacks. These users would now have to choose which stack should manage the SNS topic going forward, or make sure to explicitly imported the SNS topic each time. We should enforce this. I have detailed a scenario below on why.

Suppose an organization has an SNS topic that is being managed across 2-3 stacks (implying 2-3 applications). StackA makes a change to the topic, they now have to coordinate that change across the other two stacks and inform the owners of that topic that they are making the change, otherwise the next time StackB/C update their stacks it will revert the changes made by StackA. An even more dangerous scenario would be if StackA made a change like renaming the SNS topic. The SNS topic would be replaced and now the Topic that StackB/C are relying on no longer exists.

There may be organizations that have come to rely on this scenario, but I think the far more likely scenario is that they are unaware of this and will eventually make a dangerous change.

[VenelinMartinov]

Yeah, I'd agree about the right behaviour and perhaps the breaking change isn't this bad since it doesn't affect existing programs. It's probably fine to merge this and should be a net positive for users.

Perhaps we should add a link on how to get the previous behaviour in the error message. Users might be able to use the import resource option to get the old behaviour.

[t0yv0]

Re-reading this carefully, I'm with Cory here on moving forward with the change as it makes this resource simpler to reason about and less surprising.

Regarding the situation of a renamed provider and such, so do we have an example program? The linked analysis mentions the need to implement DiffConfig in the bridge that I don't quite follow - but anyway DiffConfig is now implemented in the bridge. I think also that if users run into this situation where renaming a provider cause an URN change on this resource and causes the engine to decide to Create and then Delete which would now fail (Create) because of a conflict on the cloud name, this type of situation calls for https://www.pulumi.com/docs/concepts/options/aliases/ I think. This may be something that can be remediated by teaching the engine to avoid the "double vision" problem and identify the resource as one resource by adding aliases.

[VenelinMartinov]

There is a test in https://github.com/pulumi/pulumi-aws/pull/3268/files#diff-bdde7c5c4464b1abd6a781ee2aa4b04235fdbf4832f1d6f584174a8528e8fe16 which reproes the issue for renamed providers.

The linked analysis mentions the need to implement DiffConfig in the bridge that I don't quite follow

Yeah, I believe that was erroneous - I was under the impression DiffConfig could tell the engine not to re-create the provider when it gets renamed but it never gets called for renames.

Aliases is indeed the workaround here but I don't think that's very obvious for any user facing this.