feat(aws-codedeploy): support setting a load balancer on a Deployment Group

[skinny85]

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

[eladb]

PascalCase please

[skinny85]

Done.

[eladb]

Typo

[skinny85]

I was just testing you 😉 Done!

[eladb]

ILoadBalancerProvider

[skinny85]

Done (ILoadBalancer).

[eladb]

Missing:

• Why would I want to do that? (you can copy & paste from the AWS docs)
• Reference to relevant AWS documentation?

[skinny85]

Done.

[eladb]

Here's how we usually formulate such interfaces:

interface ILoadBalancer {
  readonly asCodeDeployLoadBalancer: LoadBalancerProps
}

interface LoadBalancerProps {
  generation: LoadBalancerGeneration;
  name: string;
}

Then, the implementors will look like this:

class ClassicLoadBalancer implements codedeployapi.ILoadBalancer {
  public get asCodeDeployLoadBalancer(): codedeployapi.LoadBalancerProps {
    return {
      generation: ...,
      name: this.blaName
   }
 }
}

The asXxx pattern signals the user that they shouldn't need to use this property directly. It's "a cast" to the specific interface.

[skinny85]

Done.

[rix0rrr]

As another comment: we already have the "something is a load balancer TARGET" interface, usable like this:

loadBalancer.addTarget(someThing);

(It exists for both Classic ELB and ELBv2, although the interfaces are slightly different)

Does that not suffice for your use case?

[skinny85]

Does that not suffice for your use case?

Sadly, it does not. CodeDeploy has different requirements for ELB and ALB/NLB (different things need to be set as load balancers - the LB itself for ELB, the Target Group for ALB/NLB). Also, it needs to be a single type to avoid wrapping it in the Deployment Group class.

[skinny85]

Updated with Elad's feedback.

[skinny85]

Rebased on top of 0.10.0.

[skinny85]

Missed that the new test (integ.deployment-group.ts) was indented with 4 spaces instead of 2.

[skinny85]

The build failure is weird:

> @aws-cdk/aws-elasticloadbalancing@0.10.0 package /codebuild/output/src144793856/src/github.com/awslabs/aws-cdk/packages/@aws-cdk/aws-elasticloadbalancing
> cdk-package


lerna ERR! npm run package stderr:
[jsii-pacmak] [INFO] Building @aws-cdk/aws-elasticloadbalancing (java,dotnet,sphinx,js) into dist
Error: Process exited with status 1
Microsoft (R) Build Engine version 15.6.82.30579 for .NET Core
Copyright (C) Microsoft Corporation. All rights reserved.

  Restoring packages for /tmp/jsii-pacmak-code0Ql8th/Amazon.CDK.AWS.ElasticLoadBalancing/Amazon.CDK.AWS.ElasticLoadBalancing.csproj...
  Installing Amazon.CDK.AWS.EC2 0.10.0.
  Installing Amazon.CDK.AWS.CodeDeploy.Api 0.10.0.
  Generating MSBuild file /tmp/jsii-pacmak-code0Ql8th/Amazon.CDK.AWS.ElasticLoadBalancing/obj/Amazon.CDK.AWS.ElasticLoadBalancing.csproj.nuget.g.props.
  Generating MSBuild file /tmp/jsii-pacmak-code0Ql8th/Amazon.CDK.AWS.ElasticLoadBalancing/obj/Amazon.CDK.AWS.ElasticLoadBalancing.csproj.nuget.g.targets.
  Restore completed in 264.29 ms for /tmp/jsii-pacmak-code0Ql8th/Amazon.CDK.AWS.ElasticLoadBalancing/Amazon.CDK.AWS.ElasticLoadBalancing.csproj.
LoadBalancer.cs(14,59): error CS0738: 'LoadBalancer' does not implement interface member 'IILoadBalancer.AsCodeDeployLoadBalancer()'. 'LoadBalancer.AsCodeDeployLoadBalancer()' cannot implement 'IILoadBalancer.AsCodeDeployLoadBalancer()' because it does not have the matching return type of 'ILoadBalancerProps'. [/tmp/jsii-pacmak-code0Ql8th/Amazon.CDK.AWS.ElasticLoadBalancing/Amazon.CDK.AWS.ElasticLoadBalancing.csproj]

Build FAILED.

LoadBalancer.cs(14,59): error CS0738: 'LoadBalancer' does not implement interface member 'IILoadBalancer.AsCodeDeployLoadBalancer()'. 'LoadBalancer.AsCodeDeployLoadBalancer()' cannot implement 'IILoadBalancer.AsCodeDeployLoadBalancer()' because it does not have the matching return type of 'ILoadBalancerProps'. [/tmp/jsii-pacmak-code0Ql8th/Amazon.CDK.AWS.ElasticLoadBalancing/Amazon.CDK.AWS.ElasticLoadBalancing.csproj]
    0 Warning(s)
    1 Error(s)

Time Elapsed 00:00:01.90

Could this be a bug in the .NET part of JSII...?

[skinny85]

Just realized that I left out setting the DeploymentStyle.DeploymentOption property to WITH_TRAFFIC_CONTROL when there is a load balancer present - will fix.

[rix0rrr]

Sadly, it does not. CodeDeploy has different requirements for ELB and ALB/NLB (different things need to be set as load balancers - the LB itself for ELB, the Target Group for ALB/NLB). Also, it needs to be a single type to avoid wrapping it in the Deployment Group class.

Sorry, I don't understand that. Can you clarify a bit more? Because for example, if I do the following:

class ServerDeploymentGroup
    implements elb.ILoadBalancerTarget, elbv2.IApplicationLoadBalancerTarget, elbv2.INetworkLoadBalancerTarget {

    public attachToClassicLB(loadBalancer: elb.LoadBalancer): void {
        this.loadBalancerInfo.elbInfoList.push(loadBalancer.loadBalancerName);
    }
    
    public attachToApplicationTargetGroup(targetGroup: elbv2.ApplicationTargetGroup): elbv2.LoadBalancerTargetProps {
        this.loadBalancerInfo.targetGroupInfoList.push(targetGroup.targetGroupName);
        return { targetType: elbv2.TargetType.SelfRegistering };
    }

    public attachToNetworkTargetGroup(targetGroup: elbv2.NetworkTargetGroup): elbv2.LoadBalancerTargetProps {
        this.loadBalancerInfo.targetGroupInfoList.push(targetGroup.targetGroupName);
        return { targetType: elbv2.TargetType.SelfRegistering };
    }    
}

I don't understand why that wouldn't work?

const deploymentGroup = new codedeploy.ServerDeploymentGroup(stack, 'CodeDeployGroup', {
   autoScalingGroups: [asg]
});

lb.addTarget(asg);
lb.addTarget(deploymentGroup);

Granted, addTarget() on the deploymentgroup might be confusing (or not), but would like to keep some consistency in the load balancer modeling, and right now the load balancer points to things (as opposed to things pointing to the load balancer).

The duplication that I mentioned in that code review comment is even more visible here. So maybe model it differently? Like ASGs having an "ITellYouAboutMyLoadBalancers" interface?

[rix0rrr]

I don't like this duplication very much. (1/2)

[rix0rrr]

I don't like this duplication very much. (2/2)

We're both saying that the ASG should be a load balancer target (directly), AND that the ASG is a load balancer target (via the DeploymentGroup).

Why are both necessary? I mean, I can think of some mechanical reasons why this is necessary today: probably the DeploymentGroup is going to call register-instance and register-instance only when it makes changes, and the direct association between load balancer and ASG is necessary to register the initial instances, before CodeDeploy makes any changes... but I still don't really like it.

Can't we interrogate the ASGs for their load balancers and copy those onto the DeploymentGroup?

[skinny85]

Why are both necessary?

Honestly, I'm not even 100% sure that this duplication is required. Perhaps setting the ASG just on the Deployment Group, and then setting the load balancer on the Deployment Group, is actually enough? I'll do some research.

Can't we interrogate the ASGs for their load balancers and copy those onto the DeploymentGroup?

That won't work. You can have a load balancer for a Deployment Group without any ASGs (when the Deployment Group uses tags for EC2/on-premise instances).

[rix0rrr]

That won't work. You can have a load balancer for a Deployment Group without any ASGs (when the Deployment Group uses tags for EC2/on-premise instances).

One doesn't have to preclude the other. I want to (be able to) get rid of the repetition if it exists.

Also, you could still have an object that represents the tagged instances.

[skinny85]

One doesn't have to preclude the other. I want to (be able to) get rid of the repetition if it exists.

Well, I really feel like there should be one way of doing this. I don't think taking load balancers both from the ASGs and from another source (if a Deployment Group doesn't have any ASGs) is a great experience.

Also, you could still have an object that represents the tagged instances.

How would that look? So you would create a TaggedInstances object, supply it to the Deployment Group on construction, and then call loadBalancer.addTarget(taggedInstances), which would in the background set the load balancer of the Deployment Group?

That doesn't sound great to me - seems a little too magical/indirect...

EDIT: actually, I have no idea how would that work at all... What if I add the taggedInstances to the LB before creating the Deployment Group - how will that work? Does taggedInstances have some state...? Would you mind writing out the full code of how you see this working?

[skinny85]

const deploymentGroup = new codedeploy.ServerDeploymentGroup(stack, 'CodeDeployGroup', {
  autoScalingGroups: [asg]
});

lb.addTarget(asg);
lb.addTarget(deploymentGroup);

I'm not a huge fan of this mutable API... doesn't seem very discoverable, for example.

I like giving the Load Balancer in the construction properties of the Deployment Group a lot more. Seems also more consistent (why would some things be set in the construction properties, while others you have to call a method on the object for?).

[skinny85]

Added setting the DeploymentStyle.DeploymentOption property correctly to WITH_TRAFFIC_CONTROL when a load balancer is present.

@rix0rrr I've confirmed that you don't need to set the ASG as target on the LB for the integration to work - it's enough to provide it in the Deployment Group. I've removed the duplication from the integ test. Given that, are you OK with this API?

[rix0rrr]

I'm still not happy, but disagree and commit is what we're about :)

[skinny85]

I'm still not happy, but disagree and commit is what we're about :)

I appreciate your LP strength :) but can you write out (in pseudo code) a proposal that would make you happy, including support for setting a load balancer on a Deployment Group without any AGSs, only with tag filters?

Perhaps there is a way for everybody to be happy :)

[skinny85]

@rix0rrr ping

[rix0rrr]

Honestly, I'm not quite sure anymore. Just merge it.

[skinny85]

I see I wore you down :D