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

aws-ecs-patterns: Substituting IApplicationLoadBalancer for ApplicationLoadBalancer results in JSII breaking change alert #5465

Closed
bvtujo opened this issue Dec 18, 2019 · 2 comments
Assignees
Labels
@aws-cdk/aws-ecs-patterns Related to ecs-patterns library guidance Question that needs advice or information. needs-triage This issue or PR still needs to be triaged.

Comments

@bvtujo
Copy link
Contributor

bvtujo commented Dec 18, 2019

PR #5461 is an attempt to allow users to provide an imported load balancer to aws-ecs-patterns. This requires that I change some properties defined in aws-ecs-patterns from ApplicationLoadBalancer or NetworkLoadBalancer types to IApplicationLoadBalancer or INetworkLoadbalancer types.

Expected:
All unit tests and integration tests pass

Actual:
JSII causes CI build to fail with output "IApplicationLoadBalancer does not extend ApplicationLoadBalancer." This seems to be in error: objects of type ApplicationLoadBalancer implement IApplicationLoadBalancer and this should not result in any breaking UX changes.

Reproduction Steps

This issue appears in the build logs of #5461

Error Log

API elements with incompatible changes:
err  - PROP @aws-cdk/aws-ecs-patterns.ApplicationLoadBalancedEc2Service.loadBalancer: type @aws-cdk/aws-elasticloadbalancingv2.IApplicationLoadBalancer (formerly @aws-cdk/aws-elasticloadbalancingv2.ApplicationLoadBalancer): @aws-cdk/aws-elasticloadbalancingv2.IApplicationLoadBalancer does not extend @aws-cdk/aws-elasticloadbalancingv2.ApplicationLoadBalancer [changed-type:@aws-cdk/aws-ecs-patterns.ApplicationLoadBalancedEc2Service.loadBalancer]
err  - PROP @aws-cdk/aws-ecs-patterns.ApplicationLoadBalancedFargateService.loadBalancer: type @aws-cdk/aws-elasticloadbalancingv2.IApplicationLoadBalancer (formerly @aws-cdk/aws-elasticloadbalancingv2.ApplicationLoadBalancer): @aws-cdk/aws-elasticloadbalancingv2.IApplicationLoadBalancer does not extend @aws-cdk/aws-elasticloadbalancingv2.ApplicationLoadBalancer [changed-type:@aws-cdk/aws-ecs-patterns.ApplicationLoadBalancedFargateService.loadBalancer]
err  - PROP @aws-cdk/aws-ecs-patterns.ApplicationLoadBalancedServiceBase.loadBalancer: type @aws-cdk/aws-elasticloadbalancingv2.IApplicationLoadBalancer (formerly @aws-cdk/aws-elasticloadbalancingv2.ApplicationLoadBalancer): @aws-cdk/aws-elasticloadbalancingv2.IApplicationLoadBalancer does not extend @aws-cdk/aws-elasticloadbalancingv2.ApplicationLoadBalancer [changed-type:@aws-cdk/aws-ecs-patterns.ApplicationLoadBalancedServiceBase.loadBalancer]

Build logs can be searched to locate the above text.

Environment

CI Build Environment as configured for the build in #5461


This is 🐛 Bug Report

@bvtujo bvtujo added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Dec 18, 2019
@SomayaB SomayaB added the @aws-cdk/aws-ecs-patterns Related to ecs-patterns library label Dec 18, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented Dec 18, 2019

This is not a bug.

Let's look at a simplified class diagram of IApplicationLoadBalancer and ApplicationLoadBalancer:

┌ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ┐
     IApplicationLoadBalancer      
├ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ┤
                                   
│ public loadBalancerArn: string  │
                                   
└ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ─ ┘
                 △                 
                 │                 
                 │                 
                 │                 
┌─────────────────────────────────┐
│     ApplicationLoadBalancer     │
├─────────────────────────────────┤
│                                 │
│  public logAccessLogs(bucket)   │
│                                 │
└─────────────────────────────────┘

In the ORIGINAL VERSION, someone could write this:

class LoadBalancedApplication {
  public loadBalancer: ApplicationLoadBalancer;
}

const app = new LoadBalancedApplication();
app.loadBalancer.logAccessLogs(bucket); 

Now let's say you change to the type of the property toIApplicationLoadBalancer. The existing customer code will break. It's a breaking change:

class LoadBalancedApplication {
  public loadBalancer: IApplicationLoadBalancer;
}

const app = new LoadBalancedApplication();
app.loadBalancer.logAccessLogs(bucket); 
//                  ^^^^^--- error no such method 'logAccessLogs'

It is perfectly fine to substitute IApplicationLoadBalancer for INPUT properties (values that are supplied by the consumer and used by the construct), but you can't do this for OUTPUT properties (properties that are set by the construct and read by consumers), because type information will be lost.

Fortunately, loadBalancer? is already optional, so you can avoid setting the property it if the passed-in construct is not a real ApplicationLoadBalancer class.

@rix0rrr rix0rrr closed this as completed Dec 18, 2019
@rix0rrr rix0rrr added guidance Question that needs advice or information. and removed bug This issue is a bug. labels Dec 18, 2019
@bvtujo
Copy link
Contributor Author

bvtujo commented Dec 18, 2019

The actual issue here was caused by erroneously replacing the output ApplicationLoadBalancer with IApplicationLoadBalancer, and forgetting to cast the input IALB to ALB in the ApplicationLoadBalancedServiceBase constructor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ecs-patterns Related to ecs-patterns library guidance Question that needs advice or information. needs-triage This issue or PR still needs to be triaged.
Projects
None yet
Development

No branches or pull requests

5 participants