From: Tom Clegg Date: Thu, 27 Jun 2019 14:50:58 +0000 (-0400) Subject: 14287: Merge branch 'master' X-Git-Tag: 2.0.0~282^2 X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/231a86fd3f7e30e9f66d71d92ad7c26578637e37?hp=-c 14287: Merge branch 'master' Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- 231a86fd3f7e30e9f66d71d92ad7c26578637e37 diff --combined build/run-tests.sh index 78c515377d,6f8f117744..b9dcd777fa --- a/build/run-tests.sh +++ b/build/run-tests.sh @@@ -77,13 -77,10 +77,14 @@@ do lib/cli lib/cmd lib/controller +lib/controller/federation +lib/controller/railsproxy +lib/controller/router +lib/controller/rpc lib/crunchstat lib/cloud lib/cloud/azure + lib/cloud/cloudtest lib/dispatchcloud lib/dispatchcloud/container lib/dispatchcloud/scheduler @@@ -652,23 -649,8 +653,8 @@@ install_env() ln -vsfT "$WORKSPACE" "$GOPATH/src/git.curoverse.com/arvados.git" go get -v github.com/kardianos/govendor cd "$GOPATH/src/git.curoverse.com/arvados.git" - if [[ -n "$short" ]]; then - go get -v -d ... - "$GOPATH/bin/govendor" sync - else - # Remove cached source dirs in workdir. Otherwise, they will - # not qualify as +missing or +external below, and we won't be - # able to detect that they're missing from vendor/vendor.json. - rm -rf vendor/*/ - go get -v -d ... - "$GOPATH/bin/govendor" sync - [[ -z $("$GOPATH/bin/govendor" list +unused +missing +external | tee /dev/stderr) ]] \ - || fatal "vendor/vendor.json has unused or missing dependencies -- try: - - (export GOPATH=\"${GOPATH}\"; cd \$GOPATH/src/git.curoverse.com/arvados.git && \$GOPATH/bin/govendor add +missing +external && \$GOPATH/bin/govendor remove +unused) - - "; - fi + go get -v -d ... + "$GOPATH/bin/govendor" sync ) || fatal "Go setup failed" setup_virtualenv "$VENVDIR" --python python2.7 @@@ -753,7 -735,7 +739,7 @@@ do_test() services/api) stop_services ;; - gofmt | doc | lib/cli | lib/cloud/azure | lib/cloud/ec2 | lib/cmd | lib/dispatchcloud/ssh_executor | lib/dispatchcloud/worker) + gofmt | govendor | doc | lib/cli | lib/cloud/azure | lib/cloud/ec2 | lib/cloud/cloudtest | lib/cmd | lib/dispatchcloud/ssh_executor | lib/dispatchcloud/worker) # don't care whether services are running ;; *) @@@ -1001,7 -983,52 +987,7 @@@ pythonstuff= ) declare -a gostuff -gostuff=( - cmd/arvados-client - cmd/arvados-server - lib/cli - lib/cmd - lib/controller - lib/crunchstat - lib/cloud - lib/cloud/azure - lib/cloud/ec2 - lib/cloud/cloudtest - lib/config - lib/dispatchcloud - lib/dispatchcloud/container - lib/dispatchcloud/scheduler - lib/dispatchcloud/ssh_executor - lib/dispatchcloud/worker - lib/service - sdk/go/arvados - sdk/go/arvadosclient - sdk/go/auth - sdk/go/blockdigest - sdk/go/dispatch - sdk/go/health - sdk/go/httpserver - sdk/go/manifest - sdk/go/asyncbuf - sdk/go/crunchrunner - sdk/go/stats - services/arv-git-httpd - services/crunchstat - services/health - services/keep-web - services/keepstore - sdk/go/keepclient - services/keep-balance - services/keepproxy - services/crunch-dispatch-local - services/crunch-dispatch-slurm - services/crunch-run - services/ws - tools/keep-block-check - tools/keep-exercise - tools/keep-rsync - tools/sync-groups -) +gostuff=($(git grep -lw func | grep \\.go | sed -e 's/\/[^\/]*$//' | sort -u)) install_apps/workbench() { cd "$WORKSPACE/apps/workbench" \ @@@ -1027,6 -1054,27 +1013,27 @@@ test_gofmt() [[ -z "$(gofmt -e -d $dirs | tee -a /dev/stderr)" ]] } + test_govendor() { + ( + set -e + cd "$GOPATH/src/git.curoverse.com/arvados.git" + # Remove cached source dirs in workdir. Otherwise, they will + # not qualify as +missing or +external below, and we won't be + # able to detect that they're missing from vendor/vendor.json. + rm -rf vendor/*/ + go get -v -d ... + "$GOPATH/bin/govendor" sync + if [[ -n $("$GOPATH/bin/govendor" list +unused +missing +external | tee /dev/stderr) ]]; then + echo >&2 "vendor/vendor.json has unused or missing dependencies -- try: + + (export GOPATH=\"${GOPATH}\"; cd \$GOPATH/src/git.curoverse.com/arvados.git && \$GOPATH/bin/govendor add +missing +external && \$GOPATH/bin/govendor remove +unused) + + " + return 1 + fi + ) + } + test_services/api() { rm -f "$WORKSPACE/services/api/git-commit.version" cd "$WORKSPACE/services/api" \ @@@ -1147,6 -1195,7 +1154,7 @@@ test_all() fi do_test gofmt + do_test govendor do_test doc do_test sdk/ruby do_test sdk/R @@@ -1221,21 -1270,19 +1229,21 @@@ els # assume emacs, or something, is offering a history buffer # and pre-populating the command will only cause trouble nextcmd= - elif [[ "$nextcmd" != "install deps" ]]; then - : - elif [[ -e "$VENVDIR/bin/activate" ]]; then - nextcmd="test lib/cmd" - else + elif [[ ! -e "$VENVDIR/bin/activate" ]]; then nextcmd="install deps" + else + nextcmd="" fi } echo help_interactive nextcmd="install deps" setnextcmd - while read -p 'What next? ' -e -i "${nextcmd}" nextcmd; do + HISTFILE="$WORKSPACE/tmp/.history" + history -r + while read -p 'What next? ' -e -i "$nextcmd" nextcmd; do + history -s "$nextcmd" + history -w read verb target opts <<<"${nextcmd}" target="${target%/}" target="${target/\/:/:}" diff --combined lib/config/config.default.yml index adb289c196,7a31d03504..8e6ed7f2ca --- a/lib/config/config.default.yml +++ b/lib/config/config.default.yml @@@ -279,6 -279,20 +279,20 @@@ Clusters # > 0s = auto-create a new version when older than the specified number of seconds. PreserveVersionIfIdle: -1s + # Managed collection properties. At creation time, if the client didn't + # provide the listed keys, they will be automatically populated following + # one of the following behaviors: + # + # * UUID of the user who owns the containing project. + # responsible_person_uuid: {Function: original_owner, Protected: true} + # + # * Default concrete value. + # foo_bar: {Value: baz, Protected: false} + # + # If Protected is true, only an admin user can modify its value. + ManagedProperties: + SAMPLE: {Function: original_owner, Protected: true} + Login: # These settings are provided by your OAuth2 provider (e.g., # sso-provider). @@@ -486,7 -500,7 +500,7 @@@ # Shell command to execute on each worker to determine whether # the worker is booted and ready to run containers. It should # exit zero if the worker is ready. - BootProbeCommand: "docker ps" + BootProbeCommand: "docker ps -q" # Minimum interval between consecutive probes to a single # worker. @@@ -679,6 -693,3 +693,6 @@@ # Workbench2 configs VocabularyURL: "" FileViewersConfigURL: "" + + # Use experimental controller code (see https://dev.arvados.org/issues/14287) + EnableBetaController14287: false diff --combined lib/config/export.go index d0f978f76c,2f79c2b296..faf542e6f7 --- a/lib/config/export.go +++ b/lib/config/export.go @@@ -75,6 -75,9 +75,9 @@@ var whitelist = map[string]bool "Collections.CollectionVersioning": false, "Collections.DefaultReplication": true, "Collections.DefaultTrashLifetime": true, + "Collections.ManagedProperties": true, + "Collections.ManagedProperties.*": true, + "Collections.ManagedProperties.*.*": true, "Collections.PreserveVersionIfIdle": true, "Collections.TrashSweepInterval": false, "Containers": true, @@@ -98,7 -101,6 +101,7 @@@ "Containers.StaleLockTimeout": false, "Containers.SupportedDockerImageFormats": true, "Containers.UsePreemptibleInstances": true, + "EnableBetaController14287": false, "Git": false, "InstanceTypes": true, "InstanceTypes.*": true, diff --combined lib/config/generated_config.go index 80fd0cb40d,763e7b2bce..5e5222d11e --- a/lib/config/generated_config.go +++ b/lib/config/generated_config.go @@@ -285,6 -285,20 +285,20 @@@ Clusters # > 0s = auto-create a new version when older than the specified number of seconds. PreserveVersionIfIdle: -1s + # Managed collection properties. At creation time, if the client didn't + # provide the listed keys, they will be automatically populated following + # one of the following behaviors: + # + # * UUID of the user who owns the containing project. + # responsible_person_uuid: {Function: original_owner, Protected: true} + # + # * Default concrete value. + # foo_bar: {Value: baz, Protected: false} + # + # If Protected is true, only an admin user can modify its value. + ManagedProperties: + SAMPLE: {Function: original_owner, Protected: true} + Login: # These settings are provided by your OAuth2 provider (e.g., # sso-provider). @@@ -492,7 -506,7 +506,7 @@@ # Shell command to execute on each worker to determine whether # the worker is booted and ready to run containers. It should # exit zero if the worker is ready. - BootProbeCommand: "docker ps" + BootProbeCommand: "docker ps -q" # Minimum interval between consecutive probes to a single # worker. @@@ -685,7 -699,4 +699,7 @@@ # Workbench2 configs VocabularyURL: "" FileViewersConfigURL: "" + + # Use experimental controller code (see https://dev.arvados.org/issues/14287) + EnableBetaController14287: false `) diff --combined sdk/go/arvados/config.go index f03fbbebe8,d7e92e6ed9..799f2e996c --- a/sdk/go/arvados/config.go +++ b/sdk/go/arvados/config.go @@@ -78,12 -78,13 +78,13 @@@ type Cluster struct Collections struct { BlobSigning bool BlobSigningKey string - DefaultReplication int BlobSigningTTL Duration - DefaultTrashLifetime Duration - TrashSweepInterval Duration CollectionVersioning bool + DefaultTrashLifetime Duration + DefaultReplication int + ManagedProperties map[string]interface{} PreserveVersionIfIdle Duration + TrashSweepInterval Duration } Git struct { Repositories string @@@ -158,8 -159,6 +159,8 @@@ UserProfileFormMessage string VocabularyURL string } + + EnableBetaController14287 bool } type Services struct { diff --combined services/api/app/models/collection.rb index f8cca5f074,39d56997a0..ac00e5d39c --- a/services/api/app/models/collection.rb +++ b/services/api/app/models/collection.rb @@@ -22,22 -22,23 +22,24 @@@ class Collection < ArvadosMode before_validation :default_empty_manifest before_validation :default_storage_classes, on: :create + before_validation :managed_properties, on: :create before_validation :check_encoding before_validation :check_manifest_validity before_validation :check_signatures before_validation :strip_signatures_and_update_replication_confirmed + before_validation :name_null_if_empty validate :ensure_pdh_matches_manifest_text validate :ensure_storage_classes_desired_is_not_empty validate :ensure_storage_classes_contain_non_empty_strings validate :versioning_metadata_updates, on: :update validate :past_versions_cannot_be_updated, on: :update + validate :protected_managed_properties_updates, on: :update after_validation :set_file_count_and_total_size before_save :set_file_names around_update :manage_versioning, unless: :is_past_version? api_accessible :user, extend: :common do |t| - t.add :name + t.add lambda { |x| x.name || "" }, as: :name t.add :description t.add :properties t.add :portable_data_hash @@@ -76,7 -77,6 +78,7 @@@ # correct timestamp in signed_manifest_text. 'manifest_text' => ['manifest_text', 'trash_at', 'is_trashed'], 'unsigned_manifest_text' => ['manifest_text'], + 'name' => ['name'], ) end @@@ -195,12 -195,6 +197,12 @@@ end end + def name_null_if_empty + if name == "" + self.name = nil + end + end + def set_file_names if self.manifest_text_changed? self.file_names = manifest_files @@@ -614,6 -608,23 +616,23 @@@ self.storage_classes_confirmed ||= [] end + # Sets managed properties at creation time + def managed_properties + managed_props = Rails.configuration.Collections.ManagedProperties.with_indifferent_access + if managed_props.empty? + return + end + (managed_props.keys - self.properties.keys).each do |key| + if managed_props[key].has_key?('Value') + self.properties[key] = managed_props[key]['Value'] + elsif managed_props[key]['Function'].andand == 'original_owner' + self.properties[key] = self.user_owner_uuid + else + logger.warn "Unidentified default property definition '#{key}': #{managed_props[key].inspect}" + end + end + end + def portable_manifest_text self.class.munge_manifest_locators(manifest_text) do |match| if match[2] # size @@@ -675,6 -686,25 +694,25 @@@ end end + def protected_managed_properties_updates + managed_properties = Rails.configuration.Collections.ManagedProperties.with_indifferent_access + if managed_properties.empty? || !properties_changed? || current_user.is_admin + return true + end + protected_props = managed_properties.keys.select do |p| + Rails.configuration.Collections.ManagedProperties[p]['Protected'] + end + # Pre-existent protected properties can't be updated + invalid_updates = properties_was.keys.select{|p| properties_was[p] != properties[p]} & protected_props + if !invalid_updates.empty? + invalid_updates.each do |p| + errors.add("protected property cannot be updated:", p) + end + raise PermissionDeniedError.new + end + true + end + def versioning_metadata_updates valid = true if !is_past_version? && current_version_uuid_changed? diff --combined services/api/test/unit/collection_test.rb index e75ad5d773,c2bf94fe73..2dd6eedcfb --- a/services/api/test/unit/collection_test.rb +++ b/services/api/test/unit/collection_test.rb @@@ -1013,20 -1013,64 +1013,81 @@@ class CollectionTest < ActiveSupport::T assert_empty Collection.where(uuid: uuid) end + test "empty names are exempt from name uniqueness" do + act_as_user users(:active) do + c1 = Collection.new(name: nil, manifest_text: '', owner_uuid: groups(:aproject).uuid) + assert c1.save + c2 = Collection.new(name: '', manifest_text: '', owner_uuid: groups(:aproject).uuid) + assert c2.save + c3 = Collection.new(name: '', manifest_text: '', owner_uuid: groups(:aproject).uuid) + assert c3.save + c4 = Collection.new(name: 'c4', manifest_text: '', owner_uuid: groups(:aproject).uuid) + assert c4.save + c5 = Collection.new(name: 'c4', manifest_text: '', owner_uuid: groups(:aproject).uuid) + assert_raises(ActiveRecord::RecordNotUnique) do + c5.save + end + end + end ++ + test "create collections with managed properties" do + Rails.configuration.Collections.ManagedProperties = { + 'default_prop1' => {'Value' => 'prop1_value'}, + 'responsible_person_uuid' => {'Function' => 'original_owner'} + } + # Test collection without initial properties + act_as_user users(:active) do + c = create_collection 'foo', Encoding::US_ASCII + assert c.valid? + assert_not_empty c.properties + assert_equal 'prop1_value', c.properties['default_prop1'] + assert_equal users(:active).uuid, c.properties['responsible_person_uuid'] + end + # Test collection with default_prop1 property already set + act_as_user users(:active) do + c = Collection.create(manifest_text: ". d41d8cd98f00b204e9800998ecf8427e 0:34:foo.txt\n", + properties: {'default_prop1' => 'custom_value'}) + assert c.valid? + assert_not_empty c.properties + assert_equal 'custom_value', c.properties['default_prop1'] + assert_equal users(:active).uuid, c.properties['responsible_person_uuid'] + end + # Test collection inside a sub project + act_as_user users(:active) do + c = Collection.create(manifest_text: ". d41d8cd98f00b204e9800998ecf8427e 0:34:foo.txt\n", + owner_uuid: groups(:asubproject).uuid) + assert c.valid? + assert_not_empty c.properties + assert_equal users(:active).uuid, c.properties['responsible_person_uuid'] + end + end + + test "update collection with protected managed properties" do + Rails.configuration.Collections.ManagedProperties = { + 'default_prop1' => {'Value' => 'prop1_value', 'Protected' => true}, + } + act_as_user users(:active) do + c = create_collection 'foo', Encoding::US_ASCII + assert c.valid? + assert_not_empty c.properties + assert_equal 'prop1_value', c.properties['default_prop1'] + # Add new property + c.properties['prop2'] = 'value2' + c.save! + c.reload + assert_equal 'value2', c.properties['prop2'] + # Try to change protected property's value + c.properties['default_prop1'] = 'new_value' + assert_raises(ArvadosModel::PermissionDeniedError) do + c.save! + end + # Admins are allowed to change protected properties + act_as_system_user do + c.properties['default_prop1'] = 'new_value' + c.save! + c.reload + assert_equal 'new_value', c.properties['default_prop1'] + end + end + end end