Skip to content

Conversation

@erikerikson
Copy link
Contributor

@erikerikson erikerikson commented Feb 20, 2018

What did you implement:

Fixes:

  1. deep variables can contain overwrites (see Eliminate/Report Hung Promises (#4687), Prepopulate Stage and Region (#4311), Handle Quoted Strings (#4734) #4713 (comment) - thank you @laardee). Reported on All variables should resolve, timeout, or be identified #4687.
  2. deep variables can render to different variables and must thereby be deep-variable-ified too. New bug of All variables should resolve, timeout, or be identified #4687 discovered fixing previous.
  3. errors with the print command cross contaminating the given serverless.yml with the previously populated and adorned version processed by the framework. OLD, painful, and previously unreported.
  4. cleans a messy prepopulation implementation. Closes A valid service attribute to satisfy the declaration 'self:service.name' could not be found #4744.
  5. a cyclic references bug leading to crashes when using print. Closes 1.13.1 and 1.13.2 break ${self:} references. #3627, Closes Detect cyclic dependencies in Serverless Variable system #3711, Closes JavaScript heap out of memory #3740, Closes Hanging on sls deploy, with no output #3882, Closes Referencing ${self:} leads to crash in the print command #4556, Closes All variables should resolve, timeout, or be identified #4687.
  6. Silently undetected unresolved variables Closes Program exits with no error(s) when there is error inside expression in serverless.yml #4699.
  7. Support the variables "${self:service.name}" and "${self:provider}" which users expect to be valid because they are present in their serverless.yml but are mutated by the Service class. Closes A valid service attribute to satisfy the declaration 'self:service.name' could not be found #4744.

How did you implement it:

  1. ensure that the getValueFromDeep method uses the standard logic for detecting and resolving overwrites rather than assuming that all deep variables are standard single variable strings.
  2. add sophistication to getValueFromDeep, detecting recursive resolution to a new variable containing string.
  3. clean caches for each invocation of variable population methods (primary entry points only).
  4. knock out retrieval of restricted variable types for prepopulation, use existing methods.
  5. clean caches.
  6. Not exactly sure which change fixed this one.
  7. Implement variable replacement for these special cases.

How can we verify it:

npm run test

Todos:

  • Write tests
  • [n/a] Write documentation (no changes to existing documented variable system)
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

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

…ank you!)

Simple oversight that deep variables can reference overwrites too.  This fix acknowledges this oversight by using the standard logic for using overwrite instead of getValueFromSource in this case.
@HyperBrain
Copy link
Contributor

@laardee Can you give this PR a try and see if the problem is solved?

…(DV) cases

Solve for these cases.  The bug was that DVs can render to DVs when a value is being obtained from them (i.e. they are being de-referenced in getValueFromDeep).  This is particularly problematic in the case that the original DV being rendered has a deep reference into the rendered DV.  In that case the DV returned by rendering must be replaced by yet another DV.

While accomplishing the above, various bits of cleanup were implemented and I added some commentary around getDeeperValue for future engineers.

This case is particularly relevant if you have:

`serverless.yml`:
```
service: serverless-hello-world

provider:
  name: aws
  runtime: nodejs6.10
  stage: dev
  environment:
    SECRET: ${self:custom.secrets.SECRET}

functions:
  helloWorld:
    handler: handler.helloWorld
    events:
      - http:
          path: hello-world
          method: get
          cors: true

custom:
  stage: ${opt:stage, self:provider.stage}
  secrets: ${file(secrets.${self:custom.stage}.yml)}
```
AND
`secrets.dev.yml`:
```
SECRETS: secrets
```

Populating this service should result in the following sequence of rendering phases:

#########################
# PHASE 1
#
# ${self:custom.secrets.SECRET}
#   <- ${deep:0.SECRET}
#   deep[0] = ${file(secrets.${self:custom.stage}.yml)}
#
# ${opt:stage, self:provider.stage}
#   <- 'dev'
#
# ${file(secrets.${self:custom.stage}.yml)}
#   <- ${file(secrets.${deep:1}.yml)}
#   deep[1] = ${opt:stage, self:provider.stage}
#
# RESULT
#
# service: serverless-hello-world
#
# provider:
#   name: aws
#   runtime: nodejs6.10
#   stage: dev
#   environment:
#     SECRET: ${deep:0.SECRET}
#
# functions:
#   helloWorld:
#     handler: handler.helloWorld
#     events:
#       - http:
#           path: hello-world
#           method: get
#           cors: true
#
# custom:
#   stage: dev
#   secrets: ${file(secrets.${deep:1}.yml)}
#########################

#########################
# PHASE 2
#
# ${deep:0.SECRET}
#   <- this.populateValue('${file(secrets.${self:custom.stage}.yml)}') => '${file(secrets.${deep:1}.yml)}' => '${deep:2}' => '${deep:2.SECRET}'
#   deep[2] = ${file(secrets.${deep:1}.yml)}
#
# ${file(secrets.${deep:1}.yml)}
#   <- this.populateValue('${file(secrets.${deep:1}.yml)}') => '${file(secrets.dev.yml)}' => '${deep:3}'
#   deep[3] = ${file(secrets.dev.yml)}
#
# RESULT
#
# service: serverless-hello-world
#
# provider:
#   name: aws
#   runtime: nodejs6.10
#   stage: dev
#   environment:
#     SECRET: ${deep:2.SECRET}
#
# functions:
#   helloWorld:
#     handler: handler.helloWorld
#     events:
#       - http:
#           path: hello-world
#           method: get
#           cors: true
#
# custom:
#   stage: dev
#   secrets: ${deep:3}
#########################

#########################
# PHASE 3
#
# ${deep:2.SECRET}
#   <- this.populateValue('${file(secrets.${deep:1}.yml)}') => '${file(secrets.dev.yml)}' => '${deep:3}' => '${deep:3.SECRET}'
#   deep[3] is already set to ${file(secrets.dev.yml)}
#
# ${deep:3}
#   <- this.populateValue('${file(secrets.dev.yml)}') => { SECRET: 'secret' }
#
# RESULT
#
# service: serverless-hello-world
#
# provider:
#   name: aws
#   runtime: nodejs6.10
#   stage: dev
#   environment:
#     SECRET: ${deep:3.SECRET}
#
# functions:
#   helloWorld:
#     handler: handler.helloWorld
#     events:
#       - http:
#           path: hello-world
#           method: get
#           cors: true
#
# custom:
#   stage: dev
#   secrets:
#     SECRET: secret
#########################

#########################
# PHASE 4
#
# ${deep:3}
#   <- this.populateValue('${file(secrets.dev.yml)}') => this.getDeeperValue(['SECRET'], { SECRET: 'secret' }) => 'secret'
#
# RESULT
#
# service: serverless-hello-world
#
# provider:
#   name: aws
#   runtime: nodejs6.10
#   stage: dev
#   environment:
#     SECRET: secret
#
# functions:
#   helloWorld:
#     handler: handler.helloWorld
#     events:
#       - http:
#           path: hello-world
#           method: get
#           cors: true
#
# custom:
#   stage: dev
#   secrets:
#     SECRET: secret
#########################
@erikerikson
Copy link
Contributor Author

I have validated the latest push against the full materials @laardee provided. Am awaiting @laardee's validation. Apologies for the early push.

@laardee
Copy link
Contributor

laardee commented Feb 21, 2018

Yes, now it packages our project at least as good as before. This, or the previous PR, fixes also the issue that we faced with @medikoo where the region variable was not resolved early enough for SSM variables.

@erikerikson
Copy link
Contributor Author

I have detected a returned cyclic error here. So many configs to check...
Will update as I know more.

@erikerikson
Copy link
Contributor Author

This cyclic issue is restricted to the print command. Continuing investigation.

Fix `print`
The print command is highly linked to the `Variables` and `Service` codebases, keep those in sync and leave reminders about the link.  Made these explicit and separately implemented to avoid complexity.
Additionally, the print command re-populates an object with the *very similar* content as the previously pre-populated service (just not augmented as just mentioned).  This can lead to cross contamination between the two.  As such, all caches must be cleared per unique invocation of service/object/property population.
Add tests for some expected but previously unverified behaviors.

Clean pre-population
The previous implementation worked okay but was unnecessary and would have been a maintenance problem.  Instead, just knock out the population of variables depending on those config dependent services and use the standard means of resolution.

Fix cyclic bug (resulting from running print against a self-referencing serverless.yml)
The caching of values could lead to a cyclic object remaining in the caches for variable population.  This causes crashes and pain.  Solved by the cache cleaning logic.
@erikerikson erikerikson changed the title Allow Deep Variables to be Overwrites Follow up fixes for #4713 Feb 23, 2018
The framework's Service class modifies a user's service, mutating a given `service: { name: 'string' }` to `service: 'string'` but doesn't account for this in the variables system.  This is the basis for serverless#4744.

While working this area, I discovered that the Service class also does this to the provider, accepting `provider: 'aws'` and replacing it with `provider: { name: 'aws' }`, causing the natural `${self:provider}` to fail.

Catching either 'self:service.name' or 'self:provider' and replacing them with the mutated reference solves this user confusion.
@erikerikson
Copy link
Contributor Author

This should be ready for merge.

@HyperBrain HyperBrain added this to the 1.27 milestone Feb 23, 2018
@HyperBrain
Copy link
Contributor

@erikerikson Wow. Thank you. Can you add all issues that are closed by this PR (there seem to be a whole lot of them 😄 ) as Closes # to the description? We should make sure that everything is cleaned up with the merge.

Copy link
Contributor

@HyperBrain HyperBrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice stuff in here 🎉 . I have added only some minor comments.

const logWarning = require('./Error').logWarning;
const PromiseTracker = require('./PromiseTracker');

/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

let variable = variableString;
// The loaded service is altered during load in ~/lib/classes/Service (see loadServiceFileParam)
// Account for these so that user's reference to their file populate properly
if (variable === 'self:service.name') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can add a comment to the Service class at the proper location to warn people, that any changes done there that affect the service property, have to be checked with this implementation too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good point. Missed commenting this. Adding additional comment here too.

if (_.isString(this.cache.serviceProvider)) {
service.provider = { name: this.cache.serviceProvider };
}
service.provider = _.merge({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be only done if the provider is set to aws, shouldn't it? The print command is located at top level (outside of the aws plugin) and might be called for arbitrary provider types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have expected so too. However, the code in ~/lib/classes/Service.js (

this.provider = {
) does this, I believe generally, for all services. Do I misunderstand? Otherwise, I'm happy to improve this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed 🤔 . So it seems to be just an initial value there (that will be overridden by the incoming service configuration). In theory, a service.yml without provider definition would work then, which is a bit strange, but so be it.

However, here in the print command I'm not really sure if that, what we do here, is really just a preliminary intialization that will be overwritten or will influence subsequent behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it was weird to see at first but I faulted on the side of parity to avoid bugs resulting from dissonance. Thank you very much for paying attention here!

@erikerikson erikerikson changed the title Follow up fixes for #4713 Follow up fixes for #4713, Closes #3627, Closes #3711, Closes #3740, Closes #3882, Closes #4556, Closes #4687, Closes #4699, Closes #4744 Feb 23, 2018
@erikerikson
Copy link
Contributor Author

Thanks for the kick to actually read docs on how the title "closes #..."/keywords feature :D

I have added comments as discussed. If you agree with my analysis of the print.js, line 41 comment, this should be ready to merge but otherwise, I'll fix it.

@HyperBrain
Copy link
Contributor

@erikerikson I meant in the bug description, not subject 😆

@erikerikson erikerikson changed the title Follow up fixes for #4713, Closes #3627, Closes #3711, Closes #3740, Closes #3882, Closes #4556, Closes #4687, Closes #4699, Closes #4744 Follow up fixes for #4713 Feb 23, 2018
Copy link
Contributor

@HyperBrain HyperBrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, good to go 👍

@HyperBrain HyperBrain merged commit 8e6582d into serverless:master Feb 24, 2018
@HyperBrain
Copy link
Contributor

Merged. Hopefully some people will test with master and verify that the covered cases now work as expected.

@laardee
Copy link
Contributor

laardee commented Feb 24, 2018

Some people here 😀 I quickly tested with the master and now I get some errors with SSM parameters and provider/region.

I have a my-ssm-parameter stored in eu-west-1. sls package --region eu-west-1 should then resolve the value of the my-ssm-parameter. It works ok with v1.26.

provider:
  name: aws
  runtime: nodejs6.10
  stage: dev
  environment:
    MY_SSM_PARAM: ${ssm:my-ssm-parameter}

But it fails with error Variable Failure: value region set to 'ssm:my-ssm-parameter' references SSM which requires a region value for use. with the master.

Adding the region to the provider doesn't help either and packaging with sls package and sls package --region eu-west-1 fails with same error. This also works with v1.26.

provider:
  name: aws
  runtime: nodejs6.10
  stage: dev
  region: eu-west-1
  environment:
    MY_SSM_PARAM: ${ssm:my-ssm-parameter}

But when I add the region to custom and refer to it in provider, packaging works as expected.

provider:
  name: aws
  runtime: nodejs6.10
  stage: dev
  region: ${self:custom.region}
  environment:
    MY_SSM_PARAM: ${ssm:my-ssm-parameter}

custom:
  region: eu-west-1

This last setup is about the same that we have. We have the region in a file, but somewhat similar. I used that setup to test the PR earlier so I didn't catch this. 😞

@HyperBrain @erikerikson can you quickly check if my findings are valid.

@erikerikson
Copy link
Contributor Author

Thanks for reporting/finding this @laardee, we're working from an unfortunate deficit of test cases.

I'll look into it shortly.

@erikerikson
Copy link
Contributor Author

I've reproduced this and discovered the error. I created a timing issue based on the resolution order of the promises but on a positive note, it should be simple to test for and fix. Incoming...

@erikerikson erikerikson mentioned this pull request Feb 24, 2018
6 tasks
@erikerikson
Copy link
Contributor Author

Just commited tests for the case you encountered and a fix. PR #4769

I also ran the new code against your provided config using the cmd line you gave. I got the expected error from SSM rather than this codebase.

Apologies for the train of issues.

@laardee
Copy link
Contributor

laardee commented Feb 24, 2018

That PR #4769 fixes all of the issues I encountered earlier with the SSM.

@erikerikson
Copy link
Contributor Author

Thanks for all your work in testing and verification @laardee!

@HyperBrain
Copy link
Contributor

Ok. I'll merge the update/fix PR now. Then master can be given a second try and further round of testing 😄

@laardee
Copy link
Contributor

laardee commented Feb 26, 2018

All my use cases work now with the master branch 👏

@erikerikson erikerikson deleted the 4713-follow-up-fixes branch February 26, 2018 07:11
@dschep
Copy link
Contributor

dschep commented Mar 27, 2018

What about ${self:service.awsKmsKeyArn}? This doesn't seem to fix that. Should I file a separate issue?

@erikerikson
Copy link
Contributor Author

What's the issue @dschep? Link?

@dschep
Copy link
Contributor

dschep commented Mar 27, 2018

It doesn't get populated:

$ sls create -t aws-nodejs
$ sed -i'' -r -e 's/service: aws-nodejs ?.*$/service:\n  name: aws-nodejs\n  awsKmsKeyArn: arn:aws:kms:us-east-1:xxxxxxxxxxxx:key\/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx/' serverless.yml # NOTE: that's GNU sed
$ echo -e 'custom:\n  arn: ${self:service.awsKmsKeyArn}' >> serverless.yml
$ sls print
 
 Serverless Warning --------------------------------------
 
  A valid service attribute to satisfy the declaration 'self:service.awsKmsKeyArn' could not be found.
 
 
 Serverless Warning --------------------------------------
 
  A valid service attribute to satisfy the declaration 'self:service.awsKmsKeyArn' could not be found.
 
service:
  name: aws-nodejs
  awsKmsKeyArn: 'arn:aws:kms:us-east-1:xxxxxxxxxxxx:key/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx'
provider:
  name: aws
  runtime: nodejs6.10
functions:
  hello:
    handler: handler.hello
custom: {}

Note that the above is with npm i -g serverless/serverless, with sls 1.26.1 I get:

$ sls -v
1.26.1
$ sls print
 
 Serverless Warning --------------------------------------
 
  A valid service attribute to satisfy the declaration 'self:service.awsKmsKeyArn' could not be found.
 
service:
  name: aws-nodejs
  awsKmsKeyArn: 'arn:aws:kms:us-east-1:xxxxxxxxxxxx:key/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx'
provider:
  name: aws
  runtime: nodejs6.10
functions:
  hello:
    handler: handler.hello
custom:
  arn: 'arn:aws:kms:us-east-1:xxxxxxxxxxxx:key/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx'

@erikerikson
Copy link
Contributor Author

Thank you. I'll look into it.

@erikerikson
Copy link
Contributor Author

It turns out that I don't have GNU sed. To verify, your sed replaces

service: aws-nodejs

with

service:
  name: aws-nodejs
  awsKmsKeyArn: arn:aws:kms:us-east-1:xxxxxxxxxxxx:key/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx

right?

It looks like we need to rewrite self:service.[...] to self:serviceObject.[...] to cover any cases here. Simple change....

@erikerikson
Copy link
Contributor Author

@dschep fixed in #4859

Please take a look/try it out.

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