ART-21345: Expand build-time EC/ITS verification to layered products#3112
ART-21345: Expand build-time EC/ITS verification to layered products#3112ashwindasr wants to merge 2 commits into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughEC policies are selected by product and may be unset. Konflux verification gates now use ChangesEC verification controls
Layered-product EC skip wiring
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant KonfluxBuildCli
participant KonfluxImageBuilder
participant EnterpriseContract
KonfluxBuildCli->>KonfluxImageBuilder: pass product-selected EC policy
KonfluxImageBuilder->>KonfluxImageBuilder: evaluate build variant and policy
KonfluxImageBuilder->>EnterpriseContract: verify configured non-OKD build
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ashwindasr
changed the title
|
@ashwindasr: This pull request references ART-21345 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@doozer/doozerlib/cli/images_konflux.py`:
- Around line 309-315: The `images_konflux` policy selection regresses `test`
assemblies to the non-test PREGA EC policy. Update the `runtime.assembly ==
"test"` branch in the policy lookup logic to derive `prega_ec_policy` from a
test-specific mapping or constant, using a unique symbol such as
`PRODUCT_TEST_PREGA_EC_POLICY_MAP` alongside `PRODUCT_TEST_EC_POLICY_MAP`, so
`test` builds keep the test PREGA value instead of falling back to
`PRODUCT_PREGA_EC_POLICY_MAP`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bb3b33ea-fb88-46f9-a083-26a9aea4a8da
📒 Files selected for processing (6)
doozer/doozerlib/backend/konflux_image_builder.pydoozer/doozerlib/cli/images_konflux.pydoozer/doozerlib/constants.pydoozer/tests/backend/test_konflux_ec_verification.pydoozer/tests/backend/test_konflux_image_builder.pypyartcd/pyartcd/pipelines/build_layered_products.py
b89a9df to
5e9d447
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
doozer/doozerlib/backend/konflux_image_builder.py (1)
307-315: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGate EC on the selected lifecycle policy
should_run_econly checksec_policy_configuration, but this block can later pickprega_ec_policy_configurationfor pre-release builds. If that pre-GA policy is unset, EC still runs withec_policy=None. Select the policy before this gate and require the chosen value to be non-None; add a regression test for the pre-release/no-preGA-policy case.🤖 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 `@doozer/doozerlib/backend/konflux_image_builder.py` around lines 307 - 315, Update the logic around should_run_ec to select the lifecycle-appropriate EC policy first, using the pre-release policy for pre-release builds and the standard policy otherwise. Gate should_run_ec on the selected policy being non-None, then pass that same selected policy to the later EC invocation. Add a regression test covering a pre-release build with no pre-GA policy, asserting EC does not run.
🧹 Nitpick comments (2)
doozer/tests/backend/test_konflux_image_builder.py (1)
123-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the new non-OpenShift, non-OKD path.
The production change replaces the
openshift-group-name check with variant-based gating. This test only covers OKD skipping, while the existing positive test uses an OpenShift group; a regression to the old group check would therefore still pass. Add a case using a product group such as OADP with a non-OKD variant and assert SLSA validation is called.🤖 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 `@doozer/tests/backend/test_konflux_image_builder.py` around lines 123 - 136, The test coverage in test_build_skips_slsa_validation_for_okd_variant is missing the non-OpenShift, non-OKD path. Add a test case using a product group such as OADP with a non-OKD BuildVariant, and assert that SLSA validation is invoked, ensuring gating depends on the variant rather than the group-name prefix.doozer/doozerlib/backend/konflux_image_builder.py (1)
961-961: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse
zip(..., strict=True)in_resolve_source_package_nvrs. Any length mismatch between the Koji results and requested NVRs should fail fast instead of silently truncating and mis-associating builds.🤖 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 `@doozer/doozerlib/backend/konflux_image_builder.py` at line 961, Update the zip call in _resolve_source_package_nvrs to use strict=True, ensuring mismatched nvrs and build_tasks lengths raise an error instead of silently truncating; preserve the existing iteration and association behavior when lengths match.Source: Linters/SAST tools
🤖 Prompt for all review comments with 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.
Outside diff comments:
In `@doozer/doozerlib/backend/konflux_image_builder.py`:
- Around line 307-315: Update the logic around should_run_ec to select the
lifecycle-appropriate EC policy first, using the pre-release policy for
pre-release builds and the standard policy otherwise. Gate should_run_ec on the
selected policy being non-None, then pass that same selected policy to the later
EC invocation. Add a regression test covering a pre-release build with no pre-GA
policy, asserting EC does not run.
---
Nitpick comments:
In `@doozer/doozerlib/backend/konflux_image_builder.py`:
- Line 961: Update the zip call in _resolve_source_package_nvrs to use
strict=True, ensuring mismatched nvrs and build_tasks lengths raise an error
instead of silently truncating; preserve the existing iteration and association
behavior when lengths match.
In `@doozer/tests/backend/test_konflux_image_builder.py`:
- Around line 123-136: The test coverage in
test_build_skips_slsa_validation_for_okd_variant is missing the non-OpenShift,
non-OKD path. Add a test case using a product group such as OADP with a non-OKD
BuildVariant, and assert that SLSA validation is invoked, ensuring gating
depends on the variant rather than the group-name prefix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cdbaee02-28bd-4326-b776-b683da419407
📒 Files selected for processing (6)
doozer/doozerlib/backend/konflux_image_builder.pydoozer/doozerlib/cli/images_konflux.pydoozer/doozerlib/constants.pydoozer/tests/backend/test_konflux_ec_verification.pydoozer/tests/backend/test_konflux_image_builder.pypyartcd/pyartcd/pipelines/build_layered_products.py
🚧 Files skipped from review as they are similar to previous changes (4)
- doozer/tests/backend/test_konflux_ec_verification.py
- pyartcd/pyartcd/pipelines/build_layered_products.py
- doozer/doozerlib/cli/images_konflux.py
- doozer/doozerlib/constants.py
Remove the is_ocp_group guard that restricted enterprise contract verification and SLSA attestation validation to OCP-only groups. EC now runs for any product with a configured policy (OADP, MTC, Logging, MTA, OCP). SLSA validation now runs for all non-OKD variants. - Add PRODUCT_EC_POLICY_MAP and PRODUCT_TEST_EC_POLICY_MAP to constants - Replace hardcoded OCP policy selection with product-aware lookup - Add --skip-ec-verify flag to build-layered-products pipeline - Update tests to reflect new policy-availability gating Co-authored-by: Cursor <cursoragent@cursor.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
PRODUCT_PREGA_EC_POLICY_MAP only had the prod PREGA policy for ocp, so test-assembly builds in the pre-release lifecycle phase would pick the prod PREGA policy instead of the test one. Add PRODUCT_TEST_PREGA_EC_POLICY_MAP and use it in the test-assembly branch. rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
4bb8936 to
9bc1022
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@doozer/doozerlib/backend/konflux_image_builder.py`:
- Around line 307-310: Update the EC verification flow around the should_run_ec
predicate to resolve the phase-specific ec_policy first, selecting the PREGA
policy for pre-release builds and the standard policy otherwise. Gate
should_run_ec on that resolved policy being present, and reuse the same
ec_policy value in the skip log and verification call. Apply the equivalent
change to the second affected flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1e3b9587-5544-45b2-9a6b-5ae7ec73039a
📒 Files selected for processing (6)
doozer/doozerlib/backend/konflux_image_builder.pydoozer/doozerlib/cli/images_konflux.pydoozer/doozerlib/constants.pydoozer/tests/backend/test_konflux_ec_verification.pydoozer/tests/backend/test_konflux_image_builder.pypyartcd/pyartcd/pipelines/build_layered_products.py
🚧 Files skipped from review as they are similar to previous changes (5)
- doozer/tests/backend/test_konflux_ec_verification.py
- pyartcd/pyartcd/pipelines/build_layered_products.py
- doozer/tests/backend/test_konflux_image_builder.py
- doozer/doozerlib/cli/images_konflux.py
- doozer/doozerlib/constants.py
| should_run_ec = ( | ||
| outcome is KonfluxBuildOutcome.SUCCESS | ||
| and metadata.runtime.variant is not BuildVariant.OKD | ||
| and is_ocp_group | ||
| and self._config.ec_policy_configuration is not None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Gate on the phase-specific EC policy.
The predicate checks only ec_policy_configuration, but Lines 327-329 select the PREGA policy for pre-release builds. This causes both incorrect outcomes: a standard-only configuration can call EC verification with ec_policy=None, while a PREGA-only configuration is skipped despite having a usable policy. Resolve the policy before gating and use that same value for the skip log.
Proposed direction
- and self._config.ec_policy_configuration is not None
+ and ec_policy is not None
...
- elif self._config.ec_policy_configuration is None:
+ elif ec_policy is None:Ensure ec_policy is resolved from the lifecycle phase before evaluating should_run_ec.
Also applies to: 361-363
🤖 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 `@doozer/doozerlib/backend/konflux_image_builder.py` around lines 307 - 310,
Update the EC verification flow around the should_run_ec predicate to resolve
the phase-specific ec_policy first, selecting the PREGA policy for pre-release
builds and the standard policy otherwise. Gate should_run_ec on that resolved
policy being present, and reuse the same ec_policy value in the skip log and
verification call. Apply the equivalent change to the second affected flow.
2 participants
Summary
Remove the
is_ocp_groupguard that restricted enterprise contract verificationand SLSA attestation validation to OCP-only groups. EC now runs for any product
with a configured policy (OADP, MTC, Logging, MTA, OCP). SLSA validation now
runs for all non-OKD variants.
Changes
doozer/doozerlib/constants.py: AddPRODUCT_EC_POLICY_MAP,PRODUCT_TEST_EC_POLICY_MAP,and
PRODUCT_PREGA_EC_POLICY_MAPfor product-to-policy mappingdoozer/doozerlib/backend/konflux_image_builder.py: Replaceis_ocp_groupcheck withpolicy-availability check (
ec_policy_configuration is not None); expand SLSA validationto all non-OKD variants
doozer/doozerlib/cli/images_konflux.py: Use product maps for EC policy resolutioninstead of hardcoded OCP constants
pyartcd/pyartcd/pipelines/build_layered_products.py: Add--skip-ec-verifyflagDepends on
art-mtc-tenant, art-logging-tenant, art-mta-tenant)
Test plan
make unit)--skip-ec-verifyto start)Made with Cursor
Summary by CodeRabbit
New Features
--skip-ec-verifyoption for layered product builds to bypass Enterprise Contract verification when needed.Bug Fixes
Tests