Story #2776

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 about 5 years ago. Updated almost 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Start date:
05/15/2014
Due date:
% Done:

100%

Estimated time:
(Total: 6.00 h)
Story points:
1.0

Subtasks

Task #2827: Review 2776-keep-services-tableResolvedPeter Amstutz

Task #2790: Write testResolvedPeter Amstutz

Task #2782: Configure web proxy used in deployment to supply "X-External-Client" headerResolvedWard Vandewege

Task #2918: Clone an existing pipeline instance in order to edit parameters/inputs and re-run.ResolvedTom Clegg

Task #2789: Configuration option for proxy serviceResolvedPeter Amstutz

Associated revisions

Revision 2dc7f15f
Added by Peter Amstutz about 5 years ago

Merge branch '2776-keep-services-table' closes #2776

Revision c2e70e05 (diff)
Added by Peter Amstutz about 5 years ago

Changed API server to use X-External-Client instead of
X-Keep-Proxy-Required. refs #2776

History

#1 Updated by Peter Amstutz about 5 years ago

  • Assigned To set to Peter Amstutz

#2 Updated by Brett Smith about 5 years ago

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.

#3 Updated by Peter Amstutz about 5 years ago

Scope creep:

  1. Add a KeepServices ActiveRecord model
  2. Move service_* columns to keep_services
  3. Add service_type column to keep_services
  4. Add keep_service_uuid column key referencing keep_services
  5. 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)

#4 Updated by Peter Amstutz about 5 years ago

Since we decided to go a completely different route, this is completely new branch: 2776-keep-services-table

#5 Updated by Brett Smith about 5 years ago

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.

#6 Updated by Anonymous about 5 years ago

  • Status changed from New to Resolved
  • % Done changed from 45 to 100

Applied in changeset arvados|commit:2dc7f15f81ea7f460114482614d8ec5814c36fbf.

Also available in: Atom PDF