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

resource/ecs_service: Support ServiceRegistries #3906

Merged
merged 3 commits into from
Apr 12, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
57 changes: 57 additions & 0 deletions aws/resource_aws_ecs_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,24 @@ func resourceAwsEcsService() *schema.Resource {
},
},
},

"service_registries": {
Copy link

Choose a reason for hiding this comment

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

I think this can only support MaxItems == 1. When I tried using more than one registry in #4026 I got back an error from AWS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you!

Type: schema.TypeSet,
Copy link
Contributor

Choose a reason for hiding this comment

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

To simplify the implementation, we should opt for schema.TypeList here.

Copy link
Contributor Author

@atsushi-ishibashi atsushi-ishibashi Apr 5, 2018

Choose a reason for hiding this comment

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

So far, MaxItems is 1 but there is a possibility that aws increases it.
And the order of service_registries has no meaning, so I think set is better.

Optional: true,
ForceNew: true,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"port": {
Type: schema.TypeInt,
Optional: true,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably add ValidateFunc: validation.IntBetween(0, 65536), for this attribute

"registry_arn": {
Type: schema.TypeString,
Required: true,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably add ValidateFunc: validateArn, for this attribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure👍

},
},
},
},
}
}
Expand Down Expand Up @@ -305,6 +323,23 @@ func resourceAwsEcsServiceCreate(d *schema.ResourceData, meta interface{}) error
input.PlacementConstraints = pc
}

serviceRegistries := d.Get("service_registries").(*schema.Set).List()
if len(serviceRegistries) > 0 {
srs := make([]*ecs.ServiceRegistry, 0, len(serviceRegistries))
for _, v := range serviceRegistries {
Copy link
Contributor

Choose a reason for hiding this comment

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

We do not need the for loop for MaxItems == 1 attributes 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a possibility of increase.

raw := v.(map[string]interface{})
sr := &ecs.ServiceRegistry{
RegistryArn: aws.String(raw["registry_arn"].(string)),
}
if port, ok := raw["port"].(int); ok {
Copy link

Choose a reason for hiding this comment

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

I think you want ok && port != 0 to prevent what @stevecrozz was seeing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure👍 I'll fix

sr.Port = aws.Int64(int64(port))
}

srs = append(srs, sr)
}
input.ServiceRegistries = srs
}

log.Printf("[DEBUG] Creating ECS service: %s", input)

// Retry due to AWS IAM & ECS eventual consistency
Expand Down Expand Up @@ -445,6 +480,10 @@ func resourceAwsEcsServiceRead(d *schema.ResourceData, meta interface{}) error {
return fmt.Errorf("[ERR] Error setting network_configuration for (%s): %s", d.Id(), err)
}

if err := d.Set("service_registries", flattenServiceRegistries(service.ServiceRegistries)); err != nil {
return fmt.Errorf("[ERR] Error setting service_registries for (%s): %s", d.Id(), err)
}

return nil
}

Expand Down Expand Up @@ -521,6 +560,24 @@ func flattenPlacementStrategy(pss []*ecs.PlacementStrategy) []map[string]interfa
return results
}

func flattenServiceRegistries(srs []*ecs.ServiceRegistry) []map[string]interface{} {
if len(srs) == 0 {
return nil
}
results := make([]map[string]interface{}, 0)
for _, sr := range srs {
c := make(map[string]interface{})
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 simplified to:

m := map[string]interface{}{
  "port":         int(aws.Int64Value(sr.Port)),
  "registry_arn": aws.StringValue(sr.RegistryArn),
}
results = append(results, m)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When sr.Port is null, is it OK?

if sr.Port != nil {
c["port"] = *sr.Port
}
if sr.RegistryArn != nil {
c["registry_arn"] = *sr.RegistryArn
}
results = append(results, c)
}
return results
}

func resourceAwsEcsServiceUpdate(d *schema.ResourceData, meta interface{}) error {
conn := meta.(*AWSClient).ecsconn

Expand Down
105 changes: 105 additions & 0 deletions aws/resource_aws_ecs_service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -611,6 +611,30 @@ func TestAccAWSEcsService_withLaunchTypeEC2AndNetworkConfiguration(t *testing.T)
})
}

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

clusterName := fmt.Sprintf("tf-acc-cluster-svc-w-ups-%s", rString)
tdName := fmt.Sprintf("tf-acc-td-svc-w-ups-%s", rString)
svcName := fmt.Sprintf("tf-acc-svc-w-ups-%s", rString)

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckAWSEcsServiceDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSEcsService_withServiceRegistries(rString, clusterName, tdName, svcName),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSEcsServiceExists("aws_ecs_service.test", &service),
resource.TestCheckResourceAttr("aws_ecs_service.test", "service_registries.#", "1"),
Copy link
Contributor

Choose a reason for hiding this comment

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

We should test for service_registries.# of "0" in the basic test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure👍

),
},
},
})
}

func testAccCheckAWSEcsServiceDestroy(s *terraform.State) error {
conn := testAccProvider.Meta().(*AWSClient).ecsconn

Expand Down Expand Up @@ -1688,3 +1712,84 @@ resource "aws_ecs_service" "main" {
}
`, sg1Name, sg2Name, clusterName, tdName, svcName, securityGroups)
}

func testAccAWSEcsService_withServiceRegistries(rName, clusterName, tdName, svcName string) string {
return fmt.Sprintf(`
data "aws_availability_zones" "test" {}

resource "aws_vpc" "test" {
cidr_block = "10.0.0.0/16"
}

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

resource "aws_security_group" "test" {
name = "tf-acc-sg-%s"
vpc_id = "${aws_vpc.test.id}"

ingress {
protocol = "-1"
from_port = 0
to_port = 0
cidr_blocks = ["${aws_vpc.test.cidr_block}"]
}
}

resource "aws_service_discovery_private_dns_namespace" "test" {
name = "tf-acc-sd-%s.terraform.local"
description = "test"
vpc = "${aws_vpc.test.id}"
}

resource "aws_service_discovery_service" "test" {
name = "tf-acc-sd-%s"
dns_config {
namespace_id = "${aws_service_discovery_private_dns_namespace.test.id}"
dns_records {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nitpick: indentation

ttl = 5
type = "SRV"
}
}
}

resource "aws_ecs_cluster" "test" {
name = "%s"
}

resource "aws_ecs_task_definition" "test" {
family = "%s"
network_mode = "awsvpc"
container_definitions = <<DEFINITION
[
{
"cpu": 128,
"essential": true,
"image": "mongo:latest",
"memory": 128,
"name": "mongodb"
}
]
DEFINITION
}

resource "aws_ecs_service" "test" {
name = "%s"
cluster = "${aws_ecs_cluster.test.id}"
task_definition = "${aws_ecs_task_definition.test.arn}"
desired_count = 1
service_registries {
port = 34567
registry_arn = "${aws_service_discovery_service.test.arn}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nitpick: indentation

}
network_configuration {
security_groups = ["${aws_security_group.test.id}"]
subnets = ["${aws_subnet.test.*.id}"]
}
}
`, rName, rName, rName, clusterName, tdName, svcName)
}
10 changes: 9 additions & 1 deletion website/docs/r/ecs_service.html.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ into consideration during task placement. The maximum number of
* `placement_constraints` - (Optional) rules that are taken into consideration during task placement. Maximum number of
`placement_constraints` is `10`. Defined below.
* `network_configuration` - (Optional) The network configuration for the service. This parameter is required for task definitions that use the awsvpc network mode to receive their own Elastic Network Interface, and it is not supported for other network modes.
* `service_registries` - (Optional) The service discovery registries for the service.

-> **Note:** As a result of an AWS limitation, a single `load_balancer` can be attached to the ECS service at most. See [related docs](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/service-load-balancing.html#load-balancing-concepts).

Expand Down Expand Up @@ -106,6 +107,13 @@ Guide](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/cluster-query

For more information, see [Task Networking](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task-networking.html)

## service_registries

`service_registries` support the following:

* `registry_arn` - (Required) The ARN of the Service Registry. The currently supported service registry is Amazon Route 53 Auto Naming Service(`aws_service_discovery_service`). For more information, see [Service](https://docs.aws.amazon.com/Route53/latest/APIReference/API_autonaming_Service.html)
* `port` - (Optional) The port value used if your Service Discovery service specified an SRV record.

## Attributes Reference

The following attributes are exported:
Expand All @@ -122,4 +130,4 @@ ECS services can be imported using the `name` together with ecs cluster `name`,

```
$ terraform import aws_ecs_service.imported cluster-name/service-name
```
```