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

BREAKING - Refactor function arn generation for info plugin #3125

Merged
merged 4 commits into from Jan 23, 2017

Conversation

@pmuens
Copy link
Member

commented Jan 20, 2017

What did you implement:

Closes #1825 and refs #2853

The function ARNs are now computed on the fly. Function ARN outputs are no longer generated which reduces the size of CloudFormation Outputs dramatically.

How did you implement it:

Added a getAccountId() method in the provider plugin. Furthermore the adding of the function output is removed in the compile functions step. The arns will be computed on the fly when invoking the info command.

How can we verify it:

Deploy a service and run serverless info. Furthermore look into the CloudFormation Outputs section in the AWS console.

Todos:

  • Write tests
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config/commands/resources
  • Change ready for review message below

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

@pmuens pmuens added the pr/in-review label Jan 20, 2017

@pmuens pmuens added this to the 1.6 milestone Jan 20, 2017

@pmuens pmuens force-pushed the refactor-function-arn-for-info-plugin branch from 78ec9f8 to a6c3240 Jan 20, 2017

this.serverless.service.getAllFunctions().forEach((func) => {
const functionInfo = {};
const name = `${this.serverless.service.service}-${stage}-${func}`;
const arn = `arn:aws:lambda:${region}:${accountId}:function:${name}`;

This comment has been minimized.

Copy link
@ryansb

ryansb Jan 20, 2017

Contributor

Is it possible to instead get this out of CloudFormation's ListStackResources API as the PhysicalResourceId? For Lambda functions I'm not sure what CFN uses as a physical ID, but it may be the ARN, which would let us avoid auto-building ARNs here.

This comment has been minimized.

Copy link
@pmuens

pmuens Jan 23, 2017

Author Member

Thanks for pointing into that direction @ryansb ! I'll look into it.
Will be more reliable when fetching through this API

@eahefnawy eahefnawy self-requested a review Jan 22, 2017

@eahefnawy
Copy link
Member

left a comment

Thanks for taking a dive into this @pmuens ... However, I feel that the implementation still lacks solutions to the challenges I mentioned earlier, mainly with regard to regions. So the ARNs that are computed are not really accurate.

That makes me doubt the usefulness of this feature in general. I'm starting to think that displaying function ARNs is not really worth the trouble. But I'm open for ideas! 😊

_.merge(this.serverless.service.provider.compiledCloudFormationTemplate.Outputs,
newOutputObject);

// Add function versions to Outputs section
const functionVersionOutputLogicalId = this.provider.naming

This comment has been minimized.

Copy link
@eahefnawy

eahefnawy Jan 22, 2017

Member

I am assuming this is only added if users opt-in function versioning. Do you think there's a way to compute function version arns on the fly as well?

This comment has been minimized.

Copy link
@ryansb

ryansb Jan 22, 2017

Contributor

Yes, that's right. The conditional for adding it to the template (or not) is here

.then(() => this.provider.getAccountId())
.then((accountId) => {
const stage = this.provider.getStage();
const region = this.provider.getRegion();

This comment has been minimized.

Copy link
@eahefnawy

eahefnawy Jan 22, 2017

Member

If I understand this correctly, this is based on the region option, along with the us-east-1 default. So the generated ARNs don't reflect what is actually deployed. So if I only deploy to us-west-2 and then run sls info, that would generate ARNs pointing to none existing functions in us-east-1. Is that correct?

This comment has been minimized.

Copy link
@pmuens

pmuens Jan 23, 2017

Author Member

Yes. I'll update the code so that this.options.region is used. This is used anyway for the retrieval. See here:

this.options.stage,
this.options.region)


this.serverless.service.getAllFunctions().forEach((func) => {
const functionInfo = {};
const name = `${this.serverless.service.service}-${stage}-${func}`;

This comment has been minimized.

Copy link
@eahefnawy

eahefnawy Jan 22, 2017

Member

The same comment above, but for stage as well.

This comment has been minimized.

Copy link
@eahefnawy

eahefnawy Jan 22, 2017

Member

Also, this should handle the case where a lambda function has a custom name.

This comment has been minimized.

Copy link
@pmuens

pmuens Jan 23, 2017

Author Member

Yep. I agree. But in this case the getAllFunctions() method should handle the correct name retrieval.

This comment has been minimized.

Copy link
@eahefnawy

eahefnawy Jan 23, 2017

Member

Yep that works too. But maybe that would make the service class AWS specific rather than provider agnostic? I'm sensing that we need a method in the provider class, but not worth it considering the upcoming changes 😉

@@ -240,6 +240,14 @@ class AwsProvider {
}
return returnValue;
}

getAccountId() {

This comment has been minimized.

Copy link
@eahefnawy

eahefnawy Jan 22, 2017

Member

Nice!!! ❤️ ... We've needed this for so long already!

@pmuens pmuens changed the title Refactor function arn generation for info plugin BREAKING - Refactor function arn generation for info plugin Jan 23, 2017

@pmuens

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2017

@ryansb and @eahefnawy thanks for reviewing the PR and proposing changes!

I've updated the PR accordingly. The stage and region are not retrieved form this.options. It's used anyway to get information about the stack (see:

this.options.stage,
this.options.region)
).

I agree that we might drop this function if it needs more attention and gets more complicated to generate the ARNs on the fly.

The generation of the ARNs is only used when the info plugin is run. The version ARNs are not displayed in the info command so therefore I don't know if we should compute them on the fly because then we need to add the computation at the places where users might query the version ARNs (I hope that this was understandable 😆 . Need a coffee right now ☕️ )...

@eahefnawy
Copy link
Member

left a comment

@pmuens aaaah good point. So that means if the user chose a region he hasn't deployed to, he'll get an error anyway. Never thought I'd feel good about an error! 😄

Ok I think this is good to merge 👍

@eahefnawy eahefnawy merged commit a19fbfb into master Jan 23, 2017

2 checks passed

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

@eahefnawy eahefnawy deleted the refactor-function-arn-for-info-plugin branch Jan 23, 2017

@pmuens

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2017

Yep. Exactly @eahefnawy 👍 Thanks for merging!

@erikerikson

This comment has been minimized.

Copy link
Member

commented Jan 27, 2017

This change makes it so that we cannot use Serverless. We don't have "iam:GetUser" rights. We don't have users, for that matter. This is not something we can change.

We can do aws sts get-caller-identity --output text --query 'Account' --profile MY_PROFILE

@erikerikson

This comment has been minimized.

Copy link
Member

commented Jan 27, 2017

Incidentally, this code is used during sls deploy so we're dead in the water.

@erikerikson

This comment has been minimized.

Copy link
Member

commented Jan 27, 2017

I'll write up a bug

@erikerikson erikerikson referenced this pull request Jan 27, 2017
@erikerikson

This comment has been minimized.

Copy link
Member

commented Jan 27, 2017

As linked, an issue is opened. I've offered a PR (#3152) containing a potential fix as well.

@pmuens

This comment has been minimized.

Copy link
Member Author

commented Jan 27, 2017

Oh damn. Sorry for that @erikerikson and thanks for the PR! 👍

Seems like other users are also suffering from that (see: #3140).

Will review the PR today!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.