WIP OCPNODE-4561: Migrate OCP-59552 enable image signature verification for RHEL registries#31243
WIP OCPNODE-4561: Migrate OCP-59552 enable image signature verification for RHEL registries#31243BhargaviGudi wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
@BhargaviGudi: No Jira issue with key OCP-59552 exists in the tracker at https://redhat.atlassian.net. 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. |
BhargaviGudi
changed the title
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a disruptive serial node E2E that applies a worker MachineConfig, waits for MCP rollout/rollback, and verifies /etc/containers/policy.json contains Red Hat registry signature policy entries. Includes the MachineConfig fixture, generated bindata, test suite, and helper functions. ChangesImage Signature E2E Test Suite
Sequence Diagram(s)sequenceDiagram
participant Test
participant oc
participant MCP
participant WorkerNode
Test->>oc: apply MachineConfig (machineconfig-image-signature.yaml)
oc->>MCP: trigger worker MCP update
MCP->>WorkerNode: rollout/rollback configuration
Test->>WorkerNode: exec read /etc/containers/policy.json
WorkerNode->>Test: return policy.json contents
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewersp0lyn0mial 🚥 Pre-merge checks | ✅ 12 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: BhargaviGudi 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 |
|
@BhargaviGudi: No Jira issue with key OCP-59552 exists in the tracker at https://redhat.atlassian.net. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/extended/node/node_e2e/image_signature.go (1)
98-100: 💤 Low valueThread the caller's
contextintocheckImageSignatureinstead ofcontext.Background().
checkImageSignatureignores cancellation/timeout from the test'sctxby creating its owncontext.Background(). Accept actx context.Context(aswaitForMCPUpdatedoes) so the poll is cancellable. The 30s total timeout is also tight for reading a freshly-rolled-out node; consider passing the timeout in as well.As per coding guidelines: "context.Context for cancellation and timeouts".♻️ Proposed signature change
-func checkImageSignature(oc *exutil.CLI) error { +func checkImageSignature(ctx context.Context, oc *exutil.CLI, timeout time.Duration) error { g.GinkgoHelper() - return wait.PollUntilContextTimeout(context.Background(), 10*time.Second, 30*time.Second, true, func(ctx context.Context) (bool, error) { + return wait.PollUntilContextTimeout(ctx, 10*time.Second, timeout, true, func(ctx context.Context) (bool, error) {And update the call site:
- err = checkImageSignature(oc) + err = checkImageSignature(ctx, oc, 2*time.Minute)🤖 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 `@test/extended/node/node_e2e/image_signature.go` around lines 98 - 100, checkImageSignature currently creates its own context via context.Background(), ignoring caller cancellation; change the signature of checkImageSignature to accept ctx context.Context (and optionally a timeout/duration parameter instead of hard-coded 30*time.Second), use that ctx when calling wait.PollUntilContextTimeout, and update all call sites (e.g., callers like waitForMCPUpdate) to pass their ctx (and timeout) through so the poll is cancellable and the total timeout can be adjusted by the caller.
🤖 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 `@test/extended/node/node_e2e/image_signature.go`:
- Around line 64-66: The current waitForMCPUpdate helper can return immediately
if the initial poll sees Updating=False; change waitForMCPUpdate so it first
waits for the MCP to enter Updating=True and only after that waits for
Updating=False (or replace its usage with existing helpers like
WaitForMCPConditionStatus/WaitForMCPToBeReady and assert
MachineConfigPoolUpdated and expected machine counts) to ensure a real rollout
occurred before returning success; also update the cleanup delete call to check
and handle the returned error instead of ignoring it.
In `@test/extended/node/README.md`:
- Line 12: The markdown line declares reference-style tags like "[OTP]" which
triggers MD052; update the text in the README so the tag is plain text instead
of a reference label—e.g., change "[OTP]" to "(OTP)" or escape the brackets (or
wrap in inline code) in the line describing node_e2e/image_signature.go so the
tag is rendered as literal text and markdownlint no longer flags it.
---
Nitpick comments:
In `@test/extended/node/node_e2e/image_signature.go`:
- Around line 98-100: checkImageSignature currently creates its own context via
context.Background(), ignoring caller cancellation; change the signature of
checkImageSignature to accept ctx context.Context (and optionally a
timeout/duration parameter instead of hard-coded 30*time.Second), use that ctx
when calling wait.PollUntilContextTimeout, and update all call sites (e.g.,
callers like waitForMCPUpdate) to pass their ctx (and timeout) through so the
poll is cancellable and the total timeout can be adjusted by the caller.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0ead5033-d40d-45e2-93c8-e883548bf80a
📒 Files selected for processing (3)
test/extended/node/README.mdtest/extended/node/node_e2e/image_signature.gotest/extended/testdata/node/node_e2e/machineconfig-image-signature.yaml
BhargaviGudi
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@BhargaviGudi: This pull request references OCPNODE-4561 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. |
BhargaviGudi
force-pushed
the
migrate-ocp-59552
branch
from
1625768 to
59f67e9
Compare
openshift-ci
Bot
added
the
ready-for-human-review
BhargaviGudi
force-pushed
the
migrate-ocp-59552
branch
from
59f67e9 to
8f51f3e
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 `@test/extended/node/README.md`:
- Line 12: Update the README entry for node_e2e/image_signature.go to include
the missing Jira reference OCP-59552 for traceability; locate the line that
lists "**node_e2e/image_signature.go** - Image signature verification" and
append the OCP-59552 identifier using the same bracketed format as the existing
OCP-44820 (e.g., add "[OCP-59552]" to the bracket list following the test tags)
so the test description consistently references both Jiras.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: fc9f42f6-ffbe-425a-8eb6-9ac1ee6b4df9
📒 Files selected for processing (4)
test/extended/node/README.mdtest/extended/node/node_e2e/image_signature.gotest/extended/testdata/bindata.gotest/extended/testdata/node/node_e2e/machineconfig-image-signature.yaml
✅ Files skipped from review due to trivial changes (2)
- test/extended/testdata/node/node_e2e/machineconfig-image-signature.yaml
- test/extended/testdata/bindata.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/extended/node/node_e2e/image_signature.go
|
Scheduling required tests: |
|
/payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-aws-disruptive-longrunning |
|
@BhargaviGudi: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/5facd6a0-5e85-11f1-8c30-4ca1f195790f-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-disruptive-longrunning |
|
@BhargaviGudi: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/9a7b1050-5f1e-11f1-905f-95e9eb772d51-0 |
BhargaviGudi
force-pushed
the
migrate-ocp-59552
branch
from
8f51f3e to
1bc0454
Compare
|
Scheduling required tests: |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-disruptive-longrunning |
|
@BhargaviGudi: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/064e6620-5ffa-11f1-8159-424cfeffd77b-0 |
BhargaviGudi
force-pushed
the
migrate-ocp-59552
branch
from
1bc0454 to
9ddf10b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/extended/node/node_e2e/image_signature.go (1)
50-59:⚠️ Potential issue | 🟡 Minor | 💤 Low valueCapture and log the delete error in cleanup.
Line 52 discards the error from
Execute(). While--ignore-not-foundhandles the common case where the resource is already deleted, other errors (permission, network) are silently swallowed. Per Go security guidelines, errors should not be ignored.Suggested fix
g.DeferCleanup(func(ctx context.Context) { g.By("Delete the MachineConfig") - oc.AsAdmin().WithoutNamespace().Run("delete").Args("-f", imgSignatureYAML, "--ignore-not-found").Execute() + if err := oc.AsAdmin().WithoutNamespace().Run("delete").Args("-f", imgSignatureYAML, "--ignore-not-found").Execute(); err != nil { + e2e.Logf("Warning: failed to delete MachineConfig: %v", err) + } g.By("Wait for MCP to finish rolling back")🤖 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 `@test/extended/node/node_e2e/image_signature.go` around lines 50 - 59, The cleanup currently ignores the error returned by oc.AsAdmin().WithoutNamespace().Run("delete").Args("-f", imgSignatureYAML, "--ignore-not-found").Execute(); change this to capture the error (e.g., err := oc.AsAdmin().WithoutNamespace().Run("delete").Args(...).Execute()), and if err != nil log it (use e2e.Logf or the existing logger) with context including imgSignatureYAML so permission/network/other failures aren't silently swallowed; keep the existing --ignore-not-found flag and the subsequent waitForMCPUpdate call unchanged.
🧹 Nitpick comments (1)
test/extended/node/node_e2e/image_signature.go (1)
109-143: ⚡ Quick winAccept and propagate context for cancellation support.
checkImageSignatureusescontext.Background()(line 114) instead of accepting a context parameter. This is inconsistent withwaitForMCPUpdateand prevents proper cancellation if the test times out or is interrupted.Suggested refactor
-// checkImageSignature verifies that the image signature policy is correctly configured on worker nodes. -// It checks for required entries in /etc/containers/policy.json for Red Hat registries. -// This is a helper function and does not contain assertions. -func checkImageSignature(oc *exutil.CLI) error { +// checkImageSignature verifies that the image signature policy is correctly configured on worker nodes. +// It checks for required entries in /etc/containers/policy.json for Red Hat registries. +// This is a helper function and does not contain assertions. +func checkImageSignature(ctx context.Context, oc *exutil.CLI) error { g.GinkgoHelper() - return wait.PollUntilContextTimeout(context.Background(), 10*time.Second, 30*time.Second, true, func(ctx context.Context) (bool, error) { + return wait.PollUntilContextTimeout(ctx, 10*time.Second, 30*time.Second, true, func(ctx context.Context) (bool, error) {Update the call site accordingly:
g.By("Verify the signature configuration in /etc/containers/policy.json") -err = checkImageSignature(oc) +err = checkImageSignature(ctx, oc)🤖 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 `@test/extended/node/node_e2e/image_signature.go` around lines 109 - 143, Change checkImageSignature to accept a context (e.g., func checkImageSignature(ctx context.Context, oc *exutil.CLI) error), then use that ctx instead of context.Background() in the wait.PollUntilContextTimeout call and propagate it to the node command execution call (replace nodeutils.ExecOnNodeWithChroot(...) with the context-aware variant or add ctx as first arg if available). Update all call sites to pass the test context. Refer to the function name checkImageSignature, the wait.PollUntilContextTimeout invocation, and nodeutils.ExecOnNodeWithChroot so the timeout/cancellation flows through correctly.
🤖 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.
Duplicate comments:
In `@test/extended/node/node_e2e/image_signature.go`:
- Around line 50-59: The cleanup currently ignores the error returned by
oc.AsAdmin().WithoutNamespace().Run("delete").Args("-f", imgSignatureYAML,
"--ignore-not-found").Execute(); change this to capture the error (e.g., err :=
oc.AsAdmin().WithoutNamespace().Run("delete").Args(...).Execute()), and if err
!= nil log it (use e2e.Logf or the existing logger) with context including
imgSignatureYAML so permission/network/other failures aren't silently swallowed;
keep the existing --ignore-not-found flag and the subsequent waitForMCPUpdate
call unchanged.
---
Nitpick comments:
In `@test/extended/node/node_e2e/image_signature.go`:
- Around line 109-143: Change checkImageSignature to accept a context (e.g.,
func checkImageSignature(ctx context.Context, oc *exutil.CLI) error), then use
that ctx instead of context.Background() in the wait.PollUntilContextTimeout
call and propagate it to the node command execution call (replace
nodeutils.ExecOnNodeWithChroot(...) with the context-aware variant or add ctx as
first arg if available). Update all call sites to pass the test context. Refer
to the function name checkImageSignature, the wait.PollUntilContextTimeout
invocation, and nodeutils.ExecOnNodeWithChroot so the timeout/cancellation flows
through correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f081f648-5e0c-49ea-bb2c-b398d3c8abc5
📒 Files selected for processing (4)
test/extended/node/README.mdtest/extended/node/node_e2e/image_signature.gotest/extended/testdata/bindata.gotest/extended/testdata/node/node_e2e/machineconfig-image-signature.yaml
✅ Files skipped from review due to trivial changes (2)
- test/extended/testdata/bindata.go
- test/extended/testdata/node/node_e2e/machineconfig-image-signature.yaml
|
Scheduling required tests: |
|
@BhargaviGudi: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Migrates testcase OCP-59552 from openshift-tests-private to origin.
What this test validates
This test verifies that image signature verification can be enabled for Red Hat Container Registries using MachineConfig.
The test:
Implementation details
g.GinkgoHelper()in helper functionsframework.Logf()for loggingg.DeferCleanup()for proper cleanup with contextSummary by CodeRabbit