Bug #8508
closed[Data Manager] Tests assume you can't write to /
Description
it seems that there are a few datamanager tests that assume they won't be able to write to '/' and thus fail if files can in fact be written (e.g. TestPutAndGetBlocks_ErrorDuringGetCollectionsBadWriteTo expects to not be able to write to /badwritetofile and TestPutAndGetBlocks_ErrorDuringGetCollectionsBadHeapProfileFilename expects to not be able to write to /badheapprofile).
It would be better to ensure the failure by creating an explicit conflict. Perhaps the test could create a "bad" subdir in the test tmp directory and `chmod 000` it, then put the bad files under that directory.
Updated by Joshua Randall over 8 years ago
- Status changed from New to Feedback
- Assigned To set to Joshua Randall
Updated by Brett Smith over 8 years ago
- Subject changed from datamanager tests assume you can't write to / to [Data Manager] Tests assume you can't write to /
Updated by Radhika Chippada over 8 years ago
Reviewing branch wtsi-hgi-8508-datamanager-test-badpaths:
- It appears that we can ensure the dir is not accessible by simply replacing "/badwritetofile" with "/nosuchdir/badwritetofile" or fmt.Sprintf("/nosuchdir%v/badwritetofile", time.Since(time.Now()).Nanoseconds()) rather than creating a tempdir and appending a string to it? Please consider this simplified approach instead.
testOldBlocksNotDeletedOnDataManagerError(t, fmt.Sprintf("/nosuchdir%v/badwritetofile", time.Since(time.Now()).Nanoseconds()), "", true, true)
- Also, please run go fmt
Thanks.
Updated by Joshua Randall over 8 years ago
The simplified approach "/nosuchdir/badwritetofile" would probably work in most cases but it is trivial to break it by creating a "/nosuchdir". This realistically could occur accidentally, so I do not favor this option.
The slightly less simplified approach `fmt.Sprintf("/nosuchdir%v/badwritetofile", time.Since(time.Now()).Nanoseconds())` would almost definitely work in every conceivable scenario although it would still theoretically be possible to break it by creating a large number of /nosuchdir%v paths with future times. I agree this is extremely unlikely to ever be an issue unless someone is "attacking" the test intentionally.
The proposed approach (creating a temp dir that we "own" and then referring to a nonexistent path within it) should always work unless some concurrent process creates the bad path in our temporary directory after we create the temporary directory (i.e. an active attack against the test).
I went for the proposed approach because it is the safest thing I could think of. If you are concerned about the extra temporary directory creation, I'm happy to compromise on your timestamp based solution.
Updated by Radhika Chippada over 8 years ago
The extra step of creating the tmp dir is not much more expensive to execute and so let's stick with your implemented solution. I will merge your branch shortly. Thanks.
Updated by Radhika Chippada over 8 years ago
- Status changed from Feedback to Resolved
Applied in changeset arvados|commit:c6df16d2af30e989bcfb04f6ef730cde658a9dc9.