Skip to content

Resolve "Validate the nature of the parent for `Group` & `Namespace`" [RUN ALL RSPEC] [RUN AS-IF-FOSS]

Manoj M J requested to merge 301013-factories-do-not-allow-subgroups-to-be-created-under-namespaces-allow-them-to-be-created into master

What does this MR do?

Related to #301013 (closed)

This change adds the following validations to Group & Namespace objects. (Currently behind a feature flag, disabled by default)

  • For Namespace

    • A namespace cannot have any parent associated with it.

  • For Group

    • A group can or cannot have a parent associated with it.
      • If they do have an associated parent, the parent should be a Group, and not a Namespace

Reason for change

GitLab doesn't allow Namespaces to have parent, or groups to have namespaces as parents in real-world scenario. However, in many specs, we do not follow this real-world behaviour and create nested namespaces or groups with a namespace as its parent.

The problem came to light when we were working on !52658 (merged)

As noted in !52658 (comment 501222860), the newly added feature failed in unrelated tests because the feature was written considering real-world scenarios, but an unrelated spec created a subgroup under a namespace.

To enforce real-world behaviour even in specs, these new validations can prove useful.

What if these new validations break due to data issues?

We checked the data on .com database to see if any groups or namespaces currently violate this validation and the results look satisfactory.

(Data as on 22nd Feb 2021)

Does any group have a namespace as its parent?

explain SELECT "namespaces".* FROM "namespaces" WHERE "namespaces"."id" IN (SELECT "namespaces"."parent_id" FROM "namespaces" WHERE "namespaces"."type" = 'Group') AND "namespaces"."type" IS NULL

returned

Nested Loop  (cost=438261.59..644897.54 rows=60178 width=344) (actual time=221237.436..221237.438 rows=0 loops=1)

Does any namespace have a parent?

explain SELECT "namespaces".* FROM "namespaces" WHERE "namespaces"."parent_id" IS NOT NULL AND "namespaces"."type" IS NULL

returned

Index Scan using index_namespaces_on_parent_id_and_id on public.namespaces  (cost=0.56..508033.02 rows=513293 width=344) (actual time=27003.780..27003.781 rows=0 loops=1)

What if there are any data issues in any of our self-managed instances?

Unfortunately, we do not have a way of checking for this data in our self-managed instances. This is one of the reasons why this change is behind a feature flag.

The idea being: If the rollout (#322101 (closed)) goes well on .com, we will change the feature flag to default_enabled: true (instead of completely removing the flag).

This would help any self-managed instance facing trouble with the new validation to turn off the feature flag to circumvent the issue. We will be documenting this feature flag when this is going to be default enabled.

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 Manoj M J

Merge request reports