awslabs / aws-solutions-constructs

The AWS Solutions Constructs Library is an open-source extension of the AWS Cloud Development Kit (AWS CDK) that provides multi-service, well-architected patterns for quickly defining solutions
https://docs.aws.amazon.com/solutions/latest/constructs/
Apache License 2.0
1.22k stars 247 forks source link

aws-alb-lambda: Does not allow ApplicationLoadBalancerProps #1159

Closed darrenflannerypoppulo closed 1 month ago

darrenflannerypoppulo commented 1 month ago

description of the bug:

I am creating a ALbToLambda Stack using the aws-alb-lambda.

I need to specify what subnets the ALB will be created in.

On the aws-alb-lambda documentation, it states you can specify ApplicationLoadBalancerProps, but also states the following This cannot specify a VPC, it will use the VPC in existingVpc or the VPC created by the construct. .

I tried to add ApplicationLoadBalancerProps without a vpc so I can specify what subnets I want the ALB to be created in, but I get an error, saying 'vpc' is a required field.

When I add a vpc, the same as the existing_vpc property. I also get an error: Specify any existing VPC at the construct level, not within loadBalancerProps.

My Code:

alb_to_lambda = AlbToLambda(
            self,
            f"{STACK_PREFIX}{self.stack_config.function_name}AlbToLambda",
            lambda_function_props=FunctionProps(
                runtime=Runtime.PYTHON_3_9,
                handler=self.stack_config.function_handler,
                code=get_lambda_code(self.stack_config.lambda_code_path),
                function_name=f"{STACK_PREFIX}{self.stack_config.function_name}",
                description=f"{STACK_PREFIX}{self.stack_config.function_description}",
                log_retention=RetentionDays.FIVE_DAYS,
                timeout=Duration.seconds(30),
                environment=self.stack_config.environment,
                dead_letter_queue=dlq,
                memory_size=self.stack_config.memory_size,
            ),
            target_props=elbv2.ApplicationTargetGroupProps(
                target_group_name=f"{STACK_PREFIX}{self.stack_config.function_name}TG",
                health_check=elbv2.HealthCheck(
                    path="/healthcheck", interval=Duration.minutes(5), enabled=True
                ),
            ),
            existing_vpc=vpc,
            listener_props=elbv2.BaseApplicationListenerProps(
                certificates=[certificate],
            ),
            load_balancer_props=elbv2.ApplicationLoadBalancerProps(
                vpc_subnets=subnets,
            ),
            public_api=False,
        )

Reproduction Steps

Try to deploy an AlbToLambda construct using ApplicationLoadBalancerProps as a property

Error Log

Specify any existing VPC at the construct level, not within loadBalancerProps.

Environment

Other


This is :bug: Bug Report

biffgaut commented 1 month ago

Thanks, we'll take a look

biffgaut commented 1 month ago

We've got a fix for this, hope to have it pushed out sometime this week.

biffgaut commented 1 month ago

Thanks for pointing this out.

Since VPCs are shared between resources in constructs, we like to ensure they are managed at the Construct level, rather than passed in resource props. To ensure this, we throw an error when VPCs are passed in a resource prop we throw an error.

While this has served us well to this point, it was preventing you from executing your use case. For ALB constructs, we have changed the input validations, we no longer throw an error if a VPC is specified in loadBalancerProps. It's still important to manage the VPC at the construct level, so when a VPC is found in loadBalancerProps we check that existingVpc is also populated and attempt to confirm that the same VPC is specified in both places using the vpcId (which is just a token at this point in the process).

We were able to specify a subset of subnets for the ALB with these changes in place, we believe this will allow you to fulfill your use case. This PR should be published to npm, etc. sometime this week.

darrenflannerypoppulo commented 1 month ago

Thats great, thanks for the response and the fix

biffgaut commented 1 month ago

This is now available in v2.64.0