Non-idempotent SNS topic creation workaround #3235

VenelinMartinov · 2024-01-08T23:30:28Z

This addresses #2009.

Previously the creation of an SNS topic was idempotent, so you could create multiple pulumi resources corresponding to the same SNS topic, causing confusing behaviour when one was deleted/replaced.

With this PR, trying to create a topic which exists will error.

Note that this is kind of a breaking change - pulumi programs where the same SNS topic was created through multiple pulumi resources would run fine before but will now error.

Note also the mutex around the listing and creating of SNS topics - this is needed to prevent race conditions within the same program. Race conditions with other programs are still possible and that is unavoidable because of the AWS API.

t0yv0 · 2024-01-08T23:38:41Z

What does TF do in this case? Does TF have the same problem? If so is there an upstream ticket ref?

Does this solve updates as well or only creates?

t0yv0 · 2024-01-08T23:41:50Z

Actually reading the original ticket, it sounds like this fix will make it fail loudly, which is slightly better than current silent corruption behavior but still not ideal? To confirm is that what we're going for?

t0yv0 · 2024-01-08T23:42:32Z

patches/0037-Non-idempotent-sns-topic-creation.patch

+ 	// Some partitions (e.g. ISO) may not support tag-on-create.
+@@ -291,6 +322,7 @@ func resourceTopicCreate(ctx context.Context, d *schema.ResourceData, meta inter
+
+ 		output, err = conn.CreateTopic(ctx, input)


What does conn.CreateTopic return if the topic already exists?

Here is what the docs say:

This action is idempotent, so if the requester already owns a topic with the specified name, that topic's ARN is returned without creating a new topic.

And in my testing that seems to indeed be the behaviour.

Will we need to change the docs since we have changed the behavior of the resource?

Neither our docs nor terraforms mention anything about the create returning existing resources:
https://www.pulumi.com/registry/packages/aws/api-docs/sns/topic/
https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/sns_topic

Given that the new behaviour is the default behaviour for pulumi resources, I'd say this does not warrant any additional documentation, LMK if you disagree

sdk/nodejs/iot/getRegistrationCode.ts

VenelinMartinov · 2024-01-08T23:46:11Z

Yeah, exactly - this change will make the creation of a second SNS topic with the same name fail, instead of the current behaviour of silently passing and creating weirdness down the line.

patches/0037-Non-idempotent-sns-topic-creation.patch

iwahbe · 2024-01-08T23:58:42Z

patches/0037-Non-idempotent-sns-topic-creation.patch

+ 	// Some partitions (e.g. ISO) may not support tag-on-create.
+@@ -291,6 +322,7 @@ func resourceTopicCreate(ctx context.Context, d *schema.ResourceData, meta inter
+
+ 		output, err = conn.CreateTopic(ctx, input)


Will we need to change the docs since we have changed the behavior of the resource?

VenelinMartinov · 2024-01-09T00:09:52Z

This is the upstream issue: hashicorp/terraform-provider-aws#6245

Note it's closed but not completed.

patches/0037-Non-idempotent-sns-topic-creation.patch

Co-authored-by: Ian Wahbe <ian@wahbe.com>

github-actions · 2024-01-09T00:56:13Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Maintainer note: consult the runbook for dealing with any breaking changes.

t0yv0 · 2024-01-09T16:13:33Z

I'm a little wary of cost-benefit of this PR since upstream didn't fix it we're introducing a bit of slowdown and failure mode for creating the topic, but if we do it right (without slowing down for clients with 1000 topics) I think on balance the fact that it's an upvoted request both in our repo and upstream - let's try it! Thank you!

t0yv0 · 2024-01-16T17:41:28Z

patches/0037-Non-idempotent-sns-topic-creation.patch

+		return diag.Errorf("SNS Topic (%s) already exists", name)
+	}
+	if !tfresource.NotFound(err) {
+		return diag.FromErr(err)


Could go both ways here, logging or erroring out. I guess erroring out is reasonable, though it introduces a new failure path.

Unless AWS changes its URN scheme, it is wrong to create two of these. I agree that we should error here.

t0yv0

Thank you!

iwahbe

LGTM.

I would love to see an ideation doc presenting this strategy as a possible solution for structural resources. My suspicion is that we can determine the "name" (ARN for AWS) for most structural resources.

iwahbe · 2024-01-16T18:10:38Z

patches/0037-Non-idempotent-sns-topic-creation.patch

+ 	}
+ }
+
+func constructTopicArn(client *sns.Client, account, region, snsTopicName string) string {


This is a much cleaner implementation.

iwahbe · 2024-01-16T18:14:25Z

patches/0037-Non-idempotent-sns-topic-creation.patch

+		return diag.Errorf("SNS Topic (%s) already exists", name)
+	}
+	if !tfresource.NotFound(err) {
+		return diag.FromErr(err)


Unless AWS changes its URN scheme, it is wrong to create two of these. I agree that we should error here.

VenelinMartinov · 2024-01-16T18:44:17Z

pulumi/pulumi#9925 looks like the exact issue this fixes - Fraser noted they are related there. Might be worth lifting the logic into the bridge since there's a few of these resources.

I'll merge this and then raise this as a workaround for idempotent creates.

Edit: thinking about this a bit more, it might be a bit difficult to lift in the bridge, since there'd be a lot of specific logic for determining the logical name of the resource - our reads prob work off the URN, which would not be the same in this case.

iwahbe · 2024-01-16T23:43:36Z

pulumi/pulumi#9925 looks like the exact issue this fixes - Fraser noted they are related there. Might be worth lifting the logic into the bridge since there's a few of these resources.

I'll merge this and then raise this as a workaround for idempotent creates.

Edit: thinking about this a bit more, it might be a bit difficult to lift in the bridge, since there'd be a lot of specific logic for determining the logical name of the resource - our reads prob work off the URN, which would not be the same in this case.

We would be calling TFs read in the case of the bridge, not Pulumi's. In essence, we would ask TF to "import" the resource, and if the import succeeds, we would then fail.

This reverts commit 976def3.

This reverts commit 976def3. Until we sort out #3268 (comment) It hasn't been released so no need to patch it.

This is a re-roll of #3235. See [this comment](#3288 (comment)) for why we are re-rolling this. closes #3288 Co-authored-by: VenelinMartinov <venelin.v.martinov@gmail.com>

This is a re-roll of #3235. See this comment for why we are re-rolling this. closes #3288 Co-authored-by: VenelinMartinov venelin.v.martinov@gmail.com

This is a re-roll of #3235. See [this comment](#3288 (comment)) for why we are re-rolling this. Previously the creation of an SNS topic was idempotent, so you could create multiple pulumi resources corresponding to the same SNS topic, causing confusing behaviour when one was deleted/replaced. With this PR, trying to create a topic which exists will error. Note that this is kind of a breaking change - pulumi programs where the same SNS topic was created through multiple pulumi resources would run fine before but will now error. Note also the mutex around the listing and creating of SNS topics - this is needed to prevent race conditions within the same program. Race conditions with other programs are still possible and that is unavoidable because of the AWS API. fixes #3288, fixes #2009. Co-authored-by: VenelinMartinov venelin.v.martinov@gmail.com

VenelinMartinov added 2 commits January 8, 2024 14:11

workaround

c103b36

update patch

fd3969d

VenelinMartinov requested review from t0yv0, guineveresaenger and iwahbe January 8, 2024 23:30

build sdks

e3b7dd5

t0yv0 reviewed Jan 8, 2024

View reviewed changes

sdk/nodejs/iot/getRegistrationCode.ts Outdated Show resolved Hide resolved

iwahbe reviewed Jan 9, 2024

View reviewed changes

t0yv0 reviewed Jan 9, 2024

View reviewed changes

patches/0037-Non-idempotent-sns-topic-creation.patch Outdated Show resolved Hide resolved

VenelinMartinov and others added 3 commits January 8, 2024 16:19

skip if no creds

78c6c1d

Update patches/0037-Non-idempotent-sns-topic-creation.patch

15836ca

Co-authored-by: Ian Wahbe <ian@wahbe.com>

address reviews

d301a0a

update patch and sdk

ab5c512

VenelinMartinov requested review from iwahbe and t0yv0 January 16, 2024 11:24

t0yv0 reviewed Jan 16, 2024

View reviewed changes

t0yv0 approved these changes Jan 16, 2024

View reviewed changes

iwahbe approved these changes Jan 16, 2024

View reviewed changes

VenelinMartinov merged commit 976def3 into master Jan 16, 2024
18 checks passed

VenelinMartinov deleted the vvm/sns_nonidempotent_create_workaround branch January 16, 2024 18:46

VenelinMartinov mentioned this pull request Jan 17, 2024

Delete before replace sns topic #3268

Closed

VenelinMartinov added a commit that referenced this pull request Jan 17, 2024

Revert "Non-idempotent SNS topic creation workaround (#3235)"

c9c42f5

This reverts commit 976def3.

VenelinMartinov added a commit that referenced this pull request Jan 17, 2024

Revert "Non-idempotent SNS topic creation workaround (#3235)" (#3272)

6050193

This reverts commit 976def3. Until we sort out #3268 (comment) It hasn't been released so no need to patch it.

This was referenced Jan 19, 2024

Resource replacement due to parent provider rename deletes actual AWS resource #2009

Closed

Implicit importing of SNS Topics #3288

Closed

corymhall mentioned this pull request Apr 10, 2024

fix: sns topic creation should not be idempotent #3798

Closed

corymhall added a commit that referenced this pull request Apr 11, 2024

fix: sns topic creation should not be idempotent

cd17fcc

This is a re-roll of #3235. See this comment for why we are re-rolling this. closes #3288 Co-authored-by: VenelinMartinov venelin.v.martinov@gmail.com

corymhall mentioned this pull request Apr 11, 2024

fix: sns topic creation should not be idempotent #3809

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Non-idempotent SNS topic creation workaround #3235

Non-idempotent SNS topic creation workaround #3235

VenelinMartinov commented Jan 8, 2024 •

edited

t0yv0 commented Jan 8, 2024

t0yv0 commented Jan 8, 2024

t0yv0 Jan 8, 2024

VenelinMartinov Jan 8, 2024

iwahbe Jan 8, 2024

VenelinMartinov Jan 12, 2024

VenelinMartinov commented Jan 8, 2024

iwahbe Jan 8, 2024

VenelinMartinov commented Jan 9, 2024 •

edited

github-actions bot commented Jan 9, 2024

t0yv0 commented Jan 9, 2024

t0yv0 Jan 16, 2024

iwahbe Jan 16, 2024

t0yv0 left a comment

iwahbe left a comment

iwahbe Jan 16, 2024

iwahbe Jan 16, 2024

VenelinMartinov commented Jan 16, 2024 •

edited

iwahbe commented Jan 16, 2024

Non-idempotent SNS topic creation workaround #3235

Non-idempotent SNS topic creation workaround #3235

Conversation

VenelinMartinov commented Jan 8, 2024 • edited

t0yv0 commented Jan 8, 2024

t0yv0 commented Jan 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VenelinMartinov commented Jan 8, 2024

Choose a reason for hiding this comment

VenelinMartinov commented Jan 9, 2024 • edited

github-actions bot commented Jan 9, 2024

Does the PR have any schema changes?

Does the PR have any schema changes?

t0yv0 commented Jan 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0yv0 left a comment

Choose a reason for hiding this comment

iwahbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VenelinMartinov commented Jan 16, 2024 • edited

iwahbe commented Jan 16, 2024

VenelinMartinov commented Jan 8, 2024 •

edited

VenelinMartinov commented Jan 9, 2024 •

edited

VenelinMartinov commented Jan 16, 2024 •

edited