WIP: DNM Test merge of the ironic networking port configuration

[alegacy]

Test merge of the ironic networking port configuration to get the 🐰 to review ahead of opening an upstream PR.

Summary by CodeRabbit

• New Features
  • Added NetworkInterfaces validation and switch-port configuration (VLAN/MTU/mode, plus switch identity) with per-port configuration drift tracking via appliedPortConfigs.
  • Added URL validation webhooks for DataImage and HostFirmwareComponents (including allowed OCI support).
  • Enabled full HostClaim reconciliation and expanded end-to-end coverage for networking, firmware components, and network-data.
• Bug Fixes
  • Improved preservation and reconciliation of Ironic port local_link_connection metadata.
  • Standardized registry credentials to plain username:password.
• Documentation
  • Updated end-to-end testing guidance and fixtures for new networking/firmware scenarios.

[coderabbitai]

Walkthrough

The PR adds BareMetalHost network-interface validation and Ironic port configuration tracking, implements HostClaim reconciliation, introduces DataImage and HostFirmwareComponents webhooks, expands E2E coverage, updates deployment configuration, refreshes dependencies, and changes registry credentials to plaintext.

Changes

Core functionality

Layer / File(s)	Summary
Network API and reconciliation apis/metal3.io/v1alpha1/*, internal/controller/metal3.io/*, config/base/crds/*	Adds switch-port identifiers, applied port status, network-interface validation, namespace allowlists, VLAN expansion, and provisioning guards.
Ironic port management pkg/provisioner/provisioner.go, pkg/provisioner/ironic/*	Adds port configuration contracts, LLDP/local-link handling, detailed port retrieval, drift comparison, and Ironic port updates.
HostClaim controller internal/controller/metal3.io/hostclaim_controller.go, pkg/hostclaim/*, main.go	Implements HostClaim association, updates, deletion, finalizers, conditions, watches, and manager wiring.
Admission validation internal/webhooks/metal3.io/v1alpha1/*, config/base/webhook/manifests.yaml	Adds shared URL validation and validating webhooks for DataImage and HostFirmwareComponents resources.
E2E and supporting configuration test/e2e/*, config/overlays/e2e/*, hack/ci-e2e.sh	Adds networking, firmware, and network-data scenarios, adjusts Ironic test selection, adds namespaces, and introduces cached image downloads.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

• openshift/baremetal-operator#513: Shares HostClaim wiring, controller/manager refactoring, dependency updates, and credential-format changes.
• openshift/baremetal-operator#514: Shares switch-port API, applied configuration, controller, provisioner, and Ironic port-management changes.

Suggested reviewers: derekhiggins, hroyrh, dtantsur

✨ Finishing Touches 💡 2

⚔️ Resolve merge conflicts 💡

• Resolve merge conflict in branch merge-upstream-2026-07-29

🛠️ Fix failing CI checks 💡

• Fix failing CI checks

🧪 Generate unit tests (beta)

• Create PR with unit tests

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: alegacy
Once this PR has been reviewed and has the lgtm label, please assign dtantsur for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

Actionable comments posted: 16

🧹 Nitpick comments (18)

test/e2e/networking_test.go (4)

312-313: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use the metal3api.SwitchportModeAccess constant instead of the string cast.

Elsewhere in this file the typed constant is used; metal3api.SwitchPortMode("access") bypasses it for no benefit.

Also applies to: 1263-1264

🤖 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/e2e/networking_test.go` around lines 312 - 313, Replace the string cast
in the assertions for
retrievedBMH.Status.AppliedPortConfigs[0].SwitchPortConfig.Mode and the
corresponding assertion around the later occurrence with the existing
metal3api.SwitchportModeAccess constant, preserving the expected access-mode
behavior.

---

166-182: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Assert the rejection is a validation error, not just any error.

A bare HaveOccurred() also passes for connectivity/RBAC/missing-CRD failures, so the spec can go green without CRD validation working.

♻️ Proposed tightening

-		err := c.Create(ctx, invalidHNA)
-		Expect(err).To(HaveOccurred())
+		err := c.Create(ctx, invalidHNA)
+		Expect(k8serrors.IsInvalid(err)).To(BeTrue(), fmt.Sprintf("expected Invalid error, got: %v", err))
@@
-		err = c.Create(ctx, invalidMTU)
-		Expect(err).To(HaveOccurred())
+		err = c.Create(ctx, invalidMTU)
+		Expect(k8serrors.IsInvalid(err)).To(BeTrue(), fmt.Sprintf("expected Invalid error, got: %v", err))

🤖 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/e2e/networking_test.go` around lines 166 - 182, Update the invalid MTU
case in the networking validation test to assert that c.Create(ctx, invalidMTU)
returns a Kubernetes API validation error, rather than merely any error. Reuse
the project’s existing validation-error matcher or assertion pattern, and keep
the rejection assertion focused on invalidMTU.

---

1326-1341: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

This spec can pass without asserting anything.

When local_link_connection is empty the spec only logs, so "should preserve LLDP data on Ironic ports" is a no-op in the common sushy-tools setup. Consider Skip() with the reason instead of silently passing, so the gap is visible in test reports.

🤖 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/e2e/networking_test.go` around lines 1326 - 1341, The LLDP preservation
assertion in the local_link_connection validation currently passes silently when
the field is empty. In the empty branch of the surrounding test, replace the
informational-only Logf behavior with Skip() and a clear reason indicating that
sushy-tools did not provide LLDP data, while preserving the existing assertions
for populated connections.

---

315-323: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Move per-spec cleanup into DeferCleanup so failures don't leak HNAs/BMHs.

Cleanup is written as trailing Expect(c.Delete(...)) calls; any earlier failure aborts the spec and leaves the BMH and HNA behind. In NAMESPACE_SCOPED mode the namespace is shared (specName) and not deleted, so leftovers (plus the HNA protection finalizer) can cascade into the following specs. Register cleanup right after each create.

As per coding guidelines: "Setup and cleanup - use BeforeEach/AfterEach, flag tests without cleanup especially for cluster-scoped resources".

♻️ Example for the lifecycle spec

 		Expect(c.Create(ctx, hna)).To(Succeed())
+		DeferCleanup(func() {
+			_ = c.Delete(ctx, hna)
+		})
@@
 		Expect(c.Create(ctx, bmh)).To(Succeed())
+		DeferCleanup(func() {
+			_ = c.Delete(ctx, bmh)
+			WaitForBmhDeleted(ctx, WaitForBmhDeletedInput{
+				Client:    c,
+				BmhName:   bmh.Name,
+				Namespace: bmh.Namespace,
+			}, e2eConfig.GetIntervals(specName, "wait-bmh-deleted")...)
+		})

Also applies to: 394-402, 503-512

🤖 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/e2e/networking_test.go` around lines 315 - 323, Replace the trailing
BMH/HNA deletion block in the affected networking specs with DeferCleanup
registrations immediately after each resource is created, including the cleanup
flows around the referenced lifecycle sections. Ensure deferred cleanup deletes
the HNA and BMH and waits for BMH deletion, preserving the existing client,
names, namespace, and wait intervals so cleanup runs even when the spec fails.

Source: Coding guidelines

test/vbmctl/cmd/vbmctl/create.go (1)

240-249: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Add a short comment explaining the ordering dependency.

Moving BMC-emulator creation after the network/veth setup looks intentional (the emulator needs the bridges/networks to exist), but nothing records that, so a future cleanup could reorder it back.

♻️ Proposed comment

+			// The BMC emulator must be started after the libvirt networks and
+			// veth pairs exist, since it attaches to them.
 			if cfg.Spec.BMCEmulator != 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 `@test/vbmctl/cmd/vbmctl/create.go` around lines 240 - 249, Add a concise
comment immediately before the BMC emulator creation block in the command flow
containing cfg.Spec.BMCEmulator that documents its ordering dependency on the
earlier network/bridge/veth setup; state that the emulator must be created
afterward and leave the surrounding behavior unchanged.

hack/ci-e2e.sh (1)

115-136: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Fallback mirrors are fetched without integrity verification.

The new secondary sources (cirros-cloud.net, sourceforge, tarballs.opendev.org) are downloaded and used with no checksum/signature check, so a mirror or MITM substitution silently changes what CI boots. IMAGE_CHECKSUM at line 104 is passed to Ironic, not validated locally before use. Consider verifying the downloaded artifacts against the known checksums after each download.

🤖 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 `@hack/ci-e2e.sh` around lines 115 - 136, Update the fallback download paths in
the disk-image and IPA download block to verify each fetched artifact against
its known checksum before it is used. Reuse the existing checksum configuration
or add the corresponding checksum values and validate the files produced by
cache_image, wget, and the IPA fallback, failing the script on mismatch; do not
rely on IMAGE_CHECKSUM being passed to Ironic as local verification.

test/e2e/ironic_helpers_test.go (1)

17-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Accept a context.Context instead of context.Background().

Other e2e helpers thread the suite ctx, so this call can't be cancelled with the suite and has no timeout.

As per path instructions, "context.Context for cancellation and timeouts".

♻️ Proposed refactor

-func fetchIronicPorts(e2eConfig *Config, namespace, bmhName string) ([]ports.Port, error) {
+func fetchIronicPorts(ctx context.Context, e2eConfig *Config, namespace, bmhName string) ([]ports.Port, error) {
 	client := CreateIronicClient(e2eConfig)
 	nodeName := fmt.Sprintf("%s~%s", namespace, bmhName)
 
 	opts := ports.ListOpts{
 		Node: nodeName,
 	}
 
-	allPages, err := ports.ListDetail(client, opts).AllPages(context.Background())
+	allPages, err := ports.ListDetail(client, opts).AllPages(ctx)

🤖 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/e2e/ironic_helpers_test.go` around lines 17 - 31, Update
fetchIronicPorts to accept a context.Context parameter and pass it to
ports.ListDetail(client, opts).AllPages instead of context.Background(). Update
every caller to provide the suite context while preserving the existing
port-listing behavior and error handling.

Source: Path instructions

test/e2e/network_data_test.go (1)

94-203: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider extracting the shared spec body.

The two specs are near-identical apart from the BMH spec field (PreprovisioningNetworkDataName vs NetworkData) and the extra HardwareData assertion; a small table/helper would cut ~100 duplicated lines.

Also applies to: 205-299

🤖 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/e2e/network_data_test.go` around lines 94 - 203, Refactor the two
network-data specs, including the companion case later in the file, to use a
shared helper or table-driven setup for common secret creation, BMH creation,
state waits, provisioning, and config-drive verification. Parameterize the
differing network-data field (PreprovisioningNetworkDataName versus NetworkData)
and whether to assert HardwareData, while preserving each test’s existing
behavior.

internal/controller/metal3.io/hostfirmwaresettings_controller.go (1)

33-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

Avoid dot-importing pkg/logging. Both controllers pull the logging constants into the package namespace, which hides the origin of identifiers like VerbosityLevelDebug and LogFieldDifference and risks future collisions with local names. A short named import (e.g. logging "…/pkg/logging") keeps call sites self-documenting.

• internal/controller/metal3.io/hostfirmwaresettings_controller.go#L33-L33: replace the dot import with a named import and qualify VerbosityLevel*/LogField* references.
• internal/controller/metal3.io/hostfirmwarecomponents_controller.go#L27-L27: same change for the constants used in updateEventHandler and publishEvent.

🤖 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/metal3.io/hostfirmwaresettings_controller.go` at line 33,
Replace the dot import in
internal/controller/metal3.io/hostfirmwaresettings_controller.go:33-33 with a
named logging import and qualify all VerbosityLevel* and LogField* references.
Apply the same change in
internal/controller/metal3.io/hostfirmwarecomponents_controller.go:27-27,
including constants used by updateEventHandler and publishEvent.

pkg/secretutils/dockerconfig_test.go (1)

48-52: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Assert the extracted credential value, not only its shape.

strings.SplitN(result, ":", 2) only proves that a colon exists, so wrong-registry or wrong username/password results can pass. Add an expected credential to each test case and compare result exactly.

🤖 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/secretutils/dockerconfig_test.go` around lines 48 - 52, Update the tests
around the credential extraction cases to include an expected credential value
for each test case, then compare the extracted result exactly against that
expected value. Keep the existing username:password shape validation if useful,
but ensure wrong registry or credentials cause the test to fail.

pkg/hostclaim/hostclaim_manager.go (1)

222-224: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Update is an undocumented no-op.

Unlike Delete (which carries a // TODO: to be reimplemented later. comment), Update silently does nothing with no annotation. Both reconcileNormal's provisioned/associated paths and the "Provisioned"/"Associated" test entries in hostclaim_controller_test.go call Update expecting it to actually reconcile some state; right now it's a pure stub. Since the PR is explicitly WIP, consider adding a // TODO here too so reviewers/future contributors don't mistake it for a finished implementation.

Want me to sketch what Update should synchronize (e.g. BMH image/consumer state) based on the equivalent CAPI machine-manager pattern, or open a tracking issue?

🤖 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/hostclaim/hostclaim_manager.go` around lines 222 - 224, Add a TODO
comment to the no-op Manager.Update method, explicitly marking that its
implementation is pending, while leaving the current return behavior unchanged.

internal/controller/metal3.io/hostclaim_controller_test.go (1)

40-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Mock tracks condition writes but no test asserts them.

MockHostManager.conditions/mockCondition capture the reason/message/ok passed to SetConditionHostToFalse/SetConditionHostToTrue, but no DescribeTable entry ever inspects tc.HostManager.conditions. Given several entries exercise distinct failure/requeue paths (e.g. "Not associated - associate errs" vs "- associate requeue"), asserting on the recorded reason would meaningfully strengthen coverage of reconcileNormal/reconcileDelete's condition-setting behavior at low cost, since the tracking is already in place.

Also applies to: 203-276

🤖 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/metal3.io/hostclaim_controller_test.go` around lines 40 -
88, Add assertions to the relevant DescribeTable cases for
MockHostManager.conditions, especially the distinct associate error and requeue
paths, verifying the recorded condition type, reason, message, and ok value.
Reuse mockCondition and the existing HostManager test data to cover condition
writes from reconcileNormal and reconcileDelete without changing the mock
tracking behavior.

internal/controller/metal3.io/baremetalhost_network_test.go (1)

41-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

No coverage for the AllowedHNANamespaces allow-list.

isHNANamespaceAllowed gates both validation (AttachmentNamespaceDenied) and resolution (interface skipped), yet every reconciler here leaves AllowedHNANamespaces unset, so only the "all namespaces permitted" branch is exercised. A case with a populated allow-list plus a cross-namespace HNA would lock in the denial reason and the skip behavior.

🤖 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/metal3.io/baremetalhost_network_test.go` around lines 41
- 221, The TestValidateNetworkInterfaces table does not cover the
AllowedHNANamespaces restriction. Add a test case with a populated allow-list
and a cross-namespace HNA, configuring the relevant host/network interface data
so validation produces AttachmentNamespaceDenied and resolution skips the
disallowed interface. Assert the resulting dirty state and condition reason
alongside the existing cases.

internal/controller/metal3.io/baremetalhost_controller.go (2)

34-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

Dot-import makes symbol origins opaque.

. "github.com/metal3-io/baremetal-operator/pkg/logging" drops the package qualifier for LogField*/VerbosityLevel*, which is discouraged in Go outside of test DSLs and risks silent shadowing as either package grows. A named import (e.g. logging "…/pkg/logging") keeps call sites explicit.

🤖 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/metal3.io/baremetalhost_controller.go` at line 34,
Replace the dot import of pkg/logging with a named logging import, then qualify
all LogField* and VerbosityLevel* references in the controller with logging.
Preserve the existing behavior and use the explicit package qualifier at every
affected call site.

---

696-696: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Broad promotion of debug logs to Info will raise steady-state log volume.

These messages fire on routine reconcile transitions (power status refresh, DataImage attach/detach, firmware condition checks, event-record failures), so at default verbosity every host now emits them on each state change. If the intent is only to surface a subset by default, consider keeping the periodic ones (e.g. Lines 1733, 1803, 2667) gated at V(VerbosityLevelDebug).

Also applies to: 902-902, 1733-1733, 1803-1803, 1933-1933, 1951-1951, 1963-1963, 2137-2137, 2343-2354, 2385-2390, 2667-2667, 2760-2760

🤖 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/metal3.io/baremetalhost_controller.go` at line 696,
Reduce routine reconcile log noise by reverting the affected informational
messages in the BareMetalHost controller to the existing debug verbosity,
especially periodic transitions such as power status refresh, DataImage
attach/detach, firmware checks, and event-record failures. Update the log calls
near “host is detached, removed from provisioner” and the additionally listed
locations, while preserving Info level only for messages intended to be visible
by default.

internal/controller/metal3.io/baremetalhost_network.go (1)

213-235: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

setNetworkInterfaceValidation/clearNetworkInterfaceValidation never return a non-nil error.

The always-nil error results force callers to either handle a dead branch or discard it (see internal/controller/metal3.io/baremetalhost_controller.go Line 968, where it is dropped with _). Returning just bool removes the ambiguity.

🤖 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/metal3.io/baremetalhost_network.go` around lines 213 -
235, The setNetworkInterfaceValidation and clearNetworkInterfaceValidation
methods only ever return nil errors, so remove the error result from both
signatures and return only the changed bool. Update every caller, including the
BareMetalHost reconciler flow, to stop handling or discarding the obsolete error
value while preserving the existing condition update and removal behavior.

internal/controller/metal3.io/host_state_machine.go (1)

9-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Avoid dot-importing pkg/logging.

Switching to . "github.com/metal3-io/baremetal-operator/pkg/logging" makes VerbosityLevelDebug (line 143) unqualified, obscuring its origin and risking future identifier collisions. Prefer a qualified import.

♻️ Proposed fix

-	. "github.com/metal3-io/baremetal-operator/pkg/logging"
+	"github.com/metal3-io/baremetal-operator/pkg/logging"

And qualify usages, e.g. logging.VerbosityLevelDebug.

🤖 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/metal3.io/host_state_machine.go` around lines 9 - 11,
Replace the dot-import of pkg/logging in host_state_machine.go with a qualified
logging import, then update the VerbosityLevelDebug usage in the relevant
state-machine code to logging.VerbosityLevelDebug while preserving the existing
behavior.

pkg/provisioner/provisioner.go (1)

109-121: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Confirm the active-operation state list is complete.

IsActiveOperation excludes StateServicing and StateExternallyProvisioned. Both represent hosts actively running/serving, so port mutations there could be just as disruptive as the listed states (StateProvisioning, StateProvisioned, StateDeprovisioning, StatePoweringOffBeforeDelete, StateDeleting). Please confirm whether these states were intentionally left out (e.g., because they never reach createPortsForNode/updateNodePort), or whether they should be added.

🤖 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/provisioner/provisioner.go` around lines 109 - 121, Update
IsActiveOperation to include StateServicing and StateExternallyProvisioned in
the active-state switch, unless verified that those states cannot reach
createPortsForNode or updateNodePort. Preserve the existing true/false behavior
for all other provisioning states.

🤖 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 `@go.mod`:
- Line 100: Replace the unreleased google.golang.org/protobuf pseudo-version
with released v1.36.11 in go.mod:100, test/go.mod:130, and hack/tools/go.mod:53,
unless the specific pseudo-version commit is explicitly required.

In `@hack/ci-e2e.sh`:
- Around line 109-136: Update cache_image and the image download checks for
IMAGE_FILE, ISO_FILE, and IPA_FILE to download each artifact to a temporary path
using explicit output, remove any failed partial download, and move the
temporary file to the final destination only after success. Ensure fallback
downloads target the same temporary workflow so wget never creates a .1 file and
existing final files are only retained when complete.

In `@hack/tools/go.mod`:
- Around line 58-60: Update the k8s.io/apiextensions-apiserver dependency in
hack/tools/go.mod from v0.35.6 to v0.35.7, matching the sibling k8s.io/api and
k8s.io/apimachinery modules, then run go mod tidy to refresh related module
metadata.

In `@internal/controller/metal3.io/baremetalhost_controller.go`:
- Around line 953-974: Preserve network-interface condition changes through the
later dirty assignments in the registerHost flow: track validDirty and
clearDirty in a separate networkDirty flag, OR it into dirty after the
matchProfile and clearError updates, and ensure registerHost persists when only
this flag is set. Update clearNetworkInterfaceValidation handling to capture and
propagate its error instead of ignoring the return value.

In `@internal/controller/metal3.io/baremetalhost_network.go`:
- Around line 275-303: Update expandVLANRanges so every parsed VLAN, including
single entries without a range, is validated as being within 1–4094 before
appending. Deduplicate the accumulated VLAN values, including overlaps between
ranges and single entries, while preserving sorted output and existing
parse-error handling.

In `@internal/controller/metal3.io/host_state_machine.go`:
- Around line 508-516: Update the network-interface gate in the host state
machine before provisioning so that when Host.Spec.NetworkInterfaces is
non-empty, provisioning proceeds only if NetworkInterfacesValidCondition exists
with Status=True. Keep the existing wait logging and return behavior for nil,
Unknown, and False conditions, and add test coverage for the missing-condition
case.

In `@internal/controller/metal3.io/hostclaim_controller.go`:
- Around line 115-135: Update reconcileDelete so HostClaimDeletionFailedReason
is set only for genuine deletion failures, not RequeueAfterError results
returned by hostClaimMgr.Delete(ctx). Detect the requeue case before updating
the deletion-failed condition, while preserving the existing checkHostClaimError
return and retry behavior.

In `@internal/controller/metal3.io/hostfirmwaresettings_controller.go`:
- Around line 235-254: Remove the break in the mismatch collection loop within
the status/spec comparison so iteration continues after a Spec setting is absent
from newStatus.Settings. Preserve appending the missing setting to specMismatch
and collect all remaining mismatches regardless of map iteration order.

In `@internal/webhooks/metal3.io/v1alpha1/common_validation.go`:
- Around line 26-35: Update the URL validation flow after url.ParseRequestURI in
the relevant validation function to reject any URL whose parsed urlObj.Host is
empty, before the allowed-scheme check succeeds. Preserve the existing allowed
schemes and return an appropriate validation error for hostless values such as
https:relative, https:///image, and oci:foo.

In `@main.go`:
- Around line 427-436: Reuse the existing watchNamespaces parsing result when
constructing allowedHNANamespaces instead of independently splitting
watchNamespace. Ensure both cache configuration and HNA reference restrictions
use identical namespace entries, including whitespace and empty-value handling,
and update the setupLog.Info call to report that shared result.

In `@pkg/provisioner/ironic/provision_test.go`:
- Around line 1642-1646: Remove the t.Logf call that logs patches in this test,
or redact /instance_info/image_pull_secret before logging so the plaintext
ImagePullSecret from ProvisionData cannot appear in CI output.

In `@pkg/secretutils/dockerconfig.go`:
- Around line 17-18: Update the documentation for the plaintext credential
contract in the helper and the Validate API, including the comment in
image_auth_validator.go, to state that credentials are returned as
“username:password” rather than base64-encoded data. Ensure all related API
comments consistently instruct callers not to re-encode or reinterpret the
value, without changing runtime behavior.

In `@test/e2e/firmware_components_test.go`:
- Around line 23-28: Update biosFirmwareUpdateURL in the firmware component e2e
test to derive its host from the existing configurable e2e configuration instead
of embedding an IPv4 address. Preserve the firmware-update path and allow the
resulting URL to work in IPv6-only environments.
- Around line 87-101: Register the pre-created hfc and hup resources with
toCleanup immediately after each successful Create call, using the existing
cleanup registration pattern. Update the HFC setup around HostFirmwareComponents
and the corresponding HUP setup so Cleanup removes them even when the spec fails
before BMH ownership cleanup.

In `@test/e2e/network_data_test.go`:
- Around line 64-72: Update getNewIPAddress and the related network-data specs
to handle IPv6 addresses without failing: detect the address family and skip
IPv4-only cases explicitly, while preserving IPv4 derivation for IPv4 inputs and
guarding empty bmc.IPAddress before calling it. Update the ipv4/template setup
using 255.255.255.0 so IPv6 environments are not forced through IPv4-only
configuration. Flag tests containing “IPv4-only IP parsing” and “hardcoded IPv4
CIDRs” per the coding guidelines.

In `@test/e2e/README.md`:
- Around line 80-82: Update the “Required tests” list in the README to include
the networking suite associated with networking_test.go, preserving the existing
list structure and descriptions.

---

Nitpick comments:
In `@hack/ci-e2e.sh`:
- Around line 115-136: Update the fallback download paths in the disk-image and
IPA download block to verify each fetched artifact against its known checksum
before it is used. Reuse the existing checksum configuration or add the
corresponding checksum values and validate the files produced by cache_image,
wget, and the IPA fallback, failing the script on mismatch; do not rely on
IMAGE_CHECKSUM being passed to Ironic as local verification.

In `@internal/controller/metal3.io/baremetalhost_controller.go`:
- Line 34: Replace the dot import of pkg/logging with a named logging import,
then qualify all LogField* and VerbosityLevel* references in the controller with
logging. Preserve the existing behavior and use the explicit package qualifier
at every affected call site.
- Line 696: Reduce routine reconcile log noise by reverting the affected
informational messages in the BareMetalHost controller to the existing debug
verbosity, especially periodic transitions such as power status refresh,
DataImage attach/detach, firmware checks, and event-record failures. Update the
log calls near “host is detached, removed from provisioner” and the additionally
listed locations, while preserving Info level only for messages intended to be
visible by default.

In `@internal/controller/metal3.io/baremetalhost_network_test.go`:
- Around line 41-221: The TestValidateNetworkInterfaces table does not cover the
AllowedHNANamespaces restriction. Add a test case with a populated allow-list
and a cross-namespace HNA, configuring the relevant host/network interface data
so validation produces AttachmentNamespaceDenied and resolution skips the
disallowed interface. Assert the resulting dirty state and condition reason
alongside the existing cases.

In `@internal/controller/metal3.io/baremetalhost_network.go`:
- Around line 213-235: The setNetworkInterfaceValidation and
clearNetworkInterfaceValidation methods only ever return nil errors, so remove
the error result from both signatures and return only the changed bool. Update
every caller, including the BareMetalHost reconciler flow, to stop handling or
discarding the obsolete error value while preserving the existing condition
update and removal behavior.

In `@internal/controller/metal3.io/host_state_machine.go`:
- Around line 9-11: Replace the dot-import of pkg/logging in
host_state_machine.go with a qualified logging import, then update the
VerbosityLevelDebug usage in the relevant state-machine code to
logging.VerbosityLevelDebug while preserving the existing behavior.

In `@internal/controller/metal3.io/hostclaim_controller_test.go`:
- Around line 40-88: Add assertions to the relevant DescribeTable cases for
MockHostManager.conditions, especially the distinct associate error and requeue
paths, verifying the recorded condition type, reason, message, and ok value.
Reuse mockCondition and the existing HostManager test data to cover condition
writes from reconcileNormal and reconcileDelete without changing the mock
tracking behavior.

In `@internal/controller/metal3.io/hostfirmwaresettings_controller.go`:
- Line 33: Replace the dot import in
internal/controller/metal3.io/hostfirmwaresettings_controller.go:33-33 with a
named logging import and qualify all VerbosityLevel* and LogField* references.
Apply the same change in
internal/controller/metal3.io/hostfirmwarecomponents_controller.go:27-27,
including constants used by updateEventHandler and publishEvent.

In `@pkg/hostclaim/hostclaim_manager.go`:
- Around line 222-224: Add a TODO comment to the no-op Manager.Update method,
explicitly marking that its implementation is pending, while leaving the current
return behavior unchanged.

In `@pkg/provisioner/provisioner.go`:
- Around line 109-121: Update IsActiveOperation to include StateServicing and
StateExternallyProvisioned in the active-state switch, unless verified that
those states cannot reach createPortsForNode or updateNodePort. Preserve the
existing true/false behavior for all other provisioning states.

In `@pkg/secretutils/dockerconfig_test.go`:
- Around line 48-52: Update the tests around the credential extraction cases to
include an expected credential value for each test case, then compare the
extracted result exactly against that expected value. Keep the existing
username:password shape validation if useful, but ensure wrong registry or
credentials cause the test to fail.

In `@test/e2e/ironic_helpers_test.go`:
- Around line 17-31: Update fetchIronicPorts to accept a context.Context
parameter and pass it to ports.ListDetail(client, opts).AllPages instead of
context.Background(). Update every caller to provide the suite context while
preserving the existing port-listing behavior and error handling.

In `@test/e2e/network_data_test.go`:
- Around line 94-203: Refactor the two network-data specs, including the
companion case later in the file, to use a shared helper or table-driven setup
for common secret creation, BMH creation, state waits, provisioning, and
config-drive verification. Parameterize the differing network-data field
(PreprovisioningNetworkDataName versus NetworkData) and whether to assert
HardwareData, while preserving each test’s existing behavior.

In `@test/e2e/networking_test.go`:
- Around line 312-313: Replace the string cast in the assertions for
retrievedBMH.Status.AppliedPortConfigs[0].SwitchPortConfig.Mode and the
corresponding assertion around the later occurrence with the existing
metal3api.SwitchportModeAccess constant, preserving the expected access-mode
behavior.
- Around line 166-182: Update the invalid MTU case in the networking validation
test to assert that c.Create(ctx, invalidMTU) returns a Kubernetes API
validation error, rather than merely any error. Reuse the project’s existing
validation-error matcher or assertion pattern, and keep the rejection assertion
focused on invalidMTU.
- Around line 1326-1341: The LLDP preservation assertion in the
local_link_connection validation currently passes silently when the field is
empty. In the empty branch of the surrounding test, replace the
informational-only Logf behavior with Skip() and a clear reason indicating that
sushy-tools did not provide LLDP data, while preserving the existing assertions
for populated connections.
- Around line 315-323: Replace the trailing BMH/HNA deletion block in the
affected networking specs with DeferCleanup registrations immediately after each
resource is created, including the cleanup flows around the referenced lifecycle
sections. Ensure deferred cleanup deletes the HNA and BMH and waits for BMH
deletion, preserving the existing client, names, namespace, and wait intervals
so cleanup runs even when the spec fails.

In `@test/vbmctl/cmd/vbmctl/create.go`:
- Around line 240-249: Add a concise comment immediately before the BMC emulator
creation block in the command flow containing cfg.Spec.BMCEmulator that
documents its ordering dependency on the earlier network/bridge/veth setup;
state that the emulator must be created afterward and leave the surrounding
behavior unchanged.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

[coderabbitai]

🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
curl -s 'https://proxy.golang.org/google.golang.org/protobuf/@latest' | jq '.Version'
fd -t f '^go\.mod$' --exec sh -c 'echo "== {}"; rg -n "protobuf" {}'

Repository: openshift/baremetal-operator

Length of output: 524

---

Replace the protobuf pseudo-version

• go.mod#L100, test/go.mod#L130, and hack/tools/go.mod#L53 all pin google.golang.org/protobuf to the unreleased pseudo-version v1.36.12-0.20260120151049-f2248ac996af.
• Move to the latest released tag (v1.36.11) unless that specific commit is required.

📍 Affects 3 files

• go.mod#L100-L100 (this comment)
• test/go.mod#L130-L130
• hack/tools/go.mod#L53-L53

🤖 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 `@go.mod` at line 100, Replace the unreleased google.golang.org/protobuf
pseudo-version with released v1.36.11 in go.mod:100, test/go.mod:130, and
hack/tools/go.mod:53, unless the specific pseudo-version commit is explicitly
required.

Source: Path instructions

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Fallback via wget -P can leave a truncated primary file and write the fallback to <file>.1.

cache_image uses -P "${IMAGE_DIR}/". If the Artifactory download fails mid-transfer, the partial file stays on disk; the fallback wget then sees the existing name and saves as ${IMAGE_FILE}.1. The next run's [[ ! -f ... ]] check passes on the corrupt file. Use an explicit -O to a temp path and move on success.

🐛 Proposed fix

 cache_image() {
-    wget --no-verbose -P "${IMAGE_DIR}/" "$@"
+    # $1: destination file name, $2: URL
+    local dest="${IMAGE_DIR}/$1"
+    if wget --no-verbose -O "${dest}.tmp" "$2"; then
+        mv "${dest}.tmp" "${dest}"
+    else
+        rm -f "${dest}.tmp"
+        return 1
+    fi
 }
@@
 if [[ ! -f "${IMAGE_DIR}/${IMAGE_FILE}" ]]; then
-    if ! cache_image "${ARTIFACTORY_ROOT}/metal3/images/iso/${IMAGE_FILE}"; then
-        cache_image https://download.cirros-cloud.net/"${CIRROS_VERSION}/${IMAGE_FILE}"
+    if ! cache_image "${IMAGE_FILE}" "${ARTIFACTORY_ROOT}/metal3/images/iso/${IMAGE_FILE}"; then
+        cache_image "${IMAGE_FILE}" "https://download.cirros-cloud.net/${CIRROS_VERSION}/${IMAGE_FILE}"
     fi
 fi
 if [[ ! -f "${IMAGE_DIR}/${ISO_FILE}" ]]; then
-    if ! cache_image "${ARTIFACTORY_ROOT}/metal3/images/sysrescue/${ISO_FILE}"; then
-        wget --no-verbose -O "${IMAGE_DIR}/${ISO_FILE}" https://sourceforge.net/projects/systemrescuecd/files/sysresccd-x86/"${SYSRESCUE_VERSION}"/"${ISO_FILE}"/download
+    if ! cache_image "${ISO_FILE}" "${ARTIFACTORY_ROOT}/metal3/images/sysrescue/${ISO_FILE}"; then
+        cache_image "${ISO_FILE}" "https://sourceforge.net/projects/systemrescuecd/files/sysresccd-x86/${SYSRESCUE_VERSION}/${ISO_FILE}/download"
     fi
 fi
@@
 if [[ ! -f "${IMAGE_DIR}/${IPA_FILE}" ]]; then
-    if ! cache_image "${IPA_BASEURI}/${IPA_FILE}"; then
-        cache_image https://tarballs.opendev.org/openstack/ironic-python-agent/dib/"${IPA_FILE}"
+    if ! cache_image "${IPA_FILE}" "${IPA_BASEURI}/${IPA_FILE}"; then
+        cache_image "${IPA_FILE}" "https://tarballs.opendev.org/openstack/ironic-python-agent/dib/${IPA_FILE}"
     fi
 fi

📝 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.

Suggested change

	cache_image() {
	wget --no-verbose -P "${IMAGE_DIR}/" "$@"
	}
	ARTIFACTORY_ROOT=https://artifactory.nordix.org/artifactory
	## Download disk images
	if [[ ! -f "${IMAGE_DIR}/${IMAGE_FILE}" ]]; then
	wget --quiet -P "${IMAGE_DIR}/" https://artifactory.nordix.org/artifactory/metal3/images/iso/"${IMAGE_FILE}"
	if ! cache_image "${ARTIFACTORY_ROOT}/metal3/images/iso/${IMAGE_FILE}"; then
	cache_image https://download.cirros-cloud.net/"${CIRROS_VERSION}/${IMAGE_FILE}"
	fi
	fi
	if [[ ! -f "${IMAGE_DIR}/${ISO_FILE}" ]]; then
	wget --quiet -P "${IMAGE_DIR}/" https://artifactory.nordix.org/artifactory/metal3/images/sysrescue/"${ISO_FILE}"
	if ! cache_image "${ARTIFACTORY_ROOT}/metal3/images/sysrescue/${ISO_FILE}"; then
	wget --no-verbose -O "${IMAGE_DIR}/${ISO_FILE}" https://sourceforge.net/projects/systemrescuecd/files/sysresccd-x86/"${SYSRESCUE_VERSION}"/"${ISO_FILE}"/download
	fi
	fi
	## Download IPA (Ironic Python Agent) image
	# Ironic IPA downloader is configured to use this local image in the tests.
	# This saves time, especially during ironic upgrade tests and also
	# gives us early failure in case there is some issue downloading it.
	IPA_FILE="ipa-centos9-master.tar.gz"
	IPA_BASEURI=https://artifactory.nordix.org/artifactory/openstack-remote/ironic-python-agent/dib/
	IPA_BASEURI="${ARTIFACTORY_ROOT}/openstack-remote/ironic-python-agent/dib/"
	if [[ ! -f "${IMAGE_DIR}/${IPA_FILE}" ]]; then
	wget --quiet -P "${IMAGE_DIR}/" "${IPA_BASEURI}/${IPA_FILE}"
	if ! cache_image "${IPA_BASEURI}/${IPA_FILE}"; then
	cache_image https://tarballs.opendev.org/openstack/ironic-python-agent/dib/"${IPA_FILE}"
	fi
	cache_image() {
	# $1: destination file name, $2: URL
	local dest="${IMAGE_DIR}/$1"
	if wget --no-verbose -O "${dest}.tmp" "$2"; then
	mv "${dest}.tmp" "${dest}"
	else
	rm -f "${dest}.tmp"
	return 1
	fi
	}
	ARTIFACTORY_ROOT=https://artifactory.nordix.org/artifactory
	## Download disk images
	if [[ ! -f "${IMAGE_DIR}/${IMAGE_FILE}" ]]; then
	if ! cache_image "${IMAGE_FILE}" "${ARTIFACTORY_ROOT}/metal3/images/iso/${IMAGE_FILE}"; then
	cache_image "${IMAGE_FILE}" "https://download.cirros-cloud.net/${CIRROS_VERSION}/${IMAGE_FILE}"
	fi
	fi
	if [[ ! -f "${IMAGE_DIR}/${ISO_FILE}" ]]; then
	if ! cache_image "${ISO_FILE}" "${ARTIFACTORY_ROOT}/metal3/images/sysrescue/${ISO_FILE}"; then
	cache_image "${ISO_FILE}" "https://sourceforge.net/projects/systemrescuecd/files/sysresccd-x86/${SYSRESCUE_VERSION}/${ISO_FILE}/download"
	fi
	fi
	## Download IPA (Ironic Python Agent) image
	# Ironic IPA downloader is configured to use this local image in the tests.
	# This saves time, especially during ironic upgrade tests and also
	# gives us early failure in case there is some issue downloading it.
	IPA_FILE="ipa-centos9-master.tar.gz"
	IPA_BASEURI="${ARTIFACTORY_ROOT}/openstack-remote/ironic-python-agent/dib/"
	if [[ ! -f "${IMAGE_DIR}/${IPA_FILE}" ]]; then
	if ! cache_image "${IPA_FILE}" "${IPA_BASEURI}/${IPA_FILE}"; then
	cache_image "${IPA_FILE}" "https://tarballs.opendev.org/openstack/ironic-python-agent/dib/${IPA_FILE}"
	fi
	fi

🤖 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 `@hack/ci-e2e.sh` around lines 109 - 136, Update cache_image and the image
download checks for IMAGE_FILE, ISO_FILE, and IPA_FILE to download each artifact
to a temporary path using explicit output, remove any failed partial download,
and move the temporary file to the final destination only after success. Ensure
fallback downloads target the same temporary workflow so wget never creates a .1
file and existing final files are only retained when complete.

[coderabbitai]

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

k8s.io/apiextensions-apiserver left at v0.35.6 while sibling staging modules moved to v0.35.7.

test/go.mod bumped all three to v0.35.7. Keeping the k8s.io staging set on one patch level avoids awkward transitive resolutions; run go get k8s.io/apiextensions-apiserver@v0.35.7 && go mod tidy here.

🤖 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 `@hack/tools/go.mod` around lines 58 - 60, Update the
k8s.io/apiextensions-apiserver dependency in hack/tools/go.mod from v0.35.6 to
v0.35.7, matching the sibling k8s.io/api and k8s.io/apimachinery modules, then
run go mod tidy to refresh related module metadata.

[coderabbitai]

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Synchronize the credential-format contract.

This helper now returns plaintext at Line 61, but internal/controller/metal3.io/image_auth_validator.go still documents Validate as returning base64-encoded credentials at Lines 31-32. Update that comment and related API documentation so future callers do not re-encode or misinterpret the value.

Also applies to: 61-61

🤖 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/secretutils/dockerconfig.go` around lines 17 - 18, Update the
documentation for the plaintext credential contract in the helper and the
Validate API, including the comment in image_auth_validator.go, to state that
credentials are returned as “username:password” rather than base64-encoded data.
Ensure all related API comments consistently instruct callers not to re-encode
or reinterpret the value, without changing runtime behavior.

[coderabbitai]

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Hardcoded IPv4 address in the firmware update URL.

This pins the test to an IPv4 host route; make it configurable (e.g. an e2e config variable) so IPv6-only environments can run it.

As per coding guidelines, flag tests containing "hardcoded IPv4 addresses".

🤖 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/e2e/firmware_components_test.go` around lines 23 - 28, Update
biosFirmwareUpdateURL in the firmware component e2e test to derive its host from
the existing configurable e2e configuration instead of embedding an IPv4
address. Preserve the firmware-update path and allow the resulting URL to work
in IPv6-only environments.

Source: Coding guidelines

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Register the pre-created HFC/HUP in toCleanup.

hfc (Line 101) and hup (Line 306) are only removed via BMH ownership at the end of a passing spec. If the spec fails earlier, the namespace-scoped cleanup path in Cleanup leaves them behind for the next run.

As per coding guidelines, "flag tests without cleanup".

Also applies to: 296-306

🤖 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/e2e/firmware_components_test.go` around lines 87 - 101, Register the
pre-created hfc and hup resources with toCleanup immediately after each
successful Create call, using the existing cleanup registration pattern. Update
the HFC setup around HostFirmwareComponents and the corresponding HUP setup so
Cleanup removes them even when the spec fails before BMH ownership cleanup.

Source: Coding guidelines

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

IPv4-only IP derivation will fail on IPv6-based setups.

To4() returns nil for IPv6 addresses, so the Expect fails rather than skipping, and the 255.255.255.0/ipv4 template at Lines 24-39 is IPv4-only too. Consider branching on address family (or skipping explicitly when bmc.IPAddress is not IPv4) so IPv6 environments don't hard-fail. Note the first spec also calls this without the bmc.IPAddress == "" guard used at Line 206.

As per coding guidelines, flag tests containing "IPv4-only IP parsing" and "hardcoded IPv4 CIDRs".

🤖 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/e2e/network_data_test.go` around lines 64 - 72, Update getNewIPAddress
and the related network-data specs to handle IPv6 addresses without failing:
detect the address family and skip IPv4-only cases explicitly, while preserving
IPv4 derivation for IPv4 inputs and guarding empty bmc.IPAddress before calling
it. Update the ipv4/template setup using 255.255.255.0 so IPv6 environments are
not forced through IPv4-only configuration. Flag tests containing “IPv4-only IP
parsing” and “hardcoded IPv4 CIDRs” per the coding guidelines.

Source: Coding guidelines

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Document the new networking suite in the required-tests list.

networking_test.go is labeled required but the "Required tests" list (lines 141-157) doesn't mention it, so the doc is now stale.

🤖 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/e2e/README.md` around lines 80 - 82, Update the “Required tests” list in
the README to include the networking suite associated with networking_test.go,
preserving the existing list structure and descriptions.

[openshift-ci]

PR needs rebase.

Details

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 kubernetes-sigs/prow repository.

[openshift-ci]

@alegacy: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/vendor	88c0f6b	link	true	/test vendor
ci/prow/images	88c0f6b	link	true	/test images
ci/prow/unit	88c0f6b	link	true	/test unit
ci/prow/e2e-metal-ipi-serial-ipv4	88c0f6b	link	true	/test e2e-metal-ipi-serial-ipv4
ci/prow/e2e-metal-ipi-virtualmedia	88c0f6b	link	true	/test e2e-metal-ipi-virtualmedia
ci/prow/okd-scos-images	88c0f6b	link	true	/test okd-scos-images
ci/prow/e2e-metal-ipi-ovn-ipv6	88c0f6b	link	true	/test e2e-metal-ipi-ovn-ipv6
ci/prow/verify-deps	88c0f6b	link	true	/test verify-deps
ci/prow/lint	88c0f6b	link	true	/test lint

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

[coderabbitai]

♻️ Duplicate comments (1)

internal/controller/metal3.io/baremetalhost_controller.go (1)

970-971: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Propagate the validation-clear error.

Discarding this error lets registration continue after network-validation cleanup fails.

Proposed fix

-		if clearDirty, _ := r.clearNetworkInterfaceValidation(info.host); clearDirty {
+		clearDirty, clearErr := r.clearNetworkInterfaceValidation(info.host)
+		if clearErr != nil {
+			return actionError{fmt.Errorf("failed to clear network interface validation: %w", clearErr)}
+		}
+		if clearDirty {
 			networkDirty = true
 		}

As per path instructions, “Never ignore error returns”.

🤖 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/metal3.io/baremetalhost_controller.go` around lines 970 -
971, Update the clearNetworkInterfaceValidation call in the registration flow to
capture and propagate its error instead of discarding it. Preserve setting
networkDirty when cleanup succeeds, and return the encountered error immediately
so registration cannot continue after validation cleanup fails.

Source: Path instructions

🤖 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 `@internal/controller/metal3.io/baremetalhost_controller.go`:
- Around line 970-971: Update the clearNetworkInterfaceValidation call in the
registration flow to capture and propagate its error instead of discarding it.
Preserve setting networkDirty when cleanup succeeds, and return the encountered
error immediately so registration cannot continue after validation cleanup
fails.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 52bbc2a3-b8c2-4bb5-8f9a-6b11b681eff4

📥 Commits

Reviewing files that changed from the base of the PR and between f7ca0e4 and 88c0f6b.

📒 Files selected for processing (7)

• internal/controller/metal3.io/baremetalhost_controller.go
• internal/controller/metal3.io/baremetalhost_network.go
• internal/controller/metal3.io/baremetalhost_network_test.go
• internal/controller/metal3.io/host_state_machine.go
• internal/controller/metal3.io/host_state_machine_test.go
• main.go
• test/e2e/README.md

🚧 Files skipped from review as they are similar to previous changes (1)

• test/e2e/README.md