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

ALB Listener import syntax considered invalid by lambda alb event config code #7646

Open
mjhanke opened this issue Apr 30, 2020 · 7 comments

Comments

@mjhanke
Copy link

mjhanke commented Apr 30, 2020

Bug Report

Description

  1. What did you do? Used Fn::ImportValue: ${self:custom.other-service-name}-ALBListenerArn for lambda function's alb listenerArn property
  2. What happened? Invalid ALB listenerArn in function "${functionName}".
  3. What should've happened? serverless should fetch the valid Arn from another stack
  4. What's the content of your serverless.yml file?
functions:
...
function-invoked-by-alb:
  handler: wsgi.handler
    timeout: 30
    events:
      - http: ANY /api/{version}/endpoint
      - alb:
          priority: 1
          listenerArn:
            Fn::ImportValue: ${self:custom.other-service-name}-ALBListenerArn
          conditions:
            path: /api/v?/endpoint
    environment:
      ....

The source code causing this behavior is here:

throw new this.serverless.classes.Error(
`Invalid ALB listenerArn in function "${functionName}".`
);

@medikoo
Copy link
Contributor

medikoo commented Apr 30, 2020

@mjhanke thanks for report. Still it's not really a bug, more an intended limitation.

To construct properly ALB event we need to have complete ALB info upfront. Currently in code it's resolved sync way.

Also normally we allow packaging feature to work offline. Introducing here a dependency on CloudFormation, will no longer allow to package project without issuing AWS SDK requests.

@mjhanke
Copy link
Author

mjhanke commented Apr 30, 2020

Still it's not really a bug, more an intended limitation.

What do you mean by "intended limitation"?

We use the "Fn::Import" syntax all over the place in our serverless project.

The ALB event sugar creates a ListenerRule.

Can you help me understand why the generated ListenerRule would be invalid with the Fn::Import syntax?

@medikoo
Copy link
Contributor

medikoo commented May 4, 2020

@mjhanke please see the implementation, and see that on packaging level we need to resolve ALB Id and listener id from ARN:

const matches = listenerArn.match(ALB_LISTENER_PATTERN);
if (!matches) {
throw new this.serverless.classes.Error(
`Invalid ALB listenerArn in function "${functionName}".`
);
}
return { albId: matches[1], listenerId: matches[2] };

We do it on spot, without dependency on external resources. Also note that service packaging also works that way (offline, without dependency on external resources)

@mjhanke
Copy link
Author

mjhanke commented May 4, 2020

Interesting. I'm trying to reconcile this with our existing use of the "Fn::Import" syntax with the serverless framework

provider:
  name: aws
  runtime: python3.6
  ...
  role:
    Fn::ImportValue: ${self:custom.infrastructureServiceName}-lambdaExecutionRoleArn
  environment:
    ...
  vpc:
    securityGroupIds:
      - Fn::ImportValue: ${self:custom.infrastructureServiceName}-serverlessSecurityGroupId
    subnetIds:
      - Fn::ImportValue: ${self:custom.infrastructureServiceName}-PrivateSubnetNoInternet1
      - Fn::ImportValue: ${self:custom.infrastructureServiceName}-PrivateSubnetNoInternet2
      - Fn::ImportValue: ${self:custom.infrastructureServiceName}-PrivateSubnetNoInternet3

This syntax works for those cases. Is what you're saying that support of that syntax is considered a feature? not a bug?

@medikoo
Copy link
Contributor

medikoo commented May 4, 2020

This syntax works for those cases. Is what you're saying that support of that syntax is considered a feature?

It's a feature, and note it's a different case, as we do not resolve those CF instructions on spot (we just pass is as it is to result CF template)

In case of ALB, it appears, we need to resolve ALB id and listener to create a template, hence providing a CF instruction is not supported (in this case we would have to resolve that instruction for template generation)

@gudinoff
Copy link

After hitting the AWS 200 stack-resources limit I started to split my project and found this another limitation.

I understand that this may be hard to overcome when it comes to the offline packaging feature.
But I tried to deploy directly (without using the packaging feature) and the result was the same.

Is there any way that a function belonging to serverless-stack-B.yaml can reference an ALB listener created in serverless-stack-A.yaml?

@medikoo
Copy link
Contributor

medikoo commented May 12, 2020

I looked closer into ALB resolution, and it appears that what we resolve as albId is just used for resource id and name generation, and what we resolve as listenerId is not used anywhere.

In light of that I believe that we can safely refactor validateListenerArn to support CloudFormation instructions (and all other valid inputs), and change it to return a string id directly (which we would use as albId)

PR's welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants