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 (sayEngineering
) 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 (sayEngineering
) 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
-
I have included changelog trailers, or none are needed. (Does this MR need a changelog?) -
I have added/updated documentation, or it's not needed. (Is documentation required?) -
I have properly separated EE content from FOSS, or this MR is FOSS only. (Where should EE code go?) -
I have added information for database reviewers in the MR description, or it's not needed. (Does this MR have database related changes?) -
I have self-reviewed this MR per code review guidelines. -
This MR does not harm performance, or I have asked a reviewer to help assess the performance impact. (Merge request performance guidelines) -
I have followed the style guides. -
This change is backwards compatible across updates, or this does not apply.
Availability and Testing
-
I have added/updated tests following the Testing Guide, or it's not needed. (Consider all test levels. See the Test Planning Process.) -
I have tested this MR in all supported browsers, or it's not needed. -
I have informed the Infrastructure department of a default or new setting change per definition of done, or it's not needed.
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