Skip to content

Resolve "Use specialized worker to refresh authorizations when `share_with_group_lock`attribute of a group is changed"

What does this MR do?

Related to #334871 (closed)

As mentioned at &3342 (comment 614081588) and &3343 (comment 631826768), we have been noticing that the hits to the endpoint PUT /api/:version/groups/:id, (ie, group update), when it updates the value of share_with_group_lock, enqueues a bunch of AuthorizedProjectsWorker and this has been a major reason for the spikes we see in this Prometheus graph.

To prevent this from happening again, we are going to experiment with using a specialized worker for refreshing authorizations in this area (while running the old approach alongside as a safety-net).

Previously:

  • Whenever share_with_group_lock of a group (say Engineering) is changed, we -
  • identify all the other groups that the projects in Engineering are shared with.
  • for each of these groups, we call group.refresh_members_authorized_projects.

Now:

  • Whenever share_with_group_lock of a group (say Engineering) is changed, we -
  • identify all the projects under Engineering that is shared with other groups
  • for all such projects, we run AuthorizedProjectUpdate::ProjectRecalculateWorker.perform_async(project.id)

The end result of both approaches is the same - members from the shared groups end up losing or gaining access to projects in Engineering based on the current value of share_with_group_lock of Engineering group.

PS: I really wanted to move the whole thing from a callback based approach to within Groups::UpdateService, but due to the urgency of the change (it is causing spikes everyday) and a need for more thorough testing if I did so, I am opting to make the change in the callback itself. I will move the whole thing to within the service at a later time, I will create an issue for this.

No changelog added as this change is behind a feature flag.

Screenshots or Screencasts (strongly suggested)

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

Does this MR contain changes to processing or storing of credentials or tokens, authorization and authentication methods or other items described in the security review guidelines? If not, then delete this Security section.

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by Manoj M J

Merge request reports