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

bflad · 2018-02-02T16:36:24Z

Fixes #85
Closes #2873
Closes #3098

This PR is a continuation of #2559, including crash prevention for nil AssignPublicIp and testing fixes for that PR. While trying to run and fix the acceptance testing, I was having major trouble with the incorrect retry logic that was noted in #85 so fixed that as well on top of the original PR.

Tests passed: 15
=== RUN   TestAccAWSEcsService_withDeploymentValues
--- PASS: TestAccAWSEcsService_withDeploymentValues (18.69s)
=== RUN   TestAccAWSEcsService_withIamRole
--- PASS: TestAccAWSEcsService_withIamRole (30.97s)
=== RUN   TestAccAWSEcsService_withEcsClusterName
--- PASS: TestAccAWSEcsService_withEcsClusterName (32.51s)
=== RUN   TestAccAWSEcsService_withUnnormalizedPlacementStrategy
--- PASS: TestAccAWSEcsService_withUnnormalizedPlacementStrategy (34.12s)
=== RUN   TestAccAWSEcsService_withPlacementConstraints
--- PASS: TestAccAWSEcsService_withPlacementConstraints (34.33s)
=== RUN   TestAccAWSEcsService_withARN
--- PASS: TestAccAWSEcsService_withARN (38.19s)
=== RUN   TestAccAWSEcsService_withFamilyAndRevision
--- PASS: TestAccAWSEcsService_withFamilyAndRevision (44.44s)
=== RUN   TestAccAWSEcsService_withRenamedCluster
--- PASS: TestAccAWSEcsService_withRenamedCluster (45.14s)
=== RUN   TestAccAWSEcsService_withPlacementStrategy
--- PASS: TestAccAWSEcsService_withPlacementStrategy (48.49s)
=== RUN   TestAccAWSEcsService_withPlacementConstraints_emptyExpression
--- PASS: TestAccAWSEcsService_withPlacementConstraints_emptyExpression (48.62s)
=== RUN   TestAccAWSEcsService_withLaunchTypeEC2AndNetworkConfiguration
--- PASS: TestAccAWSEcsService_withLaunchTypeEC2AndNetworkConfiguration (50.84s)
=== RUN   TestAccAWSEcsService_withLbChanges
--- PASS: TestAccAWSEcsService_withLbChanges (134.42s)
=== RUN   TestAccAWSEcsService_healthCheckGracePeriodSeconds
--- PASS: TestAccAWSEcsService_healthCheckGracePeriodSeconds (211.31s)
=== RUN   TestAccAWSEcsService_withAlb
--- PASS: TestAccAWSEcsService_withAlb (225.66s)
=== RUN   TestAccAWSEcsService_withLaunchTypeFargate
--- PASS: TestAccAWSEcsService_withLaunchTypeFargate (251.76s)

…vider-aws into feature/fargate_support_v2

…orton/terraform-provider-aws into f-aws_ecs_service-assign_public_ip

… acceptance test assign_public_ip state and updates

…catching incorrect InvalidParameterException

…te only for now

…argate

…tion update testing

bflad · 2018-02-04T02:04:33Z

Pulled in #3242 commits:

=== RUN   TestAccAWSEcsService_withEcsClusterName
--- PASS: TestAccAWSEcsService_withEcsClusterName (10.88s)
=== RUN   TestAccAWSEcsService_withDeploymentValues
--- PASS: TestAccAWSEcsService_withDeploymentValues (11.12s)
=== RUN   TestAccAWSEcsService_withPlacementConstraints_emptyExpression
--- PASS: TestAccAWSEcsService_withPlacementConstraints_emptyExpression (14.90s)
=== RUN   TestAccAWSEcsService_withPlacementStrategy
--- PASS: TestAccAWSEcsService_withPlacementStrategy (26.64s)
=== RUN   TestAccAWSEcsService_withRenamedCluster
--- PASS: TestAccAWSEcsService_withRenamedCluster (27.79s)
=== RUN   TestAccAWSEcsService_withARN
--- PASS: TestAccAWSEcsService_withARN (38.56s)
=== RUN   TestAccAWSEcsService_withUnnormalizedPlacementStrategy
--- PASS: TestAccAWSEcsService_withUnnormalizedPlacementStrategy (42.26s)
=== RUN   TestAccAWSEcsService_withPlacementConstraints
--- PASS: TestAccAWSEcsService_withPlacementConstraints (42.96s)
=== RUN   TestAccAWSEcsService_withLbChanges
--- PASS: TestAccAWSEcsService_withLbChanges (43.23s)
=== RUN   TestAccAWSEcsService_withFamilyAndRevision
--- PASS: TestAccAWSEcsService_withFamilyAndRevision (49.40s)
=== RUN   TestAccAWSEcsService_withLaunchTypeEC2AndNetworkConfiguration
--- PASS: TestAccAWSEcsService_withLaunchTypeEC2AndNetworkConfiguration (80.44s)
=== RUN   TestAccAWSEcsService_withIamRole
--- PASS: TestAccAWSEcsService_withIamRole (125.37s)
=== RUN   TestAccAWSEcsService_healthCheckGracePeriodSeconds
--- PASS: TestAccAWSEcsService_healthCheckGracePeriodSeconds (201.72s)
=== RUN   TestAccAWSEcsService_withAlb
--- PASS: TestAccAWSEcsService_withAlb (214.05s)
=== RUN   TestAccAWSEcsService_withLaunchTypeFargate
--- PASS: TestAccAWSEcsService_withLaunchTypeFargate (250.18s)

catsby

If I'm not mistaken, we lose retry support on a possible eventually consistent part of a Service not yet being available via the API. Please confirm that's the intent and a little as to why, if possible.

Other than that, a documentation nit and 👍

catsby · 2018-02-05T20:54:10Z

aws/resource_aws_ecs_service.go

-				log.Printf("[DEBUG] Trying to update ECS service again: %#v", err)
-				return resource.RetryableError(err)
-			}
-			if ok && awsErr.Code() == "ServiceNotFoundException" {


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

@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?

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

catsby · 2018-02-05T20:58:47Z

website/docs/r/ecs_service.html.markdown

@@ -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`.


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

…ased on #3240 feedback

radeksimko · 2018-02-08T18:13:44Z

aws/resource_aws_ecs_service.go

-			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.") {


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. 😃

radeksimko · 2018-02-08T18:16:20Z

aws/resource_aws_ecs_service.go

-				log.Printf("[DEBUG] Trying to update ECS service again: %#v", err)
-				return resource.RetryableError(err)
-			}
-			if ok && awsErr.Code() == "ServiceNotFoundException" {


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

radeksimko · 2018-02-08T18:17:09Z

aws/resource_aws_ecs_service.go

@@ -9,7 +9,6 @@ import (
 	"time"

 	"github.com/aws/aws-sdk-go/aws"
-	"github.com/aws/aws-sdk-go/aws/awserr"


radeksimko · 2018-02-08T18:19:00Z

aws/resource_aws_ecs_service_test.go

+		sg1Name, sg2Name, clusterName, tdName, svcName,
+		`"${aws_security_group.allow_all_a.id}"`,
+	)
+}


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

…otFound for new resources

bflad · 2018-02-09T01:22:12Z

@radeksimko sorry for the delayed response, all tests are passing. I'll run them a few more times to be certain. 🎉

15 tests passed
=== RUN   TestAccAWSEcsService_withEcsClusterName
--- PASS: TestAccAWSEcsService_withEcsClusterName (14.92s)
=== RUN   TestAccAWSEcsService_withRenamedCluster
--- PASS: TestAccAWSEcsService_withRenamedCluster (21.81s)
=== RUN   TestAccAWSEcsService_withFamilyAndRevision
--- PASS: TestAccAWSEcsService_withFamilyAndRevision (34.78s)
=== RUN   TestAccAWSEcsService_withDeploymentValues
--- PASS: TestAccAWSEcsService_withDeploymentValues (35.34s)
=== RUN   TestAccAWSEcsService_withUnnormalizedPlacementStrategy
--- PASS: TestAccAWSEcsService_withUnnormalizedPlacementStrategy (36.29s)
=== RUN   TestAccAWSEcsService_withPlacementConstraints
--- PASS: TestAccAWSEcsService_withPlacementConstraints (37.70s)
=== RUN   TestAccAWSEcsService_withPlacementStrategy
--- PASS: TestAccAWSEcsService_withPlacementStrategy (40.06s)
=== RUN   TestAccAWSEcsService_withPlacementConstraints_emptyExpression
--- PASS: TestAccAWSEcsService_withPlacementConstraints_emptyExpression (44.88s)
=== RUN   TestAccAWSEcsService_withARN
--- PASS: TestAccAWSEcsService_withARN (68.66s)
=== RUN   TestAccAWSEcsService_withLaunchTypeEC2AndNetworkConfiguration
--- PASS: TestAccAWSEcsService_withLaunchTypeEC2AndNetworkConfiguration (74.69s)
=== RUN   TestAccAWSEcsService_withIamRole
--- PASS: TestAccAWSEcsService_withIamRole (124.65s)
=== RUN   TestAccAWSEcsService_withAlb
--- PASS: TestAccAWSEcsService_withAlb (214.91s)
=== RUN   TestAccAWSEcsService_healthCheckGracePeriodSeconds
--- PASS: TestAccAWSEcsService_healthCheckGracePeriodSeconds (221.19s)
=== RUN   TestAccAWSEcsService_withLbChanges
--- PASS: TestAccAWSEcsService_withLbChanges (228.72s)
=== RUN   TestAccAWSEcsService_withLaunchTypeFargate
--- PASS: TestAccAWSEcsService_withLaunchTypeFargate (271.02s)

bflad · 2018-02-09T22:45:17Z

This has been released in terraform-provider-aws version 1.9.0. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

* commit '5293a0e3b1366ee16d8742b9b2354781a79bfbd9': (224 commits) v1.9.0 Update CHANGELOG for hashicorp#1101 and hashicorp#3283 docs/resource/aws_sns_platform_application: Add note about platform_credential and platform_principal hashing resource/aws_sns_platform_application: Refactor ID parsing to its own function, use testing.T in testAccAwsSnsPlatformApplicationPlatformFromEnv Add lambda example (hashicorp#3168) Update CHANGELOG for hashicorp#3157 docs/data-source/aws_region: Remove now deprecated current argument data-source/aws_region: Refactor logic into findRegionByEc2Endpoint and findRegionByName functions Update CHANGELOG for hashicorp#3301 Update CHANGELOG for hashicorp#2559 and hashicorp#3240 Update CHANGELOG.md resource/aws_kinesis_stream: Retry deletion on LimitExceededException (hashicorp#3108) Update CHANGELOG.md resource/aws_dynamodb_table_item: Cleanup + add missing bits Added dynamodb_table_item resource hashicorp#517 Update CHANGELOG.md New Resource: aws_cloud9_environment_ec2 Update CHANGELOG.md Fixed markdown typo in docs resource/aws_kinesis_firehose_delivery_stream: Prevent crashes on empty CloudWatchLoggingOptions and fix extended_s3_configuration kms_key_arn ... # Conflicts: # aws/validators.go

…parameters-features * commit '5293a0e3b1366ee16d8742b9b2354781a79bfbd9': (752 commits) v1.9.0 Update CHANGELOG for hashicorp#1101 and hashicorp#3283 docs/resource/aws_sns_platform_application: Add note about platform_credential and platform_principal hashing resource/aws_sns_platform_application: Refactor ID parsing to its own function, use testing.T in testAccAwsSnsPlatformApplicationPlatformFromEnv Add lambda example (hashicorp#3168) Update CHANGELOG for hashicorp#3157 docs/data-source/aws_region: Remove now deprecated current argument data-source/aws_region: Refactor logic into findRegionByEc2Endpoint and findRegionByName functions Update CHANGELOG for hashicorp#3301 Update CHANGELOG for hashicorp#2559 and hashicorp#3240 Update CHANGELOG.md resource/aws_kinesis_stream: Retry deletion on LimitExceededException (hashicorp#3108) Update CHANGELOG.md resource/aws_dynamodb_table_item: Cleanup + add missing bits Added dynamodb_table_item resource hashicorp#517 Update CHANGELOG.md New Resource: aws_cloud9_environment_ec2 Update CHANGELOG.md Fixed markdown typo in docs resource/aws_kinesis_firehose_delivery_stream: Prevent crashes on empty CloudWatchLoggingOptions and fix extended_s3_configuration kms_key_arn ... # Conflicts: # aws/resource_aws_ssm_parameter_test.go

ghost · 2020-04-08T17:16:35Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

John Norton and others added 19 commits December 5, 2017 15:23

add assign public ip property to network configuration

1ee4ecd

force new resource on network config change, update docs

927d68c

set default value for assign_public_ip

3693473

Merge branch 'master' of github.com:terraform-providers/terraform-pro…

ca65a43

…vider-aws into feature/fargate_support_v2

fix networkcofnig force new resource...add test

8d71878

remove debug output

b48ae4a

Merge branch 'master' into feature/fargate_support_v2

efd2510

Merge branch 'master' into feature/fargate_support_v2

abbd3e3

Change assign_public_ip to boolean

83d8406

Update ecs_service.html.markdown

5c56c41

change to true or false, update docs.

1da6118

Use SDK Enum

642d870

remove &schema.Schema{

554f63d

Merge branch 'feature/fargate_support_v2' of https://github.com/johnn…

d953778

…orton/terraform-provider-aws into f-aws_ecs_service-assign_public_ip

resource/aws_ecs_service: Prevent crash on missing AssignPublicIp and…

82f4d9b

… acceptance test assign_public_ip state and updates

resource/aws_lb_listener: Revert extraneous ForceNew change

e303622

resource/aws_ecs_service: Refactor error handling, especially to fix …

b55f369

…catching incorrect InvalidParameterException

test/resource/aws_ecs_service: Move assign_public_ip testing to Farga…

7043e4f

…te only for now

docs/resource/aws_ecs_service: Note assign_public_ip limitation for F…

fa68bf0

…argate

ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 2, 2018

bflad added bug Addresses a defect in current functionality. enhancement Requests to existing resources that expand the functionality or scope. service/ecs Issues and PRs that pertain to the ecs service. labels Feb 2, 2018

bflad requested a review from radeksimko February 2, 2018 16:36

bflad added this to the v1.9.0 milestone Feb 2, 2018

bflad mentioned this pull request Feb 3, 2018

Fix typo of network_configuration #3242

Closed

Mause added 2 commits February 3, 2018 19:15

resource/aws_ecs_service: Merge in typo fixes of network_configuration

8d990bc

test/resource/aws_ecs_service: Merge LaunchTypeEc2AndNetworkConfigura…

542d130

…tion update testing

ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 4, 2018

catsby reviewed Feb 5, 2018

View reviewed changes

docs/resource/aws_ecs_service: Updated wording for assign_public_ip b…

e63a014

…ased on #3240 feedback

ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 5, 2018

radeksimko reviewed Feb 8, 2018

View reviewed changes

resource/aws_ecs_service: Use Read after Create and retry on ServiceN…

7551f92

…otFound for new resources

ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 8, 2018

bflad requested a review from radeksimko February 9, 2018 01:22

radeksimko approved these changes Feb 9, 2018

View reviewed changes

bflad merged commit ac4c5c1 into master Feb 9, 2018

bflad deleted the f-aws_ecs_service-assign_public_ip branch February 9, 2018 13:20

bflad added a commit that referenced this pull request Feb 9, 2018

Update CHANGELOG for #2559 and #3240

d81395c

tomelliff mentioned this pull request Feb 23, 2018

aws_ecs_service no longer retries when target group is not attached to load balancer #3495

Closed

ghost locked and limited conversation to collaborators Apr 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bflad commented Feb 2, 2018 •

edited

Loading

bflad commented Feb 4, 2018

catsby left a comment

catsby Feb 5, 2018

bflad Feb 5, 2018

radeksimko Feb 8, 2018

catsby Feb 5, 2018

bflad Feb 5, 2018

radeksimko Feb 8, 2018

radeksimko Feb 8, 2018

radeksimko Feb 8, 2018

radeksimko Feb 8, 2018

bflad commented Feb 9, 2018

bflad commented Feb 9, 2018

ghost commented Apr 8, 2020

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

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

Conversation

bflad commented Feb 2, 2018 • edited Loading

bflad commented Feb 4, 2018

catsby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad commented Feb 9, 2018

bflad commented Feb 9, 2018

ghost commented Apr 8, 2020

bflad commented Feb 2, 2018 •

edited

Loading