MCO-1906: Add Extra Validations - #6353
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@anandram2: This pull request references MCO-1906 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
Bot
added
the
needs-rebase
|
PR needs rebase. 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: anandram2 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 |
|
Caution Review failedThe pull request is closed. WalkthroughBootstrap now validates pre-built images during hybrid OCL setup using OpenShift version metadata with base-image digest fallback. The change also adds injectable image inspection, Containerfile validation, stream-aware rendering, expanded build status reporting, dependency updates, and broader controller and end-to-end test coverage. ChangesImage inspection and bootstrap/render integration
Containerfile validation and build status
MachineOSBuild status and test coverage
Toolchain and dependency updates
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning, 1 inconclusive)
✅ Passed checks (12 passed)
✨ Finishing Touches 💡 2⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
![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
🧹 Nitpick comments (2)
pkg/controller/bootstrap/bootstrap.go (2)
834-846: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSurface the version-check bailout at default verbosity.
When
FetchImageFilefails (auth, transport, missing file), the reason is only logged at V(4), so in a real install log you see neither why the primary check was skipped nor that validation degraded to a digest comparison.klog.Warningfwould make this diagnosable without extra verbosity.🤖 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 `@pkg/controller/bootstrap/bootstrap.go` around lines 834 - 846, The fallback log in validatePreBuiltImageVersion should be visible at default verbosity. Replace the V(4) Infof call that reports openshiftVersionFromImage failure with klog.Warningf, preserving the existing message and versionErr details.
361-367: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winShared 1-minute deadline covers every pre-built image validation.
preBuiltImageCtxis a single 1-minute budget for allmachineOSConfigs, and each validation does a remoteFetchImageFile(layer fetch) plus a possibleInspect. With multiple pools or slow/mirrored registries this can time out and hard-fail bootstrap. Consider a per-image timeout (or a budget scaled bylen(machineOSConfigs)).🤖 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 `@pkg/controller/bootstrap/bootstrap.go` around lines 361 - 367, Update the pre-built image validation flow around createPreBuiltImageMachineConfigs so each machineOSConfig validation receives its own timeout, or scale the overall deadline according to the number of machineOSConfigs. Ensure remote FetchImageFile and Inspect operations for every image have sufficient independent time to complete instead of sharing a fixed one-minute budget.
🤖 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 `@pkg/controller/bootstrap/bootstrap.go`:
- Around line 806-825: Update validatePreBuiltImageDigestFallback so an invalid
or non-digested expectedBaseOSImage logs a warning and skips verification
instead of returning an error, matching the missing-label behavior; retain fatal
errors for invalid label digests and mismatched digests. Update the
corresponding invalid expected base OS image case in
TestValidatePreBuiltImageDigestFallback to assert the fallback succeeds.
---
Nitpick comments:
In `@pkg/controller/bootstrap/bootstrap.go`:
- Around line 834-846: The fallback log in validatePreBuiltImageVersion should
be visible at default verbosity. Replace the V(4) Infof call that reports
openshiftVersionFromImage failure with klog.Warningf, preserving the existing
message and versionErr details.
- Around line 361-367: Update the pre-built image validation flow around
createPreBuiltImageMachineConfigs so each machineOSConfig validation receives
its own timeout, or scale the overall deadline according to the number of
machineOSConfigs. Ensure remote FetchImageFile and Inspect operations for every
image have sufficient independent time to complete instead of sharing a fixed
one-minute budget.
🪄 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/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e4a05e7d-d1f0-42af-87ac-f974d53dbb1d
📒 Files selected for processing (2)
pkg/controller/bootstrap/bootstrap.gopkg/controller/bootstrap/bootstrap_test.go
| func validatePreBuiltImageDigestFallback(labels map[string]string, imageSpec, expectedBaseOSImage string) error { | ||
| label := labels[preBuiltImageBaseOSLabelKey] | ||
| if label == "" { | ||
| klog.Warningf("pre-built image %q has no OPENSHIFT_VERSION and no %q label; skipping version verification", imageSpec, preBuiltImageBaseOSLabelKey) | ||
| return nil | ||
| } | ||
| labelDigest, err := imageDigest(label) | ||
| if err != nil { | ||
| return fmt.Errorf("pre-built image %q has an invalid %q label: %w", imageSpec, preBuiltImageBaseOSLabelKey, err) | ||
| } | ||
| expectedDigest, err := imageDigest(expectedBaseOSImage) | ||
| if err != nil { | ||
| return fmt.Errorf("cluster's resolved base OS image %q is invalid: %w", expectedBaseOSImage, err) | ||
| } | ||
| if labelDigest != expectedDigest { | ||
| return fmt.Errorf("pre-built image %q base OS image %q (digest %s) does not match the cluster's resolved base OS image %q (digest %s)", | ||
| imageSpec, label, labelDigest, expectedBaseOSImage, expectedDigest) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Non-digested cluster base OS image makes fallback fatal.
expectedBaseOSImage comes from cconfig.Spec.BaseOSContainerImage, which is not guaranteed to be digested — the OSImageStream override in Run can set a tagged reference (the bootstrap testdata itself uses registry.host.com/os:latest). In that case imageDigest fails and the whole install fails on a condition the user can't act on, even though the pre-built image may be perfectly valid. Given the missing-label case only warns, treating an unverifiable cluster reference the same way seems more consistent.
♻️ Suggested change
expectedDigest, err := imageDigest(expectedBaseOSImage)
if err != nil {
- return fmt.Errorf("cluster's resolved base OS image %q is invalid: %w", expectedBaseOSImage, err)
+ klog.Warningf("cluster's resolved base OS image %q is not digested (%v); skipping digest verification for pre-built image %q", expectedBaseOSImage, err, imageSpec)
+ return nil
}Note this would need the corresponding invalid expected base OS image case in TestValidatePreBuiltImageDigestFallback (bootstrap_test.go Lines 573-580) updated.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func validatePreBuiltImageDigestFallback(labels map[string]string, imageSpec, expectedBaseOSImage string) error { | |
| label := labels[preBuiltImageBaseOSLabelKey] | |
| if label == "" { | |
| klog.Warningf("pre-built image %q has no OPENSHIFT_VERSION and no %q label; skipping version verification", imageSpec, preBuiltImageBaseOSLabelKey) | |
| return nil | |
| } | |
| labelDigest, err := imageDigest(label) | |
| if err != nil { | |
| return fmt.Errorf("pre-built image %q has an invalid %q label: %w", imageSpec, preBuiltImageBaseOSLabelKey, err) | |
| } | |
| expectedDigest, err := imageDigest(expectedBaseOSImage) | |
| if err != nil { | |
| return fmt.Errorf("cluster's resolved base OS image %q is invalid: %w", expectedBaseOSImage, err) | |
| } | |
| if labelDigest != expectedDigest { | |
| return fmt.Errorf("pre-built image %q base OS image %q (digest %s) does not match the cluster's resolved base OS image %q (digest %s)", | |
| imageSpec, label, labelDigest, expectedBaseOSImage, expectedDigest) | |
| } | |
| return nil | |
| } | |
| func validatePreBuiltImageDigestFallback(labels map[string]string, imageSpec, expectedBaseOSImage string) error { | |
| label := labels[preBuiltImageBaseOSLabelKey] | |
| if label == "" { | |
| klog.Warningf("pre-built image %q has no OPENSHIFT_VERSION and no %q label; skipping version verification", imageSpec, preBuiltImageBaseOSLabelKey) | |
| return nil | |
| } | |
| labelDigest, err := imageDigest(label) | |
| if err != nil { | |
| return fmt.Errorf("pre-built image %q has an invalid %q label: %w", imageSpec, preBuiltImageBaseOSLabelKey, err) | |
| } | |
| expectedDigest, err := imageDigest(expectedBaseOSImage) | |
| if err != nil { | |
| klog.Warningf("cluster's resolved base OS image %q is not digested (%v); skipping digest verification for pre-built image %q", expectedBaseOSImage, err, imageSpec) | |
| return nil | |
| } | |
| if labelDigest != expectedDigest { | |
| return fmt.Errorf("pre-built image %q base OS image %q (digest %s) does not match the cluster's resolved base OS image %q (digest %s)", | |
| imageSpec, label, labelDigest, expectedBaseOSImage, expectedDigest) | |
| } | |
| return nil | |
| } |
🤖 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 `@pkg/controller/bootstrap/bootstrap.go` around lines 806 - 825, Update
validatePreBuiltImageDigestFallback so an invalid or non-digested
expectedBaseOSImage logs a warning and skips verification instead of returning
an error, matching the missing-label behavior; retain fatal errors for invalid
label digests and mismatched digests. Update the corresponding invalid expected
base OS image case in TestValidatePreBuiltImageDigestFallback to assert the
fallback succeeds.
Merges latest main (inspectorFactory/buildSysContextFactory refactor, render.RunBootstrap signature change) and adapts the pre-built image validation for hybrid OCL to use the shared inspector factory.
534ef0e to
0eb2602
Compare
|
@anandram2: The following tests failed, say
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
- What I did
- How to verify it
-
go test ./pkg/controller/bootstrap/...or
pre-built image "..." OCP version "X.Y" does not match the cluster's OCP version "A.B".could not access pre-built image "..." (registry unreachable or image not found): ....- Description for the changelog
Add OCP-version-match and registry-accessibility validation for hybrid-OCL pre-built images at bootstrap time
MCO-1906
Summary by CodeRabbit