-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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 Platform Version Support #6510
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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{ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can parallelize this testing with the others via
Suggested change
|
||||||
PreCheck: func() { testAccPreCheck(t) }, | ||||||
Providers: testAccProviders, | ||||||
CheckDestroy: testAccCheckAWSEcsServiceDestroy, | ||||||
Steps: []resource.TestStep{ | ||||||
{ | ||||||
Config: testAccAWSEcsServiceWithLaunchTypeFargateAndPlatformVersion(sg1Name, sg2Name, clusterName, tdName, svcName, "false"), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally, |
||||||
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"), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||
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{ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||||||
|
@@ -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" { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||
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" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||||||
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(` | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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`. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
|
||||||
* `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. | ||||||
|
There was a problem hiding this comment.
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 withlaunch_type
set toEC2
(or unset, which defaults toEC2
) with the following:My recommendation would be to adjust this attribute's schema to the following:
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.