Add OCI-based default UKI Configuration for HTTPBoot#266
Add OCI-based default UKI Configuration for HTTPBoot#266
Conversation
hardikdr
added
ok-to-publish
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughCentralizes UKI URL construction in a new internal/uki package, adds CLI flags for a default OCI image and UKI URL, updates server and controller flows to use the new helper, and removes local UKI constants and legacy default init. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant BootServer
participant UKIHelper as internal/uki
participant OCIRegistry as OCI Registry / Image Server
Client->>BootServer: HTTPBoot request (no explicit UKI)
BootServer->>UKIHelper: ConstructUKIURLFromOCI(image, imageServerURL, arch)
UKIHelper->>OCIRegistry: Resolve image reference / fetch manifest(s)
OCIRegistry-->>UKIHelper: Manifest / index JSON
UKIHelper->>OCIRegistry: Fetch nested manifest/layer blob
OCIRegistry-->>UKIHelper: Layer blob (UKI) with digest
UKIHelper-->>BootServer: UKI URL (imageServerURL/.../sha256-<digest>.efi)
BootServer-->>Client: HTTPBoot response with constructed UKI URL
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
hardikdr
force-pushed
the
enh/default-oci-httpboot
branch
from
b4d08c3 to
ac3a292
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@internal/uki/oci.go`:
- Around line 57-66: The loop is shadowing the outer manifest variable and may
leave targetManifestDesc set to the initial desc when no architecture match is
found; update the code in the block that iterates indexManifest.Manifests so you
do not shadow the outer manifest (rename the loop variable, e.g., to m or
entry), ensure targetManifestDesc is initialized to a zero/empty descriptor (or
use a found flag) before the loop, set targetManifestDesc only when m.Platform
!= nil && m.Platform.Architecture == architecture, and after the loop check that
targetManifestDesc is actually populated (or the found flag is true) before
using its Digest; reference: indexManifest, targetManifestDesc, manifest
(outer), architecture, desc.
- Around line 20-24: The image parsing in ConstructUKIURLFromOCI incorrectly
splits on all ':' which breaks registry host:port cases (e.g.,
myregistry:5000/repo/image:v1.0); update the function to parse the OCI reference
properly by using a robust parser like github.com/distribution/reference or
github.com/google/go-containerregistry/pkg/name to extract the repository/name
and tag/digest, or at minimum use strings.LastIndex to separate the tag (and
handle image@digest with '@') instead of strings.Split, then build the UKI URL
from the parsed reference parts (retain imageServerURL and architecture logic).
In `@server/bootserver.go`:
- Around line 409-429: The code may return an empty UKI URL when both
defaultOCIImage and defaultUKIURL are empty, causing clients to receive
"UKIURL": ""; update the logic where ukiURL is set (the block handling
defaultOCIImage/defaultUKIURL that populates httpBootResponseData) to detect the
case both defaultOCIImage == "" and defaultUKIURL == "" and either log a
warning/error via log.Warn/ log.Error and return an HTTP error (http.Error) (or
set a clear fallback value) instead of silently returning an empty UKIURL;
ensure the change references ukiURL, defaultOCIImage, defaultUKIURL,
httpBootResponseData and preserves the existing OCI construction path using
uki.ConstructUKIURLFromOCI.
🧹 Nitpick comments (4)
internal/uki/oci.go (1)
102-106: Avoidfmt.Printffor error logging in library code.Using
fmt.Printffor logging is not suitable for production code. The calling code may have structured logging configured, and this bypasses it. Consider returning the close error or accepting a logger parameter.♻️ Proposed fix to silently ignore close error or propagate it
defer func() { - if cerr := reader.Close(); cerr != nil { - fmt.Printf("failed to close reader: %v\n", cerr) - } + _ = reader.Close() }()internal/controller/serverbootconfiguration_http_controller.go (1)
182-187: Consider validatingArchitecturefield is non-empty.The method validates
ImageServerURLbut notArchitecture. IfArchitectureis empty, it will be passed toConstructUKIURLFromOCI, which may fail to find a matching manifest or produce unexpected behavior.♻️ Proposed fix to validate Architecture
func (r *ServerBootConfigurationHTTPReconciler) constructUKIURL(ctx context.Context, image string) (string, error) { if strings.TrimSpace(r.ImageServerURL) == "" { return "", fmt.Errorf("image server URL is empty") } + if strings.TrimSpace(r.Architecture) == "" { + return "", fmt.Errorf("architecture is empty") + } return uki.ConstructUKIURLFromOCI(ctx, image, r.ImageServerURL, r.Architecture) }server/bootserver.go (1)
52-52: Consider using a configuration struct for cleaner function signatures.
RunBootServernow has 8 parameters, which impacts readability and maintainability. A configuration struct would make the API cleaner and easier to extend.♻️ Example configuration struct approach
type BootServerConfig struct { Addr string IPXEServiceURL string K8sClient client.Client Log logr.Logger DefaultOCIImage string DefaultUKIURL string ImageServerURL string Architecture string } func RunBootServer(cfg BootServerConfig) { // ... }cmd/main.go (1)
316-316: Both deprecated and new flag values are passed despite "ignoring" warning.At line 132-133, when both flags are provided, a warning states
--default-httpboot-uki-urlis being ignored. However, both values are still passed toRunBootServer. While the server logic handles precedence correctly, consider clearingdefaultHTTPBootUKIURLwhen it's being ignored for consistency with the warning message.♻️ Proposed fix to clear ignored value
if defaultHTTPBootOCIImage != "" && defaultHTTPBootUKIURL != "" { setupLog.Info("Ignoring --default-httpboot-uki-url because --default-httpboot-oci-image is set") + defaultHTTPBootUKIURL = "" }
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Dockerfile (1)
2-2:⚠️ Potential issue | 🟡 MinorUsing a release candidate Go version in production builds is risky.
golang:1.26rc2is a release candidate, not a stable release. RC versions may have bugs or behavior changes before the final release. Consider using a stable Go version (e.g.,golang:1.24orgolang:1.25) for production Docker images.
🤖 Fix all issues with AI agents
In `@internal/uki/oci.go`:
- Around line 36-42: getUKIDigestFromNestedManifest currently constructs an
unauthenticated resolver (docker.NewResolver(docker.ResolverOptions{})), which
fails for private OCI registries; update this function to build ResolverOptions
with Hosts populated and an authorizer created via docker.NewDockerAuthorizer so
the resolver can authenticate. Specifically, modify
getUKIDigestFromNestedManifest to load credentials (either static from
environment/secret or from the Docker config file ~/.docker/config.json), create
an authorizer with docker.NewDockerAuthorizer(...) using those credentials, set
ResolverOptions.Hosts to include registry host info, then pass that options
struct into docker.NewResolver so Resolve() can access private registries.
Ensure the changes reference the getUKIDigestFromNestedManifest function and the
ResolverOptions/ docker.NewDockerAuthorizer usage.
🧹 Nitpick comments (2)
internal/uki/oci.go (1)
102-106: Use structured logging instead offmt.Printf.The error from
reader.Close()is logged usingfmt.Printf, which bypasses the structured logging used elsewhere in the codebase. Consider accepting a logger parameter or returning the error.♻️ Proposed fix
-func fetchContent(ctx context.Context, resolver remotes.Resolver, ref string, desc ocispec.Descriptor) ([]byte, error) { +func fetchContent(ctx context.Context, resolver remotes.Resolver, ref string, desc ocispec.Descriptor) (data []byte, err error) { fetcher, err := resolver.Fetcher(ctx, ref) if err != nil { return nil, fmt.Errorf("failed to get fetcher: %w", err) } reader, err := fetcher.Fetch(ctx, desc) if err != nil { return nil, fmt.Errorf("failed to fetch content: %w", err) } - defer func() { - if cerr := reader.Close(); cerr != nil { - fmt.Printf("failed to close reader: %v\n", cerr) - } - }() + defer func() { + if cerr := reader.Close(); cerr != nil && err == nil { + err = fmt.Errorf("failed to close reader: %w", cerr) + } + }() - data, err := io.ReadAll(reader) + data, err = io.ReadAll(reader)cmd/main.go (1)
128-133: Consider clearingdefaultHTTPBootUKIURLwhen both flags are provided.The log message states the UKI URL is being ignored, but both values are still passed to
RunBootServer. While the boot server handles precedence correctly, explicitly clearing the deprecated value here would make the behavior more obvious and prevent confusion.♻️ Proposed fix
if defaultHTTPBootOCIImage != "" && defaultHTTPBootUKIURL != "" { setupLog.Info("Ignoring --default-httpboot-uki-url because --default-httpboot-oci-image is set") + defaultHTTPBootUKIURL = "" }
hardikdr
added
area/metal-automation
and removed
ok-to-publish
hardikdr
force-pushed
the
enh/default-oci-httpboot
branch
from
ac3a292 to
c8de348
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (5)
internal/uki/oci_test.go (1)
66-88:tt := ttis no longer needed with Go 1.22+.Since this project uses Go 1.26 (per the Dockerfile), loop variables are now scoped per iteration. The
tt := ttcapture on Line 67 is a no-op and can be removed for clarity.♻️ Suggested cleanup
for _, tt := range tests { - tt := tt t.Run(tt.name, func(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/uki/oci_test.go` around lines 66 - 88, The test loop contains a redundant shadowing line "tt := tt" that was used to capture loop variables in older Go versions; remove that line in the range loop so the subtest uses the loop-scoped variable directly, leaving the t.Run body and calls to parseOCIReferenceForUKI, t.Parallel, and the assertions unchanged.internal/uki/oci.go (3)
20-34: URL construction hardcodessha256digest algorithm.Lines 31-32 assume the digest is always
sha256:...and build the URL with asha256-prefix. This is fine for current usage, but if the upstream registry or OCI spec transitions to a different algorithm, this will silently produce invalid URLs. A minor defensive improvement would be to split on:from the full digest string.♻️ More generic digest handling
- ukiDigest = strings.TrimPrefix(ukiDigest, "sha256:") - ukiURL := fmt.Sprintf("%s/%s/sha256-%s.efi", imageServerURL, repository, ukiDigest) + // Replace "sha256:abc..." → "sha256-abc..." + ukiDigest = strings.Replace(ukiDigest, ":", "-", 1) + ukiURL := fmt.Sprintf("%s/%s/%s.efi", imageServerURL, repository, ukiDigest)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/uki/oci.go` around lines 20 - 34, ConstructUKIURLFromOCI currently trims a hardcoded "sha256:" prefix and builds a URL with "sha256-" which will break for other digest algorithms; update the logic in ConstructUKIURLFromOCI (and any callers like getUKIDigestFromNestedManifest if needed) to parse the returned digest by splitting on ":" into algorithm and hex parts (e.g., strings.SplitN(digest, ":", 2)), validate both parts exist, and then build the filename as "<algorithm>-<hex>.efi" (fall back to using the original digest string if no ":" is present) so the URL uses the actual digest algorithm instead of assuming sha256.
153-160:io.ReadAllreads the entire content into memory before validating size.The size check on Line 158 only fires after the full content has been buffered. For manifests this is fine (typically < 1MB), but
fetchContentis a general-purpose helper. Consider usingio.LimitReaderto cap the read atdesc.Size + 1(to detect oversize), which provides a cheap safety net.♻️ Bounded read suggestion
- data, err := io.ReadAll(reader) + data, err := io.ReadAll(io.LimitReader(reader, desc.Size+1)) if err != nil { return nil, fmt.Errorf("failed to read content: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/uki/oci.go` around lines 153 - 160, The current code uses io.ReadAll(reader) which buffers the entire stream before checking size; update the logic in the function that reads from reader (the fetchContent helper) to wrap reader with io.LimitReader(reader, desc.Size+1) so you only read up to desc.Size+1 bytes, then call io.ReadAll on that limited reader, and validate: if len(data) > int(desc.Size) return an oversize error, if int64(len(data)) < desc.Size return a truncated error; keep the existing error messages and references to desc.Size and reader when reporting mismatches.
147-151: Avoidfmt.Printffor logging in production code.Line 149 uses
fmt.Printfto log a close error, which bypasses the structured logging (logr) used everywhere else in the codebase. This will write to stdout without log level, timestamp, or structured fields. Either propagate a logger or silently discard the error (close errors on read-only streams are rarely actionable).♻️ Minimal fix: discard non-actionable close error
defer func() { - if cerr := reader.Close(); cerr != nil { - fmt.Printf("failed to close reader: %v\n", cerr) - } + _ = reader.Close() }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/uki/oci.go` around lines 147 - 151, The deferred close uses fmt.Printf which bypasses the project's structured logging; update the defer that wraps reader.Close() to not use fmt.Printf: either silently discard the non-actionable error (e.g. remove the fmt.Printf and just ignore cerr) or, if a logr.Logger (commonly named log or logger) is in scope, call that logger (e.g. log.Error(cerr, "failed to close reader")) instead; edit the defer func containing reader.Close() to implement one of these two fixes.server/bootserver.go (1)
52-58: Consider a config struct forRunBootServer.The function now takes 8 positional parameters (mostly strings), making call sites fragile and hard to read. A
BootServerConfigstruct would improve clarity and make future additions safer.♻️ Example struct approach
type BootServerConfig struct { Addr string IPXEServiceURL string K8sClient client.Client Log logr.Logger DefaultOCIImage string DefaultUKIURL string ImageServerURL string Architecture string } func RunBootServer(cfg BootServerConfig) { ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/bootserver.go` around lines 52 - 58, Introduce a BootServerConfig struct and replace the long RunBootServer parameter list with a single cfg BootServerConfig parameter: define fields matching the current params (e.g., Addr, IPXEServiceURL, K8sClient, Log, DefaultOCIImage, DefaultUKIURL, ImageServerURL, Architecture), change the function signature to RunBootServer(cfg BootServerConfig), and update the internal http.HandleFunc calls to pass cfg.K8sClient, cfg.Log and the appropriate cfg fields into handleIPXE and handleHTTPBoot; then update all call sites of RunBootServer to construct and pass a BootServerConfig instead of positional args and adjust any unit tests or callers accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/uki/oci_test.go`:
- Around line 66-88: The test loop contains a redundant shadowing line "tt :=
tt" that was used to capture loop variables in older Go versions; remove that
line in the range loop so the subtest uses the loop-scoped variable directly,
leaving the t.Run body and calls to parseOCIReferenceForUKI, t.Parallel, and the
assertions unchanged.
In `@internal/uki/oci.go`:
- Around line 20-34: ConstructUKIURLFromOCI currently trims a hardcoded
"sha256:" prefix and builds a URL with "sha256-" which will break for other
digest algorithms; update the logic in ConstructUKIURLFromOCI (and any callers
like getUKIDigestFromNestedManifest if needed) to parse the returned digest by
splitting on ":" into algorithm and hex parts (e.g., strings.SplitN(digest, ":",
2)), validate both parts exist, and then build the filename as
"<algorithm>-<hex>.efi" (fall back to using the original digest string if no ":"
is present) so the URL uses the actual digest algorithm instead of assuming
sha256.
- Around line 153-160: The current code uses io.ReadAll(reader) which buffers
the entire stream before checking size; update the logic in the function that
reads from reader (the fetchContent helper) to wrap reader with
io.LimitReader(reader, desc.Size+1) so you only read up to desc.Size+1 bytes,
then call io.ReadAll on that limited reader, and validate: if len(data) >
int(desc.Size) return an oversize error, if int64(len(data)) < desc.Size return
a truncated error; keep the existing error messages and references to desc.Size
and reader when reporting mismatches.
- Around line 147-151: The deferred close uses fmt.Printf which bypasses the
project's structured logging; update the defer that wraps reader.Close() to not
use fmt.Printf: either silently discard the non-actionable error (e.g. remove
the fmt.Printf and just ignore cerr) or, if a logr.Logger (commonly named log or
logger) is in scope, call that logger (e.g. log.Error(cerr, "failed to close
reader")) instead; edit the defer func containing reader.Close() to implement
one of these two fixes.
In `@server/bootserver.go`:
- Around line 52-58: Introduce a BootServerConfig struct and replace the long
RunBootServer parameter list with a single cfg BootServerConfig parameter:
define fields matching the current params (e.g., Addr, IPXEServiceURL,
K8sClient, Log, DefaultOCIImage, DefaultUKIURL, ImageServerURL, Architecture),
change the function signature to RunBootServer(cfg BootServerConfig), and update
the internal http.HandleFunc calls to pass cfg.K8sClient, cfg.Log and the
appropriate cfg fields into handleIPXE and handleHTTPBoot; then update all call
sites of RunBootServer to construct and pass a BootServerConfig instead of
positional args and adjust any unit tests or callers accordingly.
94fabf0 to
97c72dc
Compare
hardikdr
removed
ok-to-publish
1 participant
Summary by CodeRabbit
New Features
Deprecations
Bug Fixes / Reliability
Tests / Chores