Draft: Add list_for_evaluation to the policy store gem [ci skip]

Marcos Rocharequested to merge
604367-policy-store-list-for-evaluation into master

What does this MR do and why?

This MR adds list_for_evaluation(organization_id:, trigger_type:) to the policy store gem: a read that returns an organization's active policies for one trigger.

It is a separate method rather than a flag on list because the two callers need incompatible answers to the same question. Policy evaluation must never see a disabled policy or another trigger's policy, while management surfaces such as the API and the UI must see all of them, including disabled ones, which is what list already serves. A separate named method makes the evaluation read impossible to call with the wrong filters. A flag on list would have to default to one of the two answers, and whichever default it picked would be silently wrong for the other caller.

The port implements the method itself rather than leaving it abstract for each adapter. An abstract method becomes an obligation on every adapter, including the ActiveRecord adapter in !249574 (merged), which does not implement it. That would mean whichever of the two merged second broke the other. A default implementation in the port removes that coupling, so the two can merge in either order. An adapter whose storage can filter should specialize it, which !250325 (closed) does for the ActiveRecord adapter. That merge request predates this hook and will be rebased onto it. The override point is the private evaluable_policies hook rather than list_for_evaluation itself, so the guard and the normalization survive the override instead of being replaced along with the read. It is private rather than protected because protected would let a sibling class reach it on another instance, skipping both.

The method has no caller yet. The consumer is the future deployment gate, which does not exist. Whether the result should be capped is still open, left to that first caller. Govern::Policy.evaluation_candidates already chose a limit of 100 on master, but it has zero callers, so this gem does not adopt that cap yet. The order is not open in the same way: the port's default returns them by ascending id, which its own unit spec pins because the shared contract cannot hold every adapter to it, so an adapter overriding evaluable_policies has to preserve that order deliberately.

How to set up and validate locally

No licence tier, no feature flag, and no seed data are needed. The facade defaults to the in-memory adapter, so nothing touches the database and organization_id: 1 need not be a real organization. The store is per-process, so run every step below in one console session.

  1. Open a rails console with gdk rails console
  2. Create an active policy and a disabled policy for the same trigger
Gitlab::PolicyStore.create(organization_id: 1, name: "Active deployment policy",
  trigger_type: "deployment_requested")
Gitlab::PolicyStore.create(organization_id: 1, name: "Disabled deployment policy",
  trigger_type: "deployment_requested", lifecycle_state: "disabled")
  1. Create a policy for a different trigger
Gitlab::PolicyStore.create(organization_id: 1, name: "Promotion policy",
  trigger_type: "deployment_promoted")
  1. Confirm list returns all three, because it filters by neither lifecycle state nor trigger unless asked
Gitlab::PolicyStore.list(organization_id: 1).map(&:name)
# => ["Active deployment policy", "Disabled deployment policy", "Promotion policy"]
  1. Confirm list_for_evaluation returns only the active policy for the requested trigger, because the disabled policy and the other trigger's policy are both excluded
Gitlab::PolicyStore.list_for_evaluation(organization_id: 1, trigger_type: "deployment_requested").map(&:name)
# => ["Active deployment policy"]
  1. Ask for the other trigger and confirm only that trigger's active policy comes back, because the same exclusions apply once the trigger changes
Gitlab::PolicyStore.list_for_evaluation(organization_id: 1, trigger_type: "deployment_promoted").map(&:name)
# => ["Promotion policy"]
  1. Confirm the adapter implements neither the read nor its hook, so what a caller reaches is the port's own method both times
Gitlab::PolicyStore::Adapters::InMemoryPolicyRepository.instance_method(:list_for_evaluation).owner
# => Gitlab::PolicyStore::Ports::PolicyRepository
Gitlab::PolicyStore::Adapters::InMemoryPolicyRepository.instance_method(:evaluable_policies).owner
# => Gitlab::PolicyStore::Ports::PolicyRepository
  1. Confirm a blank trigger is rejected rather than read as every trigger, because list treats a nil trigger as a wildcard and evaluating every trigger's policies is the over-enforcing direction
Gitlab::PolicyStore.list_for_evaluation(organization_id: 1, trigger_type: nil)
# raises Gitlab::PolicyStore::ValidationError: Missing required attributes: trigger_type
  1. Confirm a blank organization_id is rejected rather than answered as no policies apply, because a read with no organization matches nothing, and that is indistinguishable from an organization with no applicable policy
Gitlab::PolicyStore.list_for_evaluation(organization_id: nil, trigger_type: "deployment_requested")
# raises Gitlab::PolicyStore::ValidationError: Missing required attributes: organization_id
  1. Confirm a symbol trigger_type finds the same policy a string does, because the read normalizes it the way create does
Gitlab::PolicyStore.list_for_evaluation(organization_id: 1, trigger_type: :deployment_requested).map(&:name)
# => ["Active deployment policy"]

References

Edited by Marcos Rocha

Merge request reports