Chen Zhang (4e7d5fd3) at 17 Mar 12:08

🕐 total hours spent this week by all contributors: 12

🎉 achievements:

• Steps 1-3 of the POC (gitlab-org/gitlab#590009) are now code-complete with green pipelines and awaiting review:
  • gitlab-org/gitlab!226399 (Step 1: State machine transitions for maintenance state) — pipeline passing, in review by @abdwdd
  • gitlab-org/gitlab!226983 (Step 2: Namespace-scoped read-only middleware) — pipeline passing, in review by @abdwdd
  • gitlab-org/gitlab!227343 (Step 3: Web UI maintenance error page) — pipeline passing, ready for review
• Closed 2 deferred child tasks as out of scope for the POC per @abdwdd's feedback: gitlab-org/gitlab#591692 (closed) (admin toggle) and gitlab-org/gitlab#591693 (closed) (background write path audit)

blockers:

• gitlab-org/gitlab!226399 and gitlab-org/gitlab!226983 have unresolved review discussions that need to be addressed before merging
• we might need to rethink the middleware approach as @rutgerwessels pointed out that the organization resolution is implemented in the Controller layers and runs after Rails / Grape set up the routing and no middleware is used.

▶️ next:

• Address review feedback on Steps 1-2 MRs
• Begin gitlab-org/gitlab#591691 (Step 4: GraphQL mutation enforcement) — the last remaining in-scope POC task
• As an alternative to the middleware approach, create an EnforcesNamespaceReadOnly concern (similar to the existing EnforcesStepUpAuthenticationForNamespace) and include it in Groups::ApplicationController and Projects::ApplicationController as a before_action - this is being discussed
• Target completing the full POC (all 4 steps) by end of milestone 18.11

Some minor nits, the rest LGTM

What does this MR do and why?

This MR reverts !221493

In !221493, I introduced a change that would stub Current.organization for all requests specs (Grape specs). But that can mask errors because the actual code does not always have a Current.organization: it needs to be enabled per API.

So we need this disable this for request specs until #558544 is delivered.

References

• #558544
• !221493

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Might be a good idea to also update doc/development/testing_guide/testing_with_organizations.md so that it's clearly reflecting request specs are now opt-in

I think the with_current_organization here along with a few others are redundant as we have it now on the top level block on line 5

Thanks @rutgerwessels, good catch!

It seems to me that this MR would be effectively blocked by #558544?

Thanks for the context, @rutgerwessels. That seems to be a big gap we are missing. After exploring the Organization resolution pattern, I think that moving to a controller-layer approach is the right direction here - both for consistency and for future Organization extensibility.

The Organization pattern works at two levels:

• Rails controllers: BaseActionController includes CurrentOrganization, and controllers call set_current_organization as a before_action after Rails routing has resolved params like namespace_id, group_id, id
• Grape API: API::Helpers defines set_current_organization, and individual API classes opt in via their before blocks. Grape has already parsed route params by that point.

For namespace read-only enforcement, I'm planning to follow the same pattern:

1. Create an EnforcesNamespaceReadOnly concern (similar to the existing EnforcesStepUpAuthenticationForNamespace) and include it in Groups::ApplicationController and Projects::ApplicationController as a before_action - the group/project is already loaded at that point.
2. For Grape, add the maintenance check in API::Helpers - potentially in find_group! / find_project! which are the central lookup methods all Grape endpoints already use, or as a dedicated helper called from before blocks.

This eliminates the brittle PATH_PREFIXES / route_hash parsing from the middleware approach, and naturally extends to Organizations since it uses the same injection points and pattern.

@abdwdd does this direction align with what you had in mind? If so I'll draft a new work plan for this updated PoC, scratching the middleware approach.

Chen Zhang (b6923ef2) at 14 Mar 07:06

What does this MR do and why?

This MR implements Step 3 of the POC for read-only mode for top-level groups, as tracked in #590009 (task #591690).

It replaces the flash+redirect approach for HTML requests in the namespace read-only middleware with a proper 503 error page that clearly communicates the maintenance state to users.

Depends on: !226983 (Step 2 - namespace-scoped read-only middleware)

Changes

Static error page (public/-/errors/namespace_maintenance.html)

• Self-contained HTML page modeled on public/503.html styling
• Uses the existing error-503-lg.svg illustration
• Clear messaging: "This group is temporarily in maintenance mode"
• Explains that write operations are disabled but reads still work
• Includes "Go back" link (shown when browser history is available)

Middleware update (lib/gitlab/middleware/namespace_read_only/controller.rb)

• HTML requests: Now return 503 with the static error page body instead of 302 redirect + flash
• JSON requests: Unchanged (503 + JSON body + Retry-After header)
• Both paths include Retry-After: 900 header
• Added structured logging (Gitlab::AppLogger.warn) when write requests are blocked
• Extracted json_maintenance_response and html_maintenance_response for clarity

Specs (spec/lib/gitlab/middleware/namespace_read_only_spec.rb)

• Updated all HTML response expectations from 302 to 503
• Added assertions for Content-Type, Content-Length, Retry-After headers
• Added spec for error page content rendering
• Added spec for Git LFS JSON requests
• Added spec for structured logging
• Added negative assertion ensuring JSON responses don't contain HTML

Design decisions

• Static HTML (not HAML): Rack middleware runs before Rails controllers, so the view layer is not available. This matches the existing pattern used by public/503.html, public/404.html, etc.
• 503 instead of 302: A proper 503 status code correctly signals to clients (browsers, API consumers, crawlers) that the service is temporarily unavailable, rather than a redirect which implies the resource moved.
• Retry-After on both HTML and JSON: Helps automated clients know when to retry.
• Placed in public/-/errors/: Follows the convention of public/ for static error pages, namespaced under -/errors/ to avoid path conflicts with user namespaces.

How to set up and validate locally

1. Apply Steps 1 and 2 changes first (!226399, !226983)
2. Open Rails console
3. Put a namespace in maintenance: ns = Namespace.find_by_path('my-group'); ns.update_column(:state, 6)
4. Try a write request (e.g., create an issue in that group) - should see the maintenance error page
5. Try a read request (e.g., view the group page) - should work normally
6. Try an API write request with Content-Type: application/json - should get JSON 503 response

Related issues

• Parent issue: #590009
• Task: #591690
• Depends on: !226983 (Step 2)

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Chen Zhang (b9e9ccf6) at 13 Mar 19:51

... and 11 more commits

Progress update (2026-03-13):

Steps 1 and 2 are code-complete with green pipelines and ready for review:

• !226399 (Step 1: State machine transitions) — Pipeline green, awaiting review
• !226983 (Step 2: Namespace-scoped read-only middleware) — Pipeline green, mergeable, awaiting review

Remaining work:

• #591690 — Step 3: Web UI error page (ready to start, depends on Step 1)
• #591691 — Step 4: GraphQL mutation enforcement (ready to start, depends on Step 1)

Steps 3 and 4 can be worked on in parallel. Ready to begin either once review feedback on Steps 1-2 is addressed.

@abdwdd can you review this please

Chen Zhang (08d37267) at 13 Mar 16:17

Chen Zhang (203a7055) at 13 Mar 15:12

Chen Zhang (ed9a2a8d) at 13 Mar 15:02

Chen Zhang (0203d144) at 13 Mar 14:51

@rutgerwessels Could you review this please

Chen Zhang (f85af1ea) at 12 Mar 19:08

Verification: test fails without the application code change

Following the same approach used in !225973 (merged), an integration-style test was added in spec/requests/api/notes_spec.rb that asserts NotesFinder receives organization_id when the REST API note endpoints are called.

Test passes with the application code change:

$ bundle exec rspec spec/requests/api/notes_spec.rb -e 'passes organization_id to NotesFinder'

1 example, 0 failures

Test fails without the application code change (after removing the finder_params[:organization_id] = Current.organization.id line):

$ sed -i '' '/finder_params\[:organization_id\] = Current.organization.id/d' lib/api/helpers/notes_helpers.rb
$ bundle exec rspec spec/requests/api/notes_spec.rb -e 'passes organization_id to NotesFinder'

Failures:

  1) API::Notes organization_id propagation to NotesFinder passes organization_id to NotesFinder
     Failure/Error: noteable = NotesFinder.new(current_user, params).target

       #<NotesFinder (class)> received :new with unexpected arguments
         expected: (#<User id:3 @user1>, hash_including(:organization_id=>1001))
              got: (#<User id:3 @user1>, {:project=>#<Project id:2 namespace1/project-1>>,
                    :target_iid=>1, :target_type=>"issue"})

1 example, 1 failure

This confirms the code change is necessary and properly covered.