OCPBUGS-62445: prevent crash when rootDeviceHints format is invalid #2109

mhanss · 2025-09-30T11:21:52Z

This PR adds validation for the --root-device-hint flag in the Create Node Image command.
Previously, providing an invalid format would cause the command to crash with a stack trace.

openshift-ci-robot · 2025-09-30T11:21:57Z

@mhanss: This pull request references Jira Issue OCPBUGS-62445, which is invalid:

expected the bug to target the "4.21.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

This PR adds validation for the --root-device-hint flag in the Create Node Image command.
Previously, providing an invalid format would cause the command to crash with a stack trace.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

coderabbitai · 2025-09-30T11:22:00Z

Walkthrough

Adds input validation for the rootDeviceHints flag in the single-node path and introduces a corresponding negative test to ensure malformed values (not in key:value format) produce an error.

Changes

Cohort / File(s)	Summary
Implementation: rootDeviceHints validation `pkg/cli/admin/nodeimage/create.go`	Splits RootDeviceHints on ":" and returns an error unless exactly two parts are present; otherwise proceeds to populate rootDeviceHints as before.
Tests: invalid input case `pkg/cli/admin/nodeimage/create_test.go`	Adds a test case asserting that an improperly formatted RootDeviceHints ("/dev/sda") triggers the expected error path and message.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title clearly summarizes the primary change by stating the prevention of a crash when the rootDeviceHints flag format is invalid, which directly reflects the added validation logic in the pull request. It is concise, specific, and avoids extraneous details, making it easy for reviewers to understand the main purpose at a glance.
Description Check	✅ Passed	The description succinctly explains that the PR adds validation for the --root-device-hint flag in the Create Node Image command and notes the previous crash behavior, directly aligning with the changes in the code and tests. It is clearly related to the modifications and provides enough context for reviewers to grasp the intent without being overly generic.

✨ Finishing touches

📝 Generate Docstrings

🧪 Generate unit tests

Create PR with unit tests
Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

🧪 Early access (Sonnet 4.5): enabled

We are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience.

Note:

Public repositories are always opted into early access features.
You can enable or disable early access features from the CodeRabbit UI or by updating the CodeRabbit configuration file.

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

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

pkg/cli/admin/nodeimage/create.go (1)
636-640: Consider validating that both key and value are non-empty.

The current validation correctly checks for the presence of the ":" separator, but it doesn't prevent edge cases like:

":" → empty key and value

":value" → empty key

"key:" → empty value

These would pass validation but create malformed rootDeviceHints entries.

Consider adding validation to ensure both parts are non-empty:
 	if o.SingleNodeOpts.RootDeviceHints != "" {
 		parts := strings.SplitN(o.SingleNodeOpts.RootDeviceHints, ":", 2)
 		if len(parts) != 2 {
 			return nil, fmt.Errorf("incorrect rootDeviceHints format provided: %s. Expected format: <hint name>:<value>, for example: deviceName:/dev/sda", o.SingleNodeOpts.RootDeviceHints)
 		}
+		if strings.TrimSpace(parts[0]) == "" || strings.TrimSpace(parts[1]) == "" {
+			return nil, fmt.Errorf("incorrect rootDeviceHints format provided: %s. Both hint name and value must be non-empty", o.SingleNodeOpts.RootDeviceHints)
+		}
 		host["rootDeviceHints"] = map[string]interface{}{
 			parts[0]: parts[1],
 		}
 	}

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between a43ea28 and bd888e5.

📒 Files selected for processing (2)

pkg/cli/admin/nodeimage/create.go (1 hunks)
pkg/cli/admin/nodeimage/create_test.go (1 hunks)

🔇 Additional comments (1)

pkg/cli/admin/nodeimage/create_test.go (1)

665-671: LGTM! Test case correctly validates the error path.

The test case appropriately validates that an invalid rootDeviceHints format (missing the ":" separator) produces the expected error message. The test is well-isolated, only populating the RootDeviceHints field to ensure it specifically tests the format validation.

andfasano · 2025-09-30T13:09:43Z

pkg/cli/admin/nodeimage/create.go

 	}
 	if o.SingleNodeOpts.RootDeviceHints != "" {
 		parts := strings.SplitN(o.SingleNodeOpts.RootDeviceHints, ":", 2)
+		if len(parts) != 2 {


In general we don't want to duplicate the node-joiner logic here, since the oc command must be as much as possible a simple wrapper. So, if there is an issue it must be fixed on the node-joiner side, and reuse the already existing workflow for reporting back the error to the user

The format of the CLI arg is effectively already defined here (on the previous line) isn't it? And node-joiner uses a different format (proper yaml).

That's perfectly right, I misread the original issue, this is an oc interface and not related to node-joiner

/approve

mhanss · 2025-10-01T10:31:28Z

/verified bypass
/jira refresh

openshift-ci-robot · 2025-10-01T10:31:36Z

@mhanss: This pull request references Jira Issue OCPBUGS-62445, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.21.0) matches configured target version for branch (4.21.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @mhanss

In response to this:

/verified bypass
/jira refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2025-10-01T10:31:42Z

@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: mhanss.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

@mhanss: This pull request references Jira Issue OCPBUGS-62445, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)

bug target version (4.21.0) matches configured target version for branch (4.21.0)

bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @mhanss

In response to this:

/verified bypass
/jira refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

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.

mhanss · 2025-10-01T10:32:09Z

/retest

mhanss · 2025-10-01T14:54:20Z

/test e2e-aws-ovn-serial-2of2

mhanss · 2025-10-01T14:54:39Z

/verified bypass

openshift-ci-robot · 2025-10-01T14:54:52Z

@mhanss: The verified label has been added.

In response to this:

/verified bypass

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

bmanzari · 2025-10-06T08:59:13Z

/lgtm

openshift-ci · 2025-10-06T08:59:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andfasano, bmanzari, mhanss

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/cli/admin/nodeimage/OWNERS~~ [andfasano]

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

openshift-ci · 2025-10-06T12:54:14Z

@mhanss: 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/okd-scos-e2e-aws-ovn	`bd888e5`	link	false	`/test okd-scos-e2e-aws-ovn`

Full PR test history. Your PR dashboard.

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.

openshift-ci-robot · 2025-10-06T13:47:10Z

/retest-required

Remaining retests: 0 against base HEAD 6632635 and 2 for PR HEAD bd888e5 in total

openshift-ci-robot · 2025-10-06T21:50:57Z

@mhanss: Jira Issue Verification Checks: Jira Issue OCPBUGS-62445
✔️ This pull request was pre-merge verified.
✔️ All associated pull requests have merged.
✔️ All associated, merged pull requests were pre-merge verified.

Jira Issue OCPBUGS-62445 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓

In response to this:

This PR adds validation for the --root-device-hint flag in the Create Node Image command.
Previously, providing an invalid format would cause the command to crash with a stack trace.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-merge-robot · 2025-10-07T17:15:28Z

Fix included in accepted release 4.21.0-0.nightly-2025-10-07-102242

fix(node-image): prevent crash when rootDeviceHints format is invalid

bd888e5

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Sep 30, 2025

openshift-ci-robot added the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Sep 30, 2025

openshift-ci bot requested review from andfasano and bfournie September 30, 2025 11:22

coderabbitai bot reviewed Sep 30, 2025

View reviewed changes

andfasano reviewed Sep 30, 2025

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 1, 2025

openshift-ci-robot added the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label Oct 1, 2025

openshift-ci-robot removed the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Oct 1, 2025

openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Oct 1, 2025

openshift-ci bot assigned bmanzari Oct 6, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 6, 2025

openshift-merge-bot bot merged commit 229ead6 into openshift:main Oct 6, 2025
18 of 19 checks passed

OCPBUGS-62445: prevent crash when rootDeviceHints format is invalid #2109

OCPBUGS-62445: prevent crash when rootDeviceHints format is invalid #2109

Conversation

mhanss commented Sep 30, 2025

Uh oh!

openshift-ci-robot commented Sep 30, 2025

Uh oh!

coderabbitai bot commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

andfasano Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

zaneb Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

andfasano Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

mhanss commented Oct 1, 2025

Uh oh!

openshift-ci-robot commented Oct 1, 2025

Uh oh!

openshift-ci bot commented Oct 1, 2025

Uh oh!

mhanss commented Oct 1, 2025

Uh oh!

mhanss commented Oct 1, 2025

Uh oh!

mhanss commented Oct 1, 2025

Uh oh!

openshift-ci-robot commented Oct 1, 2025

Uh oh!

bmanzari commented Oct 6, 2025

Uh oh!

openshift-ci bot commented Oct 6, 2025

Uh oh!

openshift-ci bot commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Oct 6, 2025

Uh oh!

Uh oh!

openshift-ci-robot commented Oct 6, 2025

Uh oh!

openshift-merge-robot commented Oct 7, 2025

Uh oh!

Uh oh!

coderabbitai bot commented Sep 30, 2025 •

edited

Loading

openshift-ci bot commented Oct 6, 2025 •

edited

Loading