serverless icon indicating copy to clipboard operation
serverless copied to clipboard

Fix #4711 when authorizers are deployed by current CloudFormation

Open KostiantynKopytov opened this issue 6 years ago • 6 comments

What did you implement

APIGateway authorizers will receive name of function but not a name of property.

Details: Currently when sls deploys authorizers they will name defined by a property name. In following example APIG authorizer will have name authorize but with this patch it'll have {prefix}-authorize and will not conflict deploys with different prefixes:

Example serverless.yaml:

service: ${opt:prefix}-example
functions:
 authorize: # current authorizer name
   name: ${opt:prefix}-authorize # function name & patched authorizer name
   handler: api/auth.js
 getUsers:
   name : ${opt:prefix}-get-users
   handler: api/users.js
   events:
     - http:
         path: /${opt:prefix}/user
         method: get
         authorizer: authorize

#4711 is already closed by #4197 but only when configuration have custom authorizeId. This pull request closes #4711 in case I don't use custom authorizerId.

How can we verify it

Use custom function name for authorizer.

Todos

  • [ ] Write and run all tests
  • [ ] Write documentation
  • [ ] Enable "Allow edits from maintainers" for this PR
  • [ ] Update the messages below

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

KostiantynKopytov avatar Nov 27 '19 06:11 KostiantynKopytov

Codecov Report

Merging #7017 into master will decrease coverage by 0.33%. The diff coverage is 81.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7017      +/-   ##
==========================================
- Coverage   88.48%   88.15%   -0.34%     
==========================================
  Files         229      236       +7     
  Lines        8418     8643     +225     
==========================================
+ Hits         7449     7619     +170     
- Misses        969     1024      +55
Impacted Files Coverage Δ
lib/plugins/aws/deploy/lib/uploadArtifacts.js 90.47% <ø> (ø) :arrow_up:
...mpile/events/lib/ensureApiGatewayCloudWatchRole.js 100% <ø> (ø) :arrow_up:
...ins/aws/package/compile/events/apiGateway/index.js 100% <ø> (ø) :arrow_up:
...ompile/events/apiGateway/lib/method/integration.js 98.14% <ø> (ø) :arrow_up:
lib/plugins/index.js 100% <ø> (ø) :arrow_up:
...ns/aws/package/compile/events/eventBridge/index.js 100% <ø> (+1.56%) :arrow_up:
lib/classes/Utils.js 94.73% <ø> (ø) :arrow_up:
lib/utils/config/index.js 73.07% <0%> (-1.44%) :arrow_down:
lib/plugins/package/lib/packageService.js 86.06% <0%> (-1.44%) :arrow_down:
commitlint.config.js 0% <0%> (ø)
... and 57 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c34687e...b7da914. Read the comment docs.

codecov-io avatar Nov 27 '19 07:11 codecov-io

Please check my serverless.yaml example again.

Currently when serverless authorizers get a name it's defined by in function name property (authorize in the example). But there's ability to override that name with name attribute (${opt:prefix}-authorize).

When I run:

sls deploy --opt prefix1 and then sls deploy --opt prefix2

In current sls version deploy with prefix2 will fail telling that such authorizer already exists in API Gateway. But not in the patched version where each deploy will get own authorizer name.

Probably there's easier way to do it but I've found only this place where I could patch authorize name by referencing a lambda name:

const authorizerName = functionRefFromArn ? {Ref : functionRefFromArn} : authorizer.name;

KostiantynKopytov avatar Dec 07 '19 10:12 KostiantynKopytov

In current sls version deploy with prefix2 will fail telling that such authorizer already exists in API Gateway.

@KostiantynKopytov thanks for more info. Still, I was not able to reproduce that. I've successfully deployed your example using two different prefixes one after another with latest version of Serverless (specifically I've used this test service: https://github.com/medikoo/test-serverless/tree/api-gateway-custom-authorizer )

medikoo avatar Dec 09 '19 20:12 medikoo

Hmm, may be the difference is that originally project was deployed without prefix, and it created authorizer function. And that stack is still deployed in my case. But when prefix was added to project serverless was stil trying to ref authorizer without prefix. I'll try to reproduce it again - with and without prefix and let you know.

KostiantynKopytov avatar Dec 17 '19 09:12 KostiantynKopytov

I tested again and found the difference. We are using custom api gateway name:

  apiGateway:
    restApiId             : xxxxxxxx
    restApiRootResourceId : zzzzzzz

So all deployed stacks are deployed into single api-gateway. If it's not defined - each stack creates own apigateway so problem is not appearing.

To summorize: I deleted old stack with authorizer. I deployed new stack with new authorizer with custom name:

authorize: # current authorizer name
   name: ${opt:prefix}-authorize

It's deployed to api gateway but authorizer has name authorize, and next deploy with another prefix fails because it's already exists.

But with this patch it gives each authorizer the same name as lambda it's attached to.

KostiantynKopytov avatar Jan 14 '20 07:01 KostiantynKopytov

Codecov Report

Merging #7017 into master will increase coverage by 0.00%. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7017   +/-   ##
=======================================
  Coverage   88.14%   88.15%           
=======================================
  Files         236      236           
  Lines        8641     8643    +2     
=======================================
+ Hits         7617     7619    +2     
  Misses       1024     1024           
Impacted Files Coverage Δ
...ckage/compile/events/apiGateway/lib/authorizers.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update de887ac...b7da914. Read the comment docs.

codecov-commenter avatar Aug 26 '20 19:08 codecov-commenter