Merge pull request #24695 from paleg/instance-launch-template-stop

r/aws_instance: fix spot request with stop behaviour

Author: ewbankkit

.changelog/24695.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+resource/aws_instance: Prevent error `InvalidParameterCombination: The parameter GroupName within placement information cannot be specified when instanceInterruptionBehavior is set to 'STOP'` when using a launch template that sets `instance_interruption_behavior` to `stop`
+```

internal/service/ec2/ec2_instance.go

@@ -1987,62 +1987,6 @@ func blockDeviceIsRoot(bd *ec2.InstanceBlockDeviceMapping, instance *ec2.Instanc
 		aws.StringValue(bd.DeviceName) == aws.StringValue(instance.RootDeviceName)
 }
 
-func fetchLaunchTemplateAMI(specs []interface{}, conn *ec2.EC2) (string, error) {
-	if len(specs) < 1 {
-		return "", errors.New("Cannot fetch AMI for blank launch template.")
-	}
-
-	spec := specs[0].(map[string]interface{})
-
-	idValue, idOk := spec["id"]
-	nameValue, nameOk := spec["name"]
-
-	request := &ec2.DescribeLaunchTemplateVersionsInput{}
-
-	if idOk && idValue != "" {
-		request.LaunchTemplateId = aws.String(idValue.(string))
-	} else if nameOk && nameValue != "" {
-		request.LaunchTemplateName = aws.String(nameValue.(string))
-	}
-
-	var isLatest bool
-	defaultFilter := []*ec2.Filter{
-		{
-			Name:   aws.String("is-default-version"),
-			Values: aws.StringSlice([]string{"true"}),
-		},
-	}
-	if v, ok := spec["version"]; ok && v != "" {
-		switch v {
-		case LaunchTemplateVersionDefault:
-			request.Filters = defaultFilter
-		case LaunchTemplateVersionLatest:
-			isLatest = true
-		default:
-			request.Versions = []*string{aws.String(v.(string))}
-		}
-	}
-
-	dltv, err := conn.DescribeLaunchTemplateVersions(request)
-	if err != nil {
-		return "", err
-	}
-
-	var ltData *ec2.ResponseLaunchTemplateData
-	if isLatest {
-		index := len(dltv.LaunchTemplateVersions) - 1
-		ltData = dltv.LaunchTemplateVersions[index].LaunchTemplateData
-	} else {
-		ltData = dltv.LaunchTemplateVersions[0].LaunchTemplateData
-	}
-
-	if ltData.ImageId != nil {
-		return *ltData.ImageId, nil
-	}
-
-	return "", nil
-}
-
 func FetchRootDeviceName(conn *ec2.EC2, amiID string) (*string, error) {
 	if amiID == "" {
 		return nil, errors.New("Cannot fetch root device name for blank AMI ID.")
@@ -2290,21 +2234,24 @@ func readBlockDeviceMappingsFromConfig(d *schema.ResourceData, conn *ec2.EC2) ([
 			}
 
 			var amiID string
-			if v, ok := d.GetOk("launch_template"); ok {
-				var err error
-				amiID, err = fetchLaunchTemplateAMI(v.([]interface{}), conn)
+
+			if v, ok := d.GetOk("launch_template"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil {
+				launchTemplateData, err := findLaunchTemplateData(conn, expandLaunchTemplateSpecification(v.([]interface{})[0].(map[string]interface{})))
+
 				if err != nil {
 					return nil, err
 				}
+
+				amiID = aws.StringValue(launchTemplateData.ImageId)
 			}
 
-			// AMI id from attributes overrides ami from launch template
+			// AMI from configuration overrides the one from the launch template.
 			if v, ok := d.GetOk("ami"); ok {
 				amiID = v.(string)
 			}
 
 			if amiID == "" {
-				return nil, errors.New("`ami` must be set or provided via launch template")
+				return nil, errors.New("`ami` must be set or provided via `launch_template`")
 			}
 
 			if dn, err := FetchRootDeviceName(conn, amiID); err == nil {
@@ -2506,8 +2453,21 @@ func buildInstanceOpts(d *schema.ResourceData, meta interface{}) (*awsInstanceOp
 		opts.InstanceType = aws.String(v.(string))
 	}
 
-	if v, ok := d.GetOk("launch_template"); ok {
-		opts.LaunchTemplate = expandLaunchTemplateSpecification(v.([]interface{}))
+	var instanceInterruptionBehavior string
+
+	if v, ok := d.GetOk("launch_template"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil {
+		launchTemplateSpecification := expandLaunchTemplateSpecification(v.([]interface{})[0].(map[string]interface{}))
+		launchTemplateData, err := findLaunchTemplateData(conn, launchTemplateSpecification)
+
+		if err != nil {
+			return nil, err
+		}
+
+		opts.LaunchTemplate = launchTemplateSpecification
+
+		if launchTemplateData.InstanceMarketOptions != nil && launchTemplateData.InstanceMarketOptions.SpotOptions != nil {
+			instanceInterruptionBehavior = aws.StringValue(launchTemplateData.InstanceMarketOptions.SpotOptions.InstanceInterruptionBehavior)
+		}
 	}
 
 	instanceType := d.Get("instance_type").(string)
@@ -2561,7 +2521,6 @@ func buildInstanceOpts(d *schema.ResourceData, meta interface{}) (*awsInstanceOp
 	// aws_spot_instance_request. They represent the same data. :-|
 	opts.Placement = &ec2.Placement{
 		AvailabilityZone: aws.String(d.Get("availability_zone").(string)),
-		GroupName:        aws.String(d.Get("placement_group").(string)),
 	}
 
 	if v, ok := d.GetOk("placement_partition_number"); ok {
@@ -2570,7 +2529,11 @@ func buildInstanceOpts(d *schema.ResourceData, meta interface{}) (*awsInstanceOp
 
 	opts.SpotPlacement = &ec2.SpotPlacement{
 		AvailabilityZone: aws.String(d.Get("availability_zone").(string)),
-		GroupName:        aws.String(d.Get("placement_group").(string)),
+	}
+
+	if v := d.Get("placement_group").(string); instanceInterruptionBehavior == "" || instanceInterruptionBehavior == ec2.InstanceInterruptionBehaviorTerminate {
+		opts.Placement.GroupName = aws.String(v)
+		opts.SpotPlacement.GroupName = aws.String(v)
 	}
 
 	if v := d.Get("tenancy").(string); v != "" {
@@ -3069,29 +3032,26 @@ func flattenInstanceMaintenanceOptions(apiObject *ec2.InstanceMaintenanceOptions
 	return tfMap
 }
 
-func expandLaunchTemplateSpecification(specs []interface{}) *ec2.LaunchTemplateSpecification {
-	if len(specs) < 1 {
+func expandLaunchTemplateSpecification(tfMap map[string]interface{}) *ec2.LaunchTemplateSpecification {
+	if tfMap == nil {
 		return nil
 	}
 
-	spec := specs[0].(map[string]interface{})
+	apiObject := &ec2.LaunchTemplateSpecification{}
 
-	idValue, idOk := spec["id"]
-	nameValue, nameOk := spec["name"]
-
-	result := &ec2.LaunchTemplateSpecification{}
-
-	if idOk && idValue != "" {
-		result.LaunchTemplateId = aws.String(idValue.(string))
-	} else if nameOk && nameValue != "" {
-		result.LaunchTemplateName = aws.String(nameValue.(string))
+	// DescribeLaunchTemplates returns both name and id but LaunchTemplateSpecification
+	// allows only one of them to be set.
+	if v, ok := tfMap["id"]; ok && v != "" {
+		apiObject.LaunchTemplateId = aws.String(v.(string))
+	} else if v, ok := tfMap["name"]; ok && v != "" {
+		apiObject.LaunchTemplateName = aws.String(v.(string))
 	}
 
-	if v, ok := spec["version"]; ok && v != "" {
-		result.Version = aws.String(v.(string))
+	if v, ok := tfMap["version"].(string); ok && v != "" {
+		apiObject.Version = aws.String(v)
 	}
 
-	return result
+	return apiObject
 }
 
 func flattenInstanceLaunchTemplate(conn *ec2.EC2, instanceID, previousLaunchTemplateVersion string) ([]interface{}, error) {
@@ -3176,6 +3136,43 @@ func findInstanceLaunchTemplateVersion(conn *ec2.EC2, id string) (string, error)
 	return launchTemplateVersion, nil
 }
 
+func findLaunchTemplateData(conn *ec2.EC2, launchTemplateSpecification *ec2.LaunchTemplateSpecification) (*ec2.ResponseLaunchTemplateData, error) {
+	input := &ec2.DescribeLaunchTemplateVersionsInput{}
+
+	if v := aws.StringValue(launchTemplateSpecification.LaunchTemplateId); v != "" {
+		input.LaunchTemplateId = aws.String(v)
+	} else if v := aws.StringValue(launchTemplateSpecification.LaunchTemplateName); v != "" {
+		input.LaunchTemplateName = aws.String(v)
+	}
+
+	var latestVersion bool
+
+	if v := aws.StringValue(launchTemplateSpecification.Version); v != "" {
+		switch v {
+		case LaunchTemplateVersionDefault:
+			input.Filters = BuildAttributeFilterList(map[string]string{
+				"is-default-version": "true",
+			})
+		case LaunchTemplateVersionLatest:
+			latestVersion = true
+		default:
+			input.Versions = aws.StringSlice([]string{v})
+		}
+	}
+
+	output, err := FindLaunchTemplateVersions(conn, input)
+
+	if err != nil {
+		return nil, fmt.Errorf("reading EC2 Launch Template versions: %w", err)
+	}
+
+	if latestVersion {
+		return output[len(output)-1].LaunchTemplateData, nil
+	}
+
+	return output[0].LaunchTemplateData, nil
+}
+
 // findLaunchTemplateNameAndVersions returns the specified launch template's name, default version and latest version.
 func findLaunchTemplateNameAndVersions(conn *ec2.EC2, id string) (string, string, string, error) {
 	lt, err := FindLaunchTemplateByID(conn, id)

internal/service/ec2/ec2_instance_test.go

@@ -3219,6 +3219,29 @@ func TestAccEC2Instance_LaunchTemplate_swapIDAndName(t *testing.T) {
 	})
 }
 
+func TestAccEC2Instance_LaunchTemplate_spotAndStop(t *testing.T) {
+	var v ec2.Instance
+	resourceName := "aws_instance.test"
+	launchTemplateResourceName := "aws_launch_template.test"
+	rName := sdkacctest.RandomWithPrefix("tf-acc-test")
+
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:          func() { acctest.PreCheck(t) },
+		ErrorCheck:        acctest.ErrorCheck(t, ec2.EndpointsID),
+		ProviderFactories: acctest.ProviderFactories,
+		CheckDestroy:      testAccCheckInstanceDestroy,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccInstanceConfig_templateSpotAndStop(rName),
+				Check: resource.ComposeAggregateTestCheckFunc(
+					testAccCheckInstanceExists(resourceName, &v),
+					resource.TestCheckResourceAttrPair(resourceName, "launch_template.0.id", launchTemplateResourceName, "id"),
+				),
+			},
+		},
+	})
+}
+
 func TestAccEC2Instance_GetPasswordData_falseToTrue(t *testing.T) {
 	var before, after ec2.Instance
 	resourceName := "aws_instance.test"
@@ -7656,3 +7679,35 @@ resource "aws_instance" "test" {
 }
 `, rName))
 }
+
+func testAccInstanceConfig_templateSpotAndStop(rName string) string {
+	return acctest.ConfigCompose(
+		acctest.ConfigLatestAmazonLinuxHVMEBSAMI(),
+		acctest.AvailableEC2InstanceTypeForRegion("t3.micro", "t2.micro", "t1.micro", "m1.small"),
+		fmt.Sprintf(`
+resource "aws_launch_template" "test" {
+  name          = %[1]q
+  image_id      = data.aws_ami.amzn-ami-minimal-hvm-ebs.id
+  instance_type = data.aws_ec2_instance_type_offering.available.instance_type
+
+  instance_market_options {
+    market_type = "spot"
+
+    spot_options {
+      instance_interruption_behavior = "stop"
+      spot_instance_type             = "persistent"
+    }
+  }
+}
+
+resource "aws_instance" "test" {
+  launch_template {
+    name = aws_launch_template.test.name
+  }
+
+  tags = {
+    Name = %[1]q
+  }
+}
+`, rName))
+}

internal/service/ec2/find.go

@@ -4185,7 +4185,7 @@ func FindLaunchTemplateVersion(conn *ec2.EC2, input *ec2.DescribeLaunchTemplateV
 		return nil, err
 	}
 
-	if len(output) == 0 || output[0] == nil {
+	if len(output) == 0 || output[0] == nil || output[0].LaunchTemplateData == nil {
 		return nil, tfresource.NewEmptyResultError(input)
 	}
 
@@ -4213,7 +4213,7 @@ func FindLaunchTemplateVersions(conn *ec2.EC2, input *ec2.DescribeLaunchTemplate
 		return !lastPage
 	})
 
-	if tfawserr.ErrCodeEquals(err, errCodeInvalidLaunchTemplateIdNotFound, errCodeInvalidLaunchTemplateIdVersionNotFound) {
+	if tfawserr.ErrCodeEquals(err, errCodeInvalidLaunchTemplateIdNotFound, errCodeInvalidLaunchTemplateNameNotFoundException, errCodeInvalidLaunchTemplateIdVersionNotFound) {
 		return nil, &resource.NotFoundError{
 			LastError:   err,
 			LastRequest: input,