Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

resource/aws_ecs_service: Add public_assign_ip attribute and fix InvalidParameterException handling #3240

Merged
merged 23 commits into from
Feb 9, 2018
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
1ee4ecd
add assign public ip property to network configuration
Dec 5, 2017
927d68c
force new resource on network config change, update docs
Dec 5, 2017
3693473
set default value for assign_public_ip
Dec 6, 2017
ca65a43
Merge branch 'master' of github.com:terraform-providers/terraform-pro…
Dec 7, 2017
8d71878
fix networkcofnig force new resource...add test
Dec 7, 2017
b48ae4a
remove debug output
Dec 7, 2017
efd2510
Merge branch 'master' into feature/fargate_support_v2
johnnorton Dec 20, 2017
abbd3e3
Merge branch 'master' into feature/fargate_support_v2
johnnorton Jan 11, 2018
83d8406
Change assign_public_ip to boolean
Jan 16, 2018
5c56c41
Update ecs_service.html.markdown
johnnorton Jan 25, 2018
1da6118
change to true or false, update docs.
johnnorton Feb 2, 2018
642d870
Use SDK Enum
johnnorton Feb 2, 2018
554f63d
remove &schema.Schema{
johnnorton Feb 2, 2018
d953778
Merge branch 'feature/fargate_support_v2' of https://github.com/johnn…
bflad Feb 2, 2018
82f4d9b
resource/aws_ecs_service: Prevent crash on missing AssignPublicIp and…
bflad Feb 2, 2018
e303622
resource/aws_lb_listener: Revert extraneous ForceNew change
bflad Feb 2, 2018
b55f369
resource/aws_ecs_service: Refactor error handling, especially to fix …
bflad Feb 2, 2018
7043e4f
test/resource/aws_ecs_service: Move assign_public_ip testing to Farga…
bflad Feb 2, 2018
fa68bf0
docs/resource/aws_ecs_service: Note assign_public_ip limitation for F…
bflad Feb 2, 2018
8d990bc
resource/aws_ecs_service: Merge in typo fixes of network_configuration
Mause Feb 3, 2018
542d130
test/resource/aws_ecs_service: Merge LaunchTypeEc2AndNetworkConfigura…
Mause Feb 3, 2018
e63a014
docs/resource/aws_ecs_service: Updated wording for assign_public_ip b…
bflad Feb 5, 2018
7551f92
resource/aws_ecs_service: Use Read after Create and retry on ServiceN…
bflad Feb 8, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 45 additions & 48 deletions aws/resource_aws_ecs_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"time"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/aws/awserr"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

"github.com/aws/aws-sdk-go/service/ecs"
"github.com/hashicorp/terraform/helper/hashcode"
"github.com/hashicorp/terraform/helper/resource"
Expand Down Expand Up @@ -133,6 +132,11 @@ func resourceAwsEcsService() *schema.Resource {
Elem: &schema.Schema{Type: schema.TypeString},
Set: schema.HashString,
},
"assign_public_ip": {
Type: schema.TypeBool,
Optional: true,
Default: false,
},
},
},
},
Expand Down Expand Up @@ -236,7 +240,7 @@ func resourceAwsEcsServiceCreate(d *schema.ResourceData, meta interface{}) error
input.Role = aws.String(v.(string))
}

input.NetworkConfiguration = expandEcsNetworkConfigration(d.Get("network_configuration").([]interface{}))
input.NetworkConfiguration = expandEcsNetworkConfiguration(d.Get("network_configuration").([]interface{}))

strategies := d.Get("placement_strategy").(*schema.Set).List()
if len(strategies) > 0 {
Expand Down Expand Up @@ -287,21 +291,12 @@ func resourceAwsEcsServiceCreate(d *schema.ResourceData, meta interface{}) error
out, err = conn.CreateService(&input)

if err != nil {
awsErr, ok := err.(awserr.Error)
if !ok {
return resource.NonRetryableError(err)
}
if awsErr.Code() == "InvalidParameterException" {
log.Printf("[DEBUG] Trying to create ECS service again: %q",
awsErr.Message())
if isAWSErr(err, ecs.ErrCodeClusterNotFoundException, "") {
return resource.RetryableError(err)
}
if awsErr.Code() == "ClusterNotFoundException" {
log.Printf("[DEBUG] Trying to create ECS service again: %q",
awsErr.Message())
if isAWSErr(err, ecs.ErrCodeInvalidParameterException, "Please verify that the ECS service role being passed has the proper permissions.") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I trust you that you ran all ECS related acceptance tests a couple of times to verify that we're not missing any other message/format here. 😃

return resource.RetryableError(err)
}

return resource.NonRetryableError(err)
}

Expand Down Expand Up @@ -399,24 +394,30 @@ func resourceAwsEcsServiceRead(d *schema.ResourceData, meta interface{}) error {
log.Printf("[ERR] Error setting placement_constraints for (%s): %s", d.Id(), err)
}

if err := d.Set("network_configuration", flattenEcsNetworkConfigration(service.NetworkConfiguration)); err != nil {
if err := d.Set("network_configuration", flattenEcsNetworkConfiguration(service.NetworkConfiguration)); err != nil {
return fmt.Errorf("[ERR] Error setting network_configuration for (%s): %s", d.Id(), err)
}

return nil
}

func flattenEcsNetworkConfigration(nc *ecs.NetworkConfiguration) []interface{} {
func flattenEcsNetworkConfiguration(nc *ecs.NetworkConfiguration) []interface{} {
if nc == nil {
return nil
}

result := make(map[string]interface{})
result["security_groups"] = schema.NewSet(schema.HashString, flattenStringList(nc.AwsvpcConfiguration.SecurityGroups))
result["subnets"] = schema.NewSet(schema.HashString, flattenStringList(nc.AwsvpcConfiguration.Subnets))

if nc.AwsvpcConfiguration.AssignPublicIp != nil {
result["assign_public_ip"] = *nc.AwsvpcConfiguration.AssignPublicIp == ecs.AssignPublicIpEnabled
}

return []interface{}{result}
}

func expandEcsNetworkConfigration(nc []interface{}) *ecs.NetworkConfiguration {
func expandEcsNetworkConfiguration(nc []interface{}) *ecs.NetworkConfiguration {
if len(nc) == 0 {
return nil
}
Expand All @@ -426,6 +427,13 @@ func expandEcsNetworkConfigration(nc []interface{}) *ecs.NetworkConfiguration {
awsVpcConfig.SecurityGroups = expandStringSet(val.(*schema.Set))
}
awsVpcConfig.Subnets = expandStringSet(raw["subnets"].(*schema.Set))
if val, ok := raw["assign_public_ip"].(bool); ok {
awsVpcConfig.AssignPublicIp = aws.String(ecs.AssignPublicIpDisabled)
if val {
awsVpcConfig.AssignPublicIp = aws.String(ecs.AssignPublicIpEnabled)
}
}

return &ecs.NetworkConfiguration{AwsvpcConfiguration: awsVpcConfig}
}

Expand Down Expand Up @@ -495,24 +503,17 @@ func resourceAwsEcsServiceUpdate(d *schema.ResourceData, meta interface{}) error
}
}

if d.HasChange("network_configration") {
input.NetworkConfiguration = expandEcsNetworkConfigration(d.Get("network_configuration").([]interface{}))
if d.HasChange("network_configuration") {
input.NetworkConfiguration = expandEcsNetworkConfiguration(d.Get("network_configuration").([]interface{}))
}

// Retry due to IAM & ECS eventual consistency
// Retry due to IAM eventual consistency
err := resource.Retry(2*time.Minute, func() *resource.RetryError {
out, err := conn.UpdateService(&input)
if err != nil {
awsErr, ok := err.(awserr.Error)
if ok && awsErr.Code() == "InvalidParameterException" {
log.Printf("[DEBUG] Trying to update ECS service again: %#v", err)
return resource.RetryableError(err)
}
if ok && awsErr.Code() == "ServiceNotFoundException" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we're losing the retry on ServiceNotFoundException, is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@catsby Interestingly enough, I had just assumed it was some sort of typo that it was included and that retrying on a missing service would be undesirable during an update, but turns out we are currently always calling resourceAwsEcsServiceUpdate at the end of resourceAwsEcsServiceCreate. So that explains why it was in there.

Looking through the parameters set during resourceAwsEcsServiceUpdate to ecs.UpdateServiceInput, I don't see anything not already handled by resourceAwsEcsServiceCreate so I think we should instead return resourceAwsEcsServiceRead at the end of resourceAwsEcsServiceCreate and keep the ecs.ErrCodeServiceNotFoundException retry logic out of resourceAwsEcsServiceUpdate.

Does that sound reasonable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bflad that sounds like a good idea - as long as all ECS tests still pass 🤷‍♂️

log.Printf("[DEBUG] Trying to update ECS service again: %#v", err)
if isAWSErr(err, ecs.ErrCodeInvalidParameterException, "Please verify that the ECS service role being passed has the proper permissions.") {
return resource.RetryableError(err)
}

return resource.NonRetryableError(err)
}

Expand All @@ -535,11 +536,17 @@ func resourceAwsEcsServiceDelete(d *schema.ResourceData, meta interface{}) error
Cluster: aws.String(d.Get("cluster").(string)),
})
if err != nil {
if isAWSErr(err, ecs.ErrCodeServiceNotFoundException, "") {
log.Printf("[DEBUG] Removing ECS Service from state, %q is already gone", d.Id())
d.SetId("")
return nil
}
return err
}

if len(resp.Services) == 0 {
log.Printf("[DEBUG] ECS Service %q is already gone", d.Id())
log.Printf("[DEBUG] Removing ECS Service from state, %q is already gone", d.Id())
d.SetId("")
return nil
}

Expand All @@ -562,33 +569,23 @@ func resourceAwsEcsServiceDelete(d *schema.ResourceData, meta interface{}) error
}
}

input := ecs.DeleteServiceInput{
Service: aws.String(d.Id()),
Cluster: aws.String(d.Get("cluster").(string)),
}
// Wait until the ECS service is drained
err = resource.Retry(5*time.Minute, func() *resource.RetryError {
input := ecs.DeleteServiceInput{
Service: aws.String(d.Id()),
Cluster: aws.String(d.Get("cluster").(string)),
}

log.Printf("[DEBUG] Trying to delete ECS service %s", input)
_, err := conn.DeleteService(&input)
if err == nil {
return nil
}

ec2err, ok := err.(awserr.Error)
if !ok {
if err != nil {
if isAWSErr(err, ecs.ErrCodeInvalidParameterException, "The service cannot be stopped while deployments are active.") {
return resource.RetryableError(err)
}
return resource.NonRetryableError(err)
}
if ec2err.Code() == "InvalidParameterException" {
// Prevent "The service cannot be stopped while deployments are active."
log.Printf("[DEBUG] Trying to delete ECS service again: %q",
ec2err.Message())
return resource.RetryableError(err)
}

return resource.NonRetryableError(err)

return nil
})

if err != nil {
return err
}
Expand Down
73 changes: 65 additions & 8 deletions aws/resource_aws_ecs_service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -476,17 +476,48 @@ func TestAccAWSEcsService_withLaunchTypeFargate(t *testing.T) {
CheckDestroy: testAccCheckAWSEcsServiceDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSEcsServiceWithLaunchTypeFargate(sg1Name, sg2Name, clusterName, tdName, svcName),
Config: testAccAWSEcsServiceWithLaunchTypeFargate(sg1Name, sg2Name, clusterName, tdName, svcName, "false"),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSEcsServiceExists("aws_ecs_service.main"),
resource.TestCheckResourceAttr("aws_ecs_service.main", "launch_type", "FARGATE"),
resource.TestCheckResourceAttr("aws_ecs_service.main", "network_configuration.0.assign_public_ip", "false"),
resource.TestCheckResourceAttr("aws_ecs_service.main", "network_configuration.0.security_groups.#", "2"),
resource.TestCheckResourceAttr("aws_ecs_service.main", "network_configuration.0.subnets.#", "2"),
),
},
},
})
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSEcsServiceDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSEcsServiceWithLaunchTypeFargate(sg1Name, sg2Name, clusterName, tdName, svcName, "true"),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSEcsServiceExists("aws_ecs_service.main"),
resource.TestCheckResourceAttr("aws_ecs_service.main", "network_configuration.0.assign_public_ip", "true"),
),
},
},
})
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSEcsServiceDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSEcsServiceWithLaunchTypeFargate(sg1Name, sg2Name, clusterName, tdName, svcName, "false"),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSEcsServiceExists("aws_ecs_service.main"),
resource.TestCheckResourceAttr("aws_ecs_service.main", "network_configuration.0.assign_public_ip", "false"),
),
},
},
})
}

func TestAccAWSEcsService_withNetworkConfiguration(t *testing.T) {
func TestAccAWSEcsService_withLaunchTypeEC2AndNetworkConfiguration(t *testing.T) {
rString := acctest.RandString(8)

sg1Name := fmt.Sprintf("tf-acc-sg-1-svc-w-nc-%s", rString)
Expand All @@ -501,9 +532,21 @@ func TestAccAWSEcsService_withNetworkConfiguration(t *testing.T) {
CheckDestroy: testAccCheckAWSEcsServiceDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSEcsServiceWithNetworkConfigration(sg1Name, sg2Name, clusterName, tdName, svcName),
Config: testAccAWSEcsServiceWithNetworkConfiguration(sg1Name, sg2Name, clusterName, tdName, svcName),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSEcsServiceExists("aws_ecs_service.main"),
resource.TestCheckResourceAttr("aws_ecs_service.main", "network_configuration.0.assign_public_ip", "false"),
resource.TestCheckResourceAttr("aws_ecs_service.main", "network_configuration.0.security_groups.#", "2"),
resource.TestCheckResourceAttr("aws_ecs_service.main", "network_configuration.0.subnets.#", "2"),
),
},
{
Config: testAccAWSEcsServiceWithNetworkConfiguration_modified(sg1Name, sg2Name, clusterName, tdName, svcName),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSEcsServiceExists("aws_ecs_service.main"),
resource.TestCheckResourceAttr("aws_ecs_service.main", "network_configuration.0.assign_public_ip", "false"),
resource.TestCheckResourceAttr("aws_ecs_service.main", "network_configuration.0.security_groups.#", "1"),
resource.TestCheckResourceAttr("aws_ecs_service.main", "network_configuration.0.subnets.#", "2"),
),
},
},
Expand Down Expand Up @@ -750,7 +793,7 @@ resource "aws_ecs_service" "mongo" {
`, clusterName, tdName, svcName)
}

func testAccAWSEcsServiceWithLaunchTypeFargate(sg1Name, sg2Name, clusterName, tdName, svcName string) string {
func testAccAWSEcsServiceWithLaunchTypeFargate(sg1Name, sg2Name, clusterName, tdName, svcName, assignPublicIP string) string {
return fmt.Sprintf(`
provider "aws" {
region = "us-east-1"
Expand Down Expand Up @@ -828,9 +871,10 @@ resource "aws_ecs_service" "main" {
network_configuration {
security_groups = ["${aws_security_group.allow_all_a.id}", "${aws_security_group.allow_all_b.id}"]
subnets = ["${aws_subnet.main.*.id}"]
assign_public_ip = %s
}
}
`, sg1Name, sg2Name, clusterName, tdName, svcName)
`, sg1Name, sg2Name, clusterName, tdName, svcName, assignPublicIP)
}

func testAccAWSEcsService_healthCheckGracePeriodSeconds(vpcNameTag, clusterName, tdName, roleName, policyName,
Expand Down Expand Up @@ -1448,7 +1492,20 @@ resource "aws_ecs_service" "with_alb" {
`, clusterName, tdName, roleName, policyName, tgName, lbName, svcName)
}

func testAccAWSEcsServiceWithNetworkConfigration(sg1Name, sg2Name, clusterName, tdName, svcName string) string {
func testAccAWSEcsServiceWithNetworkConfiguration(sg1Name, sg2Name, clusterName, tdName, svcName string) string {
return tpl_testAccAWSEcsServiceWithNetworkConfiguration(
sg1Name, sg2Name, clusterName, tdName, svcName,
`"${aws_security_group.allow_all_a.id}", "${aws_security_group.allow_all_b.id}"`,
)
}
func testAccAWSEcsServiceWithNetworkConfiguration_modified(sg1Name, sg2Name, clusterName, tdName, svcName string) string {
return tpl_testAccAWSEcsServiceWithNetworkConfiguration(
sg1Name, sg2Name, clusterName, tdName, svcName,
`"${aws_security_group.allow_all_a.id}"`,
)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing parts of HCL is a little bit confusing at first I have to say... sometimes duplication is just better than abstraction, IMO.


func tpl_testAccAWSEcsServiceWithNetworkConfiguration(sg1Name, sg2Name, clusterName, tdName, svcName string, securityGroups string) string {
return fmt.Sprintf(`
data "aws_availability_zones" "available" {}

Expand Down Expand Up @@ -1515,9 +1572,9 @@ resource "aws_ecs_service" "main" {
task_definition = "${aws_ecs_task_definition.mongo.arn}"
desired_count = 1
network_configuration {
security_groups = ["${aws_security_group.allow_all_a.id}", "${aws_security_group.allow_all_b.id}"]
security_groups = [%s]
subnets = ["${aws_subnet.main.*.id}"]
}
}
`, sg1Name, sg2Name, clusterName, tdName, svcName)
`, sg1Name, sg2Name, clusterName, tdName, svcName, securityGroups)
}
4 changes: 2 additions & 2 deletions aws/resource_aws_security_group_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -506,7 +506,7 @@ func TestAccAWSSecurityGroup_forceRevokeRules_true(t *testing.T) {
testAddCycle,
),
},
// Verify the DependencyViolation error by using a configration with the
// Verify the DependencyViolation error by using a configuration with the
// groups removed. Terraform tries to destroy them but cannot. Expect a
// DependencyViolation error
{
Expand Down Expand Up @@ -579,7 +579,7 @@ func TestAccAWSSecurityGroup_forceRevokeRules_false(t *testing.T) {
testAddCycle,
),
},
// Verify the DependencyViolation error by using a configration with the
// Verify the DependencyViolation error by using a configuration with the
// groups removed, and the Groups not configured to revoke their ruls.
// Terraform tries to destroy them but cannot. Expect a
// DependencyViolation error
Expand Down
1 change: 0 additions & 1 deletion examples/networking/region/numbering.tf

This file was deleted.

1 change: 0 additions & 1 deletion examples/networking/subnet/numbering.tf

This file was deleted.

1 change: 1 addition & 0 deletions website/docs/r/ecs_service.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ Guide](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/cluster-query

* `subnets` - (Required) The subnets associated with the task or service.
* `security_groups` - (Optional) The security groups associated with the task or service. If you do not specify a security group, the default security group for the VPC is used.
* `assign_public_ip` - (Optional) Only valid for `FARGATE` launch type and valid values are `true` or `false`. Will assign a public IP address to the ENI. Default value is `false`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest reword to "Assign a public IP address to the ENI (Fargate launch type only). Valid values are true or false. Default false." or similar

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! 👍

For more information, see [Task Networking](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task-networking.html)

## Attributes Reference
Expand Down