Some rake tasks fail without admin mode

@reprazent @ifarkas I've been taking a more detailed look on how to fix this. My initial idea was to use some kind of block middleware that encapsulated the seed rake tasks (similar to our approach for the API requests or sidekiq jobs), so that admin mode would be auto-enabled for all relevant rake tasks. I don't think we can enable it for all rake tasks, since e.g. specs can also be launched from rake.

Unfortunately, it does not seem that there's such a middleware concept for rake. The only thing I could find are rails initializers, but these are a one-time execution, not around blocks. Additionally, we do not encapsulate the user call at the beginning, the tasks and initializers are just using an admin on-demand.

We could setup admin mode for the tasks that are actually making use of an admin user, which at the time of writing seem to be:

Multiple initializers on db/fixtures, called from gem seed_fu via rake db:seed_fu
lib/tasks/gitlab/import.rake from gitlab:import:repos
lib/tasks/gitlab/update_templates.rake from gitlab:update_project_templates
lib/tasks/gitlab/seed/group_seed.rake (as variable task parameter)

Potential issues:

Less convenient for developers, since we do not avoid the possibility of a future new task requiring admin mode and failing to be enabled.
I'm not sure if we are covering all cases, even looking for admin usage on the tasks, there's indirect references such as lib/gitlab/bare_repository_import/importer.rb being called by lib/tasks/gitlab/import.rake, or lib/tasks/gitlab/seed/group_seed.rake that takes a user parameter.

Thoughts?

@dlouzan I think the we could be okay with a minimal approach, wrapping the tasks in a Gitlab::Auth::CurrentUserMode.bypass_session! to make sure that the task can do everything it needs to.

Is that what you had in mind?

Less convenient for developers, since we do not avoid the possibility of a future new task requiring admin mode and failing to be enabled.

As soon as the admin mode is enabled by default, adding or adjusting tasks that require admin access would be immediately apparent. This should be okay.

I'm not sure if we are covering all cases, even looking for admin usage on the tasks, there's indirect references such as lib/gitlab/bare_repository_import/importer.rb being called by lib/tasks/gitlab/import.rake, or lib/tasks/gitlab/seed/group_seed.rake that takes a user parameter.

If we're not, we'll notice the moment we're enabling the feature flag by default, wouldn't we?

@dlouzan I think the we could be okay with a minimal approach, wrapping the tasks in a Gitlab::Auth::CurrentUserMode.bypass_session! to make sure that the task can do everything it needs to.

Is that what you had in mind?

@reprazent Yes, that is correct. My only doubt is in which locations to exactly put the bypass call. In principle, in all the locations I listed above, which were the tasks I could find that use an admin. It also means an explicit manual bypass on any existing fixture using an admin.

As soon as the admin mode is enabled by default, adding or adjusting tasks that require admin access would be immediately apparent. This should be okay.

If we're not, we'll notice the moment we're enabling the feature flag by default, wouldn't we?

It will be apparent for dev tasks for sure, but not so sure for all administrative tasks, unless there exists a test case for all of them Should we involve anybody that is more knolewdgeable on this part of the codebase? (not implying that you're not ).

Should we involve anybody that is more knolewdgeable on this part of the codebase (not implying that you're not ).

That's a great call, I'm not very knowledgeable on what our rake tasks do and when they are used .

Multiple initializers on db/fixtures, called from gem seed_fu via rake db:seed_fu lib/tasks/gitlab/seed/group_seed.rake (as variable task parameter)

I think these are mostly used in dev-envs, so someone from ~"group::ecosystem" (as they also work on the GDK), I think @ashmckenzie would be a good person to review.

lib/tasks/gitlab/import.rake from gitlab:import:repos
lib/tasks/gitlab/update_templates.rake from gitlab:update_project_templates

These are ~"group::import", I think. So maybe @kassio or @georgekoltsov

@reprazent I'll open an MR based on what we discussed above, but it'd be great to have somebody also from the mentioned gl team members take a look. I'm fairly sure the dev tasks will work, I'm more worried about administrative rake tasks, internally we detected issues with them after gitlab releases in the past, so we might not immediately notice that admin mode broke some of them.

@ifarkas @reprazent I had started working on siemens/gitlab!32 (closed) fixing admin mode on each case, but after our discussions about making admin mode an administrative setting, I'm wondering if a simpler solution (if possible) would be to disable the setting for all rake tasks on initialization, similar to what we do for the NullRequestStore on tests.

I don't see a lot of use for admin mode on rake tasks, if you have access to the console then you can basically do whatever you want.

Wdyt?

I don't see a lot of use for admin mode on rake tasks, if you have access to the console then you can basically do whatever you want.

@dlouzan, We can always revisit this later if needed. Let's add this to the documentation along with git operations for now.

@ifarkas Ok, but we still need to tackle the problem of the rake tasks that do actually need admin mode to be activated for them to work. What would your preferred approach be? I was thinking above about some options:

Mock somehow the setting on startup of rake, so that all calls to anything that depends on admin mode would just be ignored; either by mocking the setting to disabled, or using something like the AdminModeHelper, though I'm not so sure about putting something like this on prod code
Maybe refactor CurrentUserMode class to support ignoring completely admin mode for any admin, not only a specific admin id?

Maybe refactor CurrentUserMode class to support ignoring completely admin mode for any admin, not only a specific admin id?

@dlouzan, so that would still require someone to go through admin mode somehow, right? I think that's a difficult requirement eg. for rake tasks during dev setup initialization. Also, we don't have current_user in these rake tasks. Maybe we should always assume root?

If hooking into rake startup is possible, that might be the simplest solution.

@reprazent, WDYT?

I think that's a difficult requirement eg. for rake tasks during dev setup initialization. Also, we don't have current_user in these rake tasks. Maybe we should always assume root?

@ifarkas @dlouzan I'm not really fond of using an actual user for these kind of things. I think users can remove the root admin if they wanted to (they have console access anyway), we did that for a migration once and it could be weird: #216120 (closed)

What if we changed CurrentUserMode to check the runtime: https://gitlab.com/gitlab-org/gitlab/blob/e35cc79ac851d783ed402f74a670ade25243a7fe/lib/gitlab/runtime.rb#L52

So we could perhaps always enable admin mode if the process is rake?

diff --git a/lib/gitlab/auth/current_user_mode.rb b/lib/gitlab/auth/current_user_mode.rb
index 0f327a39f61..aeda68fb677 100644
--- a/lib/gitlab/auth/current_user_mode.rb
+++ b/lib/gitlab/auth/current_user_mode.rb
@@ -74,6 +74,7 @@ def initialize(user)
       end
 
       def admin_mode?
+        return true if Gitlab::Runtime.rake?
         return false unless user
 
         Gitlab::SafeRequestStore.fetch(admin_mode_rs_key) do

Would that do the trick?

@reprazent That is actually very nice and promising! I was looking for a way to hook into the rake tasks supported by rake itself, but this inverts the approach.

I'll give it a shot in a new MR.

See !53015 (merged)

mentioned in merge request siemens/gitlab!32 (closed)

mentioned in merge request siemens/gitlab!34 (closed)

mentioned in merge request !53015 (merged)

@mushakov Another issue in the epic that can be closed after !53015 (merged) was merged

mentioned in epic &2158

Can be closed since !53015 (merged). @dlouzan thank you, I'll assign you for future reference .

closed

assigned to @dlouzan

removed typefeature label

Some rake tasks fail without admin mode

Designs

Child items ...

Activity