r/ecs_service: Add NetworkConfiguration #2299

atsushi-ishibashi · 2017-11-15T04:31:48Z

Required: #2295
Closes #2294

make testacc TEST=./aws TESTARGS='-run=TestAccAWSEcsServiceWithNetworkConfiguration'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSEcsServiceWithNetworkConfiguration -timeout 120m
=== RUN   TestAccAWSEcsServiceWithNetworkConfiguration
--- PASS: TestAccAWSEcsServiceWithNetworkConfiguration (133.20s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	133.427s

atsushi-ishibashi · 2017-11-15T04:43:40Z

ae5992c#diff-51a2b869d60207ffb83c18f0daef501dR56
To solve the below error.

=== RUN   TestAccAWSEcsServiceWithNetworkConfiguration
--- FAIL: TestAccAWSEcsServiceWithNetworkConfiguration (133.10s)
	testing.go:503: Step 0 error: After applying this step, the plan was not empty:
		
		DIFF:
		
		DESTROY/CREATE: aws_ecs_service.main
		  cluster:                                   "arn:aws:ecs:us-west-2:111111111111:cluster/tf-ecs-cluster-vb3a8" => "arn:aws:ecs:us-west-2:1111111111111:cluster/tf-ecs-cluster-vb3a8"
		  deployment_maximum_percent:                "200" => "200"
		  deployment_minimum_healthy_percent:        "100" => "100"
		  desired_count:                             "1" => "1"
		  iam_role:                                  "aws-service-role" => "" (forces new resource)
		  name:                                      "tf-ecs-service-vb3a8" => "tf-ecs-service-vb3a8"
		  network_configuration.#:                   "1" => "1"
		  network_configuration.0.security_groups.#: "2" => "2"
		  network_configuration.0.security_groups.0: "sg-5ad2dd27" => "sg-5ad2dd27"
		  network_configuration.0.security_groups.1: "sg-00d2dd7d" => "sg-00d2dd7d"
		  network_configuration.0.subnets.#:         "2" => "2"
		  network_configuration.0.subnets.0:         "subnet-a7dbb8ef" => "subnet-a7dbb8ef"
		  network_configuration.0.subnets.1:         "subnet-892b69ef" => "subnet-892b69ef"
		  task_definition:                           "arn:aws:ecs:us-west-2:111111111111:task-definition/mongodb:8" => "arn:aws:ecs:us-west-2:11111111111111:task-definition/mongodb:8"

atsushi-ishibashi · 2017-11-15T04:44:45Z

Add awsvpc to valid network modes of task definition.
ae5992c#diff-cca2f484af572d380d4255e649d57d1dR121

FernandoMiguel · 2017-11-21T16:42:59Z

any idea when this could be merged ?
We are waiting to have support for awsvpc in our infra.

thanks for the quick PR

atsushi-ishibashi · 2017-11-27T03:05:03Z

@radeksimko @Ninir It'd be great if you could review this PR:bow:
I'm eager to use this feature!

atsushi-ishibashi · 2017-11-30T11:39:21Z

This conflicts with #2483 so please review in advance:bow: @radeksimko @Ninir

apricote · 2017-11-30T23:04:52Z

The error message at https://github.com/atsushi-ishibashi/terraform-provider-aws/blob/703a7927ac3f2135845f248896b5a96fe4888df5/aws/resource_aws_ecs_task_definition.go#L126 is still missing awsvpc as a valid network mode.

// Current
errors = append(errors, fmt.Errorf("ECS Task Definition network_mode %q is invalid, must be `bridge`, `host` or `none`", value))
// Correct
errors = append(errors, fmt.Errorf("ECS Task Definition network_mode %q is invalid, must be `awsvpc`, `bridge`, `host` or `none`", value))

radeksimko

I left you some comments there, nothing too critical. The most important ones are about TypeList vs TypeSet.

I'm interested in getting this merged asap as there's high community interest in Fargate support, so I'd like to unblock that other PR. Let me know if you need any help here.

radeksimko · 2017-12-02T03:14:47Z

aws/resource_aws_ecs_service.go

@@ -353,9 +374,36 @@ 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 {
+		log.Printf("[ERR] Error setting network_configuration for (%s): %s", d.Id(), err)


Any reason why we should silently fail here instead of returning error?

radeksimko · 2017-12-02T03:16:54Z

aws/resource_aws_ecs_task_definition.go

@@ -118,6 +118,7 @@ func validateAwsEcsTaskDefinitionNetworkMode(v interface{}, k string) (ws []stri
 	validTypes := map[string]struct{}{
 		"bridge": {},
 		"host":   {},
+		"awsvpc": {},
 		"none":   {},


It's not necessarily a blocker for this PR, but I'm pondering with an idea of removing this validation completely as theoretically the list may grow again (knowing how complex and big the container networking "market" is). What do you think?

In my opinion, it's better to prevent error as early as possible. When constructing state is better than when requesting API.
So I disagree with you.
Of course, I understood your concern but it won't release new network mode as soon as modifying codes coudn't follow it.

My opinion comes from users' perspective but you have to consider maintainability more than me...
Finally I respect your decision👍

radeksimko · 2017-12-02T03:18:06Z

aws/resource_aws_ecs_service.go

+				Elem: &schema.Resource{
+					Schema: map[string]*schema.Schema{
+						"security_groups": {
+							Type:     schema.TypeList,


As the ordering of SGs is not significant shouldn't this field be TypeSet to avoid potential spurious diffs in case the list of SGs comes back in a different order from the API?

radeksimko · 2017-12-02T03:18:19Z

aws/resource_aws_ecs_service.go

+							Elem:     &schema.Schema{Type: schema.TypeString},
+						},
+						"subnets": {
+							Type:     schema.TypeList,


As the ordering of subnets is not significant shouldn't this field be TypeSet to avoid potential spurious diffs in case the list of subnets comes back in a different order from the API?

atsushi-ishibashi · 2017-12-02T12:11:07Z

@radeksimko Ok👍

make testacc TEST=./aws TESTARGS='-run=TestAccAWSEcsServiceWithNetworkConfiguration'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSEcsServiceWithNetworkConfiguration -timeout 120m
=== RUN   TestAccAWSEcsServiceWithNetworkConfiguration
--- PASS: TestAccAWSEcsServiceWithNetworkConfiguration (105.22s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	105.288s

radeksimko

Thanks, LGTM. 👍

FernandoMiguel · 2017-12-04T10:52:49Z

thanks @radeksimko and @atsushi-ishibashi

ozbillwang · 2018-01-04T03:47:48Z

website/docs/r/ecs_service.html.markdown

+`network_configuration` support the following:
+* `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.
+For more information, see [Task Networking](http://docs.aws.amazon.com/AmazonECS/latest/developerguidetask-networking.html)


@atsushi-ishibashi

The link is wrong, correct url is:

https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task-networking.html

The documentation has already been fixed in #2694

ok, but it is still wrong, in fact, the link is https.

@ozbillwang Sure. Thanks!

ghost · 2020-04-08T17:37:06Z

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!

radeksimko added the enhancement Requests to existing resources that expand the functionality or scope. label Nov 15, 2017

Add network_configuration to ecs_service

703a792

atsushi-ishibashi force-pushed the issue2294 branch from ae5992c to 703a792 Compare November 15, 2017 11:23

radeksimko added the size/L Managed by automation to categorize the size of a PR. label Nov 15, 2017

FernandoMiguel mentioned this pull request Nov 21, 2017

Feature Request: Create support for awsvpc as network mode in resource aws_ecs_task_definition #2377

Closed

apricote mentioned this pull request Nov 30, 2017

ECS Fargate Support #2483

Merged

radeksimko reviewed Dec 2, 2017

View reviewed changes

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

Reflect reviews

eb21ac4

radeksimko approved these changes Dec 4, 2017

View reviewed changes

radeksimko merged commit 8f92e28 into hashicorp:master Dec 4, 2017

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

ozbillwang reviewed Jan 4, 2018

View reviewed changes

ghost locked and limited conversation to collaborators Apr 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

r/ecs_service: Add NetworkConfiguration #2299

r/ecs_service: Add NetworkConfiguration #2299

atsushi-ishibashi commented Nov 15, 2017 •

edited by radeksimko

Loading

atsushi-ishibashi commented Nov 15, 2017 •

edited

Loading

atsushi-ishibashi commented Nov 15, 2017

FernandoMiguel commented Nov 21, 2017

atsushi-ishibashi commented Nov 27, 2017

atsushi-ishibashi commented Nov 30, 2017

apricote commented Nov 30, 2017

radeksimko left a comment

radeksimko Dec 2, 2017

radeksimko Dec 2, 2017

atsushi-ishibashi Dec 2, 2017

atsushi-ishibashi Dec 2, 2017 •

edited

Loading

radeksimko Dec 2, 2017

radeksimko Dec 2, 2017

atsushi-ishibashi commented Dec 2, 2017

radeksimko left a comment

FernandoMiguel commented Dec 4, 2017

ozbillwang Jan 4, 2018

bflad Jan 4, 2018

ozbillwang Jan 4, 2018

atsushi-ishibashi Jan 5, 2018

ghost commented Apr 8, 2020

r/ecs_service: Add NetworkConfiguration #2299

r/ecs_service: Add NetworkConfiguration #2299

Conversation

atsushi-ishibashi commented Nov 15, 2017 • edited by radeksimko Loading

atsushi-ishibashi commented Nov 15, 2017 • edited Loading

atsushi-ishibashi commented Nov 15, 2017

FernandoMiguel commented Nov 21, 2017

atsushi-ishibashi commented Nov 27, 2017

atsushi-ishibashi commented Nov 30, 2017

apricote commented Nov 30, 2017

radeksimko 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

atsushi-ishibashi Dec 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atsushi-ishibashi commented Dec 2, 2017

radeksimko left a comment

Choose a reason for hiding this comment

FernandoMiguel commented Dec 4, 2017

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

ghost commented Apr 8, 2020

atsushi-ishibashi commented Nov 15, 2017 •

edited by radeksimko

Loading

atsushi-ishibashi commented Nov 15, 2017 •

edited

Loading

atsushi-ishibashi Dec 2, 2017 •

edited

Loading