Revert "fix(content): make verified content completely read-only (#96)"

[alexwhitman]

In our Jenkins set up we change the npm cache directory to be within the build workspace and using the Copy to Slave plugin copy it between the master node and the build nodes. This speeds up builds by not having to download dependencies for each build when the workspace is clean and is similar to Caching Dependencies and Directories on Travis. The build process is:

1. Clone repo
2. Copy .cache/ directory from master node to build node
3. npm install + other build steps, potentially updating the .cache/ directory
4. Copy .cache/ back to the master node

This has worked well until we updated to node 8 with npm 5 where we were seeing permission denied errors due to some files being read only. Adding find .cache/ -perm 444 -exec chmod 644 '{}' \; to the end of the build works around the problem but it's a hack and one which I'd rather not have to add to hundreds of jobs.

I'd like to propose that the change making some files read only be reverted as it breaks existing workflows with other tools.

[zkat]

So here's the deal: you should be able to do all the operations that you'd want to do here so long as they don't break the contract for the cache. You should only be getting permissions errors IFF you try and modify the files. That is, change their contents. Copying, moving, and deleting should all work, because the permissions are modified for files, not directories (which remain writable).

If you're seeing permissions errors, my understanding is that you're doing operations that are corrupting the cache. If that is the case, even with this patch, you'll just delay the error until later, when npm verifies cache contents on read.

So to be clear: I'm wondering what your plugin is actually trying to do, or what these modifications are, but I can say with some level of certainty that it sounds like it's corrupting your cache, which means this feature of cacache is doing exactly what it's intended to do.

Do you have more information here?

[zkat]

Needs some discussion about why this is actually necessary. It looks fine otherwise, but the premise needs exploring.

[alexwhitman]

The cache isn't being corrupted. It's simply a case of the plugin trying to copy a file to the location of a read-only file. The line of code in the plugin where the failure occurs is https://github.com/jenkinsci/copy-to-slave-plugin/blob/master/src/main/java/com/michelin/cio/hudson/plugins/copytoslave/MyFilePath.java#L189

It could be argued that the plugin should be modified to delete then write rather than overwrite but this has worked for years and is only now an issue in the newer versions of npm.

[zkat]

I think I'm leaning still towards continuing to prevent corruption. npm@5 was a breaking change, and has a very different paradigm for its cache. These sorts of things are to be expected. Having to change permissions for your cache before copying over it seems like a relatively low-effort thing to do (and you already clarified exactly what needs to be done to address that) -- but this patch would, I think, be facilitating something I consider a bit of an antipattern (arbitrarily overwriting content-addressable files in the cache). From an ops perspective, it also seems wasteful to constantly be copying over and replacing such a large number of files when they are unchanged.

[alexwhitman]

Breaking changes are fine if they add benefit but I personally don't see much benefit to these files being read only. As you stated before

Copying, moving, and deleting should all work, because the permissions are modified for files, not directories (which remain writable).

which just as easily, if not more, allows the cache to be corrupted. As it stands currently there's also an inconsistency with Windows where the read-only attribute isn't set.

From an ops perspective, it also seems wasteful to constantly be copying over and replacing such a large number of files when they are unchanged.

I'd consider locally copying caches around more "friendly" than hitting the npm registry, GitHub, Packagist etc needlessly when we already have the files, saving their bandwidth and ours.

[zkat]

I'm gonna loop @rmg into this conversation, too, since it involves reverting their patch. My main resistance at this point is the breaking which will leave caches in hybrid states.

So, if this is happening, I'd like an addition to lib/verify.js (with corresponding test) that makes files writable, so folks can do npm cache verify to normalize their cache permissions instead of having older data be RO and newer data be RW.

[zkat]

also for the record: I understand it's ok to take the time we've been taking discussing this. Since it's reverting a prior contribution and has some potential side-effects and involves a breaking change (imo), I've done what I can to make sure we go over it and that it's the right thing. Your use-case here is perfectly valid and I if I were already intending not to take it, I would've closed it by now -- correct me if I'm wrong, but doing your find/chmod is a Good Enough™ workaround until this is all figured out and lands, neh?

Just thought I'd clarify that cause we've been going back and forth for a bit here.

[alexwhitman]

https://github.com/zkat/cacache/blob/latest/lib/verify.js#L58 seems like the ideal place to fix the permissions. I guess the next question is what the permissions should be. 644 is fine in my case but 664 is more typically used, unless the umask has been changed.

We can work with the find/chmod work around for now as it's enough to test with.

[rmg]

Sorry, I completely missed the notification for this :-(

@alexwhitman the use of 444 for the permissions was inspired by how git stores its content-address objects under .git/objects/**. I suspect that means the copy-to-slave would also fail for git repos.

Thinking about the behaviour further, this is actually a pretty insidious bug in the copy-to-slave plugin, since it means it will happily clobber contents of files outside of the intended path if the file that is being overwritten is a hard-link or symbolic-link. The "correct" way of handling this is to unlink the destination file first (-U flag on both GNU and BSD tar, default behaviour on node-tar). Unlinking works because it is based on write permissions to the containing directory rather than permissions on the file itself.

As much as I loath a hacky workaround, that might be the most appropriate solution in this case. A simpler workaround would be to nuke the existing .cache directory before copying it over from the master, since you appear to be caching it in its entirety anyway.

[rmg]

@alexwhitman another option might be to take a look at the Job Cacher Plugins, which doesn't bother overwriting files if they already exist, which is appropriate behaviour for cacache.