Refactor Terraform executor

- Define a new type terraform.ExecutionStep which allows describing
  arbitrary Terraform operations declaratively.
- Allow passing one or more ExecutionSteps to the Terraform executor.

This allows making pkg/platform generic as now every platform
implementation can define its steps for deploying infrastructure in a
declarative way, thus making the code which interacts with Terraform
generic. Thanks to the PreExecutionHook concept, the same is true even
when arbitrary logic needs to be executed before some Terraform
operation: A platform implementation can include a callback function
in the PreExecutionHook field of an ExecutionStep and the generic
Terraform code will invoke it at the right time.

pkg/dns/dns.go

@@ -15,11 +15,14 @@
 package dns
 
 import (
+	"bufio"
 	"encoding/json"
 	"fmt"
 	"net"
+	"os"
 	"reflect"
 	"sort"
+	"strings"
 
 	"github.com/kinvolk/lokomotive/pkg/terraform"
 	"github.com/pkg/errors"
@@ -61,37 +64,46 @@ func (c *Config) Validate() error {
 	return fmt.Errorf("invalid DNS provider %q", c.Provider)
 }
 
-// AskToConfigure reads the required DNS entries from a Terraform output,
-// asks the user to configure them and checks if the configuration is correct.
-func (c *Config) AskToConfigure(ex *terraform.Executor) error {
-	dnsEntries, err := readDNSEntries(ex)
-	if err != nil {
-		return err
-	}
+// ManualConfigPrompt returns a terraform.ExecutionHook which prompts the user to configure DNS
+// entries manually and verifies the entries were created successfully.
+func ManualConfigPrompt(c *Config) terraform.ExecutionHook {
+	return func(ex *terraform.Executor) error {
+		dnsEntries, err := readDNSEntries(ex)
+		if err != nil {
+			return err
+		}
 
-	fmt.Printf("Please configure the following DNS entries at the DNS provider which hosts %q:\n", c.Zone)
-	prettyPrintDNSEntries(dnsEntries)
+		fmt.Printf("Please configure the following DNS entries at the DNS provider which hosts %q:\n", c.Zone)
+		prettyPrintDNSEntries(dnsEntries)
 
-	for {
-		fmt.Printf("Press Enter to check the entries or type \"skip\" to continue the installation: ")
+		for {
+			fmt.Printf("Press Enter to check the entries or type \"skip\" to continue the installation: ")
 
-		var input string
-		fmt.Scanln(&input)
+			var input string
 
-		if input == "skip" {
-			break
-		} else if input != "" {
-			continue
-		}
+			reader := bufio.NewReader(os.Stdin)
 
-		if checkDNSEntries(dnsEntries) {
-			break
+			input, err := reader.ReadString('\n')
+			if err != nil {
+				return fmt.Errorf("reading user input: %w", err)
+			}
+
+			v := strings.TrimSpace(input)
+			if v == "skip" {
+				break
+			} else if v != "" {
+				continue
+			}
+
+			if checkDNSEntries(dnsEntries) {
+				break
+			}
+
+			fmt.Println("Entries are not correctly configured, please verify.")
 		}
 
-		fmt.Println("Entries are not correctly configured, please verify.")
+		return nil
 	}
-
-	return nil
 }
 
 func readDNSEntries(ex *terraform.Executor) ([]dnsEntry, error) {

pkg/platform/packet/packet.go

@@ -263,35 +263,35 @@ func (c *config) terraformSmartApply(ex *terraform.Executor, dc dns.Config) erro
 		return ex.Apply()
 	}
 
-	arguments := []string{"apply", "-auto-approve"}
-
-	// Create controllers. We need the controllers' IP addresses before we can
-	// apply the 'dns' module.
-	arguments = append(arguments, fmt.Sprintf("-target=module.packet-%s.packet_device.controllers", c.ClusterName))
-	if err := ex.Execute(arguments...); err != nil {
-		return errors.Wrap(err, "creating controllers")
-	}
-
-	// Apply 'dns' module.
-	arguments = append(arguments, "-target=module.dns")
-	if err := ex.Execute(arguments...); err != nil {
-		return errors.Wrap(err, "applying 'dns' module")
-	}
-
-	// Run `terraform refresh`. This is required in order to make the outputs from the previous
-	// apply operations available.
-	// TODO: Likely caused by https://github.com/hashicorp/terraform/issues/23158.
-	if err := ex.Execute("refresh"); err != nil {
-		return errors.Wrap(err, "refreshing")
-	}
-
-	// Prompt user to configure DNS.
-	if err := dc.AskToConfigure(ex); err != nil {
-		return errors.Wrap(err, "prompting for manual DNS configuration")
-	}
-
-	// Finish deployment.
-	return ex.Apply()
+	steps := []terraform.ExecutionStep{
+		// We need the controllers' IP addresses before we can apply the 'dns' module.
+		{
+			Description: "create controllers",
+			Args: []string{
+				"apply",
+				"-auto-approve",
+				fmt.Sprintf("-target=module.packet-%s.packet_device.controllers", c.ClusterName),
+			},
+		},
+		{
+			Description: "construct DNS records",
+			Args:        []string{"apply", "-auto-approve", "-target=module.dns"},
+		},
+		// Run `terraform refresh`. This is required in order to make the outputs from the previous
+		// apply operations available.
+		// TODO: Likely caused by https://github.com/hashicorp/terraform/issues/23158.
+		{
+			Description: "refresh Terraform state",
+			Args:        []string{"refresh"},
+		},
+		{
+			Description:      "complete infrastructure creation",
+			Args:             []string{"apply", "-auto-approve"},
+			PreExecutionHook: dns.ManualConfigPrompt(&c.DNS),
+		},
+	}
+
+	return ex.Execute(steps...)
 }
 
 // terraformAddDeps adds explicit dependencies to cluster nodes so nodes

pkg/terraform/executor.go

@@ -70,6 +70,34 @@ const (
 	ExecutionStatusFailure ExecutionStatus = "Failure"
 )
 
+// ExecutionHook represents a callback function which should be run prior to executing a Terraform
+// operation.
+type ExecutionHook func(*Executor) error
+
+// ExecutionStep represents a single Terraform operation.
+type ExecutionStep struct {
+	// A short string describing the step in a way that is meaningful to the user. The string
+	// should begin with a lowercase letter, be in the imperative tense and have no period at the
+	// end.
+	//
+	// Examples:
+	// - "create DNS resources"
+	// - "deploy virtual machines"
+	Description string
+	// A list of arguments to be passed to the `terraform` command. Note that for "apply"
+	// operations the "-auto-approve" argument should always be included to avoid halting the
+	// Terraform execution with interactive prompts.
+	//
+	// Examples:
+	// - []string{"apply", "-target=module.foo", "-auto-approve"}
+	// - []string{"refresh"}
+	// - []string{"apply", "-auto-approve"}
+	Args []string
+	// A function which should be run prior to executing the Terraform operation. If specified and
+	// the function returns an error, execution is halted.
+	PreExecutionHook ExecutionHook
+}
+
 // Executor enables calling Terraform from Go, across platforms, with any
 // additional providers/provisioners that the currently executing binary
 // exposes.
@@ -126,25 +154,28 @@ func NewExecutor(conf Config) (*Executor, error) {
 	return ex, nil
 }
 
-// Init() is a wrapper function that runs
-// `terraform init`.
+// Init is a wrapper function that runs `terraform init`.
 func (ex *Executor) Init() error {
-	ex.logger.Println("Initializing Terraform working directory")
-	return ex.Execute("init")
+	return ex.Execute(ExecutionStep{
+		Description: "initialize Terraform",
+		Args:        []string{"init"},
+	})
 }
 
-// Apply() is a wrapper function that runs
-// `terraform apply -auto-approve`.
+// Apply is a wrapper function that runs `terraform apply -auto-approve`.
 func (ex *Executor) Apply() error {
-	ex.logger.Println("Applying Terraform configuration. This creates infrastructure so it might take a long time...")
-	return ex.Execute("apply", "-auto-approve")
+	return ex.Execute(ExecutionStep{
+		Description: "create infrastructure",
+		Args:        []string{"apply", "-auto-approve"},
+	})
 }
 
-// Destroy() is a wrapper function that runs
-// `terraform destroy -auto-approve`.
+// Destroy is a wrapper function that runs `terraform destroy -auto-approve`.
 func (ex *Executor) Destroy() error {
-	ex.logger.Println("Destroying Terraform-managed infrastructure")
-	return ex.Execute("destroy", "-auto-approve")
+	return ex.Execute(ExecutionStep{
+		Description: "destroy infrastructure",
+		Args:        []string{"destroy", "-auto-approve"},
+	})
 }
 
 // tailFile will indefinitely tail logs from the given file path, until
@@ -176,14 +207,27 @@ func tailFile(path string, done chan struct{}, wg *sync.WaitGroup) {
 	wg.Done()
 }
 
-// Execute runs the given command and arguments against Terraform, and returns
-// any errors that occur during the execution.
-//
-// An error is returned if the Terraform binary could not be found, or if the
-// Terraform call itself failed, in which case, details can be found in the
-// output.
-func (ex *Executor) Execute(args ...string) error {
-	return ex.execute(ex.verbose, args...)
+// Execute accepts one or more ExecutionSteps and executes them sequentially in the order they were
+// provided. If a step has a PreExecutionHook defined, the hook is run prior to executing the step.
+// If any error is encountered, the error is returned and the execution is halted.
+func (ex *Executor) Execute(steps ...ExecutionStep) error {
+	for _, s := range steps {
+		if s.PreExecutionHook != nil {
+			ex.logger.Printf("Running pre-execution hook for step %q", s.Description)
+
+			if err := s.PreExecutionHook(ex); err != nil {
+				return fmt.Errorf("pre-execution hook failed: %w", err)
+			}
+		}
+
+		ex.logger.Printf("Executing step %q", s.Description)
+
+		if err := ex.execute(ex.verbose, s.Args...); err != nil {
+			return err
+		}
+	}
+
+	return nil
 }
 
 func (ex *Executor) executeVerbose(args ...string) error {
@@ -193,7 +237,10 @@ func (ex *Executor) executeVerbose(args ...string) error {
 func (ex *Executor) execute(verbose bool, args ...string) error {
 	pid, done, err := ex.ExecuteAsync(args...)
 	if err != nil {
-		return fmt.Errorf("failed executing Terraform command with arguments '%s' in directory %s: %w", strings.Join(args, " "), ex.WorkingDirectory(), err)
+		return fmt.Errorf(
+			"executing Terraform with arguments '%s' in directory %s: %w",
+			strings.Join(args, " "), ex.WorkingDirectory(), err,
+		)
 	}
 
 	var wg sync.WaitGroup
@@ -351,7 +398,11 @@ func (ex *Executor) ExecuteSync(args ...string) ([]byte, error) {
 func (ex *Executor) Plan() error {
 	ex.logger.Println("Generating Terraform execution plan")
 
-	if err := ex.Execute("refresh"); err != nil {
+	s := ExecutionStep{
+		Description: "refresh Terraform state",
+		Args:        []string{"refresh"},
+	}
+	if err := ex.Execute(s); err != nil {
 		return err
 	}
 

pkg/terraform/executor_test.go

@@ -33,7 +33,7 @@ func executor(t *testing.T) *Executor {
 func TestExecuteCheckErrors(t *testing.T) {
 	ex := executor(t)
 
-	if err := ex.Execute("apply"); err == nil {
+	if err := ex.Apply(); err == nil {
 		t.Fatalf("Applying on empty directory should fail")
 	}
 }