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

elasticloadbalancingv2: elvb2.HealthCheck requires ecs.Protocol instead of elbv2.Protocol #22689

Open
cmoore1776 opened this issue Oct 28, 2022 · 4 comments
Labels
@aws-cdk/aws-elasticloadbalancingv2 Related to Amazon Elastic Load Balancing V2 bug This issue is a bug. p2

Comments

@cmoore1776
Copy link
Contributor

Describe the bug

In CDK 2.48.0+, typeguard in Python is requiring that the Protocol parameter of elasticloadbalancingv2.HealthCheck is of type ecs.Protocol, not elasticloadbalancingv2.Protocol as it was in CDK 2.47.0 and older.

Expected Behavior

elasticloadbalancingv2.HealthCheck should require elasticloadbalancingv2.Protocol, not ecs.Protocol

The following Python code should be valid:

elbv2.ApplicationTargetGroup(
  scope= self,
  id= "webserverTargetGroup",
  port= 80,
  target_type= elbv2.TargetType.IP,
  vpc= myVpc,
  health_check= elbv2.HealthCheck(
    protocol= elbv2.Protocol.HTTP,
    healthy_threshold_count= 2,
    unhealthy_threshold_count= 4
  )
)

Current Behavior

The above code results in the following error during synthesis:

TypeError: type of argument protocol must be one of (aws_cdk.aws_ecs.Protocol, NoneType); got aws_cdk.aws_elasticloadbalancingv2.Protocol instead

Reproduction Steps

See "Expected Behavior" above

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.48.0 and 2.49.0

Framework Version

2.48.0 and 2.49.0

Node.js Version

v18.12.0

OS

macOS 12.6

Language

Python

Language Version

Python 3.10.8

Other information

No response

@cmoore1776 cmoore1776 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 28, 2022
@github-actions github-actions bot added the @aws-cdk/aws-elasticloadbalancingv2 Related to Amazon Elastic Load Balancing V2 label Oct 28, 2022
@hectorvs
Copy link

I've hit this issue too, I ended up commenting out the attribute and adding it via escape hatch like so

        health_check = elbv2.HealthCheck(
            interval=Duration.seconds(11),
            path=healthcheck_path,
            port=healthcheck_port,
            # protocol=elbv2.Protocol.HTTPS, # known bug https://github.com/aws/aws-cdk/issues/22689
            timeout=Duration.seconds(10),
            healthy_http_codes="200,202",
        )

        target_group = elbv2.ApplicationTargetGroup(
            self,
            id="TargetGroup",
            target_type=elbv2.TargetType.IP,
            target_group_name=tg_name,
            health_check=health_check,
            protocol=elbv2.ApplicationProtocol.HTTPS,
            port=app_port,
            vpc=vpc,
            deregistration_delay=Duration.seconds(10),
        )

        # escape hatch
        target_group.node.default_child.add_override(
            "Properties.HealthCheckProtocol", "HTTPS"
        )

@corymhall
Copy link
Contributor

@shamelesscookie it looks like I can't reproduce the issue with the code that you provided. I know there an issue because we've received a couple other issues reporting the same type of thing. Can you provide more of your CDK app so we can reproduce your case? I'm assuming there is some ECS code in there.

@cmoore1776
Copy link
Contributor Author

@corymhall Here's a complete repro case.

Findings: If, anywhere in your stack, you instantiate ecs.FargateTaskDefintion and then invoke add_port_mapping, all usage of elbv2.HealthCheck in that stack, whether related or not, must occur prior to any ecs references, or the stack will fail to synthesize.

#!/usr/bin/env python3

from constructs import Construct
from aws_cdk import App, Stack, Environment

import aws_cdk.aws_ec2 as ec2
import aws_cdk.aws_elasticloadbalancingv2 as elbv2
import aws_cdk.aws_ecs as ecs

app = App()
AccountId = "<redacted>"

class TestStack(Stack):
  def __init__(self, scope: Construct, construct_id: str, **kwargs) -> None:
    super().__init__(scope, construct_id, **kwargs)

    self.TestVpc= ec2.Vpc.from_lookup(scope= self, id= "TestVpc",
      vpc_name= "TestVpc"
    )

    self.TestTargetGroup = elbv2.ApplicationTargetGroup(scope= self, id= "TestTargetGroup",
      vpc= self.TestVpc,
      health_check= elbv2.HealthCheck(
        protocol= elbv2.Protocol.HTTP
      )
    )

    # 👆 👇 Reverse the order of the TargetGroup and Cluster to induce/resolve the issue 
    # (even though the two resources are unrelated, except for sharing a VPC)

    self.TestEcsCluster= ecs.Cluster(scope= self, id= "TestEcsCluster",
      vpc= self.TestVpc
    )

    self.TestTaskDefinition= ecs.FargateTaskDefinition(scope= self, id= "TestTaskDefinition")

    NginxContainer = self.TestTaskDefinition.add_container(
      id= "nginx-container",
      image= ecs.ContainerImage.from_registry("nginx")
    )

    # If you comment out the add_port_mappings statement below,
    # the stack will always synthesize, regardless of order above

    NginxContainer.add_port_mappings(ecs.PortMapping(
      container_port= 80,
      protocol= ecs.Protocol.TCP
    ))

TestStack(
  app,
  construct_id= "test-stack",
  env= Environment(account= AccountId, region= "us-east-1")  
)

app.synth()

@cmoore1776 cmoore1776 changed the title (elasticloadbalancingv2): elvb2.HealthCheck requires ecs.Protocol instead of elbv2.Protocol elasticloadbalancingv2: elvb2.HealthCheck requires ecs.Protocol instead of elbv2.Protocol Oct 31, 2022
mergify bot pushed a commit that referenced this issue Oct 31, 2022
jsii 1.70.0 introduced a regression, requiring the wrong type:

#22688
#22689
#22711

Revert to 1.69.0

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
otaviomacedo added a commit that referenced this issue Oct 31, 2022
jsii 1.70.0 introduced a regression, requiring the wrong type:

#22688
#22689
#22711

Revert to 1.69.0

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@RomainMuller
Copy link
Contributor

@shamelesscookie you're correct... there is an order-of-initialization issue here caused by the use of some forward-declarations that trip the type resolver around homonymous types (the forward refs. are emitted un-qualified).

@peterwoodworth peterwoodworth removed the needs-triage This issue or PR still needs to be triaged. label Nov 3, 2022
@corymhall corymhall removed their assignment Apr 15, 2024
@pahud pahud added p2 and removed p1 labels Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-elasticloadbalancingv2 Related to Amazon Elastic Load Balancing V2 bug This issue is a bug. p2
Projects
None yet
Development

No branches or pull requests

7 participants