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

ECS Fargate Support #2483

Merged
merged 5 commits into from Dec 5, 2017

Conversation

Projects
None yet
8 participants
@apricote
Copy link
Contributor

apricote commented Nov 30, 2017

Depends on #2474
Closes #2471

Hello all,
I have never before implemented something in Go, but the features promised by Fargate were just too impressive to wait for. I Implemented most (all?) endpoint changes related to the new Launch Type.

There are still some problems, i could not find the source for the documentation, so I could not update it. Also I didnt really know what to test, or how to test it. I am glad about every hint that you can give me, so I can catch up on those.

I am particularly unhappy with the implementation of the aws_ecs_service.network_configuration. A JSON String just seems inappropriate, but I was unable to figure out how to do nested objects.

EDIT: Included wrong # for the AWS SDK PR

@Puneeth-n

This comment has been minimized.

Copy link
Contributor

Puneeth-n commented Nov 30, 2017

@apricote I think a simple Fargate Acceptance test here and here is needed

You can run the tests like make testacc TESTARGS='-run=TestAccAWSEcsTaskDefinition_basic

@apricote

This comment has been minimized.

Copy link
Contributor Author

apricote commented Nov 30, 2017

As it seems there is already an implementation of networkConfiguration in #2299, so I will remove my own implementation and depend on that PR.

Tests will be implemented later.

@radeksimko

This comment has been minimized.

Copy link
Contributor

radeksimko commented Nov 30, 2017

Hi @apricote 👋
thanks for raising this PR.

I just merged #2474 so you're free to rebase this PR and remove all vendor changes. 😉

@Puneeth-n is right about running acceptance tests. Do you mind adding a test which covers this new functionality? Feel free to take the copy-paste-modify approach. You can find all relevant existing tests in https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_ecs_service_test.go and https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_ecs_task_definition_test.go

As it seems there is already an implementation of networkConfiguration in #2299, so I will remove my own implementation and depend on that PR.

yes, please - I'll take a look at the other PR and then let you rebase here if necessary.

I have a feeling that the addition of launch_type may require state migration and possibly Default to avoid introduction of breaking change for existing users. I can verify that to be sure and point you to some examples or help with it after you do the rebase.

Also we'll need to update the relevant documentation in https://github.com/terraform-providers/terraform-provider-aws/blob/master/website/docs/r/ecs_service.html.markdown and https://github.com/terraform-providers/terraform-provider-aws/blob/master/website/docs/r/ecs_task_definition.html.markdown

@apricote apricote force-pushed the apricote:feature/ecs-fargate-support branch 2 times, most recently Nov 30, 2017

@apricote

This comment has been minimized.

Copy link
Contributor Author

apricote commented Nov 30, 2017

I have now rebased against the new master. Also, I removed my NetworkConfiguration Implementation, so now this PR depends on #2299.

Tests and Docs have been added.

$ make testacc TESTARGS='-run=TestAccAWSEcsServiceWithLaunchTypeFargate'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAWSEcsServiceWithLaunchTypeFargate -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSEcsServiceWithLaunchTypeFargate
--- PASS: TestAccAWSEcsServiceWithLaunchTypeFargate (125.99s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	126.003s

$ make testacc TESTARGS='-run=TestAccAWSEcsTaskDefinition_Fargate'                            
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAWSEcsTaskDefinition_Fargate -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSEcsTaskDefinition_Fargate
--- PASS: TestAccAWSEcsTaskDefinition_Fargate (25.31s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	25.318s

Fargate has not yet been rolled out in all Regions, I know for sure that it is supported in us-east-1. So be sure to test in these regions.

@Ninir
Copy link
Collaborator

Ninir left a comment

Hi @apricote

Thanks for the work here! 😄 🚀
Just left a few nitpicks in addition to @radeksimko's review.

Also, can you check this part of his review?

I have a feeling that the addition of launch_type may require state migration and possibly Default to avoid introduction of breaking change for existing users. I can verify that to be sure and point you to some examples or help with it after you do the rebase.

aws/resource_aws_ecs_service_test.go Outdated
data "aws_availability_zones" "available" {}

resource "aws_vpc" "main" {
cidr_block = "10.10.0.0/16"

This comment has been minimized.

@Ninir

Ninir Dec 4, 2017

Collaborator

Can you use 2 spaces per indentation in the tests & docs?

aws/resource_aws_ecs_service_test.go Outdated
}

resource "aws_security_group" "allow_all_a" {
name = "allow_all_a"

This comment has been minimized.

@Ninir

Ninir Dec 4, 2017

Collaborator

Can you add some randomisation to the names used by tests? this avoids issues when running them in parallel.

aws/resource_aws_ecs_task_definition.go Outdated
Optional: true,
ForceNew: true,
Elem: &schema.Schema{Type: schema.TypeString},
Set: schema.HashString,

This comment has been minimized.

@Ninir

Ninir Dec 4, 2017

Collaborator

This can be removed as this is taken care in the background

@@ -69,6 +69,7 @@ Load balancers support the following:
* `target_group_arn` - (Required for ALB) The ARN of the ALB target group to associate with the service.
* `container_name` - (Required) The name of the container to associate with the load balancer (as it appears in a container definition).
* `container_port` - (Required) The port on the container to associate with the load balancer.
* `launch_type` - (Optional) The launch type on which to run your service. The valid values are `EC2` and `FARGATE`. Defaults to `EC2`.

This comment has been minimized.

@Ninir

Ninir Dec 4, 2017

Collaborator

While you write defaults to EC2, we actually do not set any defaults. Is there something I'm missing?

This comment has been minimized.

@apricote

apricote Dec 4, 2017

Author Contributor

The AWS API default to API because otherwise, it would have been a breaking change for them. Might be better not the mix AWS specifics into the Terraform documentation.

This comment has been minimized.

@Ninir

Ninir Dec 4, 2017

Collaborator

The AWS API default to API

Not sure to get it here: Is API a typo or really a valid value? can not find anything on that.

Also, we already have default values in a lot of places, whether those are booleans, floats, etc, in order to mimic the AWS API correctly.

This comment has been minimized.

@apricote

apricote Dec 4, 2017

Author Contributor

It is a typo, I meant EC2. So you would suggest adopting the AWS default into the Schema? What about the requires_compatibilities field of r/aws_ecs_task_definition, it defaults to ["EC2"]

website/docs/r/ecs_task_definition.html.markdown Outdated
@@ -80,6 +80,9 @@ official [Developer Guide](https://docs.aws.amazon.com/AmazonECS/latest/develope
* `network_mode` - (Optional) The Docker networking mode to use for the containers in the task. The valid values are `none`, `bridge`, and `host`.
* `volume` - (Optional) A set of [volume blocks](#volume-block-arguments) that containers in your task may use.
* `placement_constraints` - (Optional) A set of [placement constraints](#placement-constraints-arguments) rules that are taken into consideration during task placement. Maximum number of `placement_constraints` is `10`.
* `cpu` - (Optional) The number of cpu units used by the task. If the `launch_type` is `FARGATE` this field is required.
* `memory` - (Optional) The amount (in MiB) of memory used by the task. If the `launch_type` is `FARGATE` this field is required.
* `requires_compatibilities` - A set of launch types required by the task. The valid values are `EC2` and `FARGATE`. If no value is specified, it defaults to `EC2`.

This comment has been minimized.

@Ninir

Ninir Dec 4, 2017

Collaborator

We are omitting the (Optional, Forces new resource)

This comment has been minimized.

@apricote

apricote Dec 4, 2017

Author Contributor

What do you mean by this, should I add Forces new resource to the documentation of added fields where it applies?

@radeksimko

This comment has been minimized.

Copy link
Contributor

radeksimko commented Dec 4, 2017

I just merged #2299 so you can rebase this PR. 😉

@apricote

This comment has been minimized.

Copy link
Contributor Author

apricote commented Dec 4, 2017

I have a feeling that the addition of launch_type may require state migration and possibly Default to avoid introduction of breaking change for existing users. I can verify that to be sure and point you to some examples or help with it after you do the rebase.

@radeksimko @Ninir Can I just create a TF state with the current master and then check what happens when i use my changes?

@korbin

This comment has been minimized.

Copy link

korbin commented Dec 4, 2017

Was just testing this PR after rebasing against master:

I didn't see execution_role_arn (on task definition) in this patch - it is required to create Fargate tasks from what I can tell:

http://docs.aws.amazon.com/sdkforruby/api/Aws/ECS/Types/RegisterTaskDefinitionRequest.html

@johnnorton

This comment has been minimized.

Copy link
Contributor

johnnorton commented Dec 4, 2017

@apricote Can I help?

@apricote

This comment has been minimized.

Copy link
Contributor Author

apricote commented Dec 4, 2017

@korbin Do you get some kind of error? I can use this PR to register the service with Fargate using the configuration from the test case. Currently, it crashes for me on actually running the task because of Reasons: CannotPullContainerError: API error (500): Get https://registry-1.docker.io/v2/: net/http: request canceled while waiting for connection (Client.Timeout exceeded while awaiting headers) So I can not verify it.

data "aws_availability_zones" "available" {}

resource "aws_vpc" "main" {
  cidr_block = "10.10.0.0/16"
}

resource "aws_subnet" "main" {
  count = 2
  cidr_block = "${cidrsubnet(aws_vpc.main.cidr_block, 8, count.index)}"
  availability_zone = "${data.aws_availability_zones.available.names[count.index]}"
  vpc_id = "${aws_vpc.main.id}"
}

resource "aws_security_group" "allow_all_a" {
  name        = "allow_all_a_1"
  description = "Allow all inbound traffic"
  vpc_id      = "${aws_vpc.main.id}"

  ingress {
    protocol = "6"
    from_port = 80
    to_port = 8000
    cidr_blocks = ["${aws_vpc.main.cidr_block}"]
  }
}

resource "aws_security_group" "allow_all_b" {
  name        = "allow_all_b_1"
  description = "Allow all inbound traffic"
  vpc_id      = "${aws_vpc.main.id}"

  ingress {
    protocol = "6"
    from_port = 80
    to_port = 8000
    cidr_blocks = ["${aws_vpc.main.cidr_block}"]
  }
}

resource "aws_ecs_cluster" "main" {
  name = "tf-ecs-cluster-1"
}

resource "aws_ecs_task_definition" "mongo" {
  family = "mongodb"
  network_mode = "awsvpc"
  requires_compatibilities = ["FARGATE"]
  cpu = "256"
  memory = "512"

  container_definitions = <<DEFINITION
[
  {
    "cpu": 256,
    "essential": true,
    "image": "mongo:latest",
    "memory": 512,
    "name": "mongodb",
    "networkMode": "awsvpc"
  }
]
DEFINITION
}

resource "aws_ecs_service" "main" {
  name = "tf-ecs-service-1"
  cluster = "${aws_ecs_cluster.main.id}"
  task_definition = "${aws_ecs_task_definition.mongo.arn}"
  desired_count = 1
  launch_type = "FARGATE"
  network_configuration {
    security_groups = ["${aws_security_group.allow_all_a.id}", "${aws_security_group.allow_all_b.id}"]
    subnets = ["${aws_subnet.main.*.id}"]
  }
}
@apricote

This comment has been minimized.

Copy link
Contributor Author

apricote commented Dec 4, 2017

@radeksimko @korbin I just tried creating a state with master and then switching to my branch and there is a diff for the aws_ecs_service:

-/+ aws_ecs_service.main (new resource required)
      launch_type:                                        "EC2" => "" (forces new resource)

So I will include a default of EC2 for this field. The same does not seem to be true for the requires_possibilities field of r/aws_ecs_task_definition, so I will not include a default value for this property for now.

@korbin

This comment has been minimized.

Copy link

korbin commented Dec 4, 2017

@apricote I added the execution_role_arn field by copying the task_role_arn field 1:1 and Fargate works 100% as expected.

This field is required though as far as I can tell, and whatever execution_role you are using must have access to the ECR registries and a few other things to work.

--

When using the wizard, AWS attaches this AmazonECSTaskExecutionRolePolicy to a new role they create on your behalf: ecsTaskExecutionRole

This policy contains:

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Effect": "Allow",
            "Action": [
                "ecr:GetAuthorizationToken",
                "ecr:BatchCheckLayerAvailability",
                "ecr:GetDownloadUrlForLayer",
                "ecr:BatchGetImage",
                "logs:CreateLogStream",
                "logs:PutLogEvents"
            ],
            "Resource": "*"
        }
    ]
}

apricote added some commits Nov 30, 2017

implement review
* tabs->spaces
* default value for launch_type
* randomize resource names in tests
* improve require_compatibilities doc

@apricote apricote force-pushed the apricote:feature/ecs-fargate-support branch to 16679e7 Dec 4, 2017

@apricote

This comment has been minimized.

Copy link
Contributor Author

apricote commented Dec 5, 2017

@korbin The official docs declare it as optional. I think you only need it if you are going to use some custom image from the ECR, in case you use something from Docker Hub it should work without the execution role.

Took me quite some time to find the correct statement for the trust relationship of the IAM Role, so here it is from the AWS-generated ecsTaskExecutionRole:

{
  "Version": "2008-10-17",
  "Statement": [
    {
      "Sid": "",
      "Effect": "Allow",
      "Principal": {
        "Service": "ecs-tasks.amazonaws.com"
      },
      "Action": "sts:AssumeRole"
    }
  ]
}

I added a new test case for this field, as it is not required for Fargate support.

@apricote

This comment has been minimized.

Copy link
Contributor Author

apricote commented Dec 5, 2017

$ make testacc TESTARGS='-run=TestAccAWSEcsTaskDefinition_Fargate'      
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAWSEcsTaskDefinition_Fargate -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSEcsTaskDefinition_Fargate
--- PASS: TestAccAWSEcsTaskDefinition_Fargate (23.10s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	23.106s

$ make testacc TESTARGS='-run=TestAccAWSEcsTaskDefinition_ExecutionRole'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAWSEcsTaskDefinition_ExecutionRole -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSEcsTaskDefinition_ExecutionRole
--- PASS: TestAccAWSEcsTaskDefinition_ExecutionRole (29.18s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	29.188s

$ make testacc TESTARGS='-run=TestAccAWSEcsServiceWithLaunchTypeFargate'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAWSEcsServiceWithLaunchTypeFargate -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSEcsServiceWithLaunchTypeFargate
--- PASS: TestAccAWSEcsServiceWithLaunchTypeFargate (127.69s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	127.699s
@radeksimko
Copy link
Contributor

radeksimko left a comment

@apricote thanks for all the changes!
I played with this locally by building from your branch - all looking good functional/UX-wise.

I just left two comments - once those are addressed I'm happy to merge this. 😉

d.Set("network_mode", taskDefinition.NetworkMode)
d.Set("volumes", flattenEcsVolumes(taskDefinition.Volumes))
if err := d.Set("placement_constraints", flattenPlacementConstraints(taskDefinition.PlacementConstraints)); err != nil {
log.Printf("[ERR] Error setting placement_constraints for (%s): %s", d.Id(), err)
}

d.Set("requires_compatibilities", taskDefinition.RequiresCompatibilities)
if err := d.Set("requires_compatibilities", flattenStringList(taskDefinition.RequiresCompatibilities)); err != nil {
log.Printf("[ERR] Error setting requires_compatibilities for (%s): %s", d.Id(), err)

This comment has been minimized.

@radeksimko

radeksimko Dec 5, 2017

Contributor

Is there any particular reason why we should silently ignore this error? Also why we're setting the same field twice? I think the one above won't work as we're passing []*string.

This comment has been minimized.

@apricote

apricote Dec 5, 2017

Author Contributor

No actual reason I just copied the code from the lines above. Will return the error.

@@ -597,6 +615,86 @@ resource "aws_ecs_service" "mongo" {
`, rInt, rInt)
}

func testAccAWSEcsServiceWithLaunchTypeFargate(rInt int) string {
return fmt.Sprintf(`
data "aws_availability_zones" "available" {}

This comment has been minimized.

@radeksimko

radeksimko Dec 5, 2017

Contributor

All of our acceptance tests run in us-west-2 by default and Fargate unfortunately isn't available there yet, so we'll need to pin this test to us-east-1, otherwise it fails:

=== RUN   TestAccAWSEcsServiceWithLaunchTypeFargate
--- FAIL: TestAccAWSEcsServiceWithLaunchTypeFargate (62.17s)
	testing.go:503: Step 0 error: Error applying: 1 error(s) occurred:

		* aws_ecs_service.main: 1 error(s) occurred:

		* aws_ecs_service.main: UnsupportedFeatureException: FARGATE is not supported in this region.
			status code: 400, request id: dc224bee-d9c1-11e7-a440-c32534bb3ba4 "tf-ecs-service-8420937161126328979"

We can do so by adding a provider block to the config here, e.g.

provider "aws" {
  region = "us-east-1"
}

we do that for a couple of other services already for the same reason, like Lightsail.

@apricote

This comment has been minimized.

Copy link
Contributor Author

apricote commented Dec 5, 2017

@radeksimko Done. Thanks for all the help and advice 😊

@radeksimko
Copy link
Contributor

radeksimko left a comment

Code looking good to me - I'm just waiting for all ECS acceptance test to finish. If they all come back as passing I'll merge this. 😉

@radeksimko radeksimko merged commit 26d9e1d into terraform-providers:master Dec 5, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@korbin

This comment has been minimized.

Copy link

korbin commented Dec 5, 2017

@apricote Yep! I was using ECR and needed that! Did not test the non-ECR case.

@johnnorton

This comment has been minimized.

Copy link
Contributor

johnnorton commented Dec 5, 2017

@apricote @radeksimko . -> . Looks like we missed support for VPC Config "Assign Public IP" for ECS Service. Default is DISABLED but for Fargate we need to enable it on the ENI so we can pull from ECS. I will create a new pull request.

image

@johnnorton

This comment has been minimized.

Copy link
Contributor

johnnorton commented Dec 5, 2017

@apricote Created PR #2559

@apricote apricote deleted the apricote:feature/ecs-fargate-support branch Dec 6, 2017

@jritsema

This comment has been minimized.

Copy link

jritsema commented Dec 6, 2017

Thanks @apricote, et al. for getting this done so quickly! Just curious, any idea when 1.6.0 will be released?

mdlavin added a commit to mdlavin/terraform-provider-aws that referenced this pull request Dec 9, 2017

ECS Fargate Support (terraform-providers#2483)
* ecs: fargate implementation, acctests, docs terraform-providers#2483

* implement review

* tabs->spaces
* default value for launch_type
* randomize resource names in tests
* improve require_compatibilities doc

* r/aws_ecs_task_definition: add field execution_role_arn

* r/aws_ecs_service: add region for fargate test

* r/aws_ecs_task_definition: fix setting field twice and return error
@olingern

This comment has been minimized.

Copy link

olingern commented Dec 13, 2017

Following up on @jritsema 's question. Currently building infra around fargate, and would love to use this.

Thanks for all the work and pr review to date!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment