From: Tom Clegg Date: Thu, 20 Sep 2018 17:52:47 +0000 (-0400) Subject: 10181: Permit dispatcher to update log while container is running. X-Git-Tag: 1.3.0~97^2~13 X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/df50f6c77c345507502eae91bcb4713b5f8bcd5b?hp=8f592b4a53b368f82ff8be375aa163728699b5a9 10181: Permit dispatcher to update log while container is running. Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- diff --git a/doc/api/methods/containers.html.textile.liquid b/doc/api/methods/containers.html.textile.liquid index 9ebd91d2b1..e073d1c6ad 100644 --- a/doc/api/methods/containers.html.textile.liquid +++ b/doc/api/methods/containers.html.textile.liquid @@ -28,7 +28,7 @@ table(table table-bordered table-condensed). |state|string|The allowed states are "Queued", "Locked", "Running", "Cancelled" and "Complete".|See "Container states":#container_states for more details.| |started_at|datetime|When this container started running.|Null if container has not yet started.| |finished_at|datetime|When this container finished.|Null if container has not yet finished.| -|log|string|Portable data hash of the collection containing logs from a completed container run.|Null if the container is not yet finished.| +|log|string|UUID or portable data hash of a collection containing the log messages produced when executing the container.|PDH after the container is finished, otherwise UUID or null.| |environment|hash|Environment variables and values that should be set in the container environment (@docker run --env@). This augments and (when conflicts exist) overrides environment variables given in the image's Dockerfile.|Must be equal to a ContainerRequest's environment in order to satisfy the ContainerRequest.| |cwd|string|Initial working directory.|Must be equal to a ContainerRequest's cwd in order to satisfy the ContainerRequest| |command|array of strings|Command to execute.| Must be equal to a ContainerRequest's command in order to satisfy the ContainerRequest.| diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb index 5fea2d5bc7..e8a70499a9 100644 --- a/services/api/app/models/container.rb +++ b/services/api/app/models/container.rb @@ -377,24 +377,11 @@ class Container < ArvadosModel current_user.andand.is_admin end - def permission_to_update - # Override base permission check to allow auth_uuid to set progress and - # output (only). Whether it is legal to set progress and output in the current - # state has already been checked in validate_change. - current_user.andand.is_admin || - (!current_api_client_authorization.nil? and - [self.auth_uuid, self.locked_by_uuid].include? current_api_client_authorization.uuid) - end - def ensure_owner_uuid_is_permitted - # Override base permission check to allow auth_uuid to set progress and - # output (only). Whether it is legal to set progress and output in the current - # state has already been checked in validate_change. - if !current_api_client_authorization.nil? and self.auth_uuid == current_api_client_authorization.uuid - check_update_whitelist [:progress, :output] - else - super - end + # validate_change ensures owner_uuid can't be changed at all -- + # except during create, which requires admin privileges. Checking + # permission here would be superfluous. + true end def set_timestamps @@ -420,6 +407,8 @@ class Container < ArvadosModel def validate_change permitted = [:state] + progress_attrs = [:progress, :runtime_status, :log, :output] + final_attrs = [:exit_code, :finished_at] if self.new_record? permitted.push(:owner_uuid, :command, :container_image, :cwd, @@ -430,26 +419,26 @@ class Container < ArvadosModel case self.state when Locked - permitted.push :priority, :runtime_status + permitted.push :priority, :runtime_status, :log when Queued permitted.push :priority when Running - permitted.push :priority, :progress, :output, :runtime_status + permitted.push :priority, *progress_attrs if self.state_changed? permitted.push :started_at end when Complete if self.state_was == Running - permitted.push :finished_at, :output, :log, :exit_code + permitted.push *final_attrs, *progress_attrs end when Cancelled case self.state_was when Running - permitted.push :finished_at, :output, :log + permitted.push :finished_at, *progress_attrs when Queued, Locked permitted.push :finished_at, :log end @@ -459,6 +448,15 @@ class Container < ArvadosModel return false end + if current_api_client_authorization.andand.uuid.andand == self.auth_uuid + # The contained process itself can update progress indicators, + # but can't change priority etc. + permitted = permitted & [:state, :progress, :output] + elsif self.locked_by_uuid && self.locked_by_uuid != current_api_client_authorization.andand.uuid + # When locked, progress fields cannot be updated by the wrong + # dispatcher, even though it has admin privileges. + permitted = permitted - progress_attrs + end check_update_whitelist permitted end diff --git a/services/api/test/unit/container_test.rb b/services/api/test/unit/container_test.rb index b8acd4fd09..0c5e2e7ad7 100644 --- a/services/api/test/unit/container_test.rb +++ b/services/api/test/unit/container_test.rb @@ -652,32 +652,41 @@ class ContainerTest < ActiveSupport::TestCase assert c.update_attributes(exit_code: 1, state: Container::Complete) end - test "locked_by_uuid can set output on running container" do + test "locked_by_uuid can update log when locked/running, and output when running" do c, _ = minimal_new set_user_from_auth :dispatch1 c.lock - c.update_attributes! state: Container::Running - assert_equal c.locked_by_uuid, Thread.current[:api_client_authorization].uuid + assert c.update_attributes(log: collections(:real_log_collection).portable_data_hash) + assert c.update_attributes(state: Container::Running) - assert c.update_attributes output: collections(:collection_owned_by_active).portable_data_hash - assert c.update_attributes! state: Container::Complete + assert c.update_attributes(output: collections(:collection_owned_by_active).portable_data_hash) + assert c.update_attributes(log: nil) + assert c.update_attributes(log: collections(:real_log_collection).portable_data_hash) + assert c.update_attributes(state: Container::Complete, log: collections(:real_log_collection).portable_data_hash) + c.reload + assert_equal c.output, collections(:collection_owned_by_active).portable_data_hash + assert_equal c.log, collections(:real_log_collection).portable_data_hash + refute c.update_attributes(output: nil) + refute c.update_attributes(log: nil) end - test "auth_uuid can set output on running container, but not change container state" do + test "auth_uuid can set output, progress on running container -- but not state, log" do c, _ = minimal_new set_user_from_auth :dispatch1 c.lock c.update_attributes! state: Container::Running - Thread.current[:api_client_authorization] = ApiClientAuthorization.find_by_uuid(c.auth_uuid) - Thread.current[:user] = User.find_by_id(Thread.current[:api_client_authorization].user_id) - assert c.update_attributes output: collections(:collection_owned_by_active).portable_data_hash + auth = ApiClientAuthorization.find_by_uuid(c.auth_uuid) + Thread.current[:api_client_authorization] = auth + Thread.current[:api_client] = auth.api_client + Thread.current[:token] = auth.token + Thread.current[:user] = auth.user - assert_raises ArvadosModel::PermissionDeniedError do - # auth_uuid cannot set container state - c.update_attributes state: Container::Complete - end + assert c.update_attributes(output: collections(:collection_owned_by_active).portable_data_hash) + assert c.update_attributes(progress: 0.5) + refute c.update_attributes(log: collections(:real_log_collection).portable_data_hash) + refute c.update_attributes(state: Container::Complete) end test "not allowed to set output that is not readable by current user" do @@ -701,9 +710,7 @@ class ContainerTest < ActiveSupport::TestCase c.update_attributes! state: Container::Running set_user_from_auth :running_to_be_deleted_container_auth - assert_raises ArvadosModel::PermissionDeniedError do - c.update_attributes! output: collections(:foo_file).portable_data_hash - end + refute c.update_attributes(output: collections(:foo_file).portable_data_hash) end test "can set trashed output on running container" do