Make squashfs layer optional for PXE boot images#352
Conversation
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
📝 WalkthroughWalkthroughPXE image handling now supports OCI manifests with kernel and initrd layers but no squashfs layer. Kernel and initrd URLs are required in the API and CRD, while the iPXE template omits squashfs parameters when unavailable. Tests and mock registry setup cover the new flow. ChangesPartial PXE image support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ServerBootConfiguration
participant PXEController
participant OCIRegistry
participant IPXEBootConfig
ServerBootConfiguration->>PXEController: Resolve OCI image
PXEController->>OCIRegistry: Read manifest layer digests
OCIRegistry-->>PXEController: Return kernel/initrd and optional squashfs digests
PXEController->>IPXEBootConfig: Set layer URLs
IPXEBootConfig->>IPXEBootConfig: Render iPXE script with optional squashfs parameters
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
internal/controller/serverbootconfiguration_pxe_controller.go (1)
268-268: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRetain validation for the required kernel and initrd layers.
Line 268 now returns success even when the manifest lacks kernel or initrd layers. Only squashfs should be optional; otherwise malformed images can produce boot configurations with empty required URLs. Validate
kernelDigestandinitrdDigestbefore returning, and add a negative test for each missing required layer.This matches the stated PR objective that kernel and initrd remain required while squashfs is optional.
Proposed fix
- return kernelDigest, initrdDigest, squashFSDigest, nil + if kernelDigest == "" || initrdDigest == "" { + return "", "", "", fmt.Errorf("missing required kernel or initrd layer") + } + return kernelDigest, initrdDigest, squashFSDigest, 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 `@internal/controller/serverbootconfiguration_pxe_controller.go` at line 268, Before the success return in the layer-digest parsing function, validate that both kernelDigest and initrdDigest are present and return the existing appropriate error when either required layer is missing; keep squashFSDigest optional. Add negative tests covering manifests missing the kernel layer and missing the initrd layer.
🤖 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 `@internal/controller/serverbootconfiguration_pxe_controller.go`:
- Line 268: Before the success return in the layer-digest parsing function,
validate that both kernelDigest and initrdDigest are present and return the
existing appropriate error when either required layer is missing; keep
squashFSDigest optional. Add negative tests covering manifests missing the
kernel layer and missing the initrd layer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: add2522a-22b6-4e02-ad9a-65942046ee0f
📒 Files selected for processing (4)
internal/controller/serverbootconfiguration_pxe_controller.gointernal/controller/serverbootconfiguration_pxe_controller_test.gointernal/controller/suite_test.gotest/registry/registry.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/controller/ipxebootconfig_controller_test.go (1)
41-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd negative coverage for the new API validation.
This change only exercises valid values. Add focused Ginkgo cases that attempt to create resources with missing or empty
KernelURLandInitrdURL, asserting rejection by the API server. This verifies both the generatedrequiredlist andminLengthconstraints.🤖 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 `@internal/controller/ipxebootconfig_controller_test.go` around lines 41 - 44, Extend the IPXEBootConfig API tests around the existing Spec setup with focused Ginkgo cases for missing and empty KernelURL and InitrdURL values. Attempt resource creation for each invalid case and assert that the API server rejects it, covering both required-field and minLength validation while preserving the existing valid-value test.Source: Coding guidelines
🤖 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 `@api/v1alpha1/ipxebootconfig_types.go`:
- Around line 27-34: Plan the rollout for the newly required KernelURL and
InitrdURL fields in the IPXEBootConfig API: add a migration, defaulting, or
conversion path that preserves existing v1alpha1 resources missing these fields,
or explicitly document and implement acceptance of the breaking admission
change. Ensure the chosen approach is reflected in the API conversion/defaulting
or release documentation rather than only updating validation markers.
---
Nitpick comments:
In `@internal/controller/ipxebootconfig_controller_test.go`:
- Around line 41-44: Extend the IPXEBootConfig API tests around the existing
Spec setup with focused Ginkgo cases for missing and empty KernelURL and
InitrdURL values. Attempt resource creation for each invalid case and assert
that the API server rejects it, covering both required-field and minLength
validation while preserving the existing valid-value test.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ef2fc5e-fed1-41bc-ad43-b777608ef8f3
📒 Files selected for processing (7)
api/v1alpha1/ipxebootconfig_types.goapi/v1alpha1/zz_generated.deepcopy.goconfig/crd/bases/boot.ironcore.dev_ipxebootconfigs.yamlinternal/controller/ipxebootconfig_controller_test.goserver/bootserver_test.gotemplates/ipxe-script.tpltest/registry/registry.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/registry/registry.go
| // +kubebuilder:validation:Required | ||
| // +kubebuilder:validation:MinLength=1 | ||
| KernelURL string `json:"kernelURL"` | ||
|
|
||
| // InitrdURL is the URL where the Initrd (initial RAM disk) of the OS is hosted, eg. the URL to the Initrd layer of the OS OCI image. | ||
| InitrdURL string `json:"initrdURL,omitempty"` | ||
| // +kubebuilder:validation:Required | ||
| // +kubebuilder:validation:MinLength=1 | ||
| InitrdURL string `json:"initrdURL"` |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant files =="
fd -a 'ipxebootconfig_types.go|zz_generated|crd|generated|ipxe.*yaml|IPXE' . | sed 's#^\./##' | head -200
echo
echo "== read type file =="
if [ -f api/v1alpha1/ipxebootconfig_types.go ]; then
nl -ba api/v1alpha1/ipxebootconfig_types.go | sed -n '1,180p'
fi
echo
echo "== search KernelURL/Image/initrd/url/defaulting/webhook files =="
rg -n "KernelURL|InitrdURL|Image|IPXEBootConfig|Default|Conversion|webhook|Validate|defaulting" -S --glob '!**/vendor/**' --glob '!**/.git/**' . | head -300Repository: ironcore-dev/boot-operator
Length of output: 936
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== CRD manifests mentioning IPXEBootConfig fields =="
rg -n "IPXEBootConfig|kernelURL|initrdURL|required|deprecated" -S --glob '*crd*.yaml' --glob '*.yaml' --glob '*.yml' api config manifests 2>/dev/null | sed -n '1,240p'
echo
echo "== list file under api/v1alpha1 =="
git ls-files api/v1alpha1 | sed -n '1,200p'
echo
echo "== inspect api/v1alpha1 files sizes/head =="
for f in $(git ls-files api/v1alpha1 | head -50); do
echo "--- $f ($(wc -l < "$f") lines) ---"
sed -n '1,220p' "$f"
doneRepository: ironcore-dev/boot-operator
Length of output: 2835
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== type file =="
cat -n api/v1alpha1/ipxebootconfig_types.go | sed -n '1,180p'
echo
echo "== relevant generated CRD sections =="
cat -n config/crd/bases/boot.ironcore.dev_ipxebootconfigs.yaml | sed -n '1,130p'
cat -n dist/chart/templates/crd/boot.ironcore.dev_ipxebootconfigs.yaml | sed -n '1,130人p'
echo
echo "== webhook/defaulting/conversion existence =="
git ls-files | grep -Ei '(webhook|default|convert|ipxebootconfig)' | sed -n '1,200p'
echo
echo "== relevant source references =="
rg -n "KernelURL|InitrdURL|Image |IPXEBootConfig|Deprecated|Required|webhook|defaulter|Defaulting|Conversion" api/v1alpha1 config manager pkg internal --glob '!**/vendor/**' 2>/dev/null | sed -n '1,240p'Repository: ironcore-dev/boot-operator
Length of output: 11651
Plan the rollout for the newly required fields.
KernelURL and InitrdURL are now typed as required, and the CRD schema includes them in spec.required, so existing IPXEBootConfig objects that omit these fields will fail admission after rollout. Add a migration/defaulting or conversion path, or explicitly document that breaking legacy v1alpha1 resources is acceptable.
🤖 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 `@api/v1alpha1/ipxebootconfig_types.go` around lines 27 - 34, Plan the rollout
for the newly required KernelURL and InitrdURL fields in the IPXEBootConfig API:
add a migration, defaulting, or conversion path that preserves existing v1alpha1
resources missing these fields, or explicitly document and implement acceptance
of the breaking admission change. Ensure the chosen approach is reflected in the
API conversion/defaulting or release documentation rather than only updating
validation markers.
There was a problem hiding this comment.
@afritzler do we want those fields to be required? it would at least fail faster
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
5b86cfa to
2f7eba7
Compare
Images that boot entirely from an initramfs (e.g. the sanitizer) have only kernel and initrd layers — no squashfs root filesystem. The previous validation rejected any image missing a squashfs digest, even though SquashfsURL is omitempty in the IPXEBootConfig API. Remove the hard requirement for squashfs and guard all three URL constructions so a missing digest produces an empty URL rather than a malformed one with an empty layerDigest query parameter. Signed-off-by: Stefan Hipfel <stefan.hipfel@sap.com>
Signed-off-by: Stefan Hipfel <stefan.hipfel@sap.com>
GardenLinux-specific kernel params (gl.ovl, gl.url, gl.live=1) are now conditional on SquashfsURL being non-empty. Initramfs-only images like the sanitizer (kernel + initrd, no squashfs) will no longer have empty gl.url= passed to the kernel. Signed-off-by: Stefan Hipfel <stefan.hipfel@sap.com>
A kernel URL is the minimum requirement for any iPXE boot — without it the boot attempt is meaningless. Removing omitempty documents this contract at the API level and ensures the field is always serialized. Signed-off-by: Stefan Hipfel <stefan.hipfel@sap.com>
InitrdURL is required for any real Linux boot — kernel without initrd is not a useful configuration. Remove omitempty to match KernelURL. Simplify the iPXE template back to unconditional kernel/initrd lines now that both fields are guaranteed non-empty. Signed-off-by: Stefan Hipfel <stefan.hipfel@sap.com>
Enforce non-empty at the API level via kubebuilder validation markers. Update the ipxebootconfig controller test to provide valid URLs. Signed-off-by: Stefan Hipfel <stefan.hipfel@sap.com>
Signed-off-by: Stefan Hipfel <stefan.hipfel@sap.com>
Signed-off-by: Stefan Hipfel <stefan.hipfel@sap.com>
f28fb25 to
a4de722
Compare
2 participants
Problem
The PXE boot controller rejected any OCI image that did not have a squashfs layer, even though
SquashfsURLisomitemptyin theIPXEBootConfigAPI. This broke initramfs-only images (such as the metal-maintenance-operator sanitizer) which only carry kernel and initrd layers — no squashfs root filesystem is needed since the sanitizer runs entirely in memory.The error seen in the
ServerBootConfigurationstatus:Summary by CodeRabbit
New Features
Bug Fixes
Validation
Documentation