Skip to content

Commit

Permalink
Skip query references in Rbac when not needed
Browse files Browse the repository at this point in the history
Makes calling `.references` on the Rbac scope more conservative, since
it has the possibility of causing some nasty join bombs.

The change to the CloudTenancyMixin was made so that it can be in charge
of it's own specific references instead of relying on a quirk of
`ActiveRecord`, in which `references(nil)` will call references for all
of the `includes` values.

This is a bandage for for the following issue:

* * *

Given roughly these kind of table counts

    container_images:       700
    containers:             2500
    openscap_results:       100
    openscap_rule_results:  70000
    custom_attributes:      20000

Where half of the custom_attributes are associated with
container_images.  If the following MiqReport is run:

    cols:
    - name
    - virtual_custom_attribute_build-date:SECTION:docker_labels
    - virtual_custom_attribute_io.openshift.build.name:SECTION:docker_labels
    - virtual_custom_attribute_io.openshift.build.namespace:SECTION:docker_labels
    include:
      openscap_rule_results:
        columns:
        - severity
      containers:
        columns:
        - state

(`:custom_attributes => {}` is also part of the resulting `include`, but
pretty sure that gets tacked on in the `MiqReport#generate` call)

Without needing a report, the query that gets executed can be replicated
by doing the following:

    irb> includes_for_find = {
           :openscap_rule_results => {},
           :containers => {},
           :custom_attributes => {}
         }
    irb> ContainerImages.includes(includes_for_find)
                        .references(includes_for_find)
                        .to_sql

There would be a "LEFT JOIN" bomb that would end up causing about 40GB
of data being returned from the database.  In this case as well, there
is nothing that is making use of the references for the result, but the
extra table data would be returned from the query prior to this commit
in a very inefficient manner with loads of duplicate data.
  • Loading branch information
NickLaMuro committed Mar 13, 2018
1 parent f766292 commit d24937c
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 3 deletions.
8 changes: 7 additions & 1 deletion app/models/mixins/cloud_tenancy_mixin.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
module CloudTenancyMixin
extend ActiveSupport::Concern

QUERY_REFERENCES = {
:cloud_tenant => "source_tenant",
:ext_management_system => {}
}.freeze

module ClassMethods
include TenancyCommonMixin

Expand All @@ -13,7 +18,8 @@ def tenant_id_clause_format(tenant_ids)
end

def tenant_joins_clause(scope)
scope.includes(:cloud_tenant => "source_tenant").includes(:ext_management_system)
scope.includes(QUERY_REFERENCES)
.references(QUERY_REFERENCES) # needed for the above to work
end
end
end
10 changes: 8 additions & 2 deletions lib/rbac/filterer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,7 @@ def search(options = {})

exp_sql, exp_includes, exp_attrs = search_filter.to_sql(tz) if search_filter && !klass.try(:instances_are_derived?)
attrs[:apply_limit_in_sql] = (exp_attrs.nil? || exp_attrs[:supported_by_sql]) && user_filters["belongsto"].blank?
skip_references = skip_references?(options, attrs)

# for belongs_to filters, scope_targets uses scope to make queries. want to remove limits for those.
# if you note, the limits are put back into scope a few lines down from here
Expand All @@ -249,7 +250,7 @@ def search(options = {})
.includes(include_for_find).includes(exp_includes)
.order(order)

scope = include_references(scope, klass, include_for_find, exp_includes)
scope = include_references(scope, klass, include_for_find, exp_includes, skip_references)
scope = scope.limit(limit).offset(offset) if attrs[:apply_limit_in_sql]
targets = scope

Expand Down Expand Up @@ -283,7 +284,12 @@ def is_sti?(klass)
klass.respond_to?(:finder_needs_type_condition?) ? klass.finder_needs_type_condition? : false
end

def include_references(scope, klass, include_for_find, exp_includes)
def skip_references?(options, attrs)
options[:extra_cols].blank? && !attrs[:apply_limit_in_sql]
end

def include_references(scope, klass, include_for_find, exp_includes, skip)
return scope if skip
ref_includes = Hash(include_for_find).merge(Hash(exp_includes))
unless polymorphic_include?(klass, ref_includes)
scope = scope.references(include_for_find).references(exp_includes)
Expand Down

0 comments on commit d24937c

Please sign in to comment.