Fix #3184 Compatibility with native AWS CloudFormation variable syntax #8279

mnapoli · 2020-09-23T14:36:44Z

Closes: #3184

This PR brings support for native CloudFormation variables, such as ${AWS::Region} or CloudFormation references (e.g. ${MyS3Bucket}). I haven't tested yet if !Sub works correctly after this change, maybe there are additional changes to do for that.

I applied the solution described in #3184 by @medikoo (change the variableSyntax regular expression), however I had to tune it a bit to preserve support for the ${file(...)} syntax.

I tested the regex with these patterns:

# ignored without errors:
${AWS::Region}
${Database}

# parsed by the serverless framework:
${foo:bar}
${self:custom.ini, "foo"}
${self:custom.ini, "fallback123_*"}
${self:custom.ini, "hello+world^*$@(!"}
${self:custom.ini, "+++++"}
${self:custom.ini, "システム管理者*"}
${file(~/config.yml)}
${file(~/somedir/config.yml)}
${file(../config.json):CREDS}
${self:}

I also had to add some tests to cover better some syntax. I'll add some inline comments.

mnapoli · 2020-09-23T14:38:44Z

lib/classes/Variables.test.js

+          readFileSyncStub.restore();
+          fileExistsStub.restore();
+        });
+    });


I added a higher level test here because there was no test covering that ${file(...)} is actually parsed correctly. When I changed the regex, initially the tests passed but it was a false positive: that syntax was broken. So I added that test and iterated on the regex.

codecov-commenter · 2020-09-23T14:39:47Z

Codecov Report

Merging #8279 into master will increase coverage by 0.07%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #8279      +/-   ##
==========================================
+ Coverage   88.11%   88.19%   +0.07%     
==========================================
  Files         250      250              
  Lines        9365     9400      +35     
==========================================
+ Hits         8252     8290      +38     
+ Misses       1113     1110       -3

Impacted Files	Coverage Δ
lib/classes/Service.js	`85.48% <ø> (ø)`
lib/plugins/aws/package/compile/functions/index.js	`96.64% <0.00%> (-0.40%)`	⬇️
lib/plugins/aws/package/compile/layers/index.js	`95.60% <0.00%> (ø)`
lib/plugins/aws/provider/awsProvider.js	`93.18% <0.00%> (+0.07%)`	⬆️
lib/plugins/package/lib/packageService.js	`90.16% <0.00%> (+4.09%)`	⬆️

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 76e02cc...b26eeee. Read the comment docs.

mnapoli · 2020-09-23T14:39:52Z

lib/plugins/print/print.test.js

-      expect(print.serverless.cli.consoleLog.called).to.be.equal(true);
-      expect(YAML.load(message)).to.eql(expected);
-    });
-  });


I removed that test: it tested that the ${self:} syntax worked. I had a look at the git history and the rest of the project but couldn't understand why this was tested.

Either I missed something and we should bring that back + cover it with the new regex (let me know), or it's something that isn't necessary anymore and we can move forward.

Good call. I would add testing some of ${self: ..} to `variables fixture as proposed in other comment

medikoo

@mnapoli thank you ! That looks great. See my proposal on (I believe) better approach to testing. Having that set up with runServerless, will make it really solid I believe

lib/classes/Service.js

medikoo · 2020-09-24T07:36:57Z

lib/classes/Variables.test.js

+    it('should support ${file(...)} syntax', () => {
+      // eslint-disable-next-line no-template-curly-in-string
+      const property = '${file(~/somedir/../config.yml)}';
+
+      const expectedFileName = `${os.homedir()}/somedir/../config.yml`;
+      const configYml = {
+        foo: 'bar',
+      };
+      const fileExistsStub = sinon.stub(serverless.utils, 'fileExistsSync').returns(true);
+      const realpathSync = sinon.stub(fse, 'realpathSync').returns(expectedFileName);
+      const readFileSyncStub = sinon.stub(serverless.utils, 'readFileSync').returns(configYml);
+
+      return serverless.variables
+        .populateProperty(property)
+        .then(valueToPopulate => {
+          expect(realpathSync).to.not.have.been.called;
+          expect(fileExistsStub).to.have.been.calledWithMatch(expectedFileName);
+          expect(readFileSyncStub).to.have.been.calledWithMatch(expectedFileName);
+          expect(valueToPopulate).to.deep.equal(configYml);
+        })
+        .finally(() => {
+          realpathSync.restore();
+          readFileSyncStub.restore();
+          fileExistsStub.restore();
+        });
+    });


Thanks @mnapoli that's a very good call! Still we configure all new tests with runServerless (see https://github.com/serverless/serverless/tree/master/test#unit-tests) and I think this tests can also be way better expressed with that.

I propose to introduce a variables fixture, with simple config that references real file via ${file( variable, and other cases we want to test when covering this PR.

Then in tests we will just confirm that resolved config reflects content from imported file (no stubs needed)

Oh wow that new approach is a THOUSAND times better! To be honest every time I tried to contribute to the project I was struggling so much with the tests and the mocks. But this is awesome, I did struggle to get it working at first but now it feels much better, and safer.

I updated the PR, let me know if this is what you had in mind.

lib/classes/Variables.test.js

lib/plugins/print/print.js

medikoo · 2020-09-24T07:57:07Z

lib/plugins/print/print.test.js

-      expect(print.serverless.cli.consoleLog.called).to.be.equal(true);
-      expect(YAML.load(message)).to.eql(expected);
-    });
-  });


Good call. I would add testing some of ${self: ..} to `variables fixture as proposed in other comment

See #8279 (comment)

medikoo

@mnapoli that looks great! I've proposed just one optimization and style improvement to prepared tests.

lib/classes/Variables.test.js

medikoo

That's excellent! Thank you @mnapoli

walery · 2020-09-30T11:10:40Z

Probably the ${self:} was tested because there is example in doc that uses this. I'm not sure how to use it. I've tired to resolve ${self:} with sls print and sls package (in older version where this PR wasn't merged) but both wasn't able to handle this. I think reason for failure is because there is self reference in some objects, but I didn't took deeper look on it. However, I believe ${self:} should be removed from the example in the doc.

There is another case that is uncovered. This is the ${env:} case documented here.

walery · 2020-09-30T11:20:45Z

Btw. in our serverless.yml templates we're using custom variableSyntax and we have our own test cases. I just want to share it as I've already collected the cases some time ago.

From sls doc

https://www.serverless.com/framework/docs/providers/aws/guide/variables/

${self:}
${self:custom.newService.service}-export
${opt:stage, 'dev'}
${sls:instanceId}
${env:SOME_VAR}
${env:}
${opt:stage}
${cf:another-service-dev.functionPrefix}
${cf:another-stack.functionPrefix}
${cf.us-west-2:another-service-dev.functionPrefix}
${s3:myBucket/myKey}
${ssm:/path/to/service/id}
${ssm:/path/to/secureparam~true}
${ssm:/path/to/stringlistparam~split}
${ssm:/aws/reference/secretsmanager/secret_ID_in_Secrets_Manager~true}
${file(../myCustomFile.yml)}
${file(../myCustomFile.yml):globalSchedule}
${file(../myCustomFile.js):schedule.ten}
${env:${opt:stage}_arn}
${opt:stage, self:provider.stage}
${opt:region, 'us-west-1'}

From aws Fn::Sub syntax

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/pseudo-parameter-reference.html
and
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/intrinsic-function-reference-sub.html

${MyResource}
${MyResource.Arn}
${MyResource.Arn.Port}
${AWS::AccountId}
${AWS::NotificationARNs}
${AWS::NoValue}
${AWS::Partition}
${AWS::Region}
${AWS::StackId}
${AWS::StackName}
${AWS::URLSuffix}

medikoo · 2020-09-30T11:48:57Z

@walery great thanks for that info. I believe it's some unfinished examples, at least it's hard for me to understand what's the use in ${self:}

I've opened a dedicated issue to handle that: #8306

Added Deprecation notice

damien-monni · 2020-11-06T17:53:59Z

Hi, noob question here: I tried to use ${AWS::AccountId} in my template file to inject it in an environment variable but it did not work:

environment:
  ACCOUNT_ID: ${AWS::AccountId}

process.env.ACCOUNT_ID is equal to ${AWS::AccountId} string, it is not replaced by my AWS Account ID.

Am I doing something wrong? I am using serverless 2.4.0.

mnapoli · 2020-11-06T19:06:37Z

I think it should be ACCOUNT_ID: !Sub '${AWS::AccountId}' because it's not a serverless variable, and CloudFormation needs !Sub to process it.

This is documentation of change added in serverless#8279

Update documentation for change added in serverless#8279

mnapoli commented Sep 23, 2020

View reviewed changes

mnapoli mentioned this pull request Sep 23, 2020

Variables syntax collides with AWS params syntax #3184

Closed

medikoo suggested changes Sep 24, 2020

View reviewed changes

medikoo added bug/design Functionality design flaw cat/variable labels Sep 24, 2020

medikoo assigned mnapoli Sep 24, 2020

mnapoli added 2 commits September 24, 2020 12:30

Fix #3184 Compatibility with native AWS CloudFormation variable syntax

8b593fd

#3184 Simplify variables regex

185b4e8

See #8279 (comment)

mnapoli requested a review from medikoo September 24, 2020 10:36

medikoo suggested changes Sep 24, 2020

View reviewed changes

lib/classes/Variables.test.js Outdated Show resolved Hide resolved

lib/classes/Variables.test.js Outdated Show resolved Hide resolved

#3184 Refactor tests with await and before step

b26eeee

mnapoli requested a review from medikoo September 24, 2020 12:18

medikoo approved these changes Sep 24, 2020

View reviewed changes

medikoo merged commit 2fdeb51 into serverless:master Sep 24, 2020

mnapoli deleted the fix-3184 branch September 24, 2020 13:47

medikoo mentioned this pull request Sep 30, 2020

Cleanup invalid variable examples from doc #8306

Closed

tusbar referenced this pull request in svdgraaf/serverless-pseudo-parameters Oct 1, 2020

Update README.md

91a4dac

Added Deprecation notice

medikoo mentioned this pull request Oct 5, 2020

Variables: Fix support for "${self:}" #8343

Merged

walery added a commit to walery/serverless that referenced this pull request Feb 3, 2021

Add Sub function example to Pseudo Parameters doc.

b08ee4f

This is documentation of change added in serverless#8279

walery mentioned this pull request Feb 3, 2021

Add Sub function example to Pseudo Parameters doc. #8883

Closed

walery added a commit to walery/serverless that referenced this pull request Feb 3, 2021

Add Sub function example to Pseudo Parameters doc.

cf0947d

This is documentation of change added in serverless#8279

walery added a commit to walery/serverless that referenced this pull request Feb 3, 2021

Add Sub function example to Pseudo Parameters doc.

347675b

Update documentation for change added in serverless#8279

walery mentioned this pull request Feb 3, 2021

docs: Add Sub function example to pseudo parameters doc. #8885

Merged

72636c mentioned this pull request Feb 8, 2021

Remove custom Serverless variable syntax seek-oss/skuba#350

Merged

jkeifer mentioned this pull request May 27, 2022

Support Lambda images cirrus-geo/cirrus-geo#139

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #3184 Compatibility with native AWS CloudFormation variable syntax #8279

Fix #3184 Compatibility with native AWS CloudFormation variable syntax #8279

mnapoli commented Sep 23, 2020 •

edited

Loading

mnapoli Sep 23, 2020

codecov-commenter commented Sep 23, 2020 •

edited

Loading

mnapoli Sep 23, 2020

medikoo Sep 24, 2020

medikoo left a comment

medikoo Sep 24, 2020

mnapoli Sep 24, 2020

medikoo Sep 24, 2020

medikoo left a comment

medikoo left a comment

walery commented Sep 30, 2020

walery commented Sep 30, 2020

medikoo commented Sep 30, 2020

damien-monni commented Nov 6, 2020 •

edited

Loading

mnapoli commented Nov 6, 2020

Fix #3184 Compatibility with native AWS CloudFormation variable syntax #8279

Fix #3184 Compatibility with native AWS CloudFormation variable syntax #8279

Conversation

mnapoli commented Sep 23, 2020 • edited Loading

mnapoli Sep 23, 2020

Choose a reason for hiding this comment

codecov-commenter commented Sep 23, 2020 • edited Loading

Codecov Report

mnapoli Sep 23, 2020

Choose a reason for hiding this comment

medikoo Sep 24, 2020

Choose a reason for hiding this comment

medikoo left a comment

Choose a reason for hiding this comment

medikoo Sep 24, 2020

Choose a reason for hiding this comment

mnapoli Sep 24, 2020

Choose a reason for hiding this comment

medikoo Sep 24, 2020

Choose a reason for hiding this comment

medikoo left a comment

Choose a reason for hiding this comment

medikoo left a comment

Choose a reason for hiding this comment

walery commented Sep 30, 2020

walery commented Sep 30, 2020

From sls doc

From aws Fn::Sub syntax

medikoo commented Sep 30, 2020

damien-monni commented Nov 6, 2020 • edited Loading

mnapoli commented Nov 6, 2020

mnapoli commented Sep 23, 2020 •

edited

Loading

codecov-commenter commented Sep 23, 2020 •

edited

Loading

damien-monni commented Nov 6, 2020 •

edited

Loading