-
Notifications
You must be signed in to change notification settings - Fork 68
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
Add support for referenced custom authorizer lambdas #102
Conversation
lib/stackops/apiGateway.js
Outdated
@@ -267,7 +268,7 @@ module.exports = function(currentTemplate, aliasStackTemplates, currentAliasStac | |||
const aliasName = _.find(_.keys(aliases), alias => _.startsWith(alias, functionName)); | |||
|
|||
// Adjust references and alias permissions | |||
permission.Properties.FunctionName = { Ref: aliasName }; | |||
permission.Properties.FunctionName = aliasName ? { Ref: aliasName } : permission.Properties.FunctionName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this change is not an appropriate fix. It will affect all permission resources and unconditionally use any existing function name in case something is wrong with the alias.
Using the original function name must only be done if we are really sure that the target is an external function, and not a function deployed by the current service. If that happens for a local function this leads to untraceable errors and lets the permissions be attached to $LATEST instead of the alias silently.
We should first check if the permission targets a service external function and handle that in a separate code branch. This allows for handling configuration errors differently for internal functions and external ones instead of mixing them and swallowing errors.
The _.compact() change below is also affected. It must only be in place for external functions, not internal ones. If permissions that reference internal functions are not dependent on an alias, there is a problem with the service.
lib/stackops/apiGateway.js
Outdated
@@ -287,7 +288,7 @@ module.exports = function(currentTemplate, aliasStackTemplates, currentAliasStac | |||
} | |||
|
|||
// Add dependency on function version | |||
permission.DependsOn = [ versionName, aliasName ]; | |||
permission.DependsOn = _.compact([ versionName, aliasName ]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change was fixing the problem I had. I didn't know all the consequences associated. I made some change to the PR based on the assumption that an externally referenced function of a Permission will have the arn string value instead of a GetAttr object.
Great! I'll have a look tomorrow. |
I've just started trying to add serverless-aws-alias to my project with the hopeful goal of using it for API versioning. However I also came across the linked issue:
If helpful, and when ready, I can help test this fix on a real deployment? Thanks |
Hi @alexb-uk, it would be great if you can give this PR a try and report, if it solves the problem properly. |
Hi @HyperBrain, I performed the following and all went very smoothly:
For info here is a snippet from my serverless.yml: functions:
api:
handler: index.handler
onError: arn:aws:sns:eu-west-2:xxxxxxxxxx:sysops-admins
events:
- http:
path: "{proxy+}"
method: any
cors: true
authorizer: ${self:custom.authorizer.default}
plugins:
- serverless-aws-alias
- serverless-offline
custom:
authorizer:
default:
arn: arn:aws:lambda:eu-west-2:xxxxxxxxxx:function:auth0-authorizer-dev-auth
resultTtlInSeconds: 300
identitySource: method.request.header.Authorization
identityValidationExpression: ^Bearer [-0-9a-zA-Z\._]*$ Thanks a lot both for all your efforts. Alex |
@alexb-uk Thanks for testing. I will merge this now, so that it is part of the next release. |
Thanks @alexb-uk for the testing effort |
Add support for referenced custom authorizer lambdas
Proposed solution for #101
After digging a bit I found this bit in apiGateway.js
which selects the index in the
AuthorizerUri. Fn::Join
array based on finding theGetAtt , ARN
of a referenced lambda function. This won't work if the function is directly referenced using its ARN. In this case the uriPart of theAuthorizerUri
will simply be a string with the ARN andfuncIndex
will be set to -1. Hence${stageVariables.SERVERLESS_ALIAS}
will be added to the head of the array.I would suggest to add a simple check for strings, e.g.
Concerning the missing
FunctionName
I would simple use the original one, in case no otheraliasName
can be found, e.g instead ofuse the original
Background is, that the function name of the referenced custom authorizer can't generally be extended by the alias, as it lives in a different project and might be deployed with a different alias (or no alias at all)
Concerning the null values of the Permission: in this case, there should be no dependencies as the custom authorizer lambda should already be deployed by another project.
But this might lead to some unwanted behavior, as you mentioned in #83, and needs some testing