Project

General

Profile

Actions

Idea #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 almost 10 years ago. Updated almost 10 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Start date:
05/15/2014
Due date:
Story points:
1.0

Subtasks 5 (0 open5 closed)

Task #2827: Review 2776-keep-services-tableResolvedPeter Amstutz05/15/2014Actions
Task #2790: Write testResolvedPeter Amstutz05/15/2014Actions
Task #2782: Configure web proxy used in deployment to supply "X-External-Client" headerResolvedWard Vandewege06/06/2014Actions
Task #2918: Clone an existing pipeline instance in order to edit parameters/inputs and re-run.ResolvedTom Clegg06/06/2014Actions
Task #2789: Configuration option for proxy serviceResolvedPeter Amstutz05/15/2014Actions
Actions #1

Updated by Peter Amstutz almost 10 years ago

  • Assigned To set to Peter Amstutz
Actions #2

Updated by Brett Smith almost 10 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.

Actions #3

Updated by Peter Amstutz almost 10 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)
Actions #4

Updated by Peter Amstutz almost 10 years ago

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

Actions #5

Updated by Brett Smith almost 10 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.
Actions #6

Updated by Anonymous almost 10 years ago

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

Applied in changeset arvados|commit:2dc7f15f81ea7f460114482614d8ec5814c36fbf.

Actions

Also available in: Atom PDF