-
Notifications
You must be signed in to change notification settings - Fork 164
fix: ECS Task needs memory and CPU overrides when invoked from Pipe #171
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
base: master
Are you sure you want to change the base?
Changes from all commits
d838fef
f033223
18c1d7b
0b50a15
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 |
---|---|---|
@@ -0,0 +1,65 @@ | ||
# EventBridge Pipes ECS Target | ||
|
||
Configuration in this directory creates EventBridge resource configuration including an ECS task target for a Pipe. | ||
|
||
## Usage | ||
|
||
To run this example you need to execute: | ||
|
||
```bash | ||
$ terraform init | ||
$ terraform plan | ||
$ terraform apply | ||
``` | ||
|
||
Note that this example may create resources which cost money. Run `terraform destroy` when you don't need these resources. | ||
|
||
<!-- BEGIN_TF_DOCS --> | ||
## Requirements | ||
|
||
| Name | Version | | ||
|------|---------| | ||
| <a name="requirement_terraform"></a> [terraform](#requirement\_terraform) | >= 1.5.7 | | ||
| <a name="requirement_aws"></a> [aws](#requirement\_aws) | >= 6.0 | | ||
| <a name="requirement_random"></a> [random](#requirement\_random) | >= 3.0 | | ||
|
||
## Providers | ||
|
||
| Name | Version | | ||
|------|---------| | ||
| <a name="provider_aws"></a> [aws](#provider\_aws) | 6.6.0 | | ||
| <a name="provider_random"></a> [random](#provider\_random) | 3.7.2 | | ||
|
||
## Modules | ||
|
||
| Name | Source | Version | | ||
|------|--------|---------| | ||
| <a name="module_ecs_cluster"></a> [ecs\_cluster](#module\_ecs\_cluster) | terraform-aws-modules/ecs/aws | ~> 6.1 | | ||
| <a name="module_eventbridge"></a> [eventbridge](#module\_eventbridge) | ../../ | n/a | | ||
|
||
## Resources | ||
|
||
| Name | Type | | ||
|------|------| | ||
| [aws_iam_policy.eventbridge_pipes_ecs_policy](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_policy) | resource | | ||
| [aws_iam_role_policy_attachment.eventbridge_pipes_ecs_policy](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role_policy_attachment) | resource | | ||
| [aws_sqs_queue.source](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/sqs_queue) | resource | | ||
| [random_pet.this](https://registry.terraform.io/providers/hashicorp/random/latest/docs/resources/pet) | resource | | ||
| [aws_caller_identity.current](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/caller_identity) | data source | | ||
| [aws_region.current](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/region) | data source | | ||
| [aws_security_group.default](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/security_group) | data source | | ||
| [aws_subnets.default](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/subnets) | data source | | ||
| [aws_vpc.default](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/vpc) | data source | | ||
|
||
## Inputs | ||
|
||
No inputs. | ||
|
||
## Outputs | ||
|
||
| Name | Description | | ||
|------|-------------| | ||
| <a name="output_eventbridge_bus_arn"></a> [eventbridge\_bus\_arn](#output\_eventbridge\_bus\_arn) | The EventBridge Bus ARN | | ||
| <a name="output_eventbridge_rule_arns"></a> [eventbridge\_rule\_arns](#output\_eventbridge\_rule\_arns) | The EventBridge Rule ARNs | | ||
| <a name="output_eventbridge_rule_ids"></a> [eventbridge\_rule\_ids](#output\_eventbridge\_rule\_ids) | The EventBridge Rule IDs | | ||
<!-- END_TF_DOCS --> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,161 @@ | ||
provider "aws" { | ||
region = "eu-west-1" | ||
|
||
# Make it faster by skipping something | ||
skip_metadata_api_check = true | ||
skip_region_validation = true | ||
skip_credentials_validation = true | ||
} | ||
|
||
############################################################# | ||
# Data sources to get VPC and default security group details | ||
############################################################# | ||
data "aws_vpc" "default" { | ||
default = true | ||
} | ||
|
||
data "aws_security_group" "default" { | ||
name = "default" | ||
vpc_id = data.aws_vpc.default.id | ||
} | ||
|
||
data "aws_subnets" "default" { | ||
filter { | ||
name = "vpc-id" | ||
values = [data.aws_vpc.default.id] | ||
} | ||
} | ||
|
||
data "aws_caller_identity" "current" {} | ||
data "aws_region" "current" {} | ||
|
||
resource "aws_sqs_queue" "source" { | ||
name = "${random_pet.this.id}-source" | ||
} | ||
|
||
#################### | ||
# Actual Eventbridge | ||
#################### | ||
module "eventbridge" { | ||
source = "../../" | ||
|
||
# Schedules can only be created on default bus | ||
create_bus = false | ||
|
||
create_role = true | ||
role_name = "ecs-eventbridge-${random_pet.this.id}" | ||
|
||
pipes = { | ||
test_ecs_pipe = { | ||
|
||
source = aws_sqs_queue.source.arn | ||
target = module.ecs_cluster.cluster_arn | ||
|
||
attach_policies_for_integrations = true | ||
|
||
target_parameters = { | ||
ecs_task_parameters = { | ||
assign_public_ip = "ENABLED" | ||
task_count = 1 | ||
launch_type = "FARGATE" | ||
task_definition_arn = module.ecs_cluster.services["hello-world"].task_definition_arn | ||
container_name = "hello-world" | ||
|
||
security_groups = [data.aws_security_group.default.id] | ||
subnets = data.aws_subnets.default.ids | ||
|
||
memory = 512 | ||
cpu = 256 | ||
|
||
enable_ecs_managed_tags = true | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
resource "aws_iam_policy" "eventbridge_pipes_ecs_policy" { | ||
oli-pr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
name = "test-pipes-ecs-policy" | ||
description = "Policy for EventBridge Pipes to run ECS tasks" | ||
|
||
policy = jsonencode({ | ||
Version = "2012-10-17" | ||
Statement = [ | ||
{ | ||
Effect = "Allow" | ||
Action = [ | ||
"ecs:RunTask", | ||
"ecs:TagResource" | ||
] | ||
Resource = [module.ecs_cluster.services["hello-world"].task_definition_arn] | ||
}, | ||
{ | ||
Effect = "Allow" | ||
Action = [ | ||
"iam:PassRole" | ||
] | ||
Resource = [ | ||
module.ecs_cluster.services["hello-world"].task_exec_iam_role_arn, | ||
module.ecs_cluster.services["hello-world"].tasks_iam_role_arn | ||
] | ||
Condition = { | ||
StringLike = { | ||
"iam:PassedToService" = "ecs-tasks.amazonaws.com" | ||
} | ||
} | ||
} | ||
] | ||
}) | ||
} | ||
|
||
resource "aws_iam_role_policy_attachment" "eventbridge_pipes_ecs_policy" { | ||
for_each = module.eventbridge.eventbridge_pipe_role_names | ||
|
||
role = each.value | ||
policy_arn = aws_iam_policy.eventbridge_pipes_ecs_policy.arn | ||
} | ||
|
||
###### | ||
# ECS | ||
###### | ||
|
||
module "ecs_cluster" { | ||
source = "terraform-aws-modules/ecs/aws" | ||
version = "~> 6.1" | ||
|
||
cluster_name = random_pet.this.id | ||
|
||
|
||
default_capacity_provider_strategy = { | ||
"FARGATE" = { | ||
weight = 100 | ||
} | ||
} | ||
|
||
services = { | ||
hello-world = { | ||
create_service = false | ||
subnet_ids = data.aws_subnets.default.ids | ||
desired_count = 1 | ||
deployment_maximum_percent = 100 | ||
deployment_minimum_healthy_percent = 0 | ||
|
||
container_definitions = { | ||
hello-world = { | ||
image = "public.ecr.aws/docker/library/hello-world: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. @bryantbiggs The task def does include the CPU and memory settings? Have I misunderstood your concern?
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. @bryantbiggs @antonbabenko please could I ask for a little more of your time on this? I am very happy to re-work the test code to use an existing example if that is a better fit. I still think this is a fix rather than a feature request as I have been unable to get an ECS Target working with pipes as recorded by this issue #170 If I use the existing pipes example I would need to add the ecs cluster necessary to run it and a workaround for this issue #169 If you're happy with that I will make those revisions. I think on investigation this issue is actually related to this in the provider: |
||
cpu = 256, | ||
memory = 512 | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
################## | ||
# Extra resources | ||
################## | ||
|
||
resource "random_pet" "this" { | ||
length = 2 | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
output "eventbridge_bus_arn" { | ||
description = "The EventBridge Bus ARN" | ||
value = module.eventbridge.eventbridge_bus_arn | ||
} | ||
|
||
output "eventbridge_rule_ids" { | ||
description = "The EventBridge Rule IDs" | ||
value = module.eventbridge.eventbridge_rule_ids | ||
} | ||
|
||
output "eventbridge_rule_arns" { | ||
description = "The EventBridge Rule ARNs" | ||
value = module.eventbridge.eventbridge_rule_arns | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
terraform { | ||
oli-pr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
required_version = ">= 1.5.7" | ||
|
||
required_providers { | ||
aws = { | ||
source = "hashicorp/aws" | ||
version = ">= 6.0" | ||
} | ||
random = { | ||
source = "hashicorp/random" | ||
version = ">= 3.0" | ||
} | ||
} | ||
} |
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.
why do we need a net new example for this?
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.
Happy to merge this with the other pipes example but will need to add the ECS cluster and the policy attachment required as the workaround if that works for you?
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.
you're not explaining the issues very well and instead rushing to solutions. I think this PR is more of a feature request (we should support all of the arguments provided by the underlying resources, so adding them is great) - but you are stating its an issue because you have not:
Which is why you are seeing an error - the EventBridge service is trying to invoke the task but it can't without some idea of what amount of CPU/memory is required
Thats one issue I see (*I think - again, not very well flushed out here)
The other one is that of permissions - its not clear why we are adding permissions in the example. Is this a module deficiency? If so, how have we proven that - the module has a very similar set of permissions so is there a bug somewhere or misconfiguration?
Uh oh!
There was an error while loading. Please reload this page.
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.
Without this change when the task gets run container overrides were provided which set both the CPU and Mem to zero rendering ECS target on a pipe as unusable. I think you are implying I have a possible usage error, I will go away and check this - sorry if I missed something. This was supposed to be a fix for ECS Pipe Target - ECS Task CPU and Memory are zero and invalid #170
In trying to fix this I also had to workaround this issue (to construct the test case) which I don't yet have a fix for, again if this is some kind of usage issue I apologise in advance. Incorrect IAM Policy when using ECS Target for Pipe #169
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.
If these are usage issues then I will look to see if there is something I can do to improve an example. As it stands I don't consider this a "feature request" but things I have had to put in place in our environment as I was unable to get an ECS target working in Event Bridge pipes without them.
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.
Looking at the implementation log for this I don't understand currently why this commit was made:
1eebcf5
There isn't an associated example with it so I am not 100pc sure but it seems like ECS Task target is not currently usable IMO