From aa3cc81294ca4e5f97f70e442c29df8fc0dd3213 Mon Sep 17 00:00:00 2001
From: Lucas Bajolet <lucas.bajolet@hashicorp.com>
Date: Mon, 11 Jul 2022 16:26:53 -0400
Subject: [PATCH] ssh: only rely on timeout/retries is one is set

In a configuration, when either the SSH timeout or the SSH handshake
retries are set, the other attribute would also be set to its default
value.

This in turn could cause a problem where a user expects his choice to be
the one that makes the final decision to abort, but in the end this
choice could cause the build to be cancelled independently as the other
setting may cause a premature cancellation.

To avoid this, we clarify the documentation, and ensure that when one is
set, the other will be ignored.
---
 .../communicator/SSH-not-required.mdx          |  5 +++--
 communicator/config.go                         | 13 ++++++-------
 communicator/step_connect_ssh.go               | 18 +++++++++++-------
 3 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/cmd/packer-sdc/internal/renderdocs/docs-partials/packer-plugin-sdk/communicator/SSH-not-required.mdx b/cmd/packer-sdc/internal/renderdocs/docs-partials/packer-plugin-sdk/communicator/SSH-not-required.mdx
index cb45d79d5..fcf7181a4 100644
--- a/cmd/packer-sdc/internal/renderdocs/docs-partials/packer-plugin-sdk/communicator/SSH-not-required.mdx
+++ b/cmd/packer-sdc/internal/renderdocs/docs-partials/packer-plugin-sdk/communicator/SSH-not-required.mdx
@@ -44,11 +44,12 @@
 - `ssh_timeout` (duration string | ex: "1h5m2s") - The time to wait for SSH to become available. Packer uses this to
   determine when the machine has booted so this is usually quite long.
   Example value: `10m`.
+  This defaults to `5m`, unless `ssh_handshake_attempts` is set.
 
 - `ssh_disable_agent_forwarding` (bool) - If true, SSH agent forwarding will be disabled. Defaults to `false`.
 
-- `ssh_handshake_attempts` (int) - The number of handshakes to attempt with SSH once it can connect. This
-  defaults to `10`.
+- `ssh_handshake_attempts` (int) - The number of handshakes to attempt with SSH once it can connect.
+  This defaults to `10`, unless a `ssh_timeout` is set.
 
 - `ssh_bastion_host` (string) - A bastion host to use for the actual SSH connection.
 
diff --git a/communicator/config.go b/communicator/config.go
index 53b9ba8e8..1bc2d602e 100644
--- a/communicator/config.go
+++ b/communicator/config.go
@@ -123,6 +123,7 @@ type SSH struct {
 	// The time to wait for SSH to become available. Packer uses this to
 	// determine when the machine has booted so this is usually quite long.
 	// Example value: `10m`.
+	// This defaults to `5m`, unless `ssh_handshake_attempts` is set.
 	SSHTimeout time.Duration `mapstructure:"ssh_timeout"`
 	// Deprecated in favor of SSHTimeout
 	SSHWaitTimeout time.Duration `mapstructure:"ssh_wait_timeout" undocumented:"true"`
@@ -135,8 +136,8 @@ type SSH struct {
 	SSHAgentAuth bool `mapstructure:"ssh_agent_auth" undocumented:"true"`
 	// If true, SSH agent forwarding will be disabled. Defaults to `false`.
 	SSHDisableAgentForwarding bool `mapstructure:"ssh_disable_agent_forwarding"`
-	// The number of handshakes to attempt with SSH once it can connect. This
-	// defaults to `10`.
+	// The number of handshakes to attempt with SSH once it can connect.
+	// This defaults to `10`, unless a `ssh_timeout` is set.
 	SSHHandshakeAttempts int `mapstructure:"ssh_handshake_attempts"`
 	// A bastion host to use for the actual SSH connection.
 	SSHBastionHost string `mapstructure:"ssh_bastion_host"`
@@ -472,18 +473,16 @@ func (c *Config) prepareSSH(ctx *interpolate.Context) []error {
 		c.SSHPort = 22
 	}
 
-	if c.SSHTimeout == 0 {
+	// Only set default values when neither are set
+	if c.SSHTimeout == 0 && c.SSHHandshakeAttempts == 0 {
 		c.SSHTimeout = 5 * time.Minute
+		c.SSHHandshakeAttempts = 10
 	}
 
 	if c.SSHKeepAliveInterval == 0 {
 		c.SSHKeepAliveInterval = 5 * time.Second
 	}
 
-	if c.SSHHandshakeAttempts == 0 {
-		c.SSHHandshakeAttempts = 10
-	}
-
 	if c.SSHBastionHost != "" {
 		if c.SSHBastionPort == 0 {
 			c.SSHBastionPort = 22
diff --git a/communicator/step_connect_ssh.go b/communicator/step_connect_ssh.go
index 7c6ed150f..652b5c4b3 100644
--- a/communicator/step_connect_ssh.go
+++ b/communicator/step_connect_ssh.go
@@ -50,7 +50,10 @@ func (s *StepConnectSSH) Run(ctx context.Context, state multistep.StateBag) mult
 	}()
 
 	log.Printf("[INFO] Waiting for SSH, up to timeout: %s", s.Config.SSHTimeout)
-	timeout := time.After(s.Config.SSHTimeout)
+	timeout := make(<-chan time.Time)
+	if s.Config.SSHTimeout > 0 {
+		timeout = time.After(s.Config.SSHTimeout)
+	}
 	for {
 		// Wait for either SSH to become available, a timeout to occur,
 		// or an interrupt to come through.
@@ -231,14 +234,15 @@ func (s *StepConnectSSH) waitForSSH(state multistep.StateBag, ctx context.Contex
 				handshakeAttempts += 1
 			}
 
-			if handshakeAttempts < s.Config.SSHHandshakeAttempts {
-				// Try to connect via SSH a handful of times. We sleep here
-				// so we don't get a ton of authentication errors back to back.
-				time.Sleep(2 * time.Second)
-				continue
+			if s.Config.SSHHandshakeAttempts > 0 &&
+				handshakeAttempts >= s.Config.SSHHandshakeAttempts {
+				return nil, err
 			}
 
-			return nil, err
+			// Try to connect via SSH a handful of times. We sleep here
+			// so we don't get a ton of authentication errors back to back.
+			time.Sleep(2 * time.Second)
+			continue
 		}
 
 		break