Story #2776
closed
API server /keep_disks method advertises a keep proxy instead of the real keep disks when queried by a remote client.
Added by Tom Clegg over 10 years ago.
Updated over 10 years ago.
Estimated time:
(Total: 6.00 h)
- Assigned To set to Peter Amstutz
Looking at 0172bc3
I know this is sufficient to get the rest of the current Keep code working, but I wonder if some of the implementation details might lead to surprises for future development. Like the fact that only the index method respects the new X-Keep-Proxy-Required header. Should the show method also be able to return it under the same conditions?
I also wonder about the approach of faking an ActiveRecord::Relation with monkeypatching. If everything's expecting @objects to be a real result, then all kinds of code is expecting all kinds of methods to be there. Not just our own stuff, but also other Rails code and gems we use. I worry that we're opening ourselves up to lots of weird future bugs this way, where "unrelated" changes make the Keep proxy results fail again.
What if we took an approach like we do with the system user? Actually add the proxy disk to the database, and then use or exclude it for all results based on the configuration+header combination. It sounds like ActiveRecord scopes provide a way to codify these sorts of "limited database views" once, and then keep reusing them.
Scope creep:
- Add a KeepServices ActiveRecord model
- Move service_* columns to keep_services
- Add service_type column to keep_services
- Add keep_service_uuid column key referencing keep_services
- Add an additional keep_services route called "available" (or something) that is like index but only returns services that should be used by the client (e.g. internal servers for internal clients, proxy servers for external clients)
Since we decided to go a completely different route, this is completely new branch: 2776-keep-services-table
This branch looks very good, and I like it much better than the previous implementation idea. Thanks for sticking through it. My comments are all more about clean-ups before merging:
- I hate to be the bearer of bad news, but the word is correctly spelled "accessible," and I think the API should use that. (But don't sweat it, this means you join the ranks of our esteemed forebears who put "Referer" in the HTTP spec.)
- I think the Workbench navigation for Keep Services should have a different icon than the one for Keep Disks. Maybe fa-database instead of fa-hdd-o?
- Please remove the debug prints in the accessable method.
- The service* methods in the disk model can be boiled down to
KeepService.find(keep_service_uuid).service_attr
. I think this can help clarify that you only expect to find one thing.
- Status changed from New to Resolved
- % Done changed from 45 to 100
Applied in changeset arvados|commit:2dc7f15f81ea7f460114482614d8ec5814c36fbf.
Also available in: Atom
PDF