Add organization-scoped queries to Govern::Policy

Marcos Rocharequested to merge
604367-govern-policy-organization-scopes into master

What does this MR do and why?

Second of four MRs splitting !248000 (closed). Its base, !249126 (merged), which makes govern_policies.namespace_id nullable, has merged, so this now targets master.

Govern::Policy gains two things that have to travel together, plus the index change the first of them needs.

Organization ownership. belongs_to :namespace becomes optional, because a policy may belong to an organization without belonging to a group, and name uniqueness moves from :namespace_id to :organization_id, so a policy name is unique within an organization whichever group owns it. ReplaceGovernPoliciesPartialNameIndexes follows the validation into the schema, replacing the two partial unique indexes with one on (organization_id, name). The :without_namespace factory trait covers the form with no namespace.

The evaluation read. EVALUATION_LIMIT, the for_organization scope, and .evaluation_candidates, which returns an organization's active policies for one trigger, oldest first. It deliberately fetches one row past the limit so a caller can tell a full page from a truncated one: truncating without noticing would stop enforcing policies silently, and the adapter that consumes this reports the saturation. The ordering comes from Sortable, which the model now includes rather than declaring its own order_id_asc.

The two are one MR rather than two because the scopes exist to serve an organization-wide read, which is only correct once a policy can exist without a namespace. Splitting them would leave one MR with scopes no caller uses and a model that still rejects the rows the schema now allows.

Also retargets the jsonb validation TODO to the work item, since the authored policy schemas are no longer landing in the gem integration MR.

Queries

.evaluation_candidates, the engine fetch path:

SELECT "govern_policies".* FROM "govern_policies"
WHERE "govern_policies"."organization_id" = 1
  AND "govern_policies"."lifecycle_state" = 0
  AND "govern_policies"."trigger_type" = 0
ORDER BY "govern_policies"."id" ASC LIMIT 101

https://postgres.ai/console/gitlab/gitlab-production-sec/sessions/54424/commands/157320

for_organization with order_id_asc, the management list path:

SELECT "govern_policies".* FROM "govern_policies"
WHERE "govern_policies"."organization_id" = 1
ORDER BY "govern_policies"."id" ASC

https://postgres.ai/console/gitlab/gitlab-production-sec/sessions/54424/commands/157321.

The uniqueness validation, which now carries no namespace_id predicate:

SELECT "govern_policies".* FROM "govern_policies"
WHERE "govern_policies"."organization_id" = 1
  AND "govern_policies"."name" = 'seeded-policy-10' LIMIT 1

https://postgres.ai/console/gitlab/gitlab-production-sec/sessions/54424/commands/157322

Seeding

Everything below runs in postgres.ai. The clone carries production's schema rather than this branch's, so the indexes have to be moved by hand. It does already have !249126 (merged), which reached production, so it starts with the two partial unique indexes that this MR replaces.

There is no production data, so the plans below come from 20,000 seeded rows spread over 50 organizations, 500 namespaces, and 3 triggers, with one row in ten having no namespace:

exec INSERT INTO govern_policies (organization_id, namespace_id, name, trigger_type, mode, lifecycle_state, version, rules, actions, created_at, updated_at)
SELECT (series % 50) + 1,
       CASE WHEN series % 10 = 0 THEN NULL ELSE (series % 500) + 1 END,
       format('seeded-policy-%s', series),
       series % 3, 2, series % 2, 1, '[]'::jsonb, '[]'::jsonb, now(), now()
FROM generate_series(1, 20000) AS series;
exec ANALYZE govern_policies;

Run the uniqueness query here, before touching the indexes, to reproduce the plan that scans the organization. The two partial indexes are still the only unique indexes on name, and neither can serve a query that supplies no namespace_id predicate.

Then move the clone to this branch's schema, adding the replacement before dropping what it supersedes, and run the three queries again:

exec CREATE UNIQUE INDEX unique_govern_policies_organization_id_and_name ON govern_policies USING btree (organization_id, name);
exec DROP INDEX unique_govern_policies_org_and_name_without_namespace;
exec DROP INDEX unique_govern_policies_org_namespace_and_name;

Plans

.evaluation_candidates is served entirely by the composite index added in the previous MR, with no sort step, because the index ends in id:

Limit  (cost=0.29..62.78 rows=67 width=175) (actual time=0.013..0.186 rows=101 loops=1)
  Buffers: shared hit=109
  ->  Index Scan using index_govern_policies_on_org_trigger_lifecycle_and_id on govern_policies  (cost=0.29..62.78 rows=67 width=175) (actual time=0.013..0.182 rows=101 loops=1)
        Index Cond: ((organization_id = 1) AND (trigger_type = 0) AND (lifecycle_state = 0))
        Buffers: shared hit=109
Execution Time: 0.195 ms

list uses the same index for the organization filter and sorts the result, which is expected: it has no trigger or lifecycle predicate to make the index ordering usable.

Sort  (cost=263.51..264.51 rows=400 width=175) (actual time=0.267..0.275 rows=400 loops=1)
  Sort Key: id
  Sort Method: quicksort  Memory: 62kB
  ->  Index Scan using index_govern_policies_on_org_trigger_lifecycle_and_id on govern_policies  (cost=0.29..246.22 rows=400 width=175) (actual time=0.003..0.233 rows=400 loops=1)
        Index Cond: (organization_id = 1)
Execution Time: 0.288 ms

The uniqueness check is an index lookup on the index this MR adds:

Limit  (cost=0.29..2.31 rows=1 width=175) (actual time=0.008..0.008 rows=0 loops=1)
  Buffers: shared hit=5
  ->  Index Scan using unique_govern_policies_organization_id_and_name on govern_policies  (cost=0.29..2.31 rows=1 width=175) (actual time=0.007..0.007 rows=0 loops=1)
        Index Cond: ((organization_id = 1) AND (name = 'seeded-policy-10'::text))
        Buffers: shared hit=5
Execution Time: 0.013 ms

That index is why the migration is here rather than deferred. Without it the check has no usable index at all: both superseded unique indexes are partial, on namespace_id IS NULL and namespace_id IS NOT NULL, and a validation scoped to the organization alone supplies neither predicate, so Postgres falls back to scanning the organization and filtering by name. Measured against the same seed before the migration:

Limit  (cost=0.29..250.22 rows=1 width=175) (actual time=0.126..0.127 rows=0 loops=1)
  Buffers: shared hit=407
  ->  Index Scan using index_govern_policies_on_org_trigger_lifecycle_and_id on govern_policies  (cost=0.29..250.22 rows=1 width=175) (actual time=0.126..0.126 rows=0 loops=1)
        Index Cond: (organization_id = 1)
        Filter: (name = 'seeded-policy-10'::text)
        Rows Removed by Filter: 400
        Buffers: shared hit=407
Execution Time: 0.132 ms

Rows Removed by Filter: 400 is every policy the seed gave organization 1, at 407 buffers against 5. The cost grows with policies per organization, which matters most on GitLab.com, where every namespace currently belongs to the default organization.

Design decisions

Name uniqueness is organization-wide, not per group. A policy name is unique within an organization whichever group owns it. Organizations are the ownership model this table is moving to, and scoping uniqueness to the organization is what makes a name a stable identifier inside an evaluation bundle: .evaluation_candidates collects an organization's policies without regard to namespace, and the generated Rego identifies a policy by name alone, so two policies sharing a name inside one bundle produce indistinguishable results.

The schema enforces the rule too, rather than the model alone. The two partial unique indexes from !249126 (merged) permit two groups in one organization to hold the same name, so a validation scoped to the organization would have been stricter than the schema and bypassable through insert_all or upsert. The migration replaces both with a single unique index on (organization_id, name), which closes that gap and restores the index lookup shown above. A spec covers it by saving with validate: false and expecting ActiveRecord::RecordNotUnique.

One follow-up remains, out of scope here: decide what namespace_id means for evaluation. It is currently load-bearing for nothing, since uniqueness no longer considers it and .evaluation_candidates does not filter on it.

Migration output

Up 
== 20260808120000 ReplaceGovernPoliciesPartialNameIndexes: migrating ==========
-- transaction_open?(nil)
   -> 0.0000s
-- view_exists?(:postgres_partitions)
   -> 0.0346s
-- index_exists?(:govern_policies, [:organization_id, :name], {:unique=>true, :name=>"unique_govern_policies_organization_id_and_name", :algorithm=>:concurrently})
   -> 0.0024s
-- execute("SET statement_timeout TO 0")
   -> 0.0003s
-- add_index(:govern_policies, [:organization_id, :name], {:unique=>true, :name=>"unique_govern_policies_organization_id_and_name", :algorithm=>:concurrently})
   -> 0.0022s
-- execute("RESET statement_timeout")
   -> 0.0003s
-- transaction_open?(nil)
   -> 0.0000s
-- view_exists?(:postgres_partitions)
   -> 0.0003s
-- index_name_exists?(:govern_policies, "unique_govern_policies_org_and_name_without_namespace")
   -> 0.0005s
-- remove_index(:govern_policies, {:algorithm=>:concurrently, :name=>"unique_govern_policies_org_and_name_without_namespace"})
   -> 0.0020s
-- transaction_open?(nil)
   -> 0.0000s
-- view_exists?(:postgres_partitions)
   -> 0.0003s
-- index_name_exists?(:govern_policies, "unique_govern_policies_org_namespace_and_name")
   -> 0.0004s
-- remove_index(:govern_policies, {:algorithm=>:concurrently, :name=>"unique_govern_policies_org_namespace_and_name"})
   -> 0.0014s
== 20260808120000 ReplaceGovernPoliciesPartialNameIndexes: migrated (0.1074s) =
Down 
== 20260808120000 ReplaceGovernPoliciesPartialNameIndexes: reverting ==========
-- transaction_open?(nil)
   -> 0.0000s
-- view_exists?(:postgres_partitions)
   -> 0.0366s
-- index_exists?(:govern_policies, [:organization_id, :namespace_id, :name], {:unique=>true, :where=>"namespace_id IS NOT NULL", :name=>"unique_govern_policies_org_namespace_and_name", :algorithm=>:concurrently})
   -> 0.0029s
-- execute("SET statement_timeout TO 0")
   -> 0.0005s
-- add_index(:govern_policies, [:organization_id, :namespace_id, :name], {:unique=>true, :where=>"namespace_id IS NOT NULL", :name=>"unique_govern_policies_org_namespace_and_name", :algorithm=>:concurrently})
   -> 0.0046s
-- execute("RESET statement_timeout")
   -> 0.0003s
-- transaction_open?(nil)
   -> 0.0000s
-- view_exists?(:postgres_partitions)
   -> 0.0003s
-- index_exists?(:govern_policies, [:organization_id, :name], {:unique=>true, :where=>"namespace_id IS NULL", :name=>"unique_govern_policies_org_and_name_without_namespace", :algorithm=>:concurrently})
   -> 0.0019s
-- add_index(:govern_policies, [:organization_id, :name], {:unique=>true, :where=>"namespace_id IS NULL", :name=>"unique_govern_policies_org_and_name_without_namespace", :algorithm=>:concurrently})
   -> 0.0024s
-- transaction_open?(nil)
   -> 0.0000s
-- view_exists?(:postgres_partitions)
   -> 0.0003s
-- index_name_exists?(:govern_policies, "unique_govern_policies_organization_id_and_name")
   -> 0.0005s
-- remove_index(:govern_policies, {:algorithm=>:concurrently, :name=>"unique_govern_policies_organization_id_and_name"})
   -> 0.0022s
== 20260808120000 ReplaceGovernPoliciesPartialNameIndexes: reverted (0.0905s) =

After the rollback, Govern::Policy.connection.indexes(:govern_policies) returns exactly the pre-migration set, so the two partial indexes come back and the new one is gone.

How to set up and validate locally

Assumes the branch is checked out. Apply the migration first, because a fresh checkout does not have the new index:

bundle exec rails db:migrate

The up and down output is under Migration output above.

  1. Confirm the index replacement landed, on the rails console
Govern::Policy.connection.indexes(:govern_policies).map(&:name).grep(/and_name/)
# => ["unique_govern_policies_organization_id_and_name"]

Verify exactly one name is returned. Before the migration this returns the two partial indexes, unique_govern_policies_org_and_name_without_namespace and unique_govern_policies_org_namespace_and_name.

  1. Set up an organization and one of its groups
organization = Organizations::Organization.first
group = Group.find_by(organization_id: organization.id)

group.present? # => true
  1. Create a policy that belongs to the organization with no namespace
organization_level = Govern::Policy.create!(
  organization: organization, namespace: nil,
  name: 'Organization level', trigger_type: :deployment_requested)

organization_level.namespace_id # => nil

Verify it saves, which the previous MR's column change allows and this MR's optional association permits.

  1. Verify a group-owned policy cannot reuse that name
clashing = Govern::Policy.new(
  organization: organization, namespace: group,
  name: 'Organization level', trigger_type: :deployment_requested)

clashing.valid?        # => false
clashing.errors[:name] # => ["has already been taken"]

Verify it is rejected, because uniqueness is scoped to the organization alone: the name is taken for the whole organization, whichever group asks for it.

  1. Create the group-owned policy under a name of its own
group_level = Govern::Policy.create!(
  organization: organization, namespace: group,
  name: 'Group level', trigger_type: :deployment_requested)

Verify it saves, so the rule constrains names rather than ownership.

  1. Verify a second organization-level policy of the first name is rejected too
duplicate = Govern::Policy.new(
  organization: organization, namespace: nil,
  name: 'Organization level', trigger_type: :deployment_requested)

duplicate.valid? # => false
  1. Verify the database rejects the duplicate as well, not only the model
bypass = Govern::Policy.new(
  organization: organization, namespace: group,
  name: 'Organization level', trigger_type: :deployment_requested)

bypass.save!(validate: false)
# => ActiveRecord::RecordNotUnique

Verify it raises, because the new unique index holds even when the validation is skipped. This is the step that exercises the migration: with the two partial indexes it would have saved, since they treat a group-owned row and an organization-level row as distinct.

  1. Verify the evaluation read returns both, oldest first
candidates = Govern::Policy.evaluation_candidates(
  organization_id: organization.id, trigger_type: :deployment_requested)

candidates.map(&:id) & [organization_level.id, group_level.id]
# => [organization_level.id, group_level.id]

The intersection is deliberate: the read is organization-wide, so a GDK that already holds policies for this organization returns those as well.

  1. Disable one and confirm it drops out, because evaluation must never see a disabled policy
group_level.update!(lifecycle_state: :disabled)

Govern::Policy.evaluation_candidates(
  organization_id: organization.id, trigger_type: :deployment_requested).map(&:id)
  .include?(group_level.id)
# => false
  1. Confirm the read fetches one past the limit, so a caller can detect truncation
Govern::Policy.evaluation_candidates(
  organization_id: organization.id, trigger_type: :deployment_requested).to_sql
# => ends in LIMIT 101, with Govern::Policy::EVALUATION_LIMIT == 100

References

Edited by Marcos Rocha

Merge request reports