[arvados] updated: 2.1.0-3216-g6241cccae
git repository hosting
git at public.arvados.org
Tue Dec 20 18:50:35 UTC 2022
Summary of changes:
.../api/app/controllers/application_controller.rb | 15 +++++--
.../v1/api_client_authorizations_controller.rb | 8 +++-
.../arvados/v1/collections_controller.rb | 10 +++--
.../app/controllers/arvados/v1/links_controller.rb | 30 ++++++++------
services/api/app/models/link.rb | 46 ++++++++++++++--------
.../20221219165512_dedup_permission_links.rb | 44 +++++++++++++++++++++
services/api/db/structure.sql | 4 +-
.../functional/arvados/v1/nodes_controller_test.rb | 11 ------
services/api/test/unit/permission_test.rb | 16 +++++++-
9 files changed, 134 insertions(+), 50 deletions(-)
create mode 100644 services/api/db/migrate/20221219165512_dedup_permission_links.rb
via 6241cccaeba4413883699c360bde08e0e544a10e (commit)
via 3e44f15742475b79a4bfd07ec0dcaf3ea5df8e79 (commit)
from b7f373541e5b6e7489573040df5b106b786814b5 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
commit 6241cccaeba4413883699c360bde08e0e544a10e
Author: Tom Clegg <tom at curii.com>
Date: Tue Dec 20 11:39:01 2022 -0500
18693: Add row locking.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index 4625ef654..88679f4f3 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -46,7 +46,8 @@ class ApplicationController < ActionController::Base
before_action :load_limit_offset_order_params, only: [:index, :contents]
before_action :load_select_param
before_action(:find_object_by_uuid,
- except: [:index, :create] + ERROR_ACTIONS)
+ except: [:index, :create, :update] + ERROR_ACTIONS)
+ before_action :find_object_for_update, only: [:update]
before_action :load_where_param, only: [:index, :contents]
before_action :load_filters_param, only: [:index, :contents]
before_action :find_objects_for_index, :only => :index
@@ -464,7 +465,11 @@ class ApplicationController < ActionController::Base
controller_name
end
- def find_object_by_uuid
+ def find_object_for_update
+ find_object_by_uuid(with_lock: true)
+ end
+
+ def find_object_by_uuid(with_lock: false)
if params[:id] and params[:id].match(/\D/)
params[:uuid] = params.delete :id
end
@@ -475,7 +480,11 @@ class ApplicationController < ActionController::Base
@filters = []
@objects = nil
find_objects_for_index
- @object = @objects.first
+ if with_lock
+ @object = @objects.lock.first
+ else
+ @object = @objects.first
+ end
end
def nullable_attributes
diff --git a/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb b/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb
index 22bcb6c1d..8ff5520e3 100644
--- a/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb
+++ b/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb
@@ -128,7 +128,7 @@ class Arvados::V1::ApiClientAuthorizationsController < ApplicationController
super
end
- def find_object_by_uuid
+ def find_object_by_uuid(with_lock: false)
uuid_param = params[:uuid] || params[:id]
if (uuid_param != current_api_client_authorization.andand.uuid &&
!Thread.current[:api_client].andand.is_trusted)
@@ -140,7 +140,11 @@ class Arvados::V1::ApiClientAuthorizationsController < ApplicationController
@where = {}
@filters = [['uuid', '=', uuid_param]]
find_objects_for_index
- @object = @objects.first
+ query = @objects
+ if with_lock
+ query = query.lock
+ end
+ @object = query.first
end
def current_api_client_is_trusted
diff --git a/services/api/app/controllers/arvados/v1/collections_controller.rb b/services/api/app/controllers/arvados/v1/collections_controller.rb
index c9b36e19e..d4860cce1 100644
--- a/services/api/app/controllers/arvados/v1/collections_controller.rb
+++ b/services/api/app/controllers/arvados/v1/collections_controller.rb
@@ -60,7 +60,7 @@ class Arvados::V1::CollectionsController < ApplicationController
super
end
- def find_object_by_uuid
+ def find_object_by_uuid(with_lock: false)
if loc = Keep::Locator.parse(params[:id])
loc.strip_hints!
@@ -81,7 +81,11 @@ class Arvados::V1::CollectionsController < ApplicationController
# available lifetime.
select_attrs = (@select || ["manifest_text"]) | ["portable_data_hash", "trash_at"]
- if c = Collection.
+ model = Collection
+ if with_lock
+ model = model.lock
+ end
+ if c = model.
readable_by(*@read_users, opts).
where({ portable_data_hash: loc.to_s }).
order("trash_at desc").
@@ -98,7 +102,7 @@ class Arvados::V1::CollectionsController < ApplicationController
end
end
else
- super
+ super(with_lock: with_lock)
end
end
diff --git a/services/api/app/controllers/arvados/v1/links_controller.rb b/services/api/app/controllers/arvados/v1/links_controller.rb
index c9ebe0346..32d005874 100644
--- a/services/api/app/controllers/arvados/v1/links_controller.rb
+++ b/services/api/app/controllers/arvados/v1/links_controller.rb
@@ -22,10 +22,12 @@ class Arvados::V1::LinksController < ApplicationController
resource_attrs.delete :tail_kind
if resource_attrs[:link_class] == 'permission' && Link::PermLevel[resource_attrs[:name]]
- existing = Link.where(link_class: 'permission',
- tail_uuid: resource_attrs[:tail_uuid],
- head_uuid: resource_attrs[:head_uuid],
- name: Link::PermLevel.keys).first
+ existing = Link.
+ lock. # select ... for update
+ where(link_class: 'permission',
+ tail_uuid: resource_attrs[:tail_uuid],
+ head_uuid: resource_attrs[:head_uuid],
+ name: Link::PermLevel.keys).first
if existing
@object = existing
if Link::PermLevel[resource_attrs[:name]] > Link::PermLevel[existing.name]
@@ -41,9 +43,11 @@ class Arvados::V1::LinksController < ApplicationController
resource_attrs[:name] == 'can_login' &&
resource_attrs[:properties].respond_to?(:has_key?) &&
resource_attrs[:properties].has_key?(:username)
- existing = Link.where(link_class: 'permission',
- tail_uuid: resource_attrs[:tail_uuid],
- head_uuid: resource_attrs[:head_uuid]).
+ existing = Link.
+ lock. # select ... for update
+ where(link_class: 'permission',
+ tail_uuid: resource_attrs[:tail_uuid],
+ head_uuid: resource_attrs[:head_uuid]).
where('properties @> ?', SafeJSON.dump({'username' => resource_attrs[:properties][:username]})).
first
if existing
@@ -70,7 +74,7 @@ class Arvados::V1::LinksController < ApplicationController
protected
- def find_object_by_uuid
+ def find_object_by_uuid(with_lock: false)
if params[:id] && params[:id].match(/\D/)
params[:uuid] = params.delete :id
end
@@ -81,7 +85,7 @@ class Arvados::V1::LinksController < ApplicationController
.where(uuid: params[:uuid])
.first
elsif !current_user
- super
+ super(with_lock: with_lock)
else
# The usual permission-filtering index query is unnecessarily
# inefficient, and doesn't match all permission links that
@@ -89,11 +93,15 @@ class Arvados::V1::LinksController < ApplicationController
# by UUID, then check whether (a) its tail_uuid is the current
# user or (b) its head_uuid is an object the current_user
# can_manage.
- link = Link.unscoped.where(uuid: params[:uuid]).first
+ model = Link
+ if with_lock
+ model = model.lock
+ end
+ link = model.unscoped.where(uuid: params[:uuid]).first
if link && link.link_class != 'permission'
# Not a permission link. Re-fetch using generic
# permission-filtering query.
- super
+ super(with_lock: with_lock)
elsif link && (current_user.uuid == link.tail_uuid ||
current_user.can?(manage: link.head_uuid))
# Permission granted.
diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb
index e8ac51f99..4d4c2832b 100644
--- a/services/api/app/models/link.rb
+++ b/services/api/app/models/link.rb
@@ -15,6 +15,7 @@ class Link < ArvadosModel
validate :permission_to_attach_to_objects
before_update :restrict_alter_permissions
before_update :apply_max_overlapping_permissions
+ before_create :apply_max_overlapping_permissions
after_update :delete_overlapping_permissions
after_update :call_update_permissions
after_create :call_update_permissions
@@ -40,10 +41,12 @@ class Link < ArvadosModel
def apply_max_overlapping_permissions
return if self.link_class != 'permission' || !PermLevel[self.name]
- Link.where(link_class: 'permission',
- tail_uuid: self.tail_uuid,
- head_uuid: self.head_uuid,
- name: PermLevel.keys).
+ Link.
+ lock. # select ... for update
+ where(link_class: 'permission',
+ tail_uuid: self.tail_uuid,
+ head_uuid: self.head_uuid,
+ name: PermLevel.keys).
where('uuid <> ?', self.uuid).each do |other|
if PermLevel[other.name] > PermLevel[self.name]
self.name = other.name
@@ -53,23 +56,32 @@ class Link < ArvadosModel
def delete_overlapping_permissions
return if self.link_class != 'permission'
+ redundant = nil
if PermLevel[self.name]
- Link.where(link_class: 'permission',
- tail_uuid: self.tail_uuid,
- head_uuid: self.head_uuid,
- name: PermLevel.keys).
- where('uuid <> ?', self.uuid).
- delete_all
+ redundant = Link.
+ lock. # select ... for update
+ where(link_class: 'permission',
+ tail_uuid: self.tail_uuid,
+ head_uuid: self.head_uuid,
+ name: PermLevel.keys).
+ where('uuid <> ?', self.uuid)
elsif self.name == 'can_login' &&
self.properties.respond_to?(:has_key?) &&
self.properties.has_key?('username')
- Link.where(link_class: 'permission',
- tail_uuid: self.tail_uuid,
- head_uuid: self.head_uuid,
- name: 'can_login').
- where('properties @> ?', SafeJSON.dump({'username' => self.properties['username']})).
- where('uuid <> ?', self.uuid).
- delete_all
+ redundant = Link.
+ lock. # select ... for update
+ where(link_class: 'permission',
+ tail_uuid: self.tail_uuid,
+ head_uuid: self.head_uuid,
+ name: 'can_login').
+ where('properties @> ?', SafeJSON.dump({'username' => self.properties['username']})).
+ where('uuid <> ?', self.uuid)
+ end
+ if redundant
+ redundant.each do |link|
+ link.clear_permissions
+ end
+ redundant.delete_all
end
end
diff --git a/services/api/test/functional/arvados/v1/nodes_controller_test.rb b/services/api/test/functional/arvados/v1/nodes_controller_test.rb
index c61a57ecc..47f6c5ff3 100644
--- a/services/api/test/functional/arvados/v1/nodes_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/nodes_controller_test.rb
@@ -211,17 +211,6 @@ class Arvados::V1::NodesControllerTest < ActionController::TestCase
assert_response 403
end
- test "job readable after updating other attributes" do
- authorize_with :admin
- post :update, params: {
- id: nodes(:busy).uuid,
- node: {last_ping_at: 1.second.ago},
- }
- assert_response :success
- assert_equal(jobs(:nearly_finished_job).uuid, json_response["job_uuid"],
- "mismatched job UUID after ping update")
- end
-
test "node should fail ping with invalid hostname config format" do
Rails.configuration.Containers.SLURM.Managed.AssignNodeHostname = 'compute%<slot_number>04' # should end with "04d"
post :ping, params: {
diff --git a/services/api/test/unit/permission_test.rb b/services/api/test/unit/permission_test.rb
index efc43dfde..db60b4e6e 100644
--- a/services/api/test/unit/permission_test.rb
+++ b/services/api/test/unit/permission_test.rb
@@ -529,7 +529,13 @@ class PermissionTest < ActiveSupport::TestCase
assert users(:active).can?(write: col.uuid)
assert users(:active).can?(manage: col.uuid)
- l3.destroy!
+ # Creating l3 should have automatically deleted l1 and upgraded to
+ # the max permission of {l1, l3}, i.e., can_manage (see #18693) so
+ # there should be no can_read link now.
+ refute Link.where(tail_uuid: l3.tail_uuid,
+ head_uuid: l3.head_uuid,
+ link_class: 'permission',
+ name: 'can_read').any?
assert Collection.readable_by(users(:active)).where(uuid: col.uuid).first
assert users(:active).can?(read: col.uuid)
@@ -575,7 +581,13 @@ class PermissionTest < ActiveSupport::TestCase
assert users(:active).can?(write: prj.uuid)
assert users(:active).can?(manage: prj.uuid)
- l3.destroy!
+ # Creating l3 should have automatically deleted l0 and upgraded to
+ # the max permission of {l0, l3}, i.e., can_manage (see #18693) so
+ # there should be no can_read link now.
+ refute Link.where(tail_uuid: l3.tail_uuid,
+ head_uuid: l3.head_uuid,
+ link_class: 'permission',
+ name: 'can_read').any?
assert Group.readable_by(users(:active)).where(uuid: prj.uuid).first
assert users(:active).can?(read: prj.uuid)
commit 3e44f15742475b79a4bfd07ec0dcaf3ea5df8e79
Author: Tom Clegg <tom at curii.com>
Date: Mon Dec 19 20:23:21 2022 -0500
18693: Remove existing redundant permission links from db.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/services/api/db/migrate/20221219165512_dedup_permission_links.rb b/services/api/db/migrate/20221219165512_dedup_permission_links.rb
new file mode 100644
index 000000000..6b2d923d8
--- /dev/null
+++ b/services/api/db/migrate/20221219165512_dedup_permission_links.rb
@@ -0,0 +1,44 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+class DedupPermissionLinks < ActiveRecord::Migration[5.2]
+ include CurrentApiClient
+ def up
+ act_as_system_user do
+ rows = ActiveRecord::Base.connection.select_all("SELECT MIN(uuid) AS uuid, COUNT(uuid) AS n FROM links
+ WHERE tail_uuid IS NOT NULL
+ AND head_uuid IS NOT NULL
+ AND link_class = 'permission'
+ AND name in ('can_read', 'can_write', 'can_manage')
+ GROUP BY (tail_uuid, head_uuid)
+ HAVING COUNT(uuid) > 1
+ FOR UPDATE")
+ rows.each do |row|
+ Rails.logger.debug "DedupPermissionLinks: consolidating #{row['n']} links into #{row['uuid']}"
+ link = Link.find_by_uuid(row['uuid'])
+ # This no-op update has the side effect that the update hooks
+ # will merge the highest available permission into this one
+ # and then delete the others.
+ link.update_attributes!(properties: link.properties.dup)
+ end
+
+ rows = ActiveRecord::Base.connection.select_all("SELECT MIN(uuid) AS uuid, COUNT(uuid) AS n FROM links
+ WHERE tail_uuid IS NOT NULL
+ AND head_uuid IS NOT NULL
+ AND link_class = 'permission'
+ AND name = 'can_login'
+ GROUP BY (tail_uuid, head_uuid, properties)
+ HAVING COUNT(uuid) > 1
+ FOR UPDATE")
+ rows.each do |row|
+ Rails.logger.debug "DedupPermissionLinks: consolidating #{row['n']} links into #{row['uuid']}"
+ link = Link.find_by_uuid(row['uuid'])
+ link.update_attributes!(properties: link.properties.dup)
+ end
+ end
+ end
+ def down
+ # no-op -- restoring redundant records would still be redundant
+ end
+end
diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index 45559757a..e00f60291 100644
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -3185,5 +3185,7 @@ INSERT INTO "schema_migrations" (version) VALUES
('20220401153101'),
('20220505112900'),
('20220726034131'),
-('20220804133317');
+('20220804133317'),
+('20221219165512');
+
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list