@aws-cdk/aws-ecs-patterns: Add ability to set healthCheck for NetworkLoadBalancedFargateService

[samratjha96]

Describe the feature

The QueueProcessingFargateService pattern provides the capability to supply a container health check directly as a prop. Would be great to have a similar prop for NetworkLoadBalancedFargateService so we can define container level health checks without needing to reconstruct the whole task definition from scratch:

Corresponding issue where it was added to QueueProcessingFargateService: #15636

Use Case

Need to add a healthcheck to ensure tasks are healthy and can be scheduled and run by ECS. The pattern construct doesn't allow passing one in forcing all tasks to always report HealthStatus: Unknown

Proposed Solution

Exactly the same as one applied here #18219 but in the NetworkLoadBalancedFargateService pattern:

aws-cdk/packages/@aws-cdk/aws-ecs-patterns/lib/fargate/network-load-balanced-fargate-service.ts

Line 131 in 8668e15

const container = this.taskDefinition.addContainer(containerName, {

const container = this.taskDefinition.addContainer(containerName, {
        image: taskImageOptions.image,
        logging: logDriver,
        environment: taskImageOptions.environment,
        secrets: taskImageOptions.secrets,
        dockerLabels: taskImageOptions.dockerLabels,
        _healthCheck: props.healthCheck_
      });

Other Information

Using escape hatch:

// @ts-ignore
service.taskDefinition.defaultContainer!.props.healthCheck = <ecs.HealthCheck>{
    command: ['CMD-SHELL', 'wget localhost:5000'],
    interval: cdk.Duration.seconds(15),
    retries: 3,
    timeout: cdk.Duration.seconds(5),
};

Acknowledgements

• I may be able to implement this feature request
• This feature might incur a breaking change

CDK version used

1.153.0 (build 7d90993)

Environment details (OS name and version, etc.)

MacOS Monterrey 12.3.1

[ahmetipkin]

Saw the issue yesterday myself too! How do we +1 :)

[peterwoodworth]

We accept contributions and would be happy to review a PR for this!

[corymhall]

pasting my response on the linked PR

After thinking about this more, I'm not sure we should expose this on the top level props. For an ECS service there behind a load balancer there are two different types of health checks that you can use.

• ELB health checks (configured on the NLB in this case). The NLB will make a request to the service on a specific path/port and expect a certain response.
• Container health checks (what this PR exposes). The service itself can perform health checks against the running containers (in addition to the ELB health checks).

I'm not sure this is a common use case for services running behind an ELB. The other linked PR that implemented this for the QueueProcessing service makes more sense because it is a worker service that is not running behind an ELB so container health checks are the only type of health check available.

I think in the majority of cases the user wants to configure the ELB health check and exposing this at the top level (an not the elb health check) would lead to more confusion and users configuring container health checks when they don't need to.

cc @madeline-k for a second opinion.

I'll leave this open for a little while for comments.

[github-actions]

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

[josechifflet]

Any updates ?