Project

General

Profile

Actions

Story #15795

closed

[API] Accept configured SystemRootToken without doing a database lookup

Added by Tom Clegg almost 3 years ago. Updated over 2 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
-
Start date:
11/23/2019
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Description

The main benefit is a simpler installation process (the installer can use a randomly generated string, instead of running a rake task that depends on other parts of the configuration being ready enough for API to start up). A secondary benefit is speeding up system requests a little bit.

With this change, RailsAPI can also throw an "invalid config" error at startup if SystemRootToken is not configured. (With the current code this can't be treated as an error because RailsAPI needs to load its config in order to generate a valid SystemRootToken.)


Subtasks 1 (0 open1 closed)

Task #15872: Review 15795-sys-root-tokenResolvedTom Clegg11/23/2019

Actions

Related issues

Related to Arvados - Story #15107: [controller] Implement native Google login (configurable as an alternative to sso-provider)ResolvedTom Clegg10/31/2019

Actions
Related to Arvados - Story #15720: [API] Unified user listing across all clusters in a federationResolvedTom Clegg11/19/2019

Actions
Actions #1

Updated by Tom Clegg almost 3 years ago

  • Project changed from 40 to Arvados
Actions #2

Updated by Tom Clegg almost 3 years ago

  • Related to Story #15107: [controller] Implement native Google login (configurable as an alternative to sso-provider) added
Actions #3

Updated by Tom Clegg almost 3 years ago

  • Description updated (diff)
Actions #4

Updated by Tom Clegg over 2 years ago

  • Related to Story #15720: [API] Unified user listing across all clusters in a federation added
Actions #5

Updated by Peter Amstutz over 2 years ago

  • Assigned To set to Peter Amstutz
  • Target version changed from To Be Groomed to 2019-12-04 Sprint
Actions #7

Updated by Tom Clegg over 2 years ago

Tests will probably do better after merging master.

I was surprised to find this inside the "not a v2 token" else block. Unless there's a reason SystemRootToken shouldn't be allowed to look like a v2 token, I think "if SystemRoot ... elsif v2 ... else ..." would be a bit less mysterious.

The create_superuser_token.rb script creates an api_client entry with is_trusted=true. Here we use api_client_id=0 instead, which (I think) will mean is_trusted=false. Does this have any implications for services or other expected root token usage?

Add SystemRootToken instructions to https://doc.arvados.org/master/install/install-api-server.html, and remove the steps on keepstore (and other?) pages that say to run create_superuser_token.rb and copy it to SystemRootToken

Ensure a 1.4-to-1.5 upgrade doesn't result in an API server that won't come up (is there anything in 1.4 that would have told the admin to add a SystemRootToken?)

Actions #8

Updated by Tom Clegg over 2 years ago

Seems like we could also remove the superuser token fixture from the test database, and instruct the admin to remove their old database record after upgrading (or even do that automatically -- maybe at server startup?).

Actions #9

Updated by Peter Amstutz over 2 years ago

Tom Clegg wrote:

The create_superuser_token.rb script creates an api_client entry with is_trusted=true. Here we use api_client_id=0 instead, which (I think) will mean is_trusted=false. Does this have any implications for services or other expected root token usage?

This ends up putting a stake through the heart of "don't do a database lookup" if we can't use a static api_client_id. So I went with a new approach: synchronize the configuration system root token with a special database record "zzzzz-gj3su-000000000000000".

The "without a database lookup" part of the story is no longer true, but otherwise this ensures everything behaves normally by having the system root token be a real record.

Add SystemRootToken instructions to https://doc.arvados.org/master/install/install-api-server.html, and remove the steps on keepstore (and other?) pages that say to run create_superuser_token.rb and copy it to SystemRootToken

I will be doing a big refactor/rewrite of install and configuration docs in #15572, so can I defer this and address it there?

Ensure a 1.4-to-1.5 upgrade doesn't result in an API server that won't come up (is there anything in 1.4 that would have told the admin to add a SystemRootToken?)

Ok, I made SystemRootToken optional for API to start up. However, without setting a value there, the controller callbacks that use it won't work. Tokens issued by create_superuser_token.rb will continue to work, though.

15795-sys-root-token @ 6749376dd2fbd5f6bc6f870ae382bdb5cb8aaf17

https://ci.curoverse.com/view/Developer/job/developer-run-tests/1669/

Actions #10

Updated by Tom Clegg over 2 years ago

this ensures everything behaves normally by having the system root token be a real record

Hm. Copying/synchronizing SystemRootToken into the database feels like a step backward. Could we use a new unsaved api_client record, just like the new unsaved api_client_authorization record? Or even update the small number of places where a_c_a.a_c.is_trusted gets checked?

This is blocking #15720 because #15720 behaves badly when SystemRootToken is empty -- but isn't this latest version back to allowing SystemRootToken to be empty?

Seems like the config loader should be helping with the migration: if SystemRootToken is missing, get it from the database & warn; if not in database either, must be a new cluster, so just error out.

Actions #11

Updated by Peter Amstutz over 2 years ago

Tom Clegg wrote:

this ensures everything behaves normally by having the system root token be a real record

Hm. Copying/synchronizing SystemRootToken into the database feels like a step backward. Could we use a new unsaved api_client record, just like the new unsaved api_client_authorization record? Or even update the small number of places where a_c_a.a_c.is_trusted gets checked?

Ok I backed all that out. Creates a trusted ApiClient record to go with the ApiClientAuthorization. Passes tests. Rebased so the branch history isn't confusing.

a789502c40006646e3929246cf79f50bc89c0e54 -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1673/

Actions #12

Updated by Tom Clegg over 2 years ago

  • Status changed from New to In Progress

LGTM except dispatcher expects a uuid in the arvados/v1/api_client_authorization/current response. Pushed a fix & test:

15795-sys-root-token @ 3d5e303970a342dd80a617058852a0dab686cfd0 -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1676/

Actions #13

Updated by Peter Amstutz over 2 years ago

  • Target version deleted (2019-12-04 Sprint)

Tom Clegg wrote:

LGTM except dispatcher expects a uuid in the arvados/v1/api_client_authorization/current response. Pushed a fix & test:

15795-sys-root-token @ 3d5e303970a342dd80a617058852a0dab686cfd0 -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1676/

wb integration had many failures, rerunning here:

https://ci.curoverse.com/job/developer-run-tests-apps-workbench-integration/1774/

Actions #15

Updated by Tom Clegg over 2 years ago

The merge didn't include the fix from note-12. Looks like the missing uuid also causes a crash in containers#update when using SystemRootToken, so containers can't finish.

2019-11-28T14:31:02.716988850Z error in UpdateContainerFinal: arvados API server error: #<TypeError: no implicit conversion of nil into String> (req-14awf5imf8b9s74mztv6) (422: 422 Unprocessable Entity) returned by 4xphq.arvadosapi.com
TypeError: no implicit conversion of nil into String
/data/var-www/arvados-api/current/app/models/api_client_authorization.rb:301:in `+'
/data/var-www/arvados-api/current/app/models/api_client_authorization.rb:301:in `v2token'
/data/var-www/arvados-api/current/app/models/api_client_authorization.rb:293:in `token'
/data/var-www/arvados-api/current/app/models/container.rb:505:in `validate_change'
/data/var-www/arvados-api/current/app/controllers/application_controller.rb:120:in `update'
/data/var-www/arvados-api/current/app/controllers/arvados/v1/containers_controller.rb:33:in `block in update'
/data/var-www/arvados-api/current/app/controllers/arvados/v1/containers_controller.rb:32:in `update'
/data/var-www/arvados-api/current/app/controllers/application_controller.rb:423:in `block in set_current_request_id'
/data/var-www/arvados-api/current/app/controllers/application_controller.rb:422:in `set_current_request_id'
/data/var-www/arvados-api/current/app/middlewares/arvados_api_token.rb:66:in `call'

Merged 3d5e30397 to master @ fb72a4315bfcf64621d023a38d1544319aa3666c

Actions #17

Updated by Tom Clegg over 2 years ago

  • Status changed from In Progress to Resolved
Actions #18

Updated by Peter Amstutz over 2 years ago

  • Release set to 22
Actions

Also available in: Atom PDF