Merge upstream 2026-07-01

[dtantsur]

cc170cf 🌱 Bump github.com/onsi/gomega from 1.42.0 to 1.42.1 in /test
048d152 🌱 Bump github.com/onsi/gomega from 1.42.0 to 1.42.1
1465718 🌱 Bump github.com/onsi/ginkgo/v2 from 2.31.0 to 2.32.0
85c6fe0 Rename getSecretData() to getSecretDataWithFinalizer(), as the function can add a finalizer to the Secret
2eb6d82 vbmctl: Make VM template portable across distros
8ef8c1b 🚀 Release v0.13.1
3ba039f 🚀 Release v0.12.5
903bb5c feat: implement structured logging pattern
9c52495 🌱 Bump github.com/onsi/gomega from 1.41.0 to 1.42.0 in /test
509c00d 🌱 Bump golang.org/x/crypto from 0.52.0 to 0.53.0 in /test
579e0cd Bump cluster-api to v1.13.3
8f9dcab Add test for OCI image provisioning failure error message
720f8c2 Omit empty checksum from provisioning error message
e13122b 🌱 Bump github.com/moby/moby/client from 0.4.1 to 0.5.0 in /test
5da9522 🌱 Bump github.com/moby/moby/api from 1.54.2 to 1.55.0 in /test
530bbef 🌱 Bump github.com/onsi/gomega from 1.41.0 to 1.42.0
b47c32a 🌱 Bump github.com/onsi/ginkgo/v2 from 2.29.0 to 2.32.0 in /test
c7fe88e fix dependabots build workflop step
921274e 🌱 Bump the kubernetes group across 3 directories with 5 updates
8ed80ca 🌱 Bump github.com/onsi/ginkgo/v2 from 2.29.0 to 2.31.0
a8f86bd 🌱 re-trigger CI (golangci-lint apis shard hit the 6h job timeout)
e5e15c7 Remove mariadb-image from run_local_ironic.sh
9bf1828 use proper IPA cache address
5623cac e2e: decouple systemrescue and cirros downloads
f0c6fe7 fix: preserve phased-reboot annotations instead of deleting them on soft-reboot fallback
c16043f Set cooldown days to 3 for dependabot
3d51494 Fix webhook state guard status checks
7288182 Restore delete permission on secrets

Summary by CodeRabbit

• New Features

  • Added more structured, easier-to-read logging across controller workflows and command-line output.
  • Updated local and CI scripts to better handle image downloads and environment setup.

• Bug Fixes

  • Improved reboot annotation handling so only the intended reboot marker is cleared.
  • Tightened validation around BMC address changes and external provisioning state transitions.
  • Skips hardware data creation during deletion flows to avoid unnecessary work.

• Chores

  • Updated Kubernetes, Cluster API, and related Go dependencies.
  • Added release/configuration updates and refreshed test coverage.

[coderabbitai]

Walkthrough

This PR standardizes structured logging via new verbosity/field constants applied across BareMetalHost, HostFirmwareSettings, and HostFirmwareComponents controllers and main.go, adjusts reboot-annotation cleanup, secret-finalizer handling, HardwareData creation during deletion, webhook validation gating, an Ironic error message, RBAC secrets delete permission, plus dependency bumps and tooling/CI/template updates.

Changes

Structured Logging and Controller Behavior

Layer / File(s)	Summary
Logging field and verbosity constants internal/controller/metal3.io/logging.go	Adds VerbosityLevelDebug/VerbosityLevelTrace and numerous LogField* constants for standardized structured log keys.
BareMetalHost controller logging and behavior internal/controller/metal3.io/baremetalhost_controller.go, internal/controller/metal3.io/baremetalhost_controller_test.go, config/base/rbac/role.yaml	Applies verbosity-gated structured logging throughout Reconcile and action handlers, adds delete to secrets RBAC, changes clearRebootAnnotations to remove only the base annotation, skips HardwareData creation during host deletion, and adds a new unit test.
Secret retrieval finalizer refactor internal/controller/metal3.io/host_config_data.go, internal/controller/metal3.io/action_result.go	Replaces getSecretData with getSecretDataWithFinalizer across UserData/NetworkData/PreprovisioningNetworkData/MetaData, and switches RNG import to math/rand/v2.
Host state machine logging internal/controller/metal3.io/host_state_machine.go	Converts several info logs to debug verbosity with structured fields for provisioning, detach, and registration retry flows.
Firmware controllers logging internal/controller/metal3.io/hostfirmwarecomponents_controller.go, internal/controller/metal3.io/hostfirmwaresettings_controller.go	Updates Reconcile, status update, schema retrieval, and event handling logs to structured/verbosity-gated form.
Webhook validation tightening internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go, internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation_test.go	Requires OperationalStatus not Detached for BMC address changes and both old/new Provisioning.State Available for enabling ExternallyProvisioned.
main.go structured logging main.go, main_test.go	Converts version, TLS cipher warning, and concurrency logs to structured fields with matching test updates.
Ironic deploy-fail error fix pkg/provisioner/ironic/ironic.go, pkg/provisioner/ironic/provision_test.go	Conditionally appends checksum only when non-empty in the DeployFail error message; adds an OCI image test case.

Dependency Bumps, CI/Tooling, and Templates

Layer / File(s)	Summary
Go module dependency bumps go.mod, apis/go.mod, hack/tools/go.mod, test/go.mod	Bumps k8s.io/, cluster-api, ginkgo/gomega, golang.org/x/, and moby dependency versions.
CI e2e ISO/IPA URI update hack/ci-e2e.sh	Adds ISO_FILE and its download, and updates IPA_BASEURI to a non-cache path.
Local Ironic MariaDB removal tools/run_local_ironic.sh	Removes MariaDB image, TLS cert/key handling, and password generation/container run logic, pointing to the MariaDB Operator instead.
Libvirt VM template update test/vbmctl/pkg/libvirt/templates/VM.xml.tpl	Switches EFI machine type to q35, removes explicit emulator, and changes video model to virtio.
Zizmor config and release notes .zizmor.yml, releasenotes/v0.12.5.md, releasenotes/v0.13.1.md	Adds a dependabot-cooldown rule and v0.12.5/v0.13.1 release notes.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Reconcile
  participant clearRebootAnnotations
  participant HostAnnotations
  Reconcile->>clearRebootAnnotations: host with reboot annotations
  clearRebootAnnotations->>HostAnnotations: delete base reboot annotation only
  clearRebootAnnotations-->>Reconcile: dirty flag, suffixed annotations retained

Loading

sequenceDiagram
  participant Controller
  participant SecretManager
  participant Secret
  Controller->>SecretManager: getSecretDataWithFinalizer(addFinalizer)
  SecretManager->>Secret: ObtainSecretWithFinalizer
  Secret-->>SecretManager: secret data or NoDataInSecretError
  SecretManager-->>Controller: data / error

Loading

Related Issues: Not specified in the diff.

Related PRs: Not specified in the diff.

Suggested labels: dependencies, logging, ok-to-test

Suggested reviewers: dtantsur, zaneb, hardys

🐰 A rabbit hops through logs anew,
With verbose fields in structured view,
Secrets now delete, annotations trimmed,
Dependencies bumped, versions limbed,
Q35 boots where cirrus once flew.

---

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error, 1 warning)

Check name	Status	Explanation	Resolution
No-Sensitive-Data-In-Logs	❌ Error	The PR logs raw DataImage.Spec.URL, raw reboot annotation values, and event.Message, which can expose internal hostnames or user data.	Redact those fields: log object names/reason/error summaries instead of URLs, annotation payloads, or duplicated event messages.
Docstring Coverage	⚠️ Warning	Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (13 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately reflects that this PR is an upstream merge dated 2026-07-01.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	The touched test files use plain Go t.Run table cases with static names; no Ginkgo It/Describe/Context/When titles or dynamic identifiers were added.
Test Structure And Quality	✅ Passed	The changed tests are table-driven unit tests, not Ginkgo suites; they use bounded retry helpers and no cluster resources or indefinite waits.
Microshift Test Compatibility	✅ Passed	No new Ginkgo e2e tests were added; the touched tests are unit tests and don't reference MicroShift-unsupported OpenShift APIs or need MicroShift guards.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	No new Ginkgo e2e tests were added; the only new test is a standard Go unit test, and the modified test files contain no Ginkgo DSL or SNO assumptions.
Topology-Aware Scheduling Compatibility	✅ Passed	Reviewed modified manifests/controllers; no node selectors, anti-affinity, topology spread, PDB, or replica-count logic were introduced. Changes are RBAC/logging/provisioning only.
Ote Binary Stdout Contract	✅ Passed	PASS: main.go has no stdout writes; it configures controller-runtime zap, which defaults to stderr, and klog usage stays inside tests.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	No new test/e2e Ginkgo cases were added with IPv4-only or external-network assumptions; the diff scan found none.
No-Weak-Crypto	✅ Passed	Touched app code only adds logging/checksum-message changes; no new weak ciphers, hash primitives, custom crypto, or secret comparisons were introduced.
Container-Privileges	✅ Passed	Only changed manifest-like file is RBAC role.yaml; it has no privileged, hostNetwork, hostIPC, SYS_ADMIN, or allowPrivilegeEscalation settings.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dtantsur

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [dtantsur]

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: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

hack/ci-e2e.sh (1)

102-123: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Pin the downloaded boot artifacts by digest.

These files are fetched at runtime and then used directly in the e2e boot path, but the new SystemRescue ISO and the IPA tarball are not verified against pinned digests. That makes CI trust whatever those endpoints serve on the day of the run.

🔐 Example hardening

 ISO_FILE="systemrescue-11.00-amd64.iso"
+ISO_FILE_SHA256="<published-systemrescue-sha256>"
 ...
 IPA_FILE="ipa-centos9-master.tar.gz"
+IPA_FILE_SHA256="<published-ipa-sha256>"

 if [[ ! -f "${IMAGE_DIR}/${ISO_FILE}" ]]; then
     wget --quiet -P "${IMAGE_DIR}/" https://artifactory.nordix.org/artifactory/metal3/images/sysrescue/"${ISO_FILE}"
 fi
+printf '%s  %s\n' "${ISO_FILE_SHA256}" "${IMAGE_DIR}/${ISO_FILE}" | sha256sum -c -

 ...
 if [[ ! -f "${IMAGE_DIR}/${IPA_FILE}" ]]; then
     wget --quiet -P "${IMAGE_DIR}/" "${IPA_BASEURI}/${IPA_FILE}"
 fi
+printf '%s  %s\n' "${IPA_FILE_SHA256}" "${IMAGE_DIR}/${IPA_FILE}" | sha256sum -c -

As per coding guidelines, "Pin external dependencies by SHA (containers, GitHub Actions, binaries)".

🤖 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 102 - 123, The boot artifact downloads in
ci-e2e.sh are trusted at fetch time but not verified, so pin both the
SystemRescue ISO and the IPA tarball by digest. Update the download flow around
IMAGE_FILE, ISO_FILE, IPA_FILE, and IPA_BASEURI to add fixed SHA256 values and
verify each file after wget completes before it is used in e2e boot tests.
Ensure the script fails fast if the checksum does not match, rather than
proceeding with an unverified artifact.

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 `@internal/controller/metal3.io/baremetalhost_controller.go`:
- Around line 1908-1909: The DataImage attachment logs in
baremetalhost_controller should not emit requestedURL/DataImage.Spec.URL because
it may expose internal hostnames or signed credentials. Update the logging in
the DataImage attach flow around the info.log.V(...).Info calls to log the
DataImage object identity instead (for example name/namespace or another
non-sensitive identifier) and remove the URL from the message/fields. Apply the
same fix to the related DataImage logging block later in the controller so all
DataImage log statements use the safe identifier consistently.
- Around line 2607-2615: The event logging in BareMetalHost controller is
duplicating the raw Event message, which can expose sensitive provisioner or
user-supplied data. Update the logging in the event publish/error path around
the event creation call in the baremetalhost controller to stop including the
event message field and rely on the event reason plus error details only. Keep
the existing reqLogger.V(...).Info calls and LogFieldReason/LogFieldError usage,
but remove any direct logging of event.Message.
- Around line 465-470: The reboot annotation parse-failure handling in
BareMetalHostController is logging a user-controlled payload, so remove the raw
annotation value from both the InvalidAnnotationValue event and the info.log
call. Keep the failure context in the same error path by reporting that the
reboot annotation was invalid JSON and that soft-reboot is assumed, but only
include non-sensitive metadata such as the parse error from json.Unmarshal and
omit annotation content entirely.

In `@internal/controller/metal3.io/host_state_machine.go`:
- Line 590: Keep the retry-exhaustion cleanup/power-off messages in
HostStateMachine at info level, not debug. In host_state_machine.go, update the
logging in the cleanup/power-off retry paths around the existing info.log.Info
calls so the “giving up after 3 attempts” and similar fallback notices remain
visible in production, and do not move them to V(VerbosityLevelDebug).

In `@internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go`:
- Around line 120-124: The Available-state gate in the externall yProvisioned
validation is using the wrong boolean condition and only rejects when both old
and new provisioning states are non-Available. Update the check in
baremetalhost_validation.go so the validation in the same code path as the
StateAvailable comparison requires both oldObj.Status.Provisioning.State and
newObj.Status.Provisioning.State to be StateAvailable before allowing the field
change. Keep the existing error path in the validation function, but adjust the
condition and any error context to reflect the actual state being validated.
- Around line 107-110: The BMC address update guard in validateBMCAddressChange
is using the wrong registering check: it currently allows the update when only
one side is Registering because the oldObj.Status.Provisioning.State and
newObj.Status.Provisioning.State checks are combined with &&. Change this
condition so the update is allowed only when both the old and new objects are in
the Registering state, while still preserving the existing detached-state
exception in the same validation path.

---

Outside diff comments:
In `@hack/ci-e2e.sh`:
- Around line 102-123: The boot artifact downloads in ci-e2e.sh are trusted at
fetch time but not verified, so pin both the SystemRescue ISO and the IPA
tarball by digest. Update the download flow around IMAGE_FILE, ISO_FILE,
IPA_FILE, and IPA_BASEURI to add fixed SHA256 values and verify each file after
wget completes before it is used in e2e boot tests. Ensure the script fails fast
if the checksum does not match, rather than proceeding with an unverified
artifact.

🪄 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: Enterprise

Run ID: 78269839-2001-4195-9785-5766978616ba

📥 Commits

Reviewing files that changed from the base of the PR and between 5d3be93 and 7934216.

⛔ Files ignored due to path filters (105)

• apis/go.sum is excluded by !**/*.sum
• apis/vendor/modules.txt is excluded by !**/vendor/**
• go.sum is excluded by !**/*.sum
• hack/tools/go.sum is excluded by !**/*.sum
• hack/tools/vendor/modules.txt is excluded by !**/vendor/**
• test/go.sum is excluded by !**/*.sum
• test/vendor/github.com/moby/moby/api/types/image/attestation.go is excluded by !**/vendor/**
• test/vendor/github.com/moby/moby/client/client.go is excluded by !**/vendor/**
• test/vendor/github.com/moby/moby/client/client_interfaces.go is excluded by !**/vendor/**
• test/vendor/github.com/moby/moby/client/image_attestations.go is excluded by !**/vendor/**
• test/vendor/github.com/moby/moby/client/image_attestations_opts.go is excluded by !**/vendor/**
• test/vendor/github.com/onsi/ginkgo/v2/CHANGELOG.md is excluded by !**/vendor/**
• test/vendor/github.com/onsi/ginkgo/v2/README.md is excluded by !**/vendor/**
• test/vendor/github.com/onsi/ginkgo/v2/core_dsl.go is excluded by !**/vendor/**
• test/vendor/github.com/onsi/ginkgo/v2/ginkgo/run/run_command.go is excluded by !**/vendor/**
• test/vendor/github.com/onsi/ginkgo/v2/ginkgo/watch/watch_command.go is excluded by !**/vendor/**
• test/vendor/github.com/onsi/ginkgo/v2/internal/global/init.go is excluded by !**/vendor/**
• test/vendor/github.com/onsi/ginkgo/v2/internal/suite.go is excluded by !**/vendor/**
• test/vendor/github.com/onsi/ginkgo/v2/reporters/default_reporter.go is excluded by !**/vendor/**
• test/vendor/github.com/onsi/ginkgo/v2/types/config.go is excluded by !**/vendor/**
• test/vendor/github.com/onsi/ginkgo/v2/types/errors.go is excluded by !**/vendor/**
• test/vendor/github.com/onsi/ginkgo/v2/types/version.go is excluded by !**/vendor/**
• test/vendor/github.com/onsi/gomega/CHANGELOG.md is excluded by !**/vendor/**
• test/vendor/github.com/onsi/gomega/README.md is excluded by !**/vendor/**
• test/vendor/github.com/onsi/gomega/gomega_dsl.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/crypto/cryptobyte/asn1.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/crypto/cryptobyte/asn1/asn1.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/crypto/cryptobyte/builder.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/crypto/cryptobyte/string.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/crypto/ssh/channel.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/crypto/ssh/client.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/crypto/ssh/client_auth.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/crypto/ssh/connection.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/crypto/ssh/control.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/crypto/ssh/kex.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/crypto/ssh/keys.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/crypto/ssh/mux.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/crypto/ssh/server.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/crypto/ssh/session.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/net/html/entity.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/net/html/escape.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/net/html/foreign.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/net/html/parse.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/net/html/token.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/net/http2/server_wrap.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/net/http2/transport_wrap.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/sync/errgroup/errgroup.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/tools/go/ast/edge/edge.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/tools/go/packages/golist.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/tools/go/packages/packages.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/tools/go/types/objectpath/objectpath.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/tools/internal/gcimporter/ureader.go is excluded by !**/vendor/**
• test/vendor/golang.org/x/tools/internal/gocommand/version.go is excluded by !**/vendor/**
• test/vendor/k8s.io/apimachinery/pkg/api/validation/objectmeta.go is excluded by !**/vendor/**
• test/vendor/modules.txt is excluded by !**/vendor/**
• test/vendor/sigs.k8s.io/cluster-api/internal/controllers/topology/machineset/machineset_controller.go is excluded by !**/vendor/**
• test/vendor/sigs.k8s.io/cluster-api/test/framework/bootstrap/kind_provider.go is excluded by !**/vendor/**
• test/vendor/sigs.k8s.io/cluster-api/test/infrastructure/kind/mapper.go is excluded by !**/vendor/**
• vendor/github.com/onsi/ginkgo/v2/CHANGELOG.md is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/onsi/ginkgo/v2/README.md is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/onsi/ginkgo/v2/core_dsl.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/onsi/ginkgo/v2/ginkgo/run/run_command.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/onsi/ginkgo/v2/ginkgo/watch/watch_command.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/onsi/ginkgo/v2/internal/global/init.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/onsi/ginkgo/v2/internal/suite.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/onsi/ginkgo/v2/reporters/default_reporter.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/onsi/ginkgo/v2/types/config.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/onsi/ginkgo/v2/types/errors.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/onsi/ginkgo/v2/types/version.go is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/onsi/gomega/CHANGELOG.md is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/onsi/gomega/README.md is excluded by !**/vendor/**, !vendor/**
• vendor/github.com/onsi/gomega/gomega_dsl.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/net/html/entity.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/net/html/escape.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/net/html/foreign.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/net/html/parse.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/net/html/token.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/net/http2/server_wrap.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/net/http2/transport_wrap.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/sync/errgroup/errgroup.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/sync/singleflight/singleflight.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/sys/unix/ztypes_linux.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/sys/unix/ztypes_linux_386.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/sys/unix/ztypes_linux_amd64.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/sys/unix/ztypes_linux_arm.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/sys/unix/ztypes_linux_arm64.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/sys/unix/ztypes_linux_loong64.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/sys/unix/ztypes_linux_mips.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/sys/unix/ztypes_linux_mips64.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/sys/unix/ztypes_linux_mips64le.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/sys/unix/ztypes_linux_mipsle.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/sys/unix/ztypes_linux_ppc.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/sys/unix/ztypes_linux_ppc64.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/sys/unix/ztypes_linux_ppc64le.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/sys/unix/ztypes_linux_riscv64.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/sys/unix/ztypes_linux_s390x.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/sys/unix/ztypes_linux_sparc64.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/tools/go/ast/edge/edge.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/tools/go/packages/golist.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/tools/go/packages/packages.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/tools/go/types/objectpath/objectpath.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/tools/internal/gcimporter/ureader.go is excluded by !**/vendor/**, !vendor/**
• vendor/golang.org/x/tools/internal/gocommand/version.go is excluded by !**/vendor/**, !vendor/**
• vendor/k8s.io/apimachinery/pkg/api/validation/objectmeta.go is excluded by !**/vendor/**, !vendor/**
• vendor/modules.txt is excluded by !**/vendor/**, !vendor/**

📒 Files selected for processing (25)

• .zizmor.yml
• apis/go.mod
• config/base/rbac/role.yaml
• go.mod
• hack/ci-e2e.sh
• hack/tools/go.mod
• internal/controller/metal3.io/action_result.go
• internal/controller/metal3.io/baremetalhost_controller.go
• internal/controller/metal3.io/baremetalhost_controller_test.go
• internal/controller/metal3.io/host_config_data.go
• internal/controller/metal3.io/host_state_machine.go
• internal/controller/metal3.io/hostfirmwarecomponents_controller.go
• internal/controller/metal3.io/hostfirmwaresettings_controller.go
• internal/controller/metal3.io/logging.go
• internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go
• internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation_test.go
• main.go
• main_test.go
• pkg/provisioner/ironic/ironic.go
• pkg/provisioner/ironic/provision_test.go
• releasenotes/v0.12.5.md
• releasenotes/v0.13.1.md
• test/go.mod
• test/vbmctl/pkg/libvirt/templates/VM.xml.tpl
• tools/run_local_ironic.sh

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Avoid emitting the raw reboot annotation value.

The annotation value is user-controlled and currently goes to both the Event message and controller log. Keep the parse failure context without storing the raw payload.

Proposed fix

-		info.publishEvent("InvalidAnnotationValue", fmt.Sprintf("could not parse reboot annotation (%s) - invalid json, assuming soft-reboot", annotation))
+		info.publishEvent("InvalidAnnotationValue", "could not parse reboot annotation - invalid json, assuming soft-reboot")
 		info.log.Info("could not parse reboot annotation, assuming soft-reboot",
-			LogFieldAnnotationValue, annotation,
 			LogFieldError, err.Error())

As per coding guidelines, **/*.{go,js,ts,java,py,rb,cs,cpp,c} must flag logging that may expose passwords, tokens, PII, internal hostnames, or customer data.

📝 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

	err := json.Unmarshal([]byte(annotation), &result)
	if err != nil {
	info.publishEvent("InvalidAnnotationValue", fmt.Sprintf("could not parse reboot annotation (%s) - invalid json, assuming soft-reboot", annotation))
	info.log.Info(fmt.Sprintf("Could not parse reboot annotation (%q) - invalid json, assuming soft-reboot", annotation))
	info.log.Info("could not parse reboot annotation, assuming soft-reboot",
	LogFieldAnnotationValue, annotation,
	LogFieldError, err.Error())
	err := json.Unmarshal([]byte(annotation), &result)
	if err != nil {
	info.publishEvent("InvalidAnnotationValue", "could not parse reboot annotation - invalid json, assuming soft-reboot")
	info.log.Info("could not parse reboot annotation, assuming soft-reboot",
	LogFieldError, err.Error())

🤖 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 465 -
470, The reboot annotation parse-failure handling in BareMetalHostController is
logging a user-controlled payload, so remove the raw annotation value from both
the InvalidAnnotationValue event and the info.log call. Keep the failure context
in the same error path by reporting that the reboot annotation was invalid JSON
and that soft-reboot is assumed, but only include non-sensitive metadata such as
the parse error from json.Unmarshal and omit annotation content entirely.

Source: Coding guidelines

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Do not log the DataImage URL.

DataImage.Spec.URL can contain internal hostnames or signed URL credentials. Log the DataImage object identity instead.

Proposed fix

 			info.log.V(VerbosityLevelDebug).Info("attaching DataImage",
-				LogFieldDataImage, requestedURL)
+				"dataImageName", dataImage.Name)

 		info.log.Info("error while attaching DataImage",
-			LogFieldDataImage, dataImage.Spec.URL,
+			"dataImageName", dataImage.Name,
 			LogFieldError, err.Error())

As per coding guidelines, **/*.{go,js,ts,java,py,rb,cs,cpp,c} must flag logging that may expose passwords, tokens, PII, internal hostnames, or customer data.

Also applies to: 1951-1954

🤖 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 1908
- 1909, The DataImage attachment logs in baremetalhost_controller should not
emit requestedURL/DataImage.Spec.URL because it may expose internal hostnames or
signed credentials. Update the logging in the DataImage attach flow around the
info.log.V(...).Info calls to log the DataImage object identity instead (for
example name/namespace or another non-sensitive identifier) and remove the URL
from the message/fields. Apply the same fix to the related DataImage logging
block later in the controller so all DataImage log statements use the safe
identifier consistently.

Source: Coding guidelines

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Avoid duplicating raw Event messages into logs.

Event messages can include provisioner errors and user-supplied values. Logging the reason and error is enough here.

Proposed fix

 	reqLogger.V(VerbosityLevelDebug).Info("publishing event",
-		LogFieldReason, event.Reason,
-		"message", event.Message)
+		LogFieldReason, event.Reason)
 	err := r.Create(ctx, &event)
 	if err != nil {
 		reqLogger.V(VerbosityLevelDebug).Info("failed to record event, ignoring",
 			LogFieldReason, event.Reason,
-			"message", event.Message,
 			LogFieldError, err)
 	}

As per coding guidelines, **/*.{go,js,ts,java,py,rb,cs,cpp,c} must flag logging that may expose passwords, tokens, PII, internal hostnames, or customer data.

📝 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

	reqLogger.V(VerbosityLevelDebug).Info("publishing event",
	LogFieldReason, event.Reason,
	"message", event.Message)
	err := r.Create(ctx, &event)
	if err != nil {
	reqLogger.Info("failed to record event, ignoring",
	"reason", event.Reason, "message", event.Message, "error", err)
	reqLogger.V(VerbosityLevelDebug).Info("failed to record event, ignoring",
	LogFieldReason, event.Reason,
	"message", event.Message,
	LogFieldError, err)
	reqLogger.V(VerbosityLevelDebug).Info("publishing event",
	LogFieldReason, event.Reason)
	err := r.Create(ctx, &event)
	if err != nil {
	reqLogger.V(VerbosityLevelDebug).Info("failed to record event, ignoring",
	LogFieldReason, event.Reason,
	LogFieldError, 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 `@internal/controller/metal3.io/baremetalhost_controller.go` around lines 2607
- 2615, The event logging in BareMetalHost controller is duplicating the raw
Event message, which can expose sensitive provisioner or user-supplied data.
Update the logging in the event publish/error path around the event creation
call in the baremetalhost controller to stop including the event message field
and rely on the event reason plus error details only. Keep the existing
reqLogger.V(...).Info calls and LogFieldReason/LogFieldError usage, but remove
any direct logging of event.Message.

Source: Coding guidelines

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Keep retry-exhaustion logs at Info.

These are the only visible signals that cleanup/power-off was abandoned and the host may still need manual intervention. Moving them to V(VerbosityLevelDebug) makes that fallback easy to miss in production.

🔧 Proposed fix

-			info.log.V(VerbosityLevelDebug).Info("giving up on host clean up after 3 attempts, the host may still be operational and cause issues in your clusters - clean it up manually")
+			info.log.Info("giving up on host clean up after 3 attempts, the host may still be operational and cause issues in your clusters - clean it up manually")
...
-			info.log.V(VerbosityLevelDebug).Info("giving up on host power off after 3 attempts")
+			info.log.Info("giving up on host power off after 3 attempts")

Also applies to: 626-626

🤖 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` at line 590, Keep the
retry-exhaustion cleanup/power-off messages in HostStateMachine at info level,
not debug. In host_state_machine.go, update the logging in the cleanup/power-off
retry paths around the existing info.log.Info calls so the “giving up after 3
attempts” and similar fallback notices remain visible in production, and do not
move them to V(VerbosityLevelDebug).

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Require both objects to be Registering here.

Lines 109-110 still use &&, so this rejects the update only when neither object is Registering. If one side is Registering and the other is not, the BMC address change is admitted even though this rule is meant to allow it only while detached or while both sides are in the registering phase.

Proposed fix

  if oldObj.Spec.BMC.Address != "" &&
  	newObj.Spec.BMC.Address != oldObj.Spec.BMC.Address &&
  	oldObj.Status.OperationalStatus != metal3api.OperationalStatusDetached &&
  	newObj.Status.OperationalStatus != metal3api.OperationalStatusDetached &&
- 	oldObj.Status.Provisioning.State != metal3api.StateRegistering &&
- 	newObj.Status.Provisioning.State != metal3api.StateRegistering {
+ 	(oldObj.Status.Provisioning.State != metal3api.StateRegistering ||
+ 		newObj.Status.Provisioning.State != metal3api.StateRegistering) {

📝 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

	oldObj.Status.OperationalStatus != metal3api.OperationalStatusDetached &&
	newObj.Status.OperationalStatus != metal3api.OperationalStatusDetached &&
	oldObj.Status.Provisioning.State != metal3api.StateRegistering &&
	newObj.Status.Provisioning.State != metal3api.StateRegistering {
	oldObj.Status.OperationalStatus != metal3api.OperationalStatusDetached &&
	newObj.Status.OperationalStatus != metal3api.OperationalStatusDetached &&
	(oldObj.Status.Provisioning.State != metal3api.StateRegistering ||
	newObj.Status.Provisioning.State != metal3api.StateRegistering) {

🤖 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/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go` around
lines 107 - 110, The BMC address update guard in validateBMCAddressChange is
using the wrong registering check: it currently allows the update when only one
side is Registering because the oldObj.Status.Provisioning.State and
newObj.Status.Provisioning.State checks are combined with &&. Change this
condition so the update is allowed only when both the old and new objects are in
the Registering state, while still preserving the existing detached-state
exception in the same validation path.

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

The Available gate has the same boolean inversion.

Lines 120-121 only fail when both old and new states are non-Available. That means enabling externallyProvisioned still slips through when exactly one side is Available, which is weaker than the “old and new must both be Available” behavior described for this change.

Proposed fix

  if !oldObj.Spec.ExternallyProvisioned && newObj.Spec.ExternallyProvisioned &&
- 	oldObj.Status.Provisioning.State != metal3api.StateAvailable &&
- 	newObj.Status.Provisioning.State != metal3api.StateAvailable {
+ 	(oldObj.Status.Provisioning.State != metal3api.StateAvailable ||
+ 		newObj.Status.Provisioning.State != metal3api.StateAvailable) {

📝 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

	oldObj.Status.Provisioning.State != metal3api.StateAvailable &&
	newObj.Status.Provisioning.State != metal3api.StateAvailable {
	errs = append(errs, fmt.Errorf(
	"externallyProvisioned can only be enabled when in Available state, currently in %s",
	newObj.Status.Provisioning.State))
	oldObj.Status.Provisioning.State))
	if !oldObj.Spec.ExternallyProvisioned && newObj.Spec.ExternallyProvisioned &&
	(oldObj.Status.Provisioning.State != metal3api.StateAvailable ||
	newObj.Status.Provisioning.State != metal3api.StateAvailable) {
	errs = append(errs, fmt.Errorf(
	"externallyProvisioned can only be enabled when in Available state, currently in %s",
	oldObj.Status.Provisioning.State))

🤖 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/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go` around
lines 120 - 124, The Available-state gate in the externall yProvisioned
validation is using the wrong boolean condition and only rejects when both old
and new provisioning states are non-Available. Update the check in
baremetalhost_validation.go so the validation in the same code path as the
StateAvailable comparison requires both oldObj.Status.Provisioning.State and
newObj.Status.Provisioning.State to be StateAvailable before allowing the field
change. Keep the existing error path in the validation function, but adjust the
condition and any error context to reflect the actual state being validated.

[dtantsur]

/test e2e-metal-ipi-virtualmedia

[openshift-ci]

@dtantsur: The following test 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/e2e-metal-ipi-virtualmedia	7934216	link	true	/test e2e-metal-ipi-virtualmedia

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.

[mabulgu]

Superseded by #513 which includes all these commits plus newer upstream changes.

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