RFC: Policy Extension (Slots)

[bluekeyes]

Background

Using central, standardized policies is an effective way to make sure repositories implement required approval rules and avoid configurations that could lead to unexpected behavior. Policy Bot supports several mechanisms to share policies such as direct remote references and organization defaults.

Much of Policy Bot's power comes from implementing custom rules that apply in specific situations. This means repositories often want to extend a standard policy with some customization that only applies in that project. Because Policy Bot does not support extending remote policies, repositories must copy and modify the standard policy, losing the benefits of sharing a common policy.

This proposal describes a method to allow repositories to extend shared policies with additional rules without undermining the guarantees of the shared policy.

Proposal: Slots

Policies that allow extension define "slots" at specific locations in their rule hierarchy. A slot is a placeholder that can be replaced by a rule tree, defined in the same policy or in a child policy. The result behaves as if the new rules were placed directly in the policy at the location of the slot marker.

Policies can optionally provide default rules for slots. This allows two modes of operation:

• Empty slots allow extending a policy with new rules
• Slots with defaults allow replacing specific behavior in a policy

Slots are optional by default, but may be marked as required. If after all parsing, a policy has an empty required slot, the policy is invalid. The validation error includes the names of the slots that the policy must define.

Defining Slots

Policies define slots as part of the standard policy structure in the policy key. Slots are only valid in approval policies:

policy:
  approval:
    # This is a standard rule
    - A maintainer approved any changes

    # This is a slot
    - slot: additional-rules
      required: false

Slots may appear anywhere a rule is valid, including as children of and and or rules. All slots must set a name. This is how child policies specify which rules fill which slots. The required property is optional and defaults to false.

Assume the additional-rules slot is empty. This policy allows child policies to add conditions that must be true in addition to the A maintainer approved any changes condition.

Parsing

When parsing the policy, extend the parser to consider maps with the slot key in addition to the and and or keys. The parser inserts a SlotRule at this location. A SlotRule contains an initially empty child rule. When evaluating the slot rule, if the child is empty, the SlotRule returns the skipped status. If the child rule exists, the SlotRule returns the result of evaluating the child rule. The SlotRule also contains a Required field, indicating if it is required.

The parser also collects all SlotRules and stores references to them in a map keyed by the slot name. It generates a parse error if multiple slots have the same name. Later, when populating slots, the parser uses this map to set the child rules.

For the purpose of rendering the details page, SlotRules are flattened. In other words, the child rule renders directly at the location of the slot. Empty slots do not appear on the details page at all. Rules inserted from a slot should have some visual indication that they came from a slot, ideally including the name of the slot.

Populating Slots

We introduce a new top-level map called slots. This works similarly to the existing policy.approval key:

• Each key in the map is the name of a slot
• Each value is a list of rules that fill the named slot
• The rules in the list are implicitly combined with the and operator. Explicitly use an or rule to change this.
• Rules referenced in a slot must be defined in the approval_rules block of the same policy. You cannot reference rules defined in a parent policy file.
• References to non-existent rules create an invalid policy
• Slot values cannot define additional slots

For example:

slots:
  additional-rules:
    - Security team approved authentication changes
    - SRE team approved deployment changes

approval_rules:
  - name: Security team approved authentication changes
    # ...

  - name: SRE team approved deployment changes
    # ...

Defining values for slots that do not exist is not an error.

If the slots key appears in the same policy as the slot definition, the values represent the default for that slot. These rules are used if a repository uses the policy directly or if a repository extends this policy but does not define a new value for the slot.

If a child policy provides a value for a slot that has a default value in the parent, the value from the child fully replaces the default content from the parent. If a child wants to retain some or all of the rules that were part of the default slot value, it must redefine them.

Parsing

Parsing the slots map is the same as parsing the policy.approval map, with two changes:

• slot rules are not allowed (i.e. no nested slots)
• Results are stored in a map by slot name rather than producing a single policy

If the policy uses the approval_defaults key, these defaults apply to all approval_rules in the same file. This means that the rules in a slot use the defaults from the file that defines the rules, not the defaults from the file that defines the slots.

Referencing Parent Policies

To reference another policy for extension, we add a new approval_extends key to the policy block. This key is mutually exclusive with the approval key. If both are set, the policy is invalid.

The approval_extends key contains the standard appconfig.RemoteRef type used for remote policies:

policy:
  approval_extends:
    remote: org/repo-name
    path: path/to/policy.yml
    ref: master

Policy Bot follows the same rules for loading parent policies as it does when loading remote policies, mainly that the repository must be public or Policy Bot must have access.

The approval_extends key must reference a literal policy. The target cannot itself be a remote reference, and it cannot contain another approval_extends key. A standard top-level remote reference can now point to a literal policy or a policy that contains an approval_extends key. This means Policy Bot may fetch two remote policies in some situations.

A child policy is otherwise a normal policy file. It may define a disapproval policy (which is not inherited from the parent, even if it exists), specify approval_defaults, or set any other top-level keys. Top-level keys (especially defaults) apply only to the rules defined in the child.

Loading

When implementing this behavior, consider extending the appconfig.Loader type in go-githubapp to define a LoadRemoteRef method that takes a appconfig.RemoteRef object and returns the config if it exists. This allows skipping all the other config loading behavior while reusing the relevant parts.

Design Decisions

This section highlights specific design decisions that may need further discussion.

Single-Level Inheritance

I think it is technically straightforward to support arbitrarily deep policy trees and slot values that themselves define additional slots. At the same time, I think this could make policies very hard to reason about and could encourage excessive abstraction/modularization. It could also lead to a high number of GitHub API requests to load the policy. I limited the proposal to a single level of inheritance for these reasons. A policy can extend exactly one other policy, which must stand alone. If there are arguments for deeper hierarchies, I can revise the proposal to allow this.

Local Rules Only

When a policy defines rules for a slot, those rules must be defined in the local approval_rules block and cannot reference rules defined in the parent. While it's technically feasible to allow parent rule references, I think it could make policies confusing (unclear where rules are defined) and brittle (renaming a rule in the parent would break children, even if the rule had the same function.)

This means that children cannot take the default values of a parent and extend them without redefining the rules. I think this is likely solvable by defining two slots: one with the defaults, then an empty one immediately following the defaults. If referencing parent rules should be allowed, I can revise the proposal.

Approval Only

Disapproval policies, despite being defined in the policy key, do not support arbitrary rules in the same way as approval policies. As a result, this proposal only discusses approval policies and defines approval-specific keys like approval_extends. Extension for disapproval rules will need a different RFC, likely as part of a larger redesign of how disapproval works.

Undefined Slots

To prevent a change to a parent policy from immediately breaking all children, defining values for a slot that does not exist is not an error. This has the downside that it may be hard to notice if your policy no longer applies the extra rules. This could change to an error if the risk of mass policy breaks is acceptable to avoid silent behavior changes for children.