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.

Sign up for GitHub

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 Platform Version Support #6510

Merged
merged 2 commits into from
Dec 20, 2018
Merged

ECS Fargate Platform Version Support #6510

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
950238b
ECS Fargate Platform Version Support
Nov 18, 2018
6b51a94
ECS Fargate Platform Version Support
Nov 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions aws/resource_aws_ecs_service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,13 @@ func resourceAwsEcsService() *schema.Resource {
Default: "EC2",
},

"platform_version": {
Type: schema.TypeString,
ForceNew: false,
Optional: true,
Default: "LATEST",
Copy link
Contributor

Choose a reason for hiding this comment

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

Defaulting this to LATEST breaks all the other tests with launch_type set to EC2 (or unset, which defaults to EC2) with the following:

--- FAIL: TestAccAWSEcsService_withARN (4.53s)
    testing.go:538: Step 0 error: Error applying: 1 error occurred:
        	* aws_ecs_service.mongo: 1 error occurred:
        	* aws_ecs_service.mongo: InvalidParameterException: The platform version must be null when specifying an EC2 launch type.

My recommendation would be to adjust this attribute's schema to the following:

"platform_version": {
	Type:     schema.TypeString,
	Optional: true,
	Computed: true,
},

The Computed: true here will address your original concern by allowing Terraform plans to ignore any difference if the argument is not defined in the Terraform configuration.

},

"scheduling_strategy": {
Type: schema.TypeString,
Optional: true,
Expand Down Expand Up @@ -360,6 +367,10 @@ func resourceAwsEcsServiceCreate(d *schema.ResourceData, meta interface{}) error
input.LaunchType = aws.String(v.(string))
}

if v, ok := d.GetOk("platform_version"); ok {
input.PlatformVersion = aws.String(v.(string))
}

loadBalancers := expandEcsLoadBalancers(d.Get("load_balancer").(*schema.Set).List())
if len(loadBalancers) > 0 {
log.Printf("[DEBUG] Adding ECS load balancers: %s", loadBalancers)
Expand Down Expand Up @@ -538,6 +549,7 @@ func resourceAwsEcsServiceRead(d *schema.ResourceData, meta interface{}) error {
d.Set("desired_count", service.DesiredCount)
d.Set("health_check_grace_period_seconds", service.HealthCheckGracePeriodSeconds)
d.Set("launch_type", service.LaunchType)
d.Set("platform_version", service.PlatformVersion)

// Save cluster in the same format
if strings.HasPrefix(d.Get("cluster").(string), "arn:"+meta.(*AWSClient).partition+":ecs:") {
Expand Down Expand Up @@ -797,6 +809,11 @@ func resourceAwsEcsServiceUpdate(d *schema.ResourceData, meta interface{}) error
}
}

if d.HasChange("platform_version") {
updateService = true
input.PlatformVersion = aws.String(d.Get("platform_version").(string))
}

if d.HasChange("health_check_grace_period_seconds") {
updateService = true
input.HealthCheckGracePeriodSeconds = aws.Int64(int64(d.Get("health_check_grace_period_seconds").(int)))
Expand Down
149 changes: 149 additions & 0 deletions aws/resource_aws_ecs_service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -574,6 +574,64 @@ func TestAccAWSEcsService_withLaunchTypeFargate(t *testing.T) {
})
}

func TestAccAWSEcsService_withLaunchTypeFargateAndPlatformVersion(t *testing.T) {
var service ecs.Service
rString := acctest.RandString(8)

sg1Name := fmt.Sprintf("tf-acc-sg-1-svc-ltf-w-pv-%s", rString)
sg2Name := fmt.Sprintf("tf-acc-sg-2-svc-ltf-w-pv-%s", rString)
clusterName := fmt.Sprintf("tf-acc-cluster-svc-ltf-w-pv-%s", rString)
tdName := fmt.Sprintf("tf-acc-td-svc-ltf-w-pv-%s", rString)
svcName := fmt.Sprintf("tf-acc-svc-ltf-w-pv-%s", rString)

resource.Test(t, resource.TestCase{
Copy link
Contributor

Choose a reason for hiding this comment

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

We can parallelize this testing with the others via resource.ParallelTest() 😄 (dependent on the changes below to remove the extraneous other resource.Test() calls)

Suggested change
resource.Test(t, resource.TestCase{
resource.ParallelTest(t, resource.TestCase{

PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSEcsServiceDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSEcsServiceWithLaunchTypeFargateAndPlatformVersion(sg1Name, sg2Name, clusterName, tdName, svcName, "false"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, testAccAWSEcsServiceWithLaunchTypeFargateAndPlatformVersion() should be accepting the platform version as its final parameter instead of assign public IP, which can just be hardcoded to true/false in the test configuration. This will allow us to properly test updating the platform version across multiple TestStep (e.g. 1.0.0 in the first step and 1.1.0 in the second step). 👍

Check: resource.ComposeTestCheckFunc(
testAccCheckAWSEcsServiceExists("aws_ecs_service.main", &service),
resource.TestCheckResourceAttr("aws_ecs_service.main", "launch_type", "FARGATE"),
resource.TestCheckResourceAttr("aws_ecs_service.main", "platform_version", "LATEST"),
resource.TestCheckResourceAttr("aws_ecs_service.main", "network_configuration.0.assign_public_ip", "false"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're not concerned too much with the network configuration when adjusting the platform version, these network_configuration checks can be removed in preference of just ensuring we're checking the platform_version. 👍

resource.TestCheckResourceAttr("aws_ecs_service.main", "network_configuration.0.security_groups.#", "2"),
resource.TestCheckResourceAttr("aws_ecs_service.main", "network_configuration.0.subnets.#", "2"),
),
},
},
})
resource.Test(t, resource.TestCase{
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing this acceptance testing was copied from the Fargate acceptance testing above which was unfortunately using multiple invocations of resource.Test(), a pattern not used anywhere else in the codebase (in preference of a single resource.ParallelTest() with multiple TestStep). 😅 Can you move the TestStep in these two resource.Test() calls to the Steps in the first (soon-to-be) resource.ParallelTest() above? Thanks! I'll submit a pull request to fix up the other Fargate acceptance test separately.

Copy link
Contributor

Choose a reason for hiding this comment

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

And here's what I mean, fixing the other acceptance test. 😄 https://github.com/terraform-providers/terraform-provider-aws/pull/6732/files

PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSEcsServiceDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSEcsServiceWithLaunchTypeFargateAndPlatformVersion(sg1Name, sg2Name, clusterName, tdName, svcName, "true"),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSEcsServiceExists("aws_ecs_service.main", &service),
resource.TestCheckResourceAttr("aws_ecs_service.main", "network_configuration.0.assign_public_ip", "true"),
),
},
},
})
resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSEcsServiceDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSEcsServiceWithLaunchTypeFargateAndPlatformVersion(sg1Name, sg2Name, clusterName, tdName, svcName, "false"),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSEcsServiceExists("aws_ecs_service.main", &service),
resource.TestCheckResourceAttr("aws_ecs_service.main", "network_configuration.0.assign_public_ip", "false"),
),
},
},
})
}

func TestAccAWSEcsService_withLaunchTypeEC2AndNetworkConfiguration(t *testing.T) {
var service ecs.Service
rString := acctest.RandString(8)
Expand Down Expand Up @@ -1254,6 +1312,97 @@ resource "aws_ecs_service" "main" {
`, sg1Name, sg2Name, clusterName, tdName, svcName, assignPublicIP)
}

func testAccAWSEcsServiceWithLaunchTypeFargateAndPlatformVersion(sg1Name, sg2Name, clusterName, tdName, svcName, assignPublicIP string) string {
return fmt.Sprintf(`
provider "aws" {
Copy link
Contributor

Choose a reason for hiding this comment

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

We prefer to not define the AWS provider configuration in the test configurations unless necessary. The below test configuration should work in other regions beyond us-east-1, so this provider configuration should be removed. 👍

region = "us-east-1"
}
data "aws_availability_zones" "available" {}

resource "aws_vpc" "main" {
cidr_block = "10.10.0.0/16"
tags {
Name = "terraform-testacc-ecs-service-with-launch-type-fargate-and-platform-version"
}
}

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}"
tags {
Name = "tf-acc-ecs-service-with-launch-type-fargate-and-platform-version"
}
}

resource "aws_security_group" "allow_all_a" {
name = "%s"
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 = "%s"
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 = "%s"
}

resource "aws_ecs_task_definition" "mongo" {
family = "%s"
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 = "%s"
cluster = "${aws_ecs_cluster.main.id}"
task_definition = "${aws_ecs_task_definition.mongo.arn}"
desired_count = 1
launch_type = "FARGATE"
platform_version = "LATEST"
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned above, we should swap the assign public IP parameter for this one here, e.g.

Suggested change
platform_version = "LATEST"
platform_version = %q

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 = %s
}
}
`, sg1Name, sg2Name, clusterName, tdName, svcName, assignPublicIP)
}

func testAccAWSEcsService_healthCheckGracePeriodSeconds(vpcNameTag, clusterName, tdName, roleName, policyName,
lbName, svcName string, healthCheckGracePeriodSeconds int) string {
return fmt.Sprintf(`
Expand Down
1 change: 1 addition & 0 deletions website/docs/r/ecs_service.html.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ The following arguments are supported:
* `task_definition` - (Required) The family and revision (`family:revision`) or full ARN of the task definition that you want to run in your service.
* `desired_count` - (Optional) The number of instances of the task definition to place and keep running. Defaults to 0. Do not specify if using the `DAEMON` scheduling strategy.
* `launch_type` - (Optional) The launch type on which to run your service. The valid values are `EC2` and `FARGATE`. Defaults to `EC2`.
* `platform_version` - (Optional) The platform version on which to run your service. Defaults to `LATEST`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add a link to the AWS documentation and mention this only applies to the launch_type being set to FARGATE? e.g.

Suggested change
* `platform_version` - (Optional) The platform version on which to run your service. Defaults to `LATEST`.
* `platform_version` - (Optional) The platform version on which to run your service. Only applicable for `launch_type` set to `FARGATE`. Defaults to `LATEST`. More information about Fargate platform versions can be found in the [AWS ECS User Guide](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/platform_versions.html).

* `scheduling_strategy` - (Optional) The scheduling strategy to use for the service. The valid values are `REPLICA` and `DAEMON`. Defaults to `REPLICA`. Note that [*Fargate tasks do not support the `DAEMON` scheduling strategy*](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/scheduling_tasks.html).
* `cluster` - (Optional) ARN of an ECS cluster
* `iam_role` - (Optional) ARN of the IAM role that allows Amazon ECS to make calls to your load balancer on your behalf. This parameter is required if you are using a load balancer with your service, but only if your task definition does not use the `awsvpc` network mode. If using `awsvpc` network mode, do not specify this role. If your account has already created the Amazon ECS service-linked role, that role is used by default for your service unless you specify a role here.
Expand Down