Skip to content

Investigate backend design for group level merge request approval rules

Everyone can contribute. Help move this issue forward while earning points, leveling up and collecting rewards.

Topic to Evaluate

This issue is to document backend design for group-level rules to support the delivery of &4367

Tasks to Evaluate

  • Figure out the data model (referring to this doc for inspiration)
  • Investigate application interactions (peek at existing fabrication of ApprovalProjectRules and ApprovalMergeRequestRules)

🐘 Data Model

To store group approval rules, we need to create two new entities and some associative tables

  • ApprovalGroupRule
  • GroupProtectedBranch
erDiagram
  Namespace ||--o{ ApprovalGroupRule : ""
  ApprovalGroupRule }o--o{ User : ""
  ApprovalGroupRule }o--o{ Group : ""
  ApprovalGroupRule }o--o{ GroupProtectedBranch : ""

To provide the ability to override and revert to group approval rule within project approval rule, we need to create a link table between group-level and project level approval rules

  • ApprovalProjectRuleSources
erDiagram
  MergeRequest ||--o{ ApprovalMergeRequestRule : ""
  ApprovalMergeRequestRule ||--o{ ApprovalMergeRequestRuleSources : ""
  ApprovalProjectRule ||--o{ ApprovalMergeRequestRuleSources : ""
  Project ||--o{ ApprovalProjectRule : ""
  ApprovalProjectRule ||--o{ ApprovalProjectRuleSources : ""
  ApprovalGroupRule ||--o{ ApprovalProjectRuleSources : ""
  Namespace ||--o{ ApprovalGroupRule : ""

🔀 Process Flows

Currently, MR approval rules can be defined at the project and MR level. We are introducing group MR approval rules and would like to explore how this new domain model interacts with the existing architecture. To create an MR approval rule, the following pieces of information have to be provided:

  • Name
  • Number of approvals
  • Approvers (users or groups)
  • Target branch

Manage group MR approval rules

Requirements
  • User can manage approval rules via the UI (Groups > General Settings) or API.
  • User can't update the group approval rules when Prevent users from modifying MR approval rules. is enabled on the instance-level
Implementation
  • A new Vue app interacts with a new internal RESTful API - GET/POST/PUT/DELETE groups/:id/approval_rules (similar to ee/lib/api/project_approval_rules.rb)
  • Add a new API class API::GroupApprovalRules
  • Extend ApprovalRules::CreateService to support group target (currently supporting project and MR)
  • Extend ApprovalRules::UpdateService to support group target (currently supporting project and MR)
  • Create a new service class for destroying ApprovalRules::GroupRuleDestroyService
  • Filter eligible users (allows all users that are members of the group - see group.direct_and_indirect_users)
  • Filter groups (allows all public or visible groups to the user - see group.public_or_visible_to_user)
  • Filter protected branches (get all project ids via Project.for_group_and_its_subgroups and query protected branches using those ids) - 🎱 is this query performant?
  • Ensure the instance-level setting Prevent users from modifying MR approval rules. is taken into account. If it is enabled,
    • Prohibit creating and updating group MR approval rules via the API
    • Disable creating and updating group MR approval rules via the UI
graph LR
  API::GroupApprovalRules --> ApprovalRules::CreateService --> ApprovalRules::Updater
  API::GroupApprovalRules --> ApprovalRules::UpdateService --> ApprovalRules::Updater
  ApprovalRules::Updater --> ApprovalGroupRule
  API::GroupApprovalRules --> ApprovalRules::GroupRuleDestroyService
  ApprovalRules::GroupRuleDestroyService --> ApprovalGroupRule
Questions
  • Why do we have an internal and public API for the project and MR approval rules (ie. approval_settings and approval_rules)? ➡️ Private API is to be deprecated.
  • Why do we have separate destroy service classes but the same for creating and updating service classes?

View the approval rules on the project settings page

Requirements
  • User can see inherited group approval rules
  • Might need to filter applicable rules base on project branch names, groups and users. For example, a group rule applied to develop protected branch should not viewable and applicable on a project that does not have develop protected branch.
Implementation
  • Extend ApprovalState to support group (ie. user_defined_rules should account for group approval rules)
graph LR
  API::ProjectApprovalRules --> Project --> ApprovalState
  ApprovalState --> ApprovalProjectRule
  ApprovalState --> ApprovalGroupRule

View the approval rules on the MR page

Requirements
  • User can view project MR approval rules when creating a merge request, and have options to override them if allowed. To maintain a similar experience, we also want to surface group MR approval rules in this view.
  • User can also modify group approval rules (new MR approval rules are created in the backend). The permission to modify approval rules are controlled by the Allow overrides to approval lists per merge request (MR) setting.
  • User should be able to see which rules they have overridden
  • User can reset to the default project approval rules. These rules should be the same as how it is displayed on the project settings page, ie. including inherited group approval rules.
  • User can see suggested approvers. We need to make sure it is accounted for approvers defined in group approval rules.
Implementation
  • Extend ApprovalState to support group (ie. user_defined_rules should account for group approval rules)
graph LR
  API::MergeRequestApprovalRules --> MergeRequest --> ApprovalState
  ApprovalState --> ApprovalMergeRequestRule
  ApprovalState --> ApprovalProjectRule
  ApprovalState --> ApprovalGroupRule
Code references

Approve a merge request

User can only approve merge request if they are on the list of approvers according to the approval rules. We should account for group approval rules in this MergeRquests::ApprovalService as well.

Implementation
  • Extend ApprovalState to support group (ie. user_defined_rules should account for group approval rules)
graph LR
  API::MergeRequestApprovals --> MergeRequests::ApprovalService
  MergeRequests::ApprovalService --> MergeRequest
  MergeRequest --> ApprovalState
  ApprovalState --> ApprovalMergeRequestRule
  ApprovalState --> ApprovalProjectRule
  ApprovalState --> ApprovalGroupRule
Code references
Distribution of project approval rules (100k sample)
gitlabhq_production=> select per_project_count, count(*) from (select project_id, count(*) as per_project_count from approval_project_rules group by project_id limit 100000) AS count group by per_project_count order by per_project_count;
 per_project_count | count 
-------------------+-------
                 1 | 91479
                 2 |  7426
                 3 |   857
                 4 |   135
                 5 |    55
                 6 |     6
                 7 |    10
                 8 |    15
                 9 |    14
                10 |     2
                11 |     1
(11 rows)

Risks and Implementation Considerations

  • Integrating group-level approval rules should not worsen performance on critical flows (ie. creating and updating merge requests)
  • How to effectively deal with a deeply nested group (max ~ 20). Should we just use the root namespace?
  • Efficiently search across descendant groups and users when creating rules
  • Efficiently search across descendant branches when creating rules
  • How to handle rule conflicts (e.g. rules that contain approvers span across subgroups)

Related issues

  • Rearchitecting approval rules (#296786)
Edited by 🤖 GitLab Bot 🤖