Skip to content

Merge pipeline-required-tasks from ruleData config #1786

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
joejstuart wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Merge pipeline-required-tasks from ruleData config #1786

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9b01177
Merge pipeline-required-tasks from ruleData configuration
joejstuart Jul 23, 2026
ed762e5
Add design doc for rule data merge pattern
joejstuart Jul 23, 2026
13a0b93
Remove unused attestation variable from merge test
joejstuart Jul 23, 2026
66eb73c
Fix rego formatting in pipeline_test.rego
joejstuart Jul 23, 2026
6c0586b
Add design docs pointer to AGENTS.md
joejstuart Jul 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ attestation formats. Policies consume the normalized form — don't branch on SL
**Rule data** lives in `example/data/` (required tasks, trusted task bundles, known RPM repos).
These files have `effective_on` dates — rules with future dates are warnings, not failures.

**Design docs** in `design/` capture non-obvious design rationale, cross-repo knowledge, and
architectural constraints that aren't derivable from the code. Check there before reverse-engineering
a subsystem.

## Common Change Patterns

- **Add a release policy rule:** follow the pattern in `policy/release/attestation_type/attestation_type.rego` (rule + `_test.rego` in a subdirectory, declare `collections:` in METADATA)
Expand Down
56 changes: 56 additions & 0 deletions design/rule-data-merge-pattern.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
# Rule Data Merge Pattern

Merging data-source values with ruleData configuration values using `object.union`.
Used by `pipeline_required_tasks` and `trusted_tasks` in `policy/lib/tekton/`.

## Why does `rule_data.get()` need an explicit `{}` default for merged keys?

`rule_data.get(key)` falls back to `[]` (empty array) when a key isn't found in any
data source. But `object.union` requires both arguments to be objects — if either is
an array, OPA returns `undefined` at runtime (see next section). So any key that will
be passed to `object.union` must have an explicit `{}` default registered in the
`defaults` map in `policy/lib/rule_data/rule_data.rego`. Without it, the merge silently
produces `undefined` and all downstream rules stop evaluating with no error output.

When adding a new `object.union` merge, register the key in rule_data.rego's `defaults`:

```rego
defaults := {
"pipeline-required-tasks": {}, # must be {} not [] for object.union
"trusted_tasks": {}, # same reason
...
}
```

## Why does `object.union` fail silently instead of erroring?

OPA has two behaviors depending on when it can determine types:

- **Static types known** (literals in code): `object.union({"a": 1}, [])` produces a
compile-time `rego_type_error` — policy fails to load.
- **Dynamic types from `data`** (the common case): both arguments are typed `any`, so
OPA skips compile-time checking. At runtime, `object.union` with a non-object argument
returns `undefined` — no error, no diagnostic. The rule becomes undefined and all
consumers silently skip it.

This means a misconfigured data source that provides an array instead of an object will
cause policy rules to silently stop evaluating, producing zero violations and zero errors.
The `{}` default in rule_data.rego is the primary defense against this.
Comment on lines +36 to +38

@coderabbitai coderabbitai Bot Jul 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Clarify that {} does not protect against wrong-typed values.

The defaults map is consulted only when the key is absent. If a data source explicitly supplies an array, rule_data.get(...) still returns that array and object.union remains undefined. Document this limitation and require type validation or a safe fallback for present, invalid values; otherwise this can still fail open with zero policy violations.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@design/rule-data-merge-pattern.md` around lines 36 - 38, Clarify in the
rule_data merge documentation that the defaults map, including the {} default in
rule_data.rego, applies only when the key is absent and does not protect against
explicitly supplied arrays or other invalid types. Require validating present
values or applying a safe object fallback before object.union, preventing
invalid data from silently producing zero violations and errors.


## How does the override precedence work in the merge?

`object.union(A, B)` gives B full override for matching keys. In the merge pattern:

```rego
pipeline_required_tasks := object.union(data["pipeline-required-tasks"], rule_data.get("pipeline-required-tasks"))
```

- A = `data["pipeline-required-tasks"]` (from data sources)
- B = `rule_data.get(...)` which checks, in priority order:
1. `data.rule_data__configuration__` (ECP policy ruleData)
2. `data.rule_data_custom` (custom user sources)
3. `data.rule_data` (default data sources)
4. `defaults` map (hardcoded `{}`)

When B has a matching selector key (e.g. `"docker"`), it replaces A's value entirely —
the arrays are not merged. This is intentional and matches the trusted_tasks precedent.
3 changes: 3 additions & 0 deletions policy/lib/rule_data/rule_data.rego
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,9 @@ defaults := {
# using the ruleData key. Make this default to an empty dict so we can conveniently
# merge it with with `data.trusted_tasks`
"trusted_tasks": {},
# Used in lib/tekton/pipeline.rego to merge pipeline-required-tasks from
# ruleData with data sources, matching the trusted_tasks merge pattern.
"pipeline-required-tasks": {},
# Used in lib/tekton/trusted.rego to toggle trusted_task_rules on/off
"trusted_task_rules_enabled": false,
# Number of days before a version of the Task expires that warnings are reported
Expand Down
7 changes: 5 additions & 2 deletions policy/lib/tekton/pipeline.rego
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,15 @@ package lib.tekton

import rego.v1

import data.lib.rule_data
import data.lib.time as ectime

pipeline_label := "pipelines.openshift.io/runtime"

task_label := "build.appstudio.redhat.com/build_type"

@fullsend-ai-review fullsend-ai-review Bot Jul 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[medium] test adequacy

The new pipeline_required_tasks rule merges data from two sources via object.union(data["pipeline-required-tasks"], lib_rule_data("pipeline-required-tasks")), but no test verifies this merge behavior. All existing tests set only data["pipeline-required-tasks"] via with clauses and never provide data through the ruleData configuration path. Without a test that supplies data from both sources, there is no coverage for confirming the union produces the expected merged result or that ruleData values take precedence on key conflicts.

Suggested fix: Add at least one test that provides pipeline-required-tasks via both data["pipeline-required-tasks"] and data.rule_data__configuration__["pipeline-required-tasks"] with overlapping and non-overlapping keys, and asserts the merged output matches expectations.

@fullsend-ai-review fullsend-ai-review Bot Jul 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] override semantics

object.union(A, B) gives ruleData config (B) full override precedence over data sources (A) for matching selector keys. This is consistent with the existing trusted_tasks pattern and is by design, but a ruleData entry for a selector key replaces the data-source value entirely rather than merging arrays.

Suggested fix: Consider documenting that ruleData configuration intentionally overrides data-source required tasks per selector key, matching the trusted_tasks precedent.

pipeline_required_tasks := object.union(data["pipeline-required-tasks"], rule_data.get("pipeline-required-tasks"))

latest_required_pipeline_tasks(pipeline) := _merged_required_task_list(pipeline, "newest")

current_required_pipeline_tasks(pipeline) := _merged_required_task_list(pipeline, "most_current")
Expand All @@ -19,7 +22,7 @@ required_task_list(pipeline) := pipeline_data if {
selectors := pipeline_label_selectors(pipeline)
pipeline_data := [entry |
some selector in selectors
entries := object.get(data["pipeline-required-tasks"], selector, [])
entries := object.get(pipeline_required_tasks, selector, [])
some entry in entries
]
count(pipeline_data) > 0
Expand All @@ -37,7 +40,7 @@ _merged_required_task_list(pipeline, mode) := {

resolved := [r |
some selector in selectors
entries := object.get(data["pipeline-required-tasks"], selector, [])
entries := object.get(pipeline_required_tasks, selector, [])
count(entries) > 0
r := _resolve(entries, mode)
]
Expand Down
56 changes: 56 additions & 0 deletions policy/lib/tekton/pipeline_test.rego
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,3 +535,59 @@ test_task_effective_on_with_one_of_tasks if {
assertions.assert_equal(tekton.task_effective_on(data_with_map, sorted_task), "2024-06-01T00:00:00Z")
assertions.assert_equal(tekton.task_effective_on(data_with_map, "buildah"), "2024-01-01T00:00:00Z")
}

test_pipeline_required_tasks_merges_data_and_rule_data if {
data_source := {"docker": [{
"effective_on": "2024-01-01T00:00:00Z",
"tasks": ["buildah", "git-clone"],
}]}

rule_data_config := {"fbc": [{
"effective_on": "2024-06-01T00:00:00Z",
"tasks": ["fbc-validation"],
}]}

result := tekton.pipeline_required_tasks with data["pipeline-required-tasks"] as data_source
with data.rule_data__configuration__["pipeline-required-tasks"] as rule_data_config

assertions.assert_equal(result, {
"docker": [{"effective_on": "2024-01-01T00:00:00Z", "tasks": ["buildah", "git-clone"]}],
"fbc": [{"effective_on": "2024-06-01T00:00:00Z", "tasks": ["fbc-validation"]}],
})
}

test_pipeline_required_tasks_rule_data_overrides_on_key_conflict if {
task_base := slsav1_task("build-container")
task_w_labels := with_labels(task_base, {tekton.task_label: "docker"})
task_full := with_results(
task_w_labels,
[
{"name": "IMAGE_URL", "value": "localhost:5000/repo:latest"},
# regal ignore:line-length
{"name": "IMAGE_DIGEST", "value": "sha256:abc0000000000000000000000000000000000000000000000000000000000abc"},
],
)

attestation := slsav1_attestation_full(
[task_full],
{},
{},
)

data_source := {"docker": [{
"effective_on": "2024-01-01T00:00:00Z",
"tasks": ["buildah", "git-clone"],
}]}

rule_data_config := {"docker": [{
"effective_on": "2024-06-01T00:00:00Z",
"tasks": ["buildah", "git-clone", "clair-scan"],
}]}

result := tekton.latest_required_pipeline_tasks(attestation) with data["pipeline-required-tasks"] as data_source
with data.rule_data__configuration__["pipeline-required-tasks"] as rule_data_config

# ruleData config overrides data source for the same key
assertions.assert_equal(result.tasks, {"buildah", "git-clone", "clair-scan"})
assertions.assert_equal(result.effective_on, "2024-06-01T00:00:00Z")
}
4 changes: 2 additions & 2 deletions policy/release/tasks/tasks.rego
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,7 @@ _expiry_msg_annotation := "build.appstudio.redhat.com/expiry-message"

_data_errors contains error if {
some e in j.validate_schema(
data["pipeline-required-tasks"],
tekton.pipeline_required_tasks,
{
"$schema": "http://json-schema.org/draft-07/schema#",
"type": "object",
Expand Down Expand Up @@ -481,7 +481,7 @@ _data_errors contains error if {
}

_data_errors contains error if {
some key, entries in data["pipeline-required-tasks"]
some key, entries in tekton.pipeline_required_tasks
some i, entry in entries
effective_on := entry.effective_on
not time.parse_rfc3339_ns(effective_on)
Expand Down
Loading