pulumi-aws icon indicating copy to clipboard operation
pulumi-aws copied to clipboard

Unstable FileArchive paths cause s3.BucketObject to fail on updates

Open chrisui opened this issue 2 years ago • 4 comments

Hello!

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

Issue details

It appears that if an absolute path is passed to FileArchive then pulumi will have a hard time on another machine and will fail to update :grimacing: Is this expected? If so might be worth adding a warning or explicit mention of relative path requirement on the docs. I do wonder how we should handle non-deterministic paths though between builds? I have a "file not found" error where a BucketObject is referencing a .zip with an absolute path which was created running up on another machine.

Please see slack thread

The problem is apparent when you consider the Asset resources are the only resources that do not have a stable name you can provide. It is expecting to use the file path as a stable reference for the "virtual resource" AND as a pointer to "physical resource".

Pulumi contains a reference to the local path in its state and appears to assume it exist when running a subsequent update

"source": {
    "4dabf18193072939515e22adb298388d": "0def7320c3a5731c473e5ecbe6d01bc7",
    "hash": "63d94894644f928fdc44fc353c161ca544f477d28880d07c2f8634322ce3b1d6",
    "path": "/Users/path/to/repo/modules.zip"
}
Chris 33 minutes ago Kind of feels like the local path is insignificant to the actual resource representation. It's the resolved file (and it's data) that is important. white_check_mark eyes raised_hands

tenwit:partypus: 33 minutes ago That sounds like a normal programming issue. Exception handling and testing are the way to go, here.

Chris 28 minutes ago Yes.

tenwit:partypus: 27 minutes ago There isn't a relative path requirement, so it can't be added to the docs. The relative path thing is a requirement only when running on multiple machines with different directory hierarchies. Frequently, this isn't the case.

tenwit:partypus: 26 minutes ago Absolute paths would be fine if deploying only from CI, or only within a containerized environment, or similar.

Chris 26 minutes ago It's not my code throwing the exception though. Pulumi is storing in state (validated via pulumi stack export ) an absolute path that is not valid across environments.

Chris 25 minutes ago Your last statement is more true but not entirely. If I change the working directory configuration of my CI container tomorrow paths will be invalidated.

tenwit:partypus: 25 minutes ago So the exception handling would have to wrap the execution of the Pulumi engine, which is inconvenient but not difficult.

tenwit:partypus: 25 minutes ago If you change the working directory configuration of your CI container, that's equivalent to changing the code. You need to test that.

Chris 20 minutes ago Err, not really agreeing with your sentiment. Of course we should "handle errors that could occur in your code". However, I believe this error should be handled by pulumi. In my configuration it is correctly pointing to the right file path that exists on the machine pulumi is running on. Pulumi is trying to do something with the old state and failing - which I'm pretty sure is the biggest jobs of pulumi (ie. to detect configuration drift so that it can perform the right CRUD actions). It's seeing "oh this FileArchive doesn't exist anymore" but I've got a new one in it's place and do not have control over what pulumi is doing in the background when it realises an old FileArchive resource is orphaned.

tenwit:partypus: 18 minutes ago If Pulumi did that, how would you handle the case where you require the file to be at a specific, absolute location? And where if it path changes, then deployment should correctly fail?

Chris 18 minutes ago The problem is apparent when you consider the Asset resources are the only resources that do not have a stable name you can provide. It is expecting to use the file path as a stable reference for the "virtual resource" AND as a pointer to "physical resource".

tenwit:partypus: 11 minutes ago That does complicate matters...

tenwit:partypus: 8 minutes ago In cases where this is likely to become a problem, it might be worth wrapping the Asset in a ComponentResource with the same name as the Asset's constructor. You'd achieve the same protection, but with annoying boilerplate code, and extra resources which you might have to pay for if you're using a paid plan.

tenwit:partypus: 7 minutes ago Having separate name and path properties would be better. It's probably worth opening an issue explaining this.

Chris 7 minutes ago Yeah defo will do. Thanks for helping me validate the issue

Chris 3 minutes ago for reference and anyone else exploring this issue: this is the offending BucketObject state that exists in the stack which it's trying to do something with even though it sees the path has changed. It may actually be isolated to just s3.BucketObject so will explore a bit deeper. (my config is simply {source: FileArchive(/abs/path.zip)

"source": {
    "4dabf18193072939515e22adb298388d": "0def7320c3a5731c473e5ecbe6d01bc7",
    "hash": "63d94894644f928fdc44fc353c161ca544f477d28880d07c2f8634322ce3b1d6",
    "path": "/Users/path/to/repo/modules.zip"
}

Steps to reproduce

  1. Create a s3.BucketObject with source set as a FileArchive with absolute path like process.cwd()/test.zip (which is a file that exists)
  2. pulumi up
  3. Change path like process.cwd()/test-again.zip
  4. Delete old test.zip and create test-again.zip
  5. pulumi preview

Expected: Pulumi ignores old [absolute] path stored in state and doesn't error given it's being satisfied with a new path that does exist Actual: Errors with /path/.../test.zip: no such file or directory

Note I also have https://github.com/pulumi/pulumi-aws/issues/1763 open so possible there is crossover as it's related to source and sourceHash.

chrisui avatar Jan 16 '22 21:01 chrisui

I've opened here as my understanding is each provider explicitly creates a mapping between an underlying provider and the pulumi "Asset" concept and configuration so the problem is likely to sit in the aws bindings.

As noted in the linked slack thread though I do believe it might be worth considering allowing a stable name to exist on files to make it clear that support for unstable paths should work.

chrisui avatar Jan 16 '22 21:01 chrisui

I think this might be due to "source" being defined as needing an asset translation (using TranslateAsset) which might be getting triggered for the Diff call.

Frassle avatar Jan 17 '22 11:01 Frassle

Note on further attempt to avoid this issue (calculating a stable relative path that will exist relative to pulumi execution) I still seem to be having this issue. So presumably this is more an issue that somewhere an absolute path to a .zip is being resolved and then stored in state - then it being missing is not handled gracefully in new environments.

chrisui avatar Jan 18 '22 14:01 chrisui

lol, guessing it's this todo

https://github.com/pulumi/pulumi-terraform-bridge/blob/30ae8ae5cc2d2f5987854984243c74ace4ccc952/pkg/tfbridge/assets.go#L130

chrisui avatar Jan 19 '22 13:01 chrisui