Feature #13143

[API] Add secret_mounts attribute to containers and container_requests

Added by Tom Clegg over 1 year ago. Updated about 1 year ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
03/09/2018
Due date:
% Done:

100%

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

Description


Subtasks

Task #13158: Review 13143-secret-mountsResolvedPeter Amstutz


Related issues

Related to Arvados - Story #13112: Provide a mechanism to store "secrets" securelyDuplicate

Associated revisions

Revision b4b8de21
Added by Tom Clegg over 1 year ago

Merge branch '13143-secret-mounts'

refs #13143

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <>

History

#1 Updated by Tom Clegg over 1 year ago

  • Related to Story #13112: Provide a mechanism to store "secrets" securely added

#2 Updated by Tom Clegg over 1 year ago

  • Tracker changed from Bug to Feature
  • Assigned To set to Tom Clegg
  • Target version set to 2018-03-28 Sprint

#3 Updated by Tom Morris over 1 year ago

  • Target version changed from 2018-03-28 Sprint to 2018-03-14 Sprint

#4 Updated by Tom Clegg over 1 year ago

  • Status changed from New to In Progress

#6 Updated by Peter Amstutz over 1 year ago

There are multiple ContainerRequestsTest failures in the workbench integration tests.

#7 Updated by Tom Clegg over 1 year ago

todo:
  • raise error if same key appears in both mounts and secret_mounts.

#9 Updated by Peter Amstutz over 1 year ago

  • From the "running" container fixture:
  secret_mounts_md5: <%= Digest::MD5.hexdigest(SafeJSON.dump({'/secret/6x9' => {'kind' => 'text', 'content' => "42\n"}})) %>

This will produce a JSON string with "kind" before "content" which is presumably inconsistent with deep_sort_hash? It looks like the tests for container reuse don't use this fixture.

  • The comment is in direct contradiction of what the actual test does:
      test "not reuse container with different secret_mounts" do
        secrets = {"/foo" => {"kind" => "binary"}}
        set_user_from_auth :active
        cr1 = create_minimal_req!(state: "Committed", priority: 1, secret_mounts: secrets.dup)
        cr1.save!
        cr2 = create_minimal_req!(state: "Committed", priority: 1, secret_mounts: secrets.dup)
        cr2.save!
        assert_not_nil cr1.container_uuid
        assert_equal cr1.container_uuid, cr2.container_uuid
      end
    
  • Similarly the comment is inconsistent with what is actually being tested:
      [
        Container::Complete,
        Container::Cancelled,
      ].each do |final_state|
        test "secret_mounts is null after container is #{final_state}" do
          assert_no_secrets_logged
        end
      end
    
  • Seems redundant with the test set immediately below it (lines 850-875), which includes testing reuse nil / {} secret_mounts:
      test "reuse container with same secret_mounts" do
    
  • Should this be SafeJSON.dump(self.class.deep_sort_hash(self.secret_mounts || {}))) ?
      def update_secret_mounts_md5
        if self.secret_mounts_changed?
          self.secret_mounts_md5 = Digest::MD5.hexdigest(
            SafeJSON.dump(self.class.deep_sort_hash(self.secret_mounts)))
        end
    

#10 Updated by Peter Amstutz over 1 year ago

For consistency with "Don't advertise secret_* columns in discovery doc" this should use start_with?('secret_') ?

        YAML.load(file).each do |name, ob|
          ob.reject! { |k, v| k == 'secret_mounts' }
        end

#11 Updated by Tom Clegg over 1 year ago

13143-secret-mounts @ 01126d2f7b8a9134615e0a5f0c933622750db937 https://ci.curoverse.com/job/developer-run-tests/644/
  • fix half/un-written tests / remove redundant tests noted in #9
  • rearrange hash to make more realistic fixture
  • skip secret_* not just secret_mounts

Should this be SafeJSON.dump(self.class.deep_sort_hash(self.secret_mounts || {}))) ?

A serialized attribute like secret_mounts is guaranteed to be the right type (Hash), unless I'm missing something here...

I see I did use "|| {}" in the reuse code, but that was unnecessary -- removed, and added some tests.

#12 Updated by Peter Amstutz over 1 year ago

So I'm doing some testing with my code rebased on top of the branch. Either I'm doing something wrong, or there's a bug preventing access to the secret_mounts endpoint:

2018-03-12T19:17:51.222533427Z crunch-run dev started
2018-03-12T19:17:51.222551825Z Executing container '2tlax-dz642-fj3rvcqmm1e0c89'
2018-03-12T19:17:51.222619178Z Executing on host '60c7246ce9d1'
2018-03-12T19:17:51.258858659Z error fetching secret_mounts: arvados API server error: Token is not associated with this container. (403: 403 Forbidden) returned by 172.17.0.2:8000
2018-03-12T19:17:51.316341206Z crunch-run finish
2018-03-12_19:22:07.10099 Error 1520882527+b1dde1fb: 403
2018-03-12_19:22:07.10152 {"method":"GET","path":"/arvados/v1/api_client_authorizations/2tlax-gj3su-pm6h67l2fclc4zp","format":"html","controller":"Arvados::V1::ApiClientAuthorizationsController","action":"show","status":403,"duration":0.64,"view":0.22,"db":0.0,"request_id":"req-71uxf8k9f6rxgiiad6i7","client_ipaddr":"172.17.0.2","client_auth":"2tlax-gj3su-681l6ikc3nfl2wk","params":{"include_trash":"true","select":"[\"uuid\"]"},"@timestamp":"2018-03-12T19:22:07.101339806Z","@version":"1","message":"[403] GET /arvados/v1/api_client_authorizations/2tlax-gj3su-pm6h67l2fclc4zp (Arvados::V1::ApiClientAuthorizationsController#show)"}
2018-03-12_19:22:07.17888 Error 1520882527+aaf2ed81: 403
2018-03-12_19:22:07.17935 {"method":"GET","path":"/arvados/v1/containers/2tlax-dz642-fj3rvcqmm1e0c89/secret_mounts","format":"html","controller":"Arvados::V1::ContainersController","action":"secret_mounts","status":403,"duration":3.93,"view":0.2,"db":0.68,"request_id":"req-aeb7gxw0r64xi4t722qz","client_ipaddr":"172.17.0.2","client_auth":"2tlax-gj3su-kpice4el961d1oh","params":{},"@timestamp":"2018-03-12T19:22:07.179140090Z","@version":"1","message":"[403] GET /arvados/v1/containers/2tlax-dz642-fj3rvcqmm1e0c89/secret_mounts (Arvados::V1::ContainersController#secret_mounts)"}

#13 Updated by Peter Amstutz over 1 year ago

Should the containers controller have this?

  accept_attribute_as_json :scheduling_parameters, Hash

I realized what I'm doing wrong in crunch-run, it needs to use the container token, not the dispatcher token.

#14 Updated by Tom Clegg over 1 year ago

Added accept_attribute_as_json, 13143-secret-mounts @ b20e9017fae4dc9876f4ffee56add1c58f3e3f21

#15 Updated by Peter Amstutz over 1 year ago

  1) Failure:
ContainerTest#test_find_reusable_method_should_select_complete_over_running_container [/usr/src/arvados/services/api/test/unit/container_test.rb:339]:
Expected nil to not be nil.

  2) Failure:
ContainerTest#test_find_reusable_method_should_select_higher_priority_queued_container [/usr/src/arvados/services/api/test/unit/container_test.rb:161]:
Expected nil to not be nil.

  3) Failure:
ContainerTest#test_find_reusable_method_should_select_latest_completed_container [/usr/src/arvados/services/api/test/unit/container_test.rb:189]:
Expected nil to not be nil.

  4) Failure:
ContainerTest#test_find_reusable_method_should_select_locked_container_most_likely_to_start_sooner [/usr/src/arvados/services/api/test/unit/container_test.rb:297]:
Expected nil to not be nil.

  5) Failure:
ContainerTest#test_find_reusable_method_should_select_locked_over_queued_container [/usr/src/arvados/services/api/test/unit/container_test.rb:368]:
Expected nil to not be nil.

  6) Failure:
ContainerTest#test_find_reusable_method_should_select_running_container_by_progress [/usr/src/arvados/services/api/test/unit/container_test.rb:276]:
Expected nil to not be nil.

  7) Failure:
ContainerTest#test_find_reusable_method_should_select_running_container_by_start_date [/usr/src/arvados/services/api/test/unit/container_test.rb:252]:
Expected nil to not be nil.

  8) Failure:
ContainerTest#test_find_reusable_method_should_select_running_over_failed_container [/usr/src/arvados/services/api/test/unit/container_test.rb:318]:
Expected nil to not be nil.

  9) Failure:
ContainerTest#test_find_reusable_method_should_select_running_over_locked_container [/usr/src/arvados/services/api/test/unit/container_test.rb:355]:
Expected nil to not be nil.

#18 Updated by Peter Amstutz over 1 year ago

This LGTM

#19 Updated by Peter Amstutz over 1 year ago

  • Status changed from In Progress to Resolved

#20 Updated by Tom Morris about 1 year ago

  • Release set to 17

Also available in: Atom PDF