🌱 Add validation in webhook for live-iso #2625

nerok · 2025-08-21T21:18:17Z

What this PR does / why we need it:
Add a validation for live-iso to require a bmc protocol (that is the part in the address before "://") which includes "virtualmedia", which seems to be what the requirement for live-isos are according to the docs: https://book.metal3.io/bmo/live-iso

This relates to #855

metal3-io-bot · 2025-08-21T21:18:23Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign elfosardo 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

metal3-io-bot · 2025-08-21T21:18:27Z

Hi @nerok. Thanks for your PR.

I'm waiting for a metal3-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

tuminoid · 2025-08-22T05:28:10Z

/ok-to-test

tuminoid

Thanks for the contribution! Some minor comments left.

internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go

tuminoid · 2025-08-22T07:22:46Z

internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go

+	if image != nil &&
+		image.DiskFormat != nil &&
+		*image.DiskFormat == "live-iso" &&
+		!strings.Contains(strings.Split(bmcAddress, "://")[0], "virtualmedia") {


Can we use something like hasPrefix("redfish-virtualmedia://") , seems more robust?

Do we have a list somewhere over prefixes? because I found these in the docs:

redfish-virtualmedia://

idrac-virtualmedia://

ilo5-virtualmedia://

ilo4-virtualmedia://

and all of there can also have +http or +https.
I can probably use hasPrefix instead yeah

If you can make it work cleanly, sure, but if it gets complicated, then the current one is fine as well.

I think another look at this improved it a lot.
Still got the "contains", but I believe the right way there would be to add SupportsVirtualmedia() to the BMCs, and call those.
I could give that a go, but it is a bit outside of the scope of this PR, and I'm uncertain if we would actually need it anywhere else.

I'm good with this iteration.

tuminoid

/lgtm
/cc @zaneb @elfosardo @dtantsur

zaneb · 2025-08-26T10:45:44Z

internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go

 		errs = append(errs, fmt.Errorf("BMC driver %s does not support secure boot", bmcAccess.Type()))
 	}

+	if s.Image != nil && s.Image.DiskFormat != nil && *s.Image.DiskFormat == "live-iso" && !strings.Contains(bmcAccess.Type(), "virtualmedia") {


I'm not sure it's safe to make this assumption about the driver name.

I would expect that in practice bmcAccess.SupportsISOPreprovisioningImage() corresponds to exactly what we want to know. @dtantsur can you confirm?

That worked locally atleast, pushed it up in a new version

@zaneb @dtantsur can you check if you're happy with latest version?

/cc @zaneb @dtantsur

/lgtm
The one where this gives a different answer is the UEFI HTTP driver, but I am not good enough at tracing the code through ironic to say one way or the other there whether live-iso should be allowed there. (If I had to guess though, I'd say yes and that this version is correct.)

Signed-off-by: Didrik Frimann Barroso Koren <[email protected]>

dtantsur · 2025-09-13T16:27:11Z

internal/webhooks/metal3.io/v1alpha1/baremetalhost_validation.go

 		errs = append(errs, fmt.Errorf("BMC driver %s does not support secure boot", bmcAccess.Type()))
 	}

+	if s.Image != nil && s.Image.DiskFormat != nil && *s.Image.DiskFormat == "live-iso" && !bmcAccess.SupportsISOPreprovisioningImage() {


We tried this, and people complained. The thing is: technically you can boot an ISO over iPXE. It does not work with all ISO's, but apparently does work with some.

Am I understanding you correct in that we shouldn't have this validation at all? Or is there a part of this validation that could be valuable? If not we should ensure it is documented, and I would think a test would be nice to ensure we don't break anything at a later point?

metal3-io-bot · 2025-09-24T20:15:57Z

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.

metal3-io-bot requested review from iurygregory and kashifest August 21, 2025 21:18

metal3-io-bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 21, 2025

metal3-io-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 22, 2025

tuminoid reviewed Aug 22, 2025

View reviewed changes

nerok force-pushed the validation_tests branch from bd5c79c to 33cb3ba Compare August 22, 2025 15:35

metal3-io-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 22, 2025

tuminoid mentioned this pull request Aug 25, 2025

Validating Webhook for BareMetalHost #855

Open

21 tasks

tuminoid reviewed Aug 25, 2025

View reviewed changes

metal3-io-bot requested review from dtantsur, elfosardo and zaneb August 25, 2025 09:16

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 25, 2025

zaneb reviewed Aug 26, 2025

View reviewed changes

Add validation in webhook for live-iso

0384268

Signed-off-by: Didrik Frimann Barroso Koren <[email protected]>

nerok force-pushed the validation_tests branch from 33cb3ba to 0384268 Compare August 26, 2025 12:45

metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 26, 2025

metal3-io-bot requested a review from zaneb September 3, 2025 06:45

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 4, 2025

dtantsur requested changes Sep 13, 2025

View reviewed changes

metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Sep 24, 2025

🌱 Add validation in webhook for live-iso #2625

Are you sure you want to change the base?

🌱 Add validation in webhook for live-iso #2625

Uh oh!

Conversation

nerok commented Aug 21, 2025

Uh oh!

metal3-io-bot commented Aug 21, 2025

Uh oh!

metal3-io-bot commented Aug 21, 2025

Uh oh!

tuminoid commented Aug 22, 2025

Uh oh!

tuminoid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nerok Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tuminoid left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metal3-io-bot commented Sep 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nerok Aug 22, 2025 •

edited

Loading