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

ECS Fargate Support #2483

Merged
merged 5 commits into from
Dec 5, 2017
Merged

ECS Fargate Support #2483

merged 5 commits into from
Dec 5, 2017

Conversation

apricote
Copy link
Contributor

@apricote 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
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
Copy link
Contributor Author

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 radeksimko added the enhancement Requests to existing resources that expand the functionality or scope. label Nov 30, 2017
@radeksimko
Copy link
Member

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

@radeksimko radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Nov 30, 2017
@apricote
Copy link
Contributor Author

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.

@radeksimko radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Dec 1, 2017
Copy link
Contributor

@Ninir Ninir left a comment

Choose a reason for hiding this comment

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

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.

data "aws_availability_zones" "available" {}

resource "aws_vpc" "main" {
cidr_block = "10.10.0.0/16"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

resource "aws_security_group" "allow_all_a" {
name = "allow_all_a"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Optional: true,
ForceNew: true,
Elem: &schema.Schema{Type: schema.TypeString},
Set: schema.HashString,
Copy link
Contributor

Choose a reason for hiding this comment

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

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`.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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"]

@@ -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`.
Copy link
Contributor

Choose a reason for hiding this comment

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

We are omitting the (Optional, Forces new resource)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@radeksimko
Copy link
Member

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

@apricote
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
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
Copy link
Contributor

@apricote Can I help?

@apricote
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
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
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": "*"
        }
    ]
}

* tabs->spaces
* default value for launch_type
* randomize resource names in tests
* improve require_compatibilities doc
@apricote
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
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

Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

@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)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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" {}
Copy link
Member

Choose a reason for hiding this comment

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

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.

@radeksimko radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Dec 5, 2017
@apricote
Copy link
Contributor Author

apricote commented Dec 5, 2017

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

@radeksimko radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Dec 5, 2017
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

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 hashicorp:master Dec 5, 2017
@korbin
Copy link

korbin commented Dec 5, 2017

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

@johnnorton
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
Copy link
Contributor

@apricote Created PR #2559

@apricote apricote deleted the feature/ecs-fargate-support branch December 6, 2017 14:18
@jritsema
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 pushed a commit to mdlavin/terraform-provider-aws that referenced this pull request Dec 9, 2017
* ecs: fargate implementation, acctests, docs hashicorp#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
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!

@ghost
Copy link

ghost commented Apr 10, 2020

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!

@ghost ghost locked and limited conversation to collaborators Apr 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fargate resource
8 participants