Arvados

Implementation plan:

Make CollectionFileReader seekable.

Enable keep-web to handle range requests at nonzero offset by seeking before starting to read.

10990-keep-web-ranges @ 24b137a5b3313778e2db7f5d1e0c82daf0634a9c

Known issue: Seek() should get more unit tests (seeking forward, seeking across block boundaries, illegal seeks).

Tom Clegg wrote:

10990-keep-web-ranges @ 24b137a5b3313778e2db7f5d1e0c82daf0634a9c

Known issue: Seek() should get more unit tests (seeking forward, seeking across block boundaries, illegal seeks).

Comments:

CollectionFileReader:

• Much easier to follow than the code it is replacing. I appreciate that the approach is more consistent with the approach taken by the Python SDK.
• I suggest adding the file offset to manifest.FileSegment. Then you can determine the correct file segment for a given offset using a binary search instead of a linear search. For typical sequential reads, an even simpler optimization would be to check f.segNext before searching for the correct segment from the beginning.
• I like the optimization of only enabling prefetching when reading past the first 1 MiB of the file. We should do something similar in the Python SDK.
• What happens if you call Read() after Close()? I'm not sure if the behavior of len(nil) is to return 0 or panic?
• FYI we found in Python that if a caller does read(N) and gets less than N bytes, that's sometimes interpreted as EOF (!) so we had to change the behavior to make a best effort to return the number of bytes asked for. Go probably doesn't have this problem, but I wanted to mention it.

BlockCache

• Is there a reason to cap limit the cache based on blocks instead of bytes? (the Python SDK uses bytes) The cache could be considered filled by four 1 byte blocks.
• We could stream results through instead of adding another store-and-forward stage by using streamer.AsyncStream in the BlockCache. This holds a buffer from which you can create multiple readers. Readers can start reading before the buffer is completely filled.

Peter Amstutz wrote:

• We could stream results through instead of adding another store-and-forward stage by using streamer.AsyncStream in the BlockCache. This holds a buffer from which you can create multiple readers. Readers can start reading before the buffer is completely filled.

I'm working on a branch that implements this.

keep-web:

• keep-web should have an option to adjust the cache size
• cache should be (API token)+(hash)

Peter Amstutz wrote:

keep-web:

• keep-web should have an option to adjust the cache size

Agreed

• cache should be (API token)+(hash)

The only locators keep-web uses are ones that come from the API server in a collection's manifest_text. The API server doesn't give those out unless the caller is allowed to read the data. So if we have a hash to look up, then the caller is allowed to see the data, which means salting the cache key with the token would sabotage cache utilization without providing any benefit.

A client that fetches user-supplied hashes (and handles multiple users in one process) would need to keep the caches separate, though. With the current setup it's possible, but not the default behavior, to give each keepclient its own cache. Perhaps it would be safer to make "each keepclient gets its own cache" the default, and let keep-web override that with a single global cache?

Tom Clegg wrote:

Peter Amstutz wrote:

keep-web:

• keep-web should have an option to adjust the cache size

Agreed

• cache should be (API token)+(hash)

The only locators keep-web uses are ones that come from the API server in a collection's manifest_text. The API server doesn't give those out unless the caller is allowed to read the data. So if we have a hash to look up, then the caller is allowed to see the data, which means salting the cache key with the token would sabotage cache utilization without providing any benefit.

I can't think of a way that an attacker would easily trick keep-web into requesting an arbitrary block, so I think you're right. On the other hand, if we choose to also cache collection lookups in the future, I'm pretty sure we would want to tie it to the token.

Related, if we added caching to services that do work with blocks, such as keepproxy, we would definitely want to salt the cache keys.

A client that fetches user-supplied hashes (and handles multiple users in one process) would need to keep the caches separate, though. With the current setup it's possible, but not the default behavior, to give each keepclient its own cache. Perhaps it would be safer to make "each keepclient gets its own cache" the default, and let keep-web override that with a single global cache?

I think a global cache is better if the cache keys incorporate the tokens, but separate caches are better if they don't.

Peter Amstutz wrote:

Peter Amstutz wrote:

• We could stream results through instead of adding another store-and-forward stage by using streamer.AsyncStream in the BlockCache. This holds a buffer from which you can create multiple readers. Readers can start reading before the buffer is completely filled.

I'm working on a branch that implements this.

See 10990-keep-web-ranges-pa

Passes range request tests.

Would be interesting to set this up on a test server and do some benchmarking of streaming vs store-and-forward.

Peter Amstutz wrote:

I can't think of a way that an attacker would easily trick keep-web into requesting an arbitrary block, so I think you're right. On the other hand, if we choose to also cache collection lookups in the future, I'm pretty sure we would want to tie it to the token.

Yes, collection IDs are provided by the client so we'd certainly need to avoid leaking that cache between tokens (e.g., a client who can't read a collection shouldn't even be able to see which files exist, let alone the contents). So the block cache would still be fine.

Related, if we added caching to services that do work with blocks, such as keepproxy, we would definitely want to salt the cache keys.

I think a global cache is better if the cache keys incorporate the tokens, but separate caches are better if they don't.

Surely we can solve this without having multiple copies of the same data in memory. If we have cached data, but aren't sure of the token, we could do HEAD to check permission, then return the cached data...

Successfully tested the deployed version of this.