Skip to content

Commit abeebfe

Browse files
reubenoCopilot
andcommitted
fix(image-boot): address PR review feedback
- Validate '--disk-size' is non-empty when creating an empty disk for ISO boot. - Lock test user password and disable SSH password auth when no password is provided (avoids unlocked account with no defined password). - Use prereqs.RequireExecutable for qemu-img check to give actionable install hints. - Use fileutils.MkdirTempInTempDir when WorkDir is unset so injected filesystems resolve the temp dir consistently. - Add unit tests for InstallISOPath bootindex behavior, CreateEmptyQcow2, and CheckQEMUImgPrerequisite. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent d8dde5c commit abeebfe

File tree

3 files changed

+136
-13
lines changed

3 files changed

+136
-13
lines changed

internal/app/azldev/cmds/image/boot.go

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -330,6 +330,10 @@ func validateBootOptions(options *ImageBootOptions, diskSizeExplicit, needEmptyD
330330
"(IMAGE_NAME or '--image-path')")
331331
}
332332

333+
if needEmptyDisk && strings.TrimSpace(options.DiskSize) == "" {
334+
return errors.New("'--disk-size' must be non-empty when '--iso' is used without a disk image")
335+
}
336+
333337
return nil
334338
}
335339

@@ -509,14 +513,19 @@ func runQEMUBoot(
509513
}
510514

511515
// createBootTempDir creates a temporary directory for boot artifacts. It uses the project
512-
// work directory if available, otherwise falls back to the OS temp directory.
516+
// work directory if available, otherwise falls back to the OS temp directory (resolved
517+
// via the injected filesystem).
513518
func createBootTempDir(env *azldev.Env) (string, error) {
514-
var baseDir string
515519
if env.WorkDir() != "" {
516-
baseDir = env.WorkDir()
520+
tempDir, err := fileutils.MkdirTemp(env.FS(), env.WorkDir(), tempDirPrefixBoot)
521+
if err != nil {
522+
return "", fmt.Errorf("failed to create boot temp dir:\n%w", err)
523+
}
524+
525+
return tempDir, nil
517526
}
518527

519-
tempDir, err := fileutils.MkdirTemp(env.FS(), baseDir, tempDirPrefixBoot)
528+
tempDir, err := fileutils.MkdirTempInTempDir(env.FS(), tempDirPrefixBoot)
520529
if err != nil {
521530
return "", fmt.Errorf("failed to create boot temp dir:\n%w", err)
522531
}
@@ -812,12 +821,17 @@ func buildCloudInitMetadataIso(env *azldev.Env, options *ImageBootOptions, outpu
812821

813822
// buildCloudInitConfig creates the cloud-init configuration for the test user.
814823
func buildCloudInitConfig(env *azldev.Env, options *ImageBootOptions) (*cloudinit.Config, error) {
824+
hasPassword := options.TestUserPassword != ""
825+
815826
testUserConfig := cloudinit.UserConfig{
816-
Name: options.TestUserName,
817-
Description: "Test User",
818-
Shell: "/bin/bash",
819-
Sudo: []string{"ALL=(ALL) NOPASSWD:ALL"},
820-
LockPassword: lo.ToPtr(false),
827+
Name: options.TestUserName,
828+
Description: "Test User",
829+
Shell: "/bin/bash",
830+
Sudo: []string{"ALL=(ALL) NOPASSWD:ALL"},
831+
// Only unlock the password when one is actually provided. Otherwise the
832+
// account would be unlocked with no defined password, which combined with
833+
// SSH password auth could allow passwordless login.
834+
LockPassword: lo.ToPtr(!hasPassword),
821835
PlainTextPassword: options.TestUserPassword,
822836
Groups: []string{"sudo"},
823837
}
@@ -832,7 +846,8 @@ func buildCloudInitConfig(env *azldev.Env, options *ImageBootOptions) (*cloudini
832846
}
833847

834848
return &cloudinit.Config{
835-
EnableSSHPasswordAuth: lo.ToPtr(true),
849+
// Only enable SSH password auth when a password is actually provided.
850+
EnableSSHPasswordAuth: lo.ToPtr(hasPassword),
836851
DisableRootUser: lo.ToPtr(true),
837852
ChangePasswords: &cloudinit.PasswordConfig{
838853
Expire: lo.ToPtr(false),

internal/utils/qemu/qemu.go

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -273,9 +273,11 @@ func CheckPrerequisites(ctx opctx.Ctx, arch string) error {
273273

274274
// CheckQEMUImgPrerequisite verifies that the 'qemu-img' tool is available.
275275
func CheckQEMUImgPrerequisite(ctx opctx.Ctx) error {
276-
if !ctx.CommandInSearchPath("qemu-img") {
277-
return errors.New("'qemu-img' is not installed or not found in PATH; " +
278-
"it is required to create empty disk images for ISO boot")
276+
if err := prereqs.RequireExecutable(ctx, "qemu-img", &prereqs.PackagePrereq{
277+
AzureLinuxPackages: []string{"qemu-img"},
278+
FedoraPackages: []string{"qemu-img"},
279+
}); err != nil {
280+
return fmt.Errorf("'qemu-img' prerequisite check failed:\n%w", err)
279281
}
280282

281283
return nil

internal/utils/qemu/qemu_test.go

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -469,6 +469,44 @@ func TestRun(t *testing.T) {
469469
wantErr: true,
470470
wantErrContain: "failed to run VM in QEMU",
471471
},
472+
{
473+
name: "VM with install ISO boots ISO first",
474+
options: qemu.RunOptions{
475+
Arch: qemu.ArchX86_64,
476+
FirmwarePath: "/usr/share/OVMF/OVMF_CODE.fd",
477+
NVRAMPath: "/tmp/nvram.fd",
478+
DiskPath: "/images/disk.qcow2",
479+
DiskType: "qcow2",
480+
InstallISOPath: "/tmp/installer.iso",
481+
SecureBoot: false,
482+
SSHPort: 2222,
483+
CPUs: 2,
484+
Memory: "2G",
485+
},
486+
wantArgsContain: []string{
487+
"qemu-system-x86_64",
488+
"if=none,id=installcd,file=/tmp/installer.iso,media=cdrom,readonly=on",
489+
"scsi-cd,drive=installcd,bootindex=1",
490+
"scsi-hd,drive=hd,bootindex=2",
491+
},
492+
},
493+
{
494+
name: "VM without install ISO boots disk first",
495+
options: qemu.RunOptions{
496+
Arch: qemu.ArchX86_64,
497+
FirmwarePath: "/usr/share/OVMF/OVMF_CODE.fd",
498+
NVRAMPath: "/tmp/nvram.fd",
499+
DiskPath: "/images/disk.qcow2",
500+
DiskType: "qcow2",
501+
SecureBoot: false,
502+
SSHPort: 2222,
503+
CPUs: 2,
504+
Memory: "2G",
505+
},
506+
wantArgsContain: []string{
507+
"scsi-hd,drive=hd,bootindex=1",
508+
},
509+
},
472510
}
473511

474512
for _, testCase := range tests {
@@ -650,3 +688,71 @@ func argsToString(args []string) string {
650688

651689
return result
652690
}
691+
692+
func TestCreateEmptyQcow2(t *testing.T) {
693+
t.Parallel()
694+
695+
t.Run("invokes qemu-img with expected args", func(t *testing.T) {
696+
t.Parallel()
697+
698+
ctx := testctx.NewCtx()
699+
700+
var capturedArgs []string
701+
702+
ctx.CmdFactory.RunHandler = func(cmd *exec.Cmd) error {
703+
capturedArgs = cmd.Args
704+
705+
return nil
706+
}
707+
708+
runner := qemu.NewRunner(ctx)
709+
err := runner.CreateEmptyQcow2(context.Background(), "/tmp/disk.qcow2", "10G")
710+
require.NoError(t, err)
711+
712+
require.NotEmpty(t, capturedArgs)
713+
assert.Equal(t, "qemu-img", capturedArgs[0])
714+
assert.Equal(t, []string{"qemu-img", "create", "-f", "qcow2", "/tmp/disk.qcow2", "10G"}, capturedArgs)
715+
})
716+
717+
t.Run("propagates errors", func(t *testing.T) {
718+
t.Parallel()
719+
720+
ctx := testctx.NewCtx()
721+
ctx.CmdFactory.RunHandler = func(_ *exec.Cmd) error {
722+
return errors.New("disk full")
723+
}
724+
725+
runner := qemu.NewRunner(ctx)
726+
err := runner.CreateEmptyQcow2(context.Background(), "/tmp/disk.qcow2", "10G")
727+
require.Error(t, err)
728+
assert.Contains(t, err.Error(), "failed to create empty qcow2 disk image")
729+
})
730+
}
731+
732+
func TestCheckQEMUImgPrerequisite(t *testing.T) {
733+
t.Parallel()
734+
735+
t.Run("qemu-img available", func(t *testing.T) {
736+
t.Parallel()
737+
738+
ctx := testctx.NewCtx()
739+
ctx.CmdFactory.RegisterCommandInSearchPath("qemu-img")
740+
ctx.DryRunValue = true
741+
742+
err := qemu.CheckQEMUImgPrerequisite(ctx)
743+
require.NoError(t, err)
744+
})
745+
746+
t.Run("qemu-img missing", func(t *testing.T) {
747+
t.Parallel()
748+
749+
ctx := testctx.NewCtx()
750+
ctx.DryRunValue = true
751+
ctx.PromptsAllowedValue = false
752+
ctx.AllPromptsAcceptedValue = false
753+
754+
err := qemu.CheckQEMUImgPrerequisite(ctx)
755+
require.Error(t, err)
756+
assert.Contains(t, err.Error(), "'qemu-img' prerequisite check failed")
757+
})
758+
}

0 commit comments

Comments
 (0)