Draft: Filter the evaluation read in Postgres [ci skip]

Marcos Rocharequested to merge
604367-policy-store-evaluation-read-in-postgres into master

What does this MR do and why?

Govern::PolicyStore::ActiveRecordPolicyRepository overrides evaluable_policies, the private hook list_for_evaluation calls, so the lifecycle narrowing happens in Postgres. The port's default filters the result of list in Ruby, which is correct for any adapter but reads every policy the organization has for that trigger, disabled ones included, in order to discard some of them.

The composed scopes match index_govern_policies_on_org_trigger_lifecycle_and_id on (organization_id, trigger_type, lifecycle_state, id), so the read is an index scan rather than a filter over a wider result.

The override deliberately does not call Govern::Policy.evaluation_candidates, whose limit of EVALUATION_LIMIT + 1 exists so a caller can tell a truncated read from a whole one. Nothing can do that yet, and handing a silently short list to a caller that cannot detect it is how a policy stops being enforced without anyone noticing. The cap belongs with the deployment gate that will consume this read.

This MR is stacked on !249574 (merged) and also requires !249714 (closed), which adds list_for_evaluation to the port. Since this description was written, that merge request moved the override point: list_for_evaluation now guards against a blank organization_id or trigger_type, normalizes trigger_type, and calls a private hook, evaluable_policies(organization_id:, trigger_type:), which is what an adapter overrides instead. The split keeps the guard and the normalization in the port, so an adapter overriding the hook cannot lose them. This branch predates that change: it still overrides the public list_for_evaluation and calls a helper that no longer exists, so it needs a rebase onto the dependency before it is reviewable. The port's shared contract examples will catch a rebase that forgets to move the read, because they require a blank organization_id to raise and the current override does not guard it. GitLab retargets to master automatically when the base merges.

How to set up and validate locally

On a GDK with an Ultimate license, since security_orchestration_policies is an Ultimate feature. Both merge requests named above have to be in the working tree, so start from this branch rebased onto !249714 (closed), with the override moved to the evaluable_policies hook.

  1. Create policies that differ in the two dimensions the read filters on. Run this in gdk rails console:
organization = Organizations::Organization.default_organization
group = Group.first || FactoryBot.create(:group, organization: organization)

evaluated = Govern::Policy.create!(organization: organization, namespace: group,
  name: "Evaluated policy", trigger_type: :deployment_requested)
Govern::Policy.create!(organization: organization, namespace: group,
  name: "Disabled policy", trigger_type: :deployment_requested, lifecycle_state: :disabled)
Govern::Policy.create!(organization: organization, namespace: group,
  name: "Other trigger policy", trigger_type: :deployment_promoted)
  1. Confirm the adapter returns only the active policy for the requested trigger
repository = Govern::PolicyStore::ActiveRecordPolicyRepository.new
repository.list_for_evaluation(organization_id: organization.id,
  trigger_type: "deployment_requested").map(&:name)
# => ["Evaluated policy"]
  1. Confirm the filtering happens in one statement that names lifecycle_state, which is what distinguishes this override from the port's default
ActiveRecord::QueryRecorder.new do
  repository.list_for_evaluation(organization_id: organization.id, trigger_type: "deployment_requested")
end.log.grep(/govern_policies/)
# one SELECT, whose WHERE names organization_id, trigger_type, and lifecycle_state
  1. Contrast the hook with the port's default, which returns the same policies through a wider read, because it asks for both lifecycle states and drops one in Ruby
default_read = Gitlab::PolicyStore::Ports::PolicyRepository
  .instance_method(:evaluable_policies)
  .bind(repository)
ActiveRecord::QueryRecorder.new do
  default_read.call(organization_id: organization.id, trigger_type: "deployment_requested")
end.log.grep(/govern_policies/)
# one SELECT whose WHERE names organization_id and trigger_type, but not lifecycle_state
  1. Confirm a nil trigger is still rejected, because list reads a nil trigger as every trigger. The port's guard runs ahead of the hook, so this override cannot lose it
repository.list_for_evaluation(organization_id: organization.id, trigger_type: nil)
# raises Gitlab::PolicyStore::ValidationError: Missing required attributes: trigger_type
  1. Confirm a blank organization_id is rejected too, because the port guards both arguments before the hook runs
repository.list_for_evaluation(organization_id: nil, trigger_type: "deployment_requested")
# raises Gitlab::PolicyStore::ValidationError: Missing required attributes: organization_id

This read needs no new query plan. !249130 (merged) already captured one for the same predicates at https://postgres.ai/console/gitlab/gitlab-production-sec/sessions/54424/commands/157320, an index scan with Index Cond: ((organization_id = 1) AND (trigger_type = 0) AND (lifecycle_state = 0)) and no sort step, because the index ends in id. The only difference here is the absent LIMIT, since this override does not cap.

References

Edited by Marcos Rocha

Merge request reports