Project

General

Profile

Bug #34093

Authorizer cache loads all permitted resources to memory

Added by Tomer Brisker 8 months ago. Updated 8 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Users, Roles and Permissions
Target version:
-
Difficulty:
Triaged:
No
Bugzilla link:
Fixed in Releases:
Found in Releases:

Description

The cache saves calculating the scope for the collection, but loads all records when calling `.include?` instead of checking for the existence of the specific record within the scope

  def collection_cache_lookup(subject, permission)
    collection = @cache[subject.class.to_s][permission] ||=
      find_collection(subject.class, :permission => permission)

    collection.include?(subject)
  end


Related issues

Related to Foreman - Bug #33585: Puma memory usage after update to 2.5Closed

Associated revisions

Revision 700f3a69 (diff)
Added by Tomer Brisker 8 months ago

Fixes #34093 - Don't eager load all authorized resources

The authorizer cache currently loads all permitted resources to memory
when `.includes?` is called just to check if one resource is in the
list. This can get very heavy when authorizing resources that have many
records, for example ConfigReports.
Instead of caching the scope and loading all AR objects to memory, we
can cache just the ids to reduce the memory usage of the cache. This is
still sub-optimal for resources that have millions of records, but is
faster for pages such as index pages which perform multiple permissions
checks for every item and don't need to perform an additional sql query
for every check.

Revision 92d40b23 (diff)
Added by Tomer Brisker 8 months ago

Refs #34093 - Minor authorizer clean ups

- Use `.exists?` instead of `.where.any?` and `.where.present?`.
`.any?` avoids triggering another sql query for loaded relations, but in
this specific case the relation won't be loaded. `.present?` loads the
relation to an array and checks if it is empty, which is obviously worse
then checking in sql.
- Use regular `Hash` instead of `HashWithIndefferentAccess`
HWIA comes with a performance overhead since it needs to handle both
string and symbol keys. Since the `@cache` is only used in one place, we
can control the keys and use a regular Hash instead.

Revision 5e3686a5 (diff)
Added by Tomer Brisker 8 months ago

Refs #34093 - Allow skipping cache from `User.can?`

Revision cbe90240 (diff)
Added by Tomer Brisker 8 months ago

Refs #34093 - Improve host config status performance

- Skip the authorizer cache. There can be millions of ConfigReports in
the db, there is no need to load all of them just to check if the user
can see the last one.
- Cache the status_link method result. The status_link method checks for
the user's permissions and is called 3 times - twice in
host_detailed_status_list directly and once inside
host_global_status_icon_class. We can avoid the extra permissions
check by caching the method's results.

Revision 806d9324 (diff)
Added by Tomer Brisker 8 months ago

Refs #34093 - Document Authorizer#can? and log huge cache loads

Also flatten the authorizer cache instead of a nested hash.

History

#1 Updated by Tomer Brisker 8 months ago

  • Description updated (diff)

#2 Updated by The Foreman Bot 8 months ago

  • Status changed from New to Ready For Testing
  • Pull request https://github.com/theforeman/foreman/pull/8972 added

#3 Updated by The Foreman Bot 8 months ago

  • Fixed in Releases 3.2.0 added

#4 Updated by Tomer Brisker 8 months ago

  • Status changed from Ready For Testing to Closed

#5 Updated by Tomer Brisker 8 months ago

  • Fixed in Releases 3.1.0 added
  • Fixed in Releases deleted (3.2.0)

#6 Updated by Lukas Zapletal 8 months ago

  • Related to Bug #33585: Puma memory usage after update to 2.5 added

Also available in: Atom PDF