Modify container_image policies to check container_registry_access_level [RUN ALL RSPEC] [RUN AS-IF-FOSS] (!55071) · Merge requests · GitLab.org / GitLab

Reuben Pereira requested to merge 18792-modify-container-image-policies into master Feb 24, 2021

What does this MR do?

A new project feature called container_registry was added in !54831 (merged) (project_features.container_registry_access_level). !55327 (merged) added a background migration to copy values from projects.container_registry_enabled to project_features.container_registry_access_level. A before_update callback on Project model and a before_create callback on ProjectFeature model ensures that any changes to the Project#container_registry_enabled is copied over to ProjectFeature#container_registry_access_level. Note that the other way is not true. Changes to ProjectFeature#container_registry_access_level are not synced back to Project#container_registry_enabled.

The current situation is that all code reads from and writes to the projects.container_registry_enabled column. And then the value is copied over to project_features.container_registry_access_level by the callbacks mentioned above.

This MR changes all reads to go to project_features.container_registry_access_level instead by:

Delegating Project#container_registry_enabled to ProjectFeature.
Modifying the *_container_image policies in app/policies/project_policy.rb to check the project_feature.container_registry_access_level value.
Modifying the container_registry_enabled attribute returned by the projects public API to be derived from container_registry_access_level.
This MR also contains a migration to add an index to improve the performance of the ContainerRepository.for_group_and_its_subgroups scope. We can delete the existing index (index_project_features_on_project_id) later. I didn't delete the existing index in this MR since we probably don't want there to be no index for any amount of time.

This change is behind a feature flag: read_container_registry_access_level.

I've added a changelog only for the migration in this MR, not for the other changes since the other changes are not user facing, and they're behind a feature flag.

This is the 4th in a series of MRs:

Migration output:

== 20210606143426 AddIndexForContainerRegistryAccessLevel: migrating ==========
-- indexes("project_features")
   -> 0.0034s
-- current_schema()
   -> 0.0001s
-- execute("SET statement_timeout TO 0")
   -> 0.0014s
-- execute("CREATE UNIQUE INDEX CONCURRENTLY index_project_features_on_project_id_include_container_registry ON project_features USING btree (project_id) INCLUDE (container_registry_access_level)")
   -> 0.0058s
-- execute("RESET ALL")
   -> 0.0009s
-- execute("COMMENT ON INDEX index_project_features_on_project_id_include_container_registry IS 'Included column (container_registry_access_level) improves performance of the ContainerRepository.for_group_and_its_subgroups scope query'")
   -> 0.0016s
== 20210606143426 AddIndexForContainerRegistryAccessLevel: migrated (0.0168s) =

Revert output:

== 20210606143426 AddIndexForContainerRegistryAccessLevel: reverting ==========
-- transaction_open?()
   -> 0.0000s
-- indexes("project_features")
   -> 0.0022s
-- execute("SET statement_timeout TO 0")
   -> 0.0006s
-- remove_index("project_features", {:algorithm=>:concurrently, :name=>"index_project_features_on_project_id_include_container_registry"})
   -> 0.0085s
-- execute("RESET ALL")
   -> 0.0008s
== 20210606143426 AddIndexForContainerRegistryAccessLevel: reverted (0.0130s) =

ContainerRepository.for_group_and_its_subgroups scope SQL

Using container_registry_enabled (original sql)

https://console.postgres.ai/shared/8e6b3b11-97fb-412d-813f-37f1dc3934cc (cold cache)

SELECT
    "projects"."id"
FROM
    "projects"
WHERE
    "projects"."namespace_id" IN ( WITH RECURSIVE "base_and_descendants" AS ((
                SELECT
                    "namespaces".*
                FROM
                    "namespaces"
                WHERE
                    "namespaces"."type" = 'Group'
                    AND "namespaces"."id" = 9970)
            UNION (
                SELECT
                    "namespaces".*
                FROM
                    "namespaces",
                    "base_and_descendants"
                WHERE
                    "namespaces"."type" = 'Group'
                    AND "namespaces"."parent_id" = "base_and_descendants"."id"))
            SELECT
                "id"
            FROM
                "base_and_descendants" AS "namespaces")
        AND "projects"."container_registry_enabled" = TRUE

Time: 1.208 s
  - planning: 7.081 ms
  - execution: 1.201 s
    - I/O read: 1.181 s
    - I/O write: 0.000 ms

Shared buffers:
  - hits: 1248 (~9.80 MiB) from the buffer pool
  - reads: 2134 (~16.70 MiB) from the OS file cache, including disk I/O
  - dirtied: 44 (~352.00 KiB)
  - writes: 0

Using container_registry_access_level (modified sql)

https://console.postgres.ai/shared/566e4f1e-b500-4f91-85ae-961fde5811d0 (cold cache)

SELECT
    "projects"."id"
FROM
    "projects"
    LEFT JOIN project_features ON projects.id = project_features.project_id
WHERE
    "projects"."namespace_id" IN ( WITH RECURSIVE "base_and_descendants" AS ((
                SELECT
                    "namespaces".*
                FROM
                    "namespaces"
                WHERE
                    "namespaces"."type" = 'Group'
                    AND "namespaces"."id" = 9970)
            UNION (
                SELECT
                    "namespaces".*
                FROM
                    "namespaces",
                    "base_and_descendants"
                WHERE
                    "namespaces"."type" = 'Group'
                    AND "namespaces"."parent_id" = "base_and_descendants"."id"))
            SELECT
                "id"
            FROM
                "base_and_descendants" AS "namespaces")
        AND ("project_features"."container_registry_access_level" > 0
            OR "project_features"."container_registry_access_level" IS NULL)

Time: 2.273 s
  - planning: 6.777 ms
  - execution: 2.266 s
    - I/O read: 2.230 s
    - I/O write: 0.000 ms

Shared buffers:
  - hits: 6618 (~51.70 MiB) from the buffer pool
  - reads: 3750 (~29.30 MiB) from the OS file cache, including disk I/O
  - dirtied: 161 (~1.30 MiB)
  - writes: 0

Using container_registry_access_level with index (modified sql with index)

Index:

CREATE UNIQUE INDEX index_project_features_on_project_id1 ON project_features USING btree (project_id) include (container_registry_access_level)

https://console.postgres.ai/shared/d7b2f358-0d0b-4e2d-81db-d745945a94ef (cold cache)

Time: 1.083 s
  - planning: 5.763 ms
  - execution: 1.077 s
    - I/O read: 1.051 s
    - I/O write: 0.000 ms

Shared buffers:
  - hits: 6035 (~47.10 MiB) from the buffer pool
  - reads: 2272 (~17.80 MiB) from the OS file cache, including disk I/O
  - dirtied: 64 (~512.00 KiB)
  - writes: 0

Related to #18792 (closed)

Screenshots (strongly suggested)

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process.
Tested in all supported browsers
Informed Infrastructure department of a default or new setting change, if applicable per definition of done

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 Jun 08, 2021 by Reuben Pereira

Modify container_image policies to check container_registry_access_level [RUN ALL RSPEC] [RUN AS-IF-FOSS]