Resolve cross DB issues in ee/app/models/instance_security_dashboard.rb

Summary

The instances security dashboard retrieves a bunch of information by their relationship to a project subset. Likely a variety of these will need to be fixed here.

Further details

See https://gitlab.com/gitlab-org/gitlab/-/blob/efca8216da456678a28c4601626955534f7a9b26/ee/app/models/instance_security_dashboard.rb#L30-40

  def projects
    Project.where(id: visible_users_security_dashboard_projects)
           .with_feature_available_for_user(:security_and_compliance, user)
           .allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/485658')
  end

  def vulnerabilities
    return Vulnerability.none if projects.empty?

    Vulnerability.for_projects(projects).allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/485658')
  end

Proposal

• Update InstanceSecurityDashboard methods that query vulnerability tables, and make them pluck project IDs to avoid a cross-join.
• Other methods that query tables of the main schema can still rely on the #projects method, which return a relation.
• Remove allow_cross_joins_across_databases from #projects.

added gitlab-org#14197 (closed) as parent epic

marked this issue as blocked by #471791 (closed)

changed milestone to %Backlog

added backend database devopsgovern groupthreat insights maintenanceperformance sec-decomposition sectionsec typemaintenance workflowrefinement labels

set weight to 3

changed the description

mentioned in issue gitlab-org/database-team/team-tasks#458 (closed)

marked this issue as blocked by #471808 (closed)

assigned to @ghavenga

added workflowin dev label and removed workflowrefinement label

changed milestone to %17.6

mentioned in merge request !169278 (closed)

mentioned in issue gitlab-org/quality/triage-reports#20331 (closed)

mentioned in issue gitlab-org/quality/triage-reports#20447 (closed)

mentioned in issue gitlab-org/quality/triage-reports#20584 (closed)

mentioned in issue gitlab-org/quality/triage-reports#20752 (closed)

added devopssecurity risk management label and removed devopssoftware supply chain security label

added groupsecurity infrastructure label and removed groupthreat insights label

changed milestone to %17.7

added missed:17.6 label

mentioned in issue #478017 (closed)

marked this issue as related to #478017 (closed)

assigned to @fcatteau

@arpitgogia Are you addressing Resolve cross DB issues in ee/app/models/instan... (#485658 - closed) (this very issue) as part of Resolve cross join issue and dependencies for e... (#499694 - closed)? See Gregory's comment in the implementation MR, which has been closed.

I'm asking this b/c I might end up using InstanceSecurityDashboard#project to solve Resolve cross-join in ProjectsGrades.grades_for... (#503387 - closed) when .grades_for gets an instance sec dashboard. Then I would have to address this very issue, and remove allow_cross_joins_across_databases.

  def projects
    Project.where(id: visible_users_security_dashboard_projects)
           .with_feature_available_for_user(:security_and_compliance, user)
           .allow_cross_joins_across_databases(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/485658')
  end

In any case, it looks like this issue isn't in dev since the related MR has been closed, so I'm moving it back to refinement.

@fcatteau Good question.. I handled the one usage of vulnerability scopes in the instance_security_dashboard: !171725 (diffs)

The diff is already large enough so I wouldn't want to handle the projects cross join. WDYT?

OK, so in !171725 (closed) you're changing #vulnerabilities to collect project IDs separately (with a limit), which solves one of the two cross-database joins that link to this issue.

  def vulnerabilities
    return Vulnerability.none if projects.empty?

    project_ids = projects.limit(::UsersSecurityDashboardProject::SECURITY_DASHBOARD_PROJECTS_LIMIT).pluck(:id)
    Vulnerability.where(project_id: project_ids)
  end

For .grades_for with an instance, we need to change #projects to solve the cross-join, so this doesn't overlap with !171725 (closed) – I think.

Cool so I guess we can handle that in this issue's MR...

Yes, so this very issue should be resolved by #499694 (closed) and #503387 (closed), which I've marked as related issues.

I've updated the issue description to reflect this:

• Resolve cross-join in #vulnerabilities as part of Resolve cross join issue and dependencies for e... (#499694 - closed). See #485658 (comment 2252705534)
• Resolve cross-join in #projects as part of Resolve cross-join in ProjectsGrades.grades_for... (#503387 - closed). See #485658 (comment 2252636338)

Actually many tests fail when #projects is changed, and these tests aren't related to .grades_for (#503387 (closed)). See https://gitlab.com/gitlab-org/gitlab/-/pipelines/1586231478/test_report?job_name=rspec-ee+integration+pg14+single-db-sec-connection

I'm marking this issue as blocked. Let's re-assess the effort and implementation plan once the two others are done.

I've collected the failed specs and the corresponding failures of https://gitlab.com/gitlab-org/gitlab/-/pipelines/1586231478. Again, in that pipeline temporary_* was removed from #projects.

ee/spec/graphql/resolvers/vulnerabilities_resolver_spec.rb
ee/spec/graphql/resolvers/vulnerabilities/scanners_resolver_spec.rb
ee/spec/graphql/resolvers/vulnerability_severities_count_resolver_spec.rb
ee/spec/models/instance_security_dashboard_spec.rb
ee/spec/requests/api/graphql/vulnerabilities/cve_enrichment_spec.rb
ee/spec/requests/api/graphql/vulnerabilities/details_spec.rb
ee/spec/requests/api/graphql/vulnerabilities/external_issue_links_spec.rb
ee/spec/requests/api/graphql/vulnerabilities/fields_spec.rb
ee/spec/requests/api/graphql/vulnerabilities/identifiers_spec.rb
ee/spec/requests/api/graphql/vulnerabilities/issue_links_spec.rb
ee/spec/requests/api/graphql/vulnerabilities/location_spec.rb
ee/spec/requests/api/graphql/vulnerabilities/primary_identifier_spec.rb
ee/spec/requests/api/graphql/vulnerabilities/scanner_spec.rb
ee/spec/requests/api/graphql/vulnerabilities/sort_spec.rb

cc @arpitgogia For coodination.

Failed specs

rspec ./ee/spec/graphql/resolvers/vulnerabilities_resolver_spec.rb:262 # Resolvers::VulnerabilitiesResolver#resolve when resolving vulnerabilities for an instance security dashboard when user has valid projects returns vulnerabilities for all projects on the current user's instance security dashboard
rspec ./ee/spec/graphql/resolvers/vulnerabilities/scanners_resolver_spec.rb:46 # Resolvers::Vulnerabilities::ScannersResolver#resolve when listing scanners for instance dashboard 
rspec './ee/spec/graphql/resolvers/vulnerability_severities_count_resolver_spec.rb[1:1:3:1:9:1:1:1:1:1]' # Resolvers::VulnerabilitySeveritiesCountResolver#resolve when resolving vulnerabilities for an instance security dashboard when there is a current user behaves like vulnerability filterable #validate_filters for owasp_top_10 when filters are valid filter_value: "none" does not raise any error
rspec './ee/spec/graphql/resolvers/vulnerability_severities_count_resolver_spec.rb[1:1:3:1:9:1:1:1:2:1]' # Resolvers::VulnerabilitySeveritiesCountResolver#resolve when resolving vulnerabilities for an instance security dashboard when there is a current user behaves like vulnerability filterable #validate_filters for owasp_top_10 when filters are valid filter_value: "A1_2017" does not raise any error
rspec ./ee/spec/graphql/resolvers/vulnerability_severities_count_resolver_spec.rb:374 # Resolvers::VulnerabilitySeveritiesCountResolver#resolve when resolving vulnerabilities for an instance security dashboard when there is a current user returns vulnerabilities for all projects on the current user's instance security dashboard
rspec ./ee/spec/graphql/resolvers/vulnerability_severities_count_resolver_spec.rb:383 # Resolvers::VulnerabilitySeveritiesCountResolver#resolve when resolving vulnerabilities for an instance security dashboard when there is a current user when given a dismissal_reason returns vulnerability counts that match the dismissal reason
rspec ./ee/spec/graphql/resolvers/vulnerability_severities_count_resolver_spec.rb:391 # Resolvers::VulnerabilitySeveritiesCountResolver#resolve when resolving vulnerabilities for an instance security dashboard when there is a current user when filtering by state only returns vulnerabilities with matching states
rspec ./ee/spec/graphql/resolvers/vulnerability_severities_count_resolver_spec.rb:399 # Resolvers::VulnerabilitySeveritiesCountResolver#resolve when resolving vulnerabilities for an instance security dashboard when there is a current user when filtering by state and report type only returns vulnerabilities with matching states
rspec ./ee/spec/graphql/resolvers/vulnerability_severities_count_resolver_spec.rb:414 # Resolvers::VulnerabilitySeveritiesCountResolver#resolve when resolving vulnerabilities for an instance security dashboard when there is a current user when filtering by capped returns count with limit applied
rspec ./ee/spec/graphql/resolvers/vulnerability_severities_count_resolver_spec.rb:422 # Resolvers::VulnerabilitySeveritiesCountResolver#resolve when resolving vulnerabilities for an instance security dashboard when there is a current user when has_ai_resolution is set to true only returns count for vulnerabilities with AI resolutions
rspec ./ee/spec/graphql/resolvers/vulnerability_severities_count_resolver_spec.rb:430 # Resolvers::VulnerabilitySeveritiesCountResolver#resolve when resolving vulnerabilities for an instance security dashboard when there is a current user when has_ai_resolution is set to false only returns count for vulnerabilities without AI resolutions
rspec ./ee/spec/graphql/resolvers/vulnerability_severities_count_resolver_spec.rb:443 # Resolvers::VulnerabilitySeveritiesCountResolver#resolve when resolving vulnerabilities for an instance security dashboard when there is a current user when vulnerability_report_vr_filter FF is disabled when has_ai_resolution is set to true only ignores filter and returns all count
rspec ./ee/spec/graphql/resolvers/vulnerability_severities_count_resolver_spec.rb:451 # Resolvers::VulnerabilitySeveritiesCountResolver#resolve when resolving vulnerabilities for an instance security dashboard when there is a current user when vulnerability_report_vr_filter FF is disabled when has_ai_resolution is set to false only ignores filter and returns all count
rspec ./ee/spec/models/instance_security_dashboard_spec.rb:210 # InstanceSecurityDashboard#vulnerability_reads when the user cannot read all resources returns only vulnerability_reads from projects on their dashboard that they can read
rspec ./ee/spec/models/instance_security_dashboard_spec.rb:218 # InstanceSecurityDashboard#vulnerability_reads when the user can read all resources returns vulnerability_reads from all projects on the user's dashboard
rspec ./ee/spec/models/instance_security_dashboard_spec.rb:229 # InstanceSecurityDashboard#vulnerability_scanners when the user cannot read all resources returns only vulnerability scanners from projects on their dashboard that they can read
rspec ./ee/spec/models/instance_security_dashboard_spec.rb:237 # InstanceSecurityDashboard#vulnerability_scanners when the user can read all resources returns vulnerability scanners from all projects on the user's dashboard
rspec ./ee/spec/requests/api/graphql/vulnerabilities/cve_enrichment_spec.rb:51 # Query.vulnerabilities.cveEnrichment when security_dashboard feature is licensed returns cve enrichment
rspec ./ee/spec/requests/api/graphql/vulnerabilities/cve_enrichment_spec.rb:61 # Query.vulnerabilities.cveEnrichment when security_dashboard feature is licensed returns nil for non-cve identifier
rspec ./ee/spec/requests/api/graphql/vulnerabilities/details_spec.rb:180 # Query.vulnerabilities.details returns a vulnerability details
rspec ./ee/spec/requests/api/graphql/vulnerabilities/external_issue_links_spec.rb:125 # Query.vulnerabilities.externalIssueLinks when queried without reactive caching returns a list of all VulnerabilityExternalIssueLink
rspec ./ee/spec/requests/api/graphql/vulnerabilities/external_issue_links_spec.rb:60 # Query.vulnerabilities.externalIssueLinks when queried for the first time with reactive caching schedules a background job to fetch data from Jira
rspec ./ee/spec/requests/api/graphql/vulnerabilities/external_issue_links_spec.rb:67 # Query.vulnerabilities.externalIssueLinks when queried for the first time with reactive caching returns null as value for externalIssue
rspec ./ee/spec/requests/api/graphql/vulnerabilities/fields_spec.rb:50 # Query.vulnerabilities {...fields} allows nil
rspec ./ee/spec/requests/api/graphql/vulnerabilities/fields_spec.rb:56 # Query.vulnerabilities {...fields} populates required fields
rspec ./ee/spec/requests/api/graphql/vulnerabilities/fields_spec.rb:63 # Query.vulnerabilities {...fields} when finding has solution returns solution
rspec ./ee/spec/requests/api/graphql/vulnerabilities/fields_spec.rb:72 # Query.vulnerabilities {...fields} when vulnerability has no description and finding has description returns finding information
rspec ./ee/spec/requests/api/graphql/vulnerabilities/fields_spec.rb:86 # Query.vulnerabilities {...fields} when vulnerability has description and finding has description returns vulnerability information
rspec ./ee/spec/requests/api/graphql/vulnerabilities/identifiers_spec.rb:58 # Query.vulnerabilities.identifiers returns a vulnerability identifiers
rspec ./ee/spec/requests/api/graphql/vulnerabilities/issue_links_spec.rb:49 # Query.vulnerabilities.issueLinks when valid linkType argument is provided returns a list of VulnerabilityIssueLink with `CREATED` linkType
rspec ./ee/spec/requests/api/graphql/vulnerabilities/issue_links_spec.rb:55 # Query.vulnerabilities.issueLinks when valid linkType argument is provided returns a list of VulnerabilityIssueLink with `RELATED` linkType
rspec ./ee/spec/requests/api/graphql/vulnerabilities/issue_links_spec.rb:63 # Query.vulnerabilities.issueLinks when no arguments are provided returns a list of all VulnerabilityIssueLink
rspec ./ee/spec/requests/api/graphql/vulnerabilities/location_spec.rb:127 # Query.vulnerabilities.location when the vulnerability was found by a container scan returns a container location
rspec ./ee/spec/requests/api/graphql/vulnerabilities/location_spec.rb:157 # Query.vulnerabilities.location when the vulnerability was found by a generic scanner returns a generic location
rspec ./ee/spec/requests/api/graphql/vulnerabilities/location_spec.rb:200 # Query.vulnerabilities.location when the vulnerability was found by a cluster image scan returns a cluster image scanning location
rspec ./ee/spec/requests/api/graphql/vulnerabilities/location_spec.rb:248 # Query.vulnerabilities.location when the vulnerability was found by a dependency scan returns a location in a dependency
rspec ./ee/spec/requests/api/graphql/vulnerabilities/location_spec.rb:287 # Query.vulnerabilities.location when the vulnerability was found by a SAST scan returns the file and line numbers where the vulnerability is located
rspec ./ee/spec/requests/api/graphql/vulnerabilities/location_spec.rb:327 # Query.vulnerabilities.location when the vulnerability was found by a secret detection scan returns the file and line numbers where the vulnerability is located
rspec ./ee/spec/requests/api/graphql/vulnerabilities/location_spec.rb:363 # Query.vulnerabilities.location when the vulnerability was found by a DAST scan returns the URL where the vulnerability was found
rspec ./ee/spec/requests/api/graphql/vulnerabilities/primary_identifier_spec.rb:55 # Query.vulnerabilities.primaryIdentifier returns a vulnerability identifiers
rspec ./ee/spec/requests/api/graphql/vulnerabilities/scanner_spec.rb:52 # Query.vulnerabilities.scanner returns a vulnerability scanner
rspec './ee/spec/requests/api/graphql/vulnerabilities/sort_spec.rb[1:1:1:1:2:1:1]' # Query.vulnerabilities.sort sort by severity sort by SEVERITY_ASC behaves like sorted paginated query  when sorting sorts correctly
rspec './ee/spec/requests/api/graphql/vulnerabilities/sort_spec.rb[1:1:1:1:2:1:2:1]' # Query.vulnerabilities.sort sort by severity sort by SEVERITY_ASC behaves like sorted paginated query  when sorting when paginating paginates correctly
rspec './ee/spec/requests/api/graphql/vulnerabilities/sort_spec.rb[1:1:2:1:2:1:1]' # Query.vulnerabilities.sort sort by severity sort by SEVERITY_DESC behaves like sorted paginated query  when sorting sorts correctly
rspec './ee/spec/requests/api/graphql/vulnerabilities/sort_spec.rb[1:1:2:1:2:1:2:1]' # Query.vulnerabilities.sort sort by severity sort by SEVERITY_DESC behaves like sorted paginated query  when sorting when paginating paginates correctly
rspec './ee/spec/requests/api/graphql/vulnerabilities/sort_spec.rb[1:2:1:1:2:1:1]' # Query.vulnerabilities.sort sort by severity sort by SEVERITY_ASC behaves like sorted paginated query  when sorting sorts correctly
rspec './ee/spec/requests/api/graphql/vulnerabilities/sort_spec.rb[1:2:1:1:2:1:2:1]' # Query.vulnerabilities.sort sort by severity sort by SEVERITY_ASC behaves like sorted paginated query  when sorting when paginating paginates correctly
rspec './ee/spec/requests/api/graphql/vulnerabilities/sort_spec.rb[1:2:2:1:2:1:1]' # Query.vulnerabilities.sort sort by severity sort by SEVERITY_DESC behaves like sorted paginated query  when sorting sorts correctly
rspec './ee/spec/requests/api/graphql/vulnerabilities/sort_spec.rb[1:2:2:1:2:1:2:1]' # Query.vulnerabilities.sort sort by severity sort by SEVERITY_DESC behaves like sorted paginated query  when sorting when paginating paginates correctly

ee/spec/requests/api/graphql/vulnerabilities/issue_links_spec.rb

Related: Resolve cross DB issues in ee/spec/requests/api... (#480882)

ee/spec/requests/api/graphql/vulnerabilities/location_spec.rb

Related: Resolve incorrect connection usage raised in ee... (#501581 - closed)

Based on the long list of failed spec, I'm changing the weight to 5.

added workflowrefinement label and removed workflowin dev label

mentioned in merge request !172720 (merged)

changed the description

changed milestone to %17.8

changed the description

marked this issue as related to #499694 (closed)

marked this issue as related to #503387 (closed)

unassigned @ghavenga

mentioned in merge request !175602 (merged)

changed the description

changed milestone to %Backlog

changed weight to 5 from 3

mentioned in merge request !175970 (closed)

added workflowblocked label and removed workflowrefinement label

changed the description

mentioned in merge request !178177 (closed)

created branch 485658-resolve-cross-join-from-instance_security_dashboard to address this issue

mentioned in merge request !178422 (merged)

changed milestone to %17.9

added workflowrefinement label and removed workflowblocked label

changed weight to 3 from 5

mentioned in epic gitlab-org#14165

changed the description

added workflowin dev label and removed workflowrefinement label

Implementation MR !178422 (merged) is blocked by Refactor vulnerability scopes to remove cross join (!175951 - merged), which is part of Resolve cross join issue and dependencies for e... (#499694 - closed).

added workflowblocked label and removed workflowin dev label

added workflowin review label and removed workflowblocked label

closed

added closedcomplete workflowcomplete labels and removed workflowin review label

mentioned in issue #504334 (closed)

mentioned in issue #515252 (closed)