Feature/fargate support v2 #2559

johnnorton · 2017-12-05T23:25:43Z

The previous merge for ECS Fargate support did not include the ability to ENABLE "Assign Public IP" when creating the ECS Service.

From what I can tell, this is an option that only works for services on Fargate. This option assigns a public IP to the ENI. This is required to access the internet from the container if you do not have a NAT gateway setup.

radeksimko · 2017-12-06T10:39:38Z

Hi @johnnorton
AFAICT the networking block is updatable per API docs:
http://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_UpdateService.html#ECS-UpdateService-request-networkConfiguration

Can you tell us more about the motivation behind the ForceNew in all fields?

Also Elem is used to define elements nested under the field, which is only relevant to TypeList or TypeSet, potentially TypeMap, but certainly not TypeString.

Finally we'll need to add a test to exercise this new field and ensure it works. Let me know if you need any help with that.

Thanks.

johnnorton · 2017-12-07T17:16:22Z

@radeksimko
Thanks for the input. My reasoning behind forcenew on networking was based on the Console not allowing updates to a service. I will test with the CLI and see.

This is my first foray into GoLang. I will will take a look at your suggestions and create a test.

Standby....

John

…vider-aws into feature/fargate_support_v2

johnnorton · 2017-12-07T18:20:16Z

aws/resource_aws_ecs_service.go

-	if d.HasChange("network_configration") {
-		input.NetworkConfiguration = expandEcsNetworkConfigration(d.Get("network_configuration").([]interface{}))
-	}
+	//d.HasChange("network_configration") is not working, so explicity calling method.


Not sure why the the func HadChange is not detecting a change on "network_configuration". Maybe we need to implement equals on the structure?

johnnorton · 2017-12-07T18:22:45Z

@radeksimko. Yes updating the network config does not need to force a new resource but the existing tasks will not get the new settings as per the docs.

Please review my comments in the code and let me know.

Thanks!

johnnorton · 2017-12-07T18:25:53Z

aws/resource_aws_ecs_service_test.go

@@ -1261,6 +1261,7 @@ 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 = "ENABLED"


I am not sure if this is all I need to do for the test. Seems like I am missing something

johnnorton · 2017-12-13T06:08:33Z

@radeksimko Anything else needed here?

headlessme · 2018-01-05T11:19:57Z

Would also like to see this merged

sleepdeprecation · 2018-01-15T20:22:56Z

Is there any news on getting this merged? Not being able to set auto assign public IP is preventing me from configuring fargate-backed ECS services in terraform.

As a note on the PR itself, I think it would be nicer to be able to use true/false rather than ENABLED/DISABLED as the value, given that this is just a boolean value and that feels like it's exposing too much of the underlying API in a way that isn't particularly helpful.

johnnorton · 2018-01-16T18:43:18Z

@radeksimko

Made change suggested by @dkuntz2 dkuntz2

austinkelleher · 2018-01-21T17:58:41Z

This is also preventing me from configuring Fargate with Terraform. @radeksimko Can we please get this merged soon?

bflad

Hi @johnnorton! This is getting there and thanks for your effort here. I'm providing a drive by review here since I know a couple of folks are waiting for this. If you don't have time or would like us to take this across the finish line, please let us know. 🚀

bflad · 2018-01-23T01:00:22Z

aws/resource_aws_ecs_service.go

@@ -133,6 +133,11 @@ func resourceAwsEcsService() *schema.Resource {
 							Elem:     &schema.Schema{Type: schema.TypeString},
 							Set:      schema.HashString,
 						},
+						"assign_public_ip": &schema.Schema{


You can remove &schema.Schema from this line here.

bflad · 2018-01-23T01:03:33Z

aws/resource_aws_ecs_service.go

 	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))
+	result["assign_public_ip"] = "true"
+	if *nc.AwsvpcConfiguration.AssignPublicIp == "DISABLED" {


The AWS SDK provides constants we can use here instead of directly testing our own strings (this applies for all "DISABLED"/"ENABLED" in this PR), e.g. ecs.AssignPublicIpDisabled/ecs.AssignPublicIpEnabled

bflad · 2018-01-23T01:05:34Z

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) Valid values are "ENABLED" or "DISABLED".  Will assign a public IP address to the ENI.


Documentation needs to be updated since the switch from string values to boolean values and should also note the default of false.

bflad · 2018-01-23T01:17:44Z

aws/resource_aws_ecs_service_test.go

@@ -1517,6 +1517,7 @@ 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 = true


Can you please add an appropriate attribute test where this is used (and one in the other Fargate test where it should be the default of false)? e.g. resource.TestCheckResourceAttr("aws_ecs_service.main", "network_configuration.0.assign_public_ip", "true")

johnnorton · 2018-02-02T06:05:37Z

@bflad @radeksimko @paddycarver @austinkelleher --> Sorry I didn't get on this sooner. Sill learning Go (🤞 ) . Anyway please review and see if we can get this merged.

bflad · 2018-02-02T12:32:48Z

I'll take another look in a few hours. Thanks for the updates and contributions while learning Go! 😃

austinkelleher · 2018-02-02T12:34:24Z

Thanks for your work on this @johnnorton. Looking forward to seeing this merged.

bflad · 2018-02-02T14:14:41Z

aws/resource_aws_ecs_service.go

 	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))
+	result["assign_public_ip"] = "true"
+	if *nc.AwsvpcConfiguration.AssignPublicIp == ecs.AssignPublicIpDisabled {


If nc.AwsvpcConfiguration.AssignPublicIp is nil (its listed as optional in the ECS API documentation), this can cause Terraform to crash. I can fix this (and clean it up a little since it will automatically default to "false") prior to merge via:

if nc.AwsvpcConfiguration.AssignPublicIp != nil { result["assign_public_ip"] = fmt.Sprintf("%v", *nc.AwsvpcConfiguration.AssignPublicIp == ecs.AssignPublicIpEnabled) }

bflad · 2018-02-02T14:16:22Z

aws/resource_aws_lb_listener.go

@@ -34,7 +34,6 @@ func resourceAwsLbListener() *schema.Resource {
 			"load_balancer_arn": {
 				Type:     schema.TypeString,
 				Required: true,
-				ForceNew: true,


Unrelated change - will fix prior to merge

bflad · 2018-02-02T14:57:45Z

@johnnorton thank you so much for your contributions here! I will be pulling in your commits, but I am running into some issues acceptance testing this appropriately, which isn't really from your PR. I will fix those on top of this as it requires some refactoring of the resource/testing itself.

I wound up down this rabbit hole because ECS will return this error if you try to enable assign_public_ip on EC2 launch types:
InvalidParameterException: Assign public IP is not supported for this launch type.

Unfortunately, as noted in #85, the error handling in this resource is already causing trouble as it is retrying on some conditions like the above when it shouldn't be. The launch type and assign_public_ip combination is something we can validate at plan time, but the implementation is a little more complex than just few lines. I think it'll be best if I fix both of these issues together as its already hindering my acceptance testing. More soon.

johnnorton · 2018-02-02T17:05:18Z

@bflad Excellent. Please let me know if I can help.

cemo · 2018-02-09T18:05:24Z

@bflad milestone seems v1.10.0, it should be v1.9.0

bflad · 2018-02-09T18:21:00Z

@cemo good catch, thanks and updated. Also, shout out to @johnnorton for working though the original PR.

bflad · 2018-02-09T22:37:56Z

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:13:49Z

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 added 2 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

johnnorton mentioned this pull request Dec 5, 2017

ECS Fargate Support #2483

Merged

set default value for assign_public_ip

3693473

radeksimko added enhancement Requests to existing resources that expand the functionality or scope. waiting-response Maintainers are waiting on response from community or contributor. labels Dec 6, 2017

John Norton added 2 commits December 7, 2017 10:18

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

johnnorton commented Dec 7, 2017

View reviewed changes

remove debug output

b48ae4a

johnnorton commented Dec 7, 2017

View reviewed changes

radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Dec 8, 2017

Merge branch 'master' into feature/fargate_support_v2

efd2510

Merge branch 'master' into feature/fargate_support_v2

abbd3e3

radeksimko added this to the v1.10.0 milestone Jan 16, 2018

Change assign_public_ip to boolean

83d8406

austinkelleher mentioned this pull request Jan 21, 2018

aws_ecs_service doesn't provide assignPublicIp option in network_configuration #2873

Closed

bflad added the service/ecs Issues and PRs that pertain to the ecs service. label Jan 23, 2018

bflad mentioned this pull request Jan 23, 2018

ECS service definition is missing AssignPublicIp property #3098

Closed

bflad reviewed Jan 23, 2018

View reviewed changes

Update ecs_service.html.markdown

5c56c41

paddycarver self-assigned this Jan 29, 2018

change to true or false, update docs.

1da6118

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

johnnorton added 2 commits February 1, 2018 23:01

Use SDK Enum

642d870

remove &schema.Schema{

554f63d

bflad self-requested a review February 2, 2018 12:35

bflad reviewed Feb 2, 2018

View reviewed changes

bflad mentioned this pull request Feb 2, 2018

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

Merged

bflad merged commit 554f63d into hashicorp:master Feb 9, 2018

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

Update CHANGELOG for #2559 and #3240

d81395c

bflad modified the milestones: v1.10.0, v1.9.0 Feb 9, 2018

johnnorton deleted the feature/fargate_support_v2 branch February 13, 2018 01:54

eschwartz mentioned this pull request Feb 19, 2018

How can I access IP from aws_ecs_service using assign_public_ip #3444

Open

ghost locked and limited conversation to collaborators Apr 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/fargate support v2 #2559

Feature/fargate support v2 #2559

johnnorton commented Dec 5, 2017

radeksimko commented Dec 6, 2017

johnnorton commented Dec 7, 2017

johnnorton Dec 7, 2017

johnnorton commented Dec 7, 2017

johnnorton Dec 7, 2017

johnnorton commented Dec 13, 2017

headlessme commented Jan 5, 2018

sleepdeprecation commented Jan 15, 2018

johnnorton commented Jan 16, 2018

austinkelleher commented Jan 21, 2018

bflad left a comment

bflad Jan 23, 2018

bflad Jan 23, 2018

bflad Jan 23, 2018

bflad Jan 23, 2018

johnnorton commented Feb 2, 2018 •

edited

Loading

bflad commented Feb 2, 2018

austinkelleher commented Feb 2, 2018

bflad Feb 2, 2018

bflad Feb 2, 2018

bflad commented Feb 2, 2018

johnnorton commented Feb 2, 2018

cemo commented Feb 9, 2018

bflad commented Feb 9, 2018

bflad commented Feb 9, 2018

ghost commented Apr 8, 2020

Feature/fargate support v2 #2559

Feature/fargate support v2 #2559

Conversation

johnnorton commented Dec 5, 2017

radeksimko commented Dec 6, 2017

johnnorton commented Dec 7, 2017

Choose a reason for hiding this comment

johnnorton commented Dec 7, 2017

Choose a reason for hiding this comment

johnnorton commented Dec 13, 2017

headlessme commented Jan 5, 2018

sleepdeprecation commented Jan 15, 2018

johnnorton commented Jan 16, 2018

austinkelleher commented Jan 21, 2018

bflad 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

johnnorton commented Feb 2, 2018 • edited Loading

bflad commented Feb 2, 2018

austinkelleher commented Feb 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad commented Feb 2, 2018

johnnorton commented Feb 2, 2018

cemo commented Feb 9, 2018

bflad commented Feb 9, 2018

bflad commented Feb 9, 2018

ghost commented Apr 8, 2020

johnnorton commented Feb 2, 2018 •

edited

Loading