From 4f2e9117c3143bf66f7c8f64a47f9952dc8ec320 Mon Sep 17 00:00:00 2001 From: Darwin Liu <76200225+somerandomqaguy@users.noreply.github> Date: Thu, 7 Jan 2021 19:46:07 -0700 Subject: [PATCH 1/3] QEMU: Minor fix to VNC mapping parameters and output * Set the QEMU builder vnc_port_min to have a minimum value of 5900, with updated documentation to explain why. * Changed output messages so that the correct port is listed in packer's messaging to the user. LONGER DESCRIPTION If vnc_min_port is set to anything above 5900, then packer will fail to connect via VNC to the QEMU instance. This is due to how QEMU parses the port for listening to VNC: host:d TCP connections will only be allowed from host on display d. By convention the TCP port is 5900+d. Previously the calculation for the port was vncPort - VNCPortMin, however this will result in an incorrect port being displayed in packer's messages and potentially packer being unable to connect via VNC to the host. For example if vnc_port_min=5990 nad vnc_port_max=5999: ``` Looking for available port between 5990 and 5999 on 0.0.0.0 Found available port: 5996 on IP: 0.0.0.0 Found available VNC port: 5996 on IP: 0.0.0.0 [....] ==> Starting VM, booting disk image view the screen of the VM, connect via VNC without a password to vnc://0.0.0.0:6 ``` This will cause QEMU to set the listening port to 5906 while packer's VNC client is attempting to connect to 5996. I am a touch concerned as this commit undoes pull request #9905 (specfically commit 7335695c), but I am also confused as to how he was able to get QEMU to get a VNC listening port below 5900, as examining QEMU's git history has shown that this behavior has been in since at least 2017, probably older. Hopefully the more explicit error messaging and documentation can make it clear why this is being undone. --- builder/qemu/config.go | 7 +++++++ builder/qemu/step_run.go | 11 ++++++----- .../partials/builder/qemu/Config-not-required.mdx | 2 ++ 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/builder/qemu/config.go b/builder/qemu/config.go index 605f101e9..2fc12ab8c 100644 --- a/builder/qemu/config.go +++ b/builder/qemu/config.go @@ -374,6 +374,8 @@ type Config struct { // the initial boot_command. Because Packer generally runs in parallel, // Packer uses a randomly chosen port in this range that appears available. By // default this is 5900 to 6000. The minimum and maximum ports are inclusive. + // The minimum port cannot be set below 5900 due to a quirk in how QEMU parses + // vnc display address. VNCPortMin int `mapstructure:"vnc_port_min" required:"false"` VNCPortMax int `mapstructure:"vnc_port_max"` // This is the name of the image (QCOW2 or IMG) file for @@ -592,6 +594,11 @@ func (c *Config) Prepare(raws ...interface{}) ([]string, error) { } } + if c.VNCPortMin < 5900 { + errs = packersdk.MultiErrorAppend( + errs, fmt.Errorf("vnc_port_min cannot be below 5900")) + } + if c.VNCPortMin > c.VNCPortMax { errs = packersdk.MultiErrorAppend( errs, fmt.Errorf("vnc_port_min must be less than vnc_port_max")) diff --git a/builder/qemu/step_run.go b/builder/qemu/step_run.go index 4932eaace..5360955f4 100644 --- a/builder/qemu/step_run.go +++ b/builder/qemu/step_run.go @@ -120,17 +120,18 @@ func (s *stepRun) getDefaultArgs(config *Config, state multistep.StateBag) map[s vncPort := state.Get("vnc_port").(int) vncIP := config.VNCBindAddress - vncPort = vncPort - config.VNCPortMin - vnc := fmt.Sprintf("%s:%d", vncIP, vncPort) + vncRealAddress := fmt.Sprintf("%s:%d", vncIP, vncPort) + vncPort = vncPort - 5900 + vncArgs := fmt.Sprintf("%s:%d", vncIP, vncPort) if config.VNCUsePassword { - vnc = fmt.Sprintf("%s:%d,password", vncIP, vncPort) + vncArgs = fmt.Sprintf("%s:%d,password", vncIP, vncPort) } - defaultArgs["-vnc"] = vnc + defaultArgs["-vnc"] = vncArgs // Track the connection for the user vncPass, _ := state.Get("vnc_password").(string) - message = getVncConnectionMessage(config.Headless, vnc, vncPass) + message = getVncConnectionMessage(config.Headless, vncRealAddress, vncPass) if message != "" { s.ui.Message(message) } diff --git a/website/content/partials/builder/qemu/Config-not-required.mdx b/website/content/partials/builder/qemu/Config-not-required.mdx index a5fced8ca..dcd274282 100644 --- a/website/content/partials/builder/qemu/Config-not-required.mdx +++ b/website/content/partials/builder/qemu/Config-not-required.mdx @@ -301,6 +301,8 @@ the initial boot_command. Because Packer generally runs in parallel, Packer uses a randomly chosen port in this range that appears available. By default this is 5900 to 6000. The minimum and maximum ports are inclusive. + The minimum port cannot be set below 5900 due to a quirk in how QEMU parses + vnc display address. - `vnc_port_max` (int) - VNC Port Max From 2b698478a6f62f0ba9c53d53fc1172bb86acb982 Mon Sep 17 00:00:00 2001 From: Darwin Liu <76200225+somerandomqaguy@users.noreply.github> Date: Thu, 7 Jan 2021 21:26:32 -0700 Subject: [PATCH 2/3] Adjusted QEMU step_run_tests.go to reflect previous changes --- builder/qemu/step_run_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/builder/qemu/step_run_test.go b/builder/qemu/step_run_test.go index e74f8b312..533b5317a 100644 --- a/builder/qemu/step_run_test.go +++ b/builder/qemu/step_run_test.go @@ -105,7 +105,7 @@ func Test_UserOverrides(t *testing.T) { "-fda", "fake_floppy_path", "-name", "myvm", "-netdev", "user,id=user.0,hostfwd=tcp::5000-:0", - "-vnc", ":5905", + "-vnc", ":5", "-machine", "type=,accel="}, tc.Expected...) @@ -416,7 +416,7 @@ func Test_DriveAndDeviceArgs(t *testing.T) { "-fda", "fake_floppy_path", "-name", "", "-netdev", "user,id=user.0,hostfwd=tcp::5000-:0", - "-vnc", ":5905", + "-vnc", ":5", "-machine", "type=,accel=", "-device", ",netdev=user.0"}, tc.Expected...) @@ -453,7 +453,7 @@ func Test_OptionalConfigOptionsGetSet(t *testing.T) { "-fda", "fake_floppy_path", "-name", "MyFancyName", "-netdev", "user,id=user.0,hostfwd=tcp::5000-:0", - "-vnc", ":5905,password", + "-vnc", ":5,password", "-machine", "type=pc,accel=hvf", "-device", ",netdev=user.0", "-drive", "file=/path/to/test.iso,index=0,media=cdrom", @@ -596,7 +596,7 @@ func Test_Defaults(t *testing.T) { "vnc_port": 5959, }, &stepRun{ui: packersdk.TestUi(t)}, - []string{"-vnc", "1.1.1.1:5959"}, + []string{"-vnc", "1.1.1.1:59"}, "no VNC password should be set", }, { @@ -608,7 +608,7 @@ func Test_Defaults(t *testing.T) { "vnc_port": 5959, }, &stepRun{ui: packersdk.TestUi(t)}, - []string{"-vnc", "1.1.1.1:5959,password"}, + []string{"-vnc", "1.1.1.1:59,password"}, "VNC password should be set", }, { From 8ef4cfa070573256e9770cce2bbd25a73a32c8c7 Mon Sep 17 00:00:00 2001 From: Darwin Liu <76200225+somerandomqaguy@users.noreply.github> Date: Fri, 8 Jan 2021 13:39:17 -0700 Subject: [PATCH 3/3] Add in a sanity check for valid vnc ports Check to make sure that the max and min VNC ports be below 65535 in config. --- builder/qemu/config.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/builder/qemu/config.go b/builder/qemu/config.go index 2fc12ab8c..29b0c71ff 100644 --- a/builder/qemu/config.go +++ b/builder/qemu/config.go @@ -599,6 +599,11 @@ func (c *Config) Prepare(raws ...interface{}) ([]string, error) { errs, fmt.Errorf("vnc_port_min cannot be below 5900")) } + if c.VNCPortMin > 65535 || c.VNCPortMax > 65535 { + errs = packersdk.MultiErrorAppend( + errs, fmt.Errorf("vmc_port_min and vnc_port_max must both be below 65535 to be valid TCP ports")) + } + if c.VNCPortMin > c.VNCPortMax { errs = packersdk.MultiErrorAppend( errs, fmt.Errorf("vnc_port_min must be less than vnc_port_max"))