From b7d199af0a1844a4b6db38d315c26365617bfe41 Mon Sep 17 00:00:00 2001 From: Peter Amstutz Date: Mon, 20 Aug 2018 21:20:21 -0400 Subject: [PATCH] 14082: Datatype validation for command, environment, mounts Arvados-DCO-1.1-Signed-off-by: Peter Amstutz --- services/api/app/models/container_request.rb | 38 +++++++++++++++++++ .../api/test/unit/container_request_test.rb | 30 ++++++++------- services/api/test/unit/container_test.rb | 8 ++-- 3 files changed, 59 insertions(+), 17 deletions(-) diff --git a/services/api/app/models/container_request.rb b/services/api/app/models/container_request.rb index dd3ff767dd..c434ee0317 100644 --- a/services/api/app/models/container_request.rb +++ b/services/api/app/models/container_request.rb @@ -33,6 +33,7 @@ class ContainerRequest < ArvadosModel validates :command, :container_image, :output_path, :cwd, :presence => true validates :output_ttl, numericality: { only_integer: true, greater_than_or_equal_to: 0 } validates :priority, numericality: { only_integer: true, greater_than_or_equal_to: 0, less_than_or_equal_to: 1000 } + validate :validate_datatypes validate :validate_scheduling_parameters validate :validate_state_change validate :check_update_whitelist @@ -228,6 +229,43 @@ class ContainerRequest < ArvadosModel end end + def validate_datatypes + command.each do |c| + if !c.is_a? String + errors.add(:command, "must be an array of strings but has entry #{c.class}") + end + end + environment.each do |k,v| + if !k.is_a?(String) || !v.is_a?(String) + errors.add(:environment, "must be an map of String to String but has entry #{k.class} to #{v.class}") + end + end + [:mounts, :secret_mounts].each do |m| + self[m].each do |k, v| + if !k.is_a?(String) || !v.is_a?(Hash) + errors.add(m, "must be an map of String to Hash but is has entry #{k.class} to #{v.class}") + end + if v["kind"].nil? + errors.add(m, "each item must have a 'kind' field") + end + [[String, ["kind", "portable_data_hash", "uuid", "device_type", + "path", "commit", "repository_name", "git_url"]], + [Integer, ["capacity"]]].each do |t, fields| + fields.each do |f| + if !v[f].nil? && !v[f].is_a?(t) + errors.add(m, "#{k}: #{f} must be a #{t} but is #{v[f].class}") + end + end + end + ["writable", "exclude_from_output"].each do |f| + if !v[f].nil? && !v[f].is_a?(TrueClass) && !v[f].is_a?(FalseClass) + errors.add(m, "#{k}: #{f} must be a #{t} but is #{v[f].class}") + end + end + end + end + end + def validate_scheduling_parameters if self.state == Committed if scheduling_parameters.include? 'partitions' and diff --git a/services/api/test/unit/container_request_test.rb b/services/api/test/unit/container_request_test.rb index f266c096b4..0a3b9b285e 100644 --- a/services/api/test/unit/container_request_test.rb +++ b/services/api/test/unit/container_request_test.rb @@ -69,7 +69,7 @@ class ContainerRequestTest < ActiveSupport::TestCase cr.container_image = "img3" cr.cwd = "/tmp3" cr.environment = {"BUP" => "BOP"} - cr.mounts = {"BAR" => "BAZ"} + cr.mounts = {"BAR" => {"kind" => "BAZ"}} cr.output_path = "/tmp4" cr.priority = 2 cr.runtime_constraints = {"vcpus" => 4} @@ -81,29 +81,33 @@ class ContainerRequestTest < ActiveSupport::TestCase end [ - {"vcpus" => 1}, - {"vcpus" => 1, "ram" => nil}, - {"vcpus" => 0, "ram" => 123}, - {"vcpus" => "1", "ram" => "123"} - ].each do |invalid_constraints| - test "Create with #{invalid_constraints}" do + {"runtime_constraints" => {"vcpus" => 1}}, + {"runtime_constraints" => {"vcpus" => 1, "ram" => nil}}, + {"runtime_constraints" => {"vcpus" => 0, "ram" => 123}}, + {"runtime_constraints" => {"vcpus" => "1", "ram" => "123"}}, + {"mounts" => {"FOO" => "BAR"}}, + {"mounts" => {"FOO" => {}}}, + {"mounts" => {"FOO" => {"kind" => "tmp", "capacity" => 42.222}}}, + {"command" => ["echo", 55]}, + {"environment" => {"FOO" => 55}} + ].each do |value| + test "Create with invalid #{value}" do set_user_from_auth :active assert_raises(ActiveRecord::RecordInvalid) do - cr = create_minimal_req!(state: "Committed", - priority: 1, - runtime_constraints: invalid_constraints) + cr = create_minimal_req!({state: "Committed", + priority: 1}.merge(value)) cr.save! end end - test "Update with #{invalid_constraints}" do + test "Update with invalid #{value}" do set_user_from_auth :active cr = create_minimal_req!(state: "Uncommitted", priority: 1) cr.save! assert_raises(ActiveRecord::RecordInvalid) do cr = ContainerRequest.find_by_uuid cr.uuid - cr.update_attributes!(state: "Committed", - runtime_constraints: invalid_constraints) + cr.update_attributes!({state: "Committed", + priority: 1}.merge(value)) end end end diff --git a/services/api/test/unit/container_test.rb b/services/api/test/unit/container_test.rb index 7ee5921e0c..83ab59b607 100644 --- a/services/api/test/unit/container_test.rb +++ b/services/api/test/unit/container_test.rb @@ -84,7 +84,7 @@ class ContainerTest < ActiveSupport::TestCase test "Container create" do act_as_system_user do c, _ = minimal_new(environment: {}, - mounts: {"BAR" => "FOO"}, + mounts: {"BAR" => {"kind" => "FOO"}}, output_path: "/tmp", priority: 1, runtime_constraints: {"vcpus" => 1, "ram" => 1}) @@ -101,7 +101,7 @@ class ContainerTest < ActiveSupport::TestCase test "Container valid priority" do act_as_system_user do c, _ = minimal_new(environment: {}, - mounts: {"BAR" => "FOO"}, + mounts: {"BAR" => {"kind" => "FOO"}}, output_path: "/tmp", priority: 1, runtime_constraints: {"vcpus" => 1, "ram" => 1}) @@ -133,8 +133,8 @@ class ContainerTest < ActiveSupport::TestCase test "Container serialized hash attributes sorted before save" do - env = {"C" => 3, "B" => 2, "A" => 1} - m = {"F" => {"kind" => 3}, "E" => {"kind" => 2}, "D" => {"kind" => 1}} + env = {"C" => "3", "B" => "2", "A" => "1"} + m = {"F" => {"kind" => "3"}, "E" => {"kind" => "2"}, "D" => {"kind" => "1"}} rc = {"vcpus" => 1, "ram" => 1, "keep_cache_ram" => 1} c, _ = minimal_new(environment: env, mounts: m, runtime_constraints: rc) assert_equal c.environment.to_json, Container.deep_sort_hash(env).to_json -- 2.30.2