Skip to content

Draft: POC: Cascade delayed project removal setting to parent namespace

Drew Blessing requested to merge dblessing_poc_cascading_settings into master

What does this MR do?

Proof of Concept for #291082 (closed)

  • Define a lockable/cascadable attribute in NamespaceSetting via lockable_attr class method.
  • For each lockable attribute, the following methods are defined (Using delayed_project_removal as an example):
    • Overridden getter - delayed_project_removal
    • delayed_project_removal_locked?
    • delayed_project_removal? alias if the attribute type is a boolean
  • For each lockable attribute, the following validators and callbacks are defined to ensure integrity:
    • Validator: delayed_project_removal_changeable? (private method)
    • Validator: lock_delayed_project_removal_changeable? (private method)
    • After update: clear_descendant_delayed_project_removal_locks (private method)

The current implementation queries for each attribute the first time it is called. This means multiple database queries if multiple lockable attributes are defined. In later iterations I suggest we add the ability to preload lockable attributes and we can build a single query. But this is probably only (most) useful when loading settings pages where we need everything at once.

The database migrations are a bit messy and should only be used as a loose guide. They were mostly quick and dirty migrations to allow me to test. The code is the most pertinent part of this POC.

What does this MR not do:

  • Does not attempt to answer frontend questions about UX/UI in terms of how we will allow a user to push the settings or lock them.
  • Change the way instance-level ApplicationSetting are stored. In the future its been suggested that we consider moving instance-level settings into a proper group in the future to make things easier. For now I think that adds too much complexity and has implications for some methods like has_parent?, etc. We should carefully consider how to implement this, and any changes there in the future can be made to work with this POC.
  • Fully implement the necessary validations and after_update on ApplicationSetting. This can mostly mirror what's in the CascadingNamespaceSettingAttribute concern, but maybe with a few more constraints. For example, I believe we want to limit the ability to push settings on GitLab.com because it wouldn't be performant. But for self-managed this should be allowed.

Database

Loading locked ancestors for a given attribute is a surprisingly fast query:

SQL:

explain SELECT "namespace_settings".* FROM "namespace_settings" WHERE (namespace_id IN (107,108,109) AND lock_delayed_project_removal = TRUE) LIMIT 1;

Explain: https://explain.depesz.com/s/EqrS

In the explain I used prevent_forking_outside_group since this already exists on GitLab.com. The performance should be functionally equivalent, though.

 Limit  (cost=0.43..7.42 rows=1 width=29) (actual time=15.306..15.307 rows=0 loops=1)
   Buffers: shared hit=9 read=4
   I/O Timings: read=15.152
   ->  Index Scan using namespace_settings_pkey on public.namespace_settings  (cost=0.43..7.42 rows=1 width=29) (actual time=15.304..15.305 rows=0 loops=1)
         Index Cond: (namespace_settings.namespace_id = ANY ('{107,108,109}'::integer[]))
         Filter: namespace_settings.prevent_forking_outside_group
         Rows Removed by Filter: 3
         Buffers: shared hit=9 read=4

Screenshots (strongly suggested)

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • 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 Drew Blessing

Merge request reports