From 025260f60402078735a6ab310599cc713f2a9e3e Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Fri, 23 Sep 2022 09:15:02 -0400 Subject: [PATCH 1/2] builder: call Comm.Prepare before resetting host A regression was introduced in the plugin in commit f65be3cb, where the logic for detecting a missing host, and falling back to localhost used to work with SSH only, and was generalised through the `Host()' method to work with WinRM too. This logic however was flawed, as it required an explicit Type for the communicator to work. This is the case for winrm, but ssh being the default didn't need to, leading to the case where we resetted the host to 127.0.0.1. To prevent this, we ensure the Type is properly set, which is done in the `c.Comm.Prepare` function, from the packer SDK. This in turn makes `Host()` report the right host for implicit ssh connections, and not an empty one as previously would happen. --- builder/qemu/comm_config.go | 13 +++++---- builder/qemu/comm_config_test.go | 48 ++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 5 deletions(-) diff --git a/builder/qemu/comm_config.go b/builder/qemu/comm_config.go index befb618..adde975 100644 --- a/builder/qemu/comm_config.go +++ b/builder/qemu/comm_config.go @@ -3,6 +3,7 @@ package qemu import ( "errors" + "log" "github.com/hashicorp/packer-plugin-sdk/communicator" "github.com/hashicorp/packer-plugin-sdk/template/interpolate" @@ -46,11 +47,6 @@ func (c *CommConfig) Prepare(ctx *interpolate.Context) (warnings []string, errs c.HostPortMax = c.SSHHostPortMax } - if c.Comm.Host() == "" && c.SkipNatMapping { - c.Comm.SSHHost = "127.0.0.1" - c.Comm.WinRMHost = "127.0.0.1" - } - if c.HostPortMin == 0 { c.HostPortMin = 2222 } @@ -60,6 +56,13 @@ func (c *CommConfig) Prepare(ctx *interpolate.Context) (warnings []string, errs } errs = c.Comm.Prepare(ctx) + + if c.Comm.Host() == "" && c.SkipNatMapping { + log.Printf("Resetting ssh host to 127.0.0.1") + c.Comm.SSHHost = "127.0.0.1" + c.Comm.WinRMHost = "127.0.0.1" + } + if c.HostPortMin > c.HostPortMax { errs = append(errs, errors.New("host_port_min must be less than host_port_max")) diff --git a/builder/qemu/comm_config_test.go b/builder/qemu/comm_config_test.go index 44eeee2..49a2741 100644 --- a/builder/qemu/comm_config_test.go +++ b/builder/qemu/comm_config_test.go @@ -70,6 +70,54 @@ func TestCommConfigPrepare_SSHHostPort(t *testing.T) { if len(warns) != 0 { t.Fatal("should not have any warnings") } + + tc := []struct { + name string + host, expectedHost string + skipNat bool + }{ + { + name: "skip_nat_mapping false should not change host", + host: "192.168.1.1", + expectedHost: "192.168.1.1", + }, + { + name: "skip_nat_mapping true should not change host", + host: "192.168.1.1", + expectedHost: "192.168.1.1", + skipNat: true, + }, + { + name: "skip_nat_mapping true with no set host", + expectedHost: "127.0.0.1", + skipNat: true, + }, + } + + for _, tt := range tc { + tt := tt + t.Run(tt.name, func(t *testing.T) { + c := &CommConfig{ + SkipNatMapping: tt.skipNat, + Comm: communicator.Config{ + SSH: communicator.SSH{ + SSHUsername: "tester", + SSHHost: tt.host, + }, + }, + } + warns, errs := c.Prepare(interpolate.NewContext()) + if len(errs) > 0 { + t.Fatalf("should not have error: %s", errs) + } + if len(warns) != 0 { + t.Fatal("should not have any warnings") + } + if c.Comm.SSHHost != tt.expectedHost { + t.Errorf("unexpected SSHHost: wanted: %s, got: %s", tt.expectedHost, c.Comm.SSHHost) + } + }) + } } func TestCommConfigPrepare_SSHPrivateKey(t *testing.T) { From 7bb4a464304f426f2761e5ae004fb92b0223f741 Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Fri, 23 Sep 2022 09:26:38 -0400 Subject: [PATCH 2/2] builder: remove unncessary fmt.Sprintf --- builder/qemu/step_run.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builder/qemu/step_run.go b/builder/qemu/step_run.go index f15cff8..61d6362 100644 --- a/builder/qemu/step_run.go +++ b/builder/qemu/step_run.go @@ -112,7 +112,7 @@ func (s *stepRun) getDefaultArgs(config *Config, state multistep.StateBag) map[s // Configure "-netdev" arguments defaultArgs["-netdev"] = fmt.Sprintf("bridge,id=user.0,br=%s", config.NetBridge) if config.NetBridge == "" { - defaultArgs["-netdev"] = fmt.Sprintf("user,id=user.0") + defaultArgs["-netdev"] = "user,id=user.0" if config.CommConfig.Comm.Type != "none" { commHostPort := state.Get("commHostPort").(int) defaultArgs["-netdev"] = fmt.Sprintf("user,id=user.0,hostfwd=tcp::%v-:%d", commHostPort, config.CommConfig.Comm.Port())