In-place recreation of the resource with same `path` leads to an empty bucket

gbegher commented 1 year ago

What happened?

I tried the following as a temporary fix for https://github.com/pulumi/pulumi-synced-folder/issues/27:

new S3BucketFolder(
    // TMP FIX for https://github.com/pulumi/pulumi-synced-folder/issues/27
    `bucket-folder-${randomUUID()}`,
    {
      bucketName: frontendBucket.bucket,
      path,
      acl: s3.PrivateAcl,
      managedObjects: false,
    }
  )

My idea was to enforce the creation of a new resource to trigger the folder/bucket sync.

What happened was that on running the program:

First: A new resource was created (presumably sucessfully syncing the files)
Then: The previous resources delete executes and deletes all the contents of the bucket.

Here's the log (shortened)

 +  synced-folder:index:S3BucketFolder bucket-folder-0677a88d-9c95-443b-a1d6-bfe490ea3c3d creating (0s)
 +  command:local:Command bucket-folder-0677a88d-9c95-443b-a1d6-bfe490ea3c3d-sync-command creating (0s) 
 +  command:local:Command bucket-folder-0677a88d-9c95-443b-a1d6-bfe490ea3c3d-sync-command created (6s) 
 -  command:local:Command bucket-folder-sync-command deleting (0s)
 -  command:local:Command bucket-folder-sync-command deleted (2s) 
 -  synced-folder:index:S3BucketFolder bucket-folder deleting (0s) 
 -  synced-folder:index:S3BucketFolder bucket-folder deleted

As a result, the bucket is empty.

Expected Behavior

After running the program, the folder's contents should be synced.

Steps to reproduce

I think the code snippet above should reproduce the behaviour.

Output of `pulumi about`

(I run the program via the automation API and so not sure how to get the pulumi about)

Additional context

Here's the relevant line that defines the 'delete':

https://github.com/pulumi/pulumi-synced-folder/blob/9506b2d719a177f64536aa6bd1348e1d792d10de/provider/cmd/pulumi-resource-synced-folder/s3-bucket-folder.ts#L72

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

cnunciato commented 1 year ago

@gbegher Thanks for calling this out and apologies for the surprising behavior.

First off, we should have a release out soon to pick up the fix in pulumi/command. We're aiming to get that out today.

This particular issue is a bit tricky in that in a sense, this behavior is by design. In changing the name of the synced-folder resource, you're implicitly telling Pulumi to create a new resource (one with the new name) and then delete the existing one, which, for this component (as you correctly pointed out), would mean running aws s3 rm and deleting the files in the bucket. We wrote the component this way to ensure it'd be able to clean up all cloud resources properly on destroy -- including any objects created by the synced-folder resource itself.

That said, I can certainly understand why you didn't expect this to happen, so we discussed this issue as a team this morning, and we'll be adding some documentation to make this behavior a bit clearer.

One option to consider in situations like these (where you want to be able to destroy a Pulumi resource without deleting the cloud resources it's managing) would be to use the retainOnDelete resource option. That option should tell Pulumi not to run the delete operation at all, thereby leaving any previously synced files alone. Unfortunately, however, it seems there may be another bug in the Command provider that's currently preventing this option from being respected.

I've filed an issue on the Command provider repo for the team to investigate, and will keep this one open until that one resolves (and of course update here accordingly).

Thank you again for taking the time to report these issues -- we really appreciate it. And thanks for using Pulumi!

cnunciato commented 1 year ago

Just a quick update here -- turns out this wasn't a bug on the Command provider, but one on Pulumi itself, and unfortunately there's no easy workaround for it. The team is working on https://github.com/pulumi/pulumi/issues/12154, though, which, once it's done (specifically the Node.js SDK task), should make it possible for us to update the component to respect the retainOnDelete resource option and mark this issue fixed. Thanks for your patience!

cnunciato commented 1 year ago

Blocked on https://github.com/pulumi/pulumi/issues/12154.

pulumi / pulumi-synced-folder