search

aws-cloudformation / cfn-lint

CloudFormation Linter
MIT No Attribution
2.4k stars 578 forks source link

The task definition is configured to use a dynamic host port, but the target group has a health check port specified #817

Open ion9 opened 5 years ago

ion9 commented 5 years ago

cfn-lint version: (cfn-lint --version)

pip install cfn-lint
Collecting cfn-lint
  Downloading https://files.pythonhosted.org/packages/97/1c/eca74684dea7385c505c52eb5adf578e83f7d3e6890c50791753c54b1639/cfn-lint-0.18.1.tar.gz (1.8MB)

Description of issue.

When updating a stack I found that there is no check for this

The task definition is configured to use a dynamic host port, but the target group with targetGroupArn arn:aws:elasticloadbalancing:us-east-1:**********:targetgroup/*************/bcd1d8e7fa7324f8 has a health check port specified. (Service: AmazonECS; Status Code: 400; Error Code: InvalidParameterException; Request ID: **************)
, removeing the healthcheckport works as the default is what we need to support dynamic ports.

Please provide as much information as possible:

    Type: AWS::ElasticLoadBalancingV2::TargetGroup
    Properties:
      HealthCheckIntervalSeconds: 30
      HealthCheckPath: '/'
      HealthCheckPort: '3000'
      HealthCheckProtocol: HTTP
      HealthCheckTimeoutSeconds: 25
      Matcher:
        HttpCode: '200'
      Port: 3000
      Protocol: HTTP

working template,

  TaskTargetGroup:
    Type: AWS::ElasticLoadBalancingV2::TargetGroup
    Properties:
      HealthCheckIntervalSeconds: 30
      HealthCheckPath: '/'
      HealthCheckPort: 'traffic-port'
      HealthCheckProtocol: HTTP
      HealthCheckTimeoutSeconds: 25
      Matcher:
        HttpCode: '200'
      Port: 3000
      Protocol: HTTP

Cfn-lint uses the CloudFormation Resource Specifications as the base to do validation. These files are included as part of the application version. Please update to the latest version of cfn-lint or update the spec files manually (cfn-lint -u)

kddejong commented 5 years ago

Interesting. So this looks associated to the ECS Task Definition? So we would have to associate the ECS Task to the ALB being used? Validating that ECS is using Port 0 which means HealthCheckPort: 'traffic-port'

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ecs-taskdefinition-containerdefinitions-portmappings.html#cfn-ecs-taskdefinition-containerdefinition-portmappings-hostport

ion9 commented 5 years ago

So many odd little quirks across the services. This one failed the pipe line, so I thought why not try an issue. The template might have the info you need to make a check. Let me know if I can help.

On Wed, Apr 10, 2019, 8:32 PM Kevin DeJong notifications@github.com wrote:

Interesting. So this looks associated to the ECS Task Definition? So we would have to associate the ECS Task to the ALB being used? Validating that ECS is using Port 0 which means HealthCheckPort: 'traffic-port'

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ecs-taskdefinition-containerdefinitions-portmappings.html#cfn-ecs-taskdefinition-containerdefinition-portmappings-hostport

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/aws-cloudformation/cfn-python-lint/issues/817#issuecomment-481920861, or mute the thread https://github.com/notifications/unsubscribe-auth/ABjykKWQkGYwanetjxTsdujk-o1yJ9QCks5vfoKWgaJpZM4cibYA .

kddejong commented 5 years ago

Yes please create issues for this. These are the types of things we are trying to detect. We do have rules that compare values across resources so we should be able to check for this. We would need the ALB resource in the same template to do this work though.

gabrieleCutowl commented 7 months ago

Of course cdk has the same behaviour.

Using "traffic-port", as suggested, fixes the error.

...
healthCheck: {
   protocol: elb.Protocol.HTTP,
   path: '/healthz',
   port: "traffic-port",
   healthyThresholdCount: 2,
   unhealthyThresholdCount: 2,
   interval: cdk.Duration.seconds(10),
   timeout: cdk.Duration.seconds(5)
}
...
myahl-uncomn commented 1 month ago

This one got me, too. Even though the default HealthCheckPort is 'traffic-port', you have to specify it anyways, or the update fails when using dynamic host ports. Is there any intent to add this check to the standard rules?

kddejong commented 2 days ago

Working on trying to replicate this issue. This doesn't give me an error. I'm wondering if there was a change in how the API works?

taskdefinition:
    Type: 'AWS::ECS::TaskDefinition'
    Properties:
....
         PortMappings:
            - ContainerPort: 80
              HostPort: 0
....

ECSTG:
    Type: 'AWS::ElasticLoadBalancingV2::TargetGroup'
    DependsOn: ECSALB
    Properties:
      HealthCheckIntervalSeconds: 10
      HealthCheckPath: /
      HealthCheckProtocol: HTTP
      HealthCheckTimeoutSeconds: 5
      HealthyThresholdCount: 2
      Name: ECSTG
      Port: 80
      Protocol: HTTP
      UnhealthyThresholdCount: 2
      VpcId: !Ref VpcId
kddejong commented 2 days ago

Was able to replicate this by adding HealthCheckPort: '3000' and it would only fail on create not an update? Which is weird.

kddejong commented 2 days ago

Also found while investigating is multiple containers in the same task cannot have the same HostPort

Container port 80 is used in more than one port mapping for container simple-app (but the task is created fine. This is only a failure when doing the service).

Logic looks like matching ContainerName to the ContainerDefinitions. Then if the PortMappings has a matching ContainerPort and HostPort is 0 then we can validate the TargetGroup configuration.