AWS: Throw an error if ${ssm} fail to populate value, as same as ${cf} and ${s3} by exoego · Pull Request #5770 · serverless/serverless

exoego · 2019-01-31T23:50:46Z

What did you implement:

Closes #4447
Closes #5757
Closes #5781

How did you implement it:

Before this PR, ${ssm} catches 400 error from AWS SSM, and returns undefined as fulfilled value.
This may cause broken service deployment and user may not notice that.
And, such behavior is incosistent with ${cf} and ${s3}.

After this PR, ${ssm} aborts whenver SSM returns error.
The error can be recovered with the fallback value in the form of ${ssm:/path/param, "fallback"} (already implemented).
So every AWS-specific variable syntaxes has uniform behavior.

How can we verify it:

Install the patched serverless from branch.

npm install -g exoego/serverless#ssm-should-fail`
sls create -t aws-nodejs --path ssm-fail
cd ssm-fail

Update serverless.yml with

service:
  name: aws-nodejs

provider:
  name: aws
  runtime: nodejs8.10

functions:
  hello:
    handler: handler.hello

custom:
  ssmVar: ${ssm:/path/to/service/myParamS}

Run sls deploy and confirm it aborts like

$ sls deploy

  Serverless Error ---------------------------------------
 
  Error getting value for ssm:/path/to/service/myParamS. ParameterNotFound
 
  Get Support --------------------------------------------
     Docs:          docs.serverless.com
     Bugs:          github.com/serverless/serverless/issues
     Issues:        forum.serverless.com
 
  Your Environment Information -----------------------------
     OS:                     linux
     Node Version:           8.10.0
     Serverless Version:     1.36.3

Add fallback value to serverless.yml like ssmVar: ${ssm:/path/to/service/myParamS, 'fallback'}
Run sls deploy and confirm it complete successfully

Todos:

Write tests
~~Write documentation~~ N/A
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?: COULD BE

eahefnawy · 2019-02-25T12:23:51Z

@exoego I see some updates here, what's the status of this PR?

# Conflicts: # lib/classes/Variables.test.js

exoego · 2019-02-25T13:34:47Z

This PR is now ready to review.

The change introduced in this PR will provider a broad-based and long-time-awaited behavior, but will be a breaking change, as @pmuens said in #3367 (comment),

I believe fixing this drastically improve DX by preventing deploying broken services, unifying behavior of all variable syntaxes, and providing choices to users.
I also understand serverless framework have strict policy to avoid breaking change as much as possible.

I have a little preference on merging this breaking change to v1, since it is breaking but helps users in essentials. And eventually will open a PR to introduce same behavior to rest of variables (env and other).
But it is OK to postpone the changes to v2.

pmuens

@exoego thank you very much for working on this! 👍

I just tested it today and it works fine. Unfortuantely this will intorduce a breaking change which means that we would have to bump to version 2 (as you already mentioned in your comment). Given this I'd say that we should postpone this PR / feature for the v2 release.

Thanks again for taking the time to work on this 🙏

We'll revisit this PR once we're looking into v2!

pmuens · 2019-03-07T11:41:09Z

+          expect(warnIfNotFoundSpy.callCount).to.equal(0);
+        })
+        .finally(() => {
+          requestStub.restore();


Restores like this should be moved in a beforeEach / afterEach block so that the test suite takes care of the cleanup.

pmuens · 2019-03-07T11:41:54Z

+          `Error getting value for ssm:${param}.`,
+          `Parameter ${param} not found.`,
+        ].join(' '))
+        .then().finally(() => requestStub.restore());


Same comment about restore applies here.

drissamri · 2019-03-07T13:49:11Z

Is there any workaround to have similar behaviour in v1.38? I don't feel confident if a deploy could go through to production with invalid SSM param setup

simlu · 2019-07-04T20:00:13Z

I very much agree. This is very unfortunate behavior.

troyready · 2020-04-22T20:59:00Z

Also would love this to be revisited in 1.0 -- what's the scenario where a deployment should continue if the lookup fails?

Sincerely asking, as I've never encountered this situation without immediately following it up with another deploy.

(EDIT: Obligatory xkcd reference)

simlu · 2020-04-22T21:12:09Z

@troyready I've encountered a few cases where these should optional. E.g. A logger service has the ability to send to different SaaS. But it only sends to those that have an api key configured.

The fallback would solve that nicely though and also make it explicit.

bryantbiggs · 2020-08-14T14:52:21Z

@eahefnawy / @medikoo will a v2 branch be opened soon to address some of these changes that have been around for quite some time?

medikoo · 2020-08-18T11:21:55Z

@bryantbiggs See #3367 it outlines a proposed solution, for which we will welcome a PR.

exoego added 2 commits February 1, 2019 08:27

Add tests for #5757.

d14d57e

Reject instead of returning undefined, since fallback is supported.

8cbc3ac

eahefnawy added the pr/in-progress label Feb 1, 2019

eahefnawy self-assigned this Feb 1, 2019

eahefnawy self-requested a review February 1, 2019 09:55

exoego changed the title ~~[WIP] Throw an error if ${ssm} fail to populate value, as same as ${cf} and ${s3}~~ Throw an error if ${ssm} fail to populate value, as same as ${cf} and ${s3} Feb 2, 2019

exoego mentioned this pull request Feb 5, 2019

Improve error messages for ssm permission issue #5781

Closed

exoego added pr/in-review and removed pr/in-progress labels Feb 5, 2019

Merge remote-tracking branch 'upstream/master' into ssm-should-fail_

7d6f116

exoego changed the title ~~Throw an error if ${ssm} fail to populate value, as same as ${cf} and ${s3}~~ AWS: Throw an error if ${ssm} fail to populate value, as same as ${cf} and ${s3} Feb 17, 2019

Merge remote-tracking branch 'upstream/master' into ssm-should-fail

741d746

# Conflicts: # lib/classes/Variables.test.js

pmuens suggested changes Mar 7, 2019

View reviewed changes

pmuens closed this Mar 7, 2019

exoego mentioned this pull request Jul 5, 2019

Change of versioning agreement #6195

Closed

exoego deleted the ssm-should-fail branch January 10, 2022 03:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

AWS: Throw an error if ${ssm} fail to populate value, as same as ${cf} and ${s3} #5770

AWS: Throw an error if ${ssm} fail to populate value, as same as ${cf} and ${s3} #5770
exoego wants to merge 4 commits into
serverless:masterfrom
exoego:ssm-should-fail

exoego commented Jan 31, 2019 •

edited

Loading

Uh oh!

eahefnawy commented Feb 25, 2019

Uh oh!

exoego commented Feb 25, 2019 •

edited

Loading

Uh oh!

pmuens left a comment

Uh oh!

pmuens Mar 7, 2019

Uh oh!

pmuens Mar 7, 2019

Uh oh!

drissamri commented Mar 7, 2019

Uh oh!

simlu commented Jul 4, 2019

Uh oh!

troyready commented Apr 22, 2020 •

edited

Loading

Uh oh!

simlu commented Apr 22, 2020

Uh oh!

bryantbiggs commented Aug 14, 2020

Uh oh!

medikoo commented Aug 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Uh oh!

Conversation

exoego commented Jan 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What did you implement:

How did you implement it:

How can we verify it:

Todos:

Uh oh!

eahefnawy commented Feb 25, 2019

Uh oh!

exoego commented Feb 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmuens left a comment

Choose a reason for hiding this comment

Uh oh!

pmuens Mar 7, 2019

Choose a reason for hiding this comment

Uh oh!

pmuens Mar 7, 2019

Choose a reason for hiding this comment

Uh oh!

drissamri commented Mar 7, 2019

Uh oh!

simlu commented Jul 4, 2019

Uh oh!

troyready commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simlu commented Apr 22, 2020

Uh oh!

bryantbiggs commented Aug 14, 2020

Uh oh!

medikoo commented Aug 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

exoego commented Jan 31, 2019 •

edited

Loading

exoego commented Feb 25, 2019 •

edited

Loading

troyready commented Apr 22, 2020 •

edited

Loading