Arvados

I think this can be as simple as, in CollectionDirectory.__init__:

self.poll = True
self.poll_time = 60 * 60

This should cause it to refresh at minimum every hour.

A slightly more clever solution would be to look up the expiry time in the discovery document and divide by half.

Peter Amstutz wrote:

I think this can be as simple as, in CollectionDirectory.__init__:

self.poll = True
self.poll_time = 60 * 60

This basic approach seems sound, but this will still have the original bug if the TTL is an hour or less, which I could imagine happening in very security-sensitive deployments that want to be able to revoke access quickly. Could we do something like self.poll_time = ttl_from_discovery_document / 2, with some bounds checking and all that?

Branch 6833-arv-mount-cache-refresh retrieves blobSignatureTtl from discovery document and uses it to set self.poll_time.

•         # TODO: mock something in arvados_fuse here so it thinks
          # manifests/signatures expire in 1 second
  

...then it should reproduce the bug: i.e., the manifest is fetched, then time passes, then fuse attempts to read data using the old signatures.

A more realistic test would be to mock the collections API so the signatures really do expire in 1 second. However, since the proposed fix involves looking at the TTL advertised in the discovery doc and ignoring the signatures themselves, mocking the signatures wouldn't be enough to trigger the fix, so we might as well just mock the discovery doc and ensure fuse retrieves fresh (now+2w) signatures from the API server.

I am having an issue using the strategy from ef5692e0 in writing a test for this update.

The issue is: the test uses local_store and kernel caching for the fuse mount. Due to this I am not able to write a test that does something like: listdir, read file; sleep ttl; read file where token is expired.

The other alternative is to write a test with steps: listdir, sleep ttl, read file where token is expired. However, in ef5692e0, the _test_refresh_old_manifest method is being executed even before any other test steps are executed (may be because it is a @staticmethod). It would help if I can figure out how to make this fire when get is actually invoked, rather that at initialization of test.

Reviewing 6833-test-token-expiry @ 8a2645e (the parts I didn't write)

Please rebase this to master, and mock the discovery document TTL instead of the poll_time attribute of the mount. As written, the test passes even though the bug is still present. For example, I'm noticing the bugfix says things about self.poll, not self._poll, and ProjectDirectory's constructor looks like it will overwrite whatever CollectionDirectory puts in self._poll.

f82d809 LGTM

• it uses the real fuse setup code, so you test the way the mounts get set up in real life, which is a bit hairy
• you can make a class with a bunch of tests (one test_* and one matching static test* method) instead of having to make a class and a lone function outside the class for each test

It also uses a non-admin user by default -- right now the tests are pretty heavy on the "how well things work for admins" side.