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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Application Load Balancer event source #6073

Merged
merged 2 commits into from Jun 3, 2019

Conversation

@pmuens
Copy link
Member

commented May 1, 2019

What did you implement:

Closes #5572

Adds support for the alb event type.


Shoutouts to @cbm-egoubely who provided his plugin (see comments below) and the design for the abstraction we're using here. 馃挴 馃


How did you implement it:

Added all the code to get an alb event setup up and running.

How can we verify it:

Configuration is based on this serverless.yml and this blog post.

Use the following serverless.yml and run serverless deploy -v:

Notice that the LoadBalancerDNSName output is printed on the terminal. Copy this value and append the /hello path/. curl it.

// handler.js

module.exports.hello = async () => ({
  isBase64Encoded: false,
  statusCode: 200,
  statusDescription: "200 OK",
  headers: { "Set-cookie": "cookies", "Content-Type": "application/json" },
  body: "Hello from Lambda"
});
service: test

provider:
  name: aws
  runtime: nodejs10.x
  versionFunctions: false

functions:
  test:
    handler: handler.hello
    events:
      - alb:
          listenerArn: { Ref: HTTPListener }
          priority: 1
          conditions:
            path: /hello

custom:
  defaultRegion: us-east-1
  defaultStage: dev
  region: ${opt:region, self:custom.defaultRegion}
  stage: ${opt:stage, self:custom.defaultStage}
  objectPrefix: '${self:service}-${self:custom.stage}'

resources:
  Outputs:
    LoadBalancerDNSName:
      Value: { 'Fn::GetAtt': [ LoadBalancer, 'DNSName' ] }
      Export: { Name: '${self:custom.objectPrefix}-LoadBalancerDNSName' }
  Resources:
    VPC:
      Type: 'AWS::EC2::VPC'
      Properties:
        CidrBlock: 172.31.0.0/16
        EnableDnsHostnames: true
    InternetGateway:
      Type: 'AWS::EC2::InternetGateway'
    VPCGatewayAttachment:
      Type: 'AWS::EC2::VPCGatewayAttachment'
      Properties:
        VpcId: { Ref: VPC }
        InternetGatewayId: { Ref: InternetGateway }
    RouteTable:
      Type: 'AWS::EC2::RouteTable'
      Properties:
        VpcId: { Ref: VPC }
    InternetRoute:
      Type: 'AWS::EC2::Route'
      DependsOn: VPCGatewayAttachment
      Properties:
        DestinationCidrBlock: 0.0.0.0/0
        GatewayId: { Ref: InternetGateway }
        RouteTableId: { Ref: RouteTable }
    SubnetA:
      Type: 'AWS::EC2::Subnet'
      Properties:
        AvailabilityZone: '${self:custom.region}a'
        CidrBlock: 172.31.0.0/20
        MapPublicIpOnLaunch: false
        VpcId: { Ref: VPC }
    SubnetB:
      Type: 'AWS::EC2::Subnet'
      Properties:
        AvailabilityZone: '${self:custom.region}b'
        CidrBlock: 172.31.16.0/20
        MapPublicIpOnLaunch: false
        VpcId: { Ref: VPC }
    SubnetARouteTableAssociation:
      Type: 'AWS::EC2::SubnetRouteTableAssociation'
      Properties:
        SubnetId: { Ref: SubnetA }
        RouteTableId: { Ref: RouteTable }
    SubnetBRouteTableAssociation:
      Type: 'AWS::EC2::SubnetRouteTableAssociation'
      Properties:
        SubnetId: { Ref: SubnetB }
        RouteTableId: { Ref: RouteTable }
    SecurityGroup:
      Type: 'AWS::EC2::SecurityGroup'
      Properties:
        GroupName: 'http-https'
        GroupDescription: 'HTTPS / HTTPS inbound; Nothing outbound'
        VpcId: { Ref: VPC }
        SecurityGroupIngress:
          -
            IpProtocol: tcp
            FromPort: '80'
            ToPort: '80'
            CidrIp: 0.0.0.0/0
          -
            IpProtocol: tcp
            FromPort: '443'
            ToPort: '443'
            CidrIp: 0.0.0.0/0
        SecurityGroupEgress:
          -
            IpProtocol: -1
            FromPort: '1'
            ToPort: '1'
            CidrIp: 127.0.0.1/32
    LoadBalancer:
      Type: 'AWS::ElasticLoadBalancingV2::LoadBalancer'
      Properties:
        Type: 'application'
        Name: '${self:custom.objectPrefix}'
        IpAddressType: 'ipv4'
        Scheme: 'internet-facing'
        LoadBalancerAttributes:
          - { Key: 'deletion_protection.enabled', Value: false }
          - { Key: 'routing.http2.enabled', Value: false }
          - { Key: 'access_logs.s3.enabled', Value: false }
        SecurityGroups:
          - { Ref: SecurityGroup }
        Subnets:
          - { Ref: SubnetA }
          - { Ref: SubnetB }
    HTTPListener:
      Type: 'AWS::ElasticLoadBalancingV2::Listener'
      Properties:
        LoadBalancerArn: { Ref: LoadBalancer }
        Port: 80
        Protocol: 'HTTP'
        DefaultActions:
          -
            Type: 'fixed-response'
            Order: 1
            FixedResponseConfig:
              StatusCode: 404
              ContentType: 'application/json'
              MessageBody: '{ "not": "found" }'

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@pmuens pmuens self-assigned this May 1, 2019

@pmuens pmuens added this to In progress in Serverless via automation May 1, 2019

@pmuens pmuens force-pushed the alb-event branch 4 times, most recently from 722d6f3 to 3226704 May 1, 2019

@pmuens pmuens added pr/in-review and removed pr/in-progress labels May 3, 2019

@pmuens pmuens marked this pull request as ready for review May 3, 2019

@pmuens

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

Alright, so I just finalized the first iteration on this one (added the tests and the documentation).

IMPORTANT: It would be really helpful if develpoers who plan to use this comment on the implementation since we can provide a lot of configuration for this event source but we'd like to keep this as minimal / simple as possible for the first pass while providing as much value out of the box.

@pmuens pmuens requested review from eahefnawy and dschep May 3, 2019

Serverless automation moved this from In progress to Reviewer approved May 3, 2019

@dschep
Copy link
Member

left a comment

lgtm, but need community feedback on what exactly this implementation should look like

@cbm-egoubely

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

Hello,
I don鈥檛 see the point of having Serverless create a listener but not a load balancer. And what about listener rules? I would expect function alb events to be defined with a path-pattern or/and a host, which would create the appropriate listener rules (this is actually in the feature proposal #5572).

To me, there are to use cases :

  • You want to you use an existing balancer which has already HTTP/S listeners, in that case you would habe to supply serverless with the listener ARN. Serverless will then add rules to that listener based on the events you defined.
  • You want serverless to create all the resources (ALB, listeners, rules, target groups, etc.). In that case you would have to supply the ALB name, type of listener and certificate ARN in addition to ALB events.

In my company we want the first option, as it is more cost efficient since ALB have a base cost of ~25$ per month; this would grow expensive quite fast as we have a lot a serverless stacks (microservices). Also it would slow down the deployment/rollback for these stacks a lot. FYI, I just made public the plugin we use (https://github.com/axel-springer-kugawana/serverless-alb-plugin). We had to hurry to migration from API Gateway to ALB since it will so much cheaper (https://serverless-training.com/articles/save-money-by-replacing-api-gateway-with-application-load-balancer/). It would be nice if Serverless provides similar features.

If you agree to add one of the features I propose, I would gladly help.

@pmuens pmuens force-pushed the alb-event branch from 3226704 to d82dd91 May 6, 2019

Serverless automation moved this from Reviewer approved to Needs review May 6, 2019

@pmuens pmuens force-pushed the alb-event branch from d82dd91 to 81ce0f3 May 7, 2019

@pmuens pmuens added this to the 1.43.0 milestone May 8, 2019

@pmuens pmuens force-pushed the alb-event branch 5 times, most recently from 166448a to 4a2557d May 8, 2019

@pmuens pmuens removed this from the 1.43.0 milestone May 14, 2019

@pmuens

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

@cbm-egoubely thanks for following-up here!

Yep, that did the trick! It works. Thanks for the guidance. I guess that this is now ready for a final review.

@pmuens pmuens force-pushed the alb-event branch from 0c0c3dc to 21e0d93 May 30, 2019

@pmuens pmuens requested review from medikoo and dschep May 30, 2019

});
});

return BbPromise.resolve();

This comment has been minimized.

Copy link
@medikoo

medikoo May 30, 2019

Member

Those are sync functions, why do we return promise?

I take it's about following some convention (?) If it's the case, then convention in which we define every method as async (even if there's nothing async about them) feels quite controversial.

Can't we just define them as regular sync methods and treat them as such?

This comment has been minimized.

Copy link
@pmuens

pmuens May 31, 2019

Author Member

Yes, this is a convention we've introduced when starting the Framework. The main reason being that the Plugin Manager treats every single plugin as an async entity.

Another reason was that some plugins to some async stuff, while other's don't. This introduced a lot of confusion and non-returned promises because some code got copy and pasted (and hence sync and async got mixed).

This comment has been minimized.

Copy link
@medikoo

medikoo May 31, 2019

Member

Ok, it may explain why package:compileEvents hook is exposed as a promise returning function.
Still here every internal sync method (not exposed to the Plugin Manager in any way) was configured to return a promise, which seems a bit overkill (that also increases noise to signal ratio among framework modules)

Also when we deal with generic situation where given function might be async or not.
Instead of forcing async resolution for any case, we may consume result as:

new Promise(resolve => resolve(hookCallback()))

Then hook callback may be sync if it's a sync, and be async when needed.

I'm a bit worried about forcing async notation everywhere, seems to raise complexity unnecessary.

This comment has been minimized.

Copy link
@pmuens

pmuens May 31, 2019

Author Member

Yes, I see. That makes sense.

Let me turn the internal code to smth. sync and maybe we can open up another issue to audit the codebase and see where we can change the code to be sync (in cases where it's async right now).

This comment has been minimized.

Copy link
@medikoo

medikoo May 31, 2019

Member

We may refactor on the go, gradually, when there's a good moment.

With such large codebase I think it might be very difficult (when agreeing on an improvement) ensure every bit is updated with it.

It might be that the only effective way is just to start somewhere, and with time cover the rest.

Show resolved Hide resolved lib/plugins/aws/package/compile/events/alb/lib/listenerRules.test.js Outdated
Show resolved Hide resolved lib/plugins/aws/package/compile/events/alb/lib/validate.js

@pmuens pmuens force-pushed the alb-event branch 4 times, most recently from fb4408c to 5f8cc0f May 30, 2019

@pmuens pmuens requested a review from medikoo May 31, 2019

return BbPromise.bind(this)
.then(this.compileTargetGroups)
.then(this.compileListenerRules)
.then(this.compilePermissions);

This comment has been minimized.

Copy link
@medikoo

medikoo May 31, 2019

Member

Those are sync functions, so it seems no point to construct promise chain out of them (?)

If for some technical reason this hook callback needs to return promise (be an async function). I would at this point rely on BbPromise.try as it allows us to write 1:1 code as we would write it with async functions support:

return BbPromise.try(() => {
  if (noEvents) return;

  this.compileTargetGroups();
  this.compileListenerRules();
 // etc.
});

Then when migrating to async functions we can just remove BbPromise.try wrap and put async in front (outcome will be exactly same).

Or if we support sync callback hooks (which I think will be cleanest), just make this callback naturally sync

Show resolved Hide resolved lib/plugins/aws/package/compile/events/alb/lib/validate.js

@pmuens pmuens force-pushed the alb-event branch from 5f8cc0f to 9b84481 Jun 3, 2019

@pmuens pmuens requested a review from medikoo Jun 3, 2019

@pmuens pmuens force-pushed the alb-event branch from 9b84481 to 2023e50 Jun 3, 2019

@pmuens pmuens requested a review from medikoo Jun 3, 2019

Serverless automation moved this from Needs review to Reviewer approved Jun 3, 2019

@medikoo

medikoo approved these changes Jun 3, 2019

Copy link
Member

left a comment

LGTM!

@pmuens pmuens merged commit 0965a6b into master Jun 3, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Serverless automation moved this from Reviewer approved to Done Jun 3, 2019

@pmuens pmuens deleted the alb-event branch Jun 3, 2019

@@ -93,6 +93,7 @@ If you have any questions, [search the forums](https://forum.serverless.com?utm_
<li><a href="./events/schedule.md">Schedule</a></li>
<li><a href="./events/sns.md">SNS</a></li>
<li><a href="./events/sqs.md">SQS</a></li>
<li><a href="./evetns/alb.md">ALB</a></li>

This comment has been minimized.

Copy link
@JoeNyland

JoeNyland Jun 3, 2019

There's a typo here. Should be ./events/alb.md.

This comment has been minimized.

Copy link
@schellack

schellack Jun 23, 2019

Contributor

I submitted a separate pull request to fix the typo, since this one is already marked as Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.