serverless icon indicating copy to clipboard operation
serverless copied to clipboard

In serverless@3 serverless.yml aws requests halt execution, even when a defaultValue is specified

Open jdgcj opened this issue 2 years ago • 8 comments

Are you certain it's a bug?

  • [X] Yes, it looks like a bug

Is the issue caused by a plugin?

  • [X] It is not a plugin issue

Are you using the latest v3 release?

  • [X] Yes, I'm using the latest v3 release

Is there an existing issue for this?

  • [X] I have searched existing issues, it hasn't been reported yet

Issue description

When upgrading a project from serverless@2 to serverless@3, I'm noticing that there is a breaking change where any AWS related errors in serverless.yml immediately halt execution, even if a defaultValue is specified.

For example, in serverless@2 this worked fine:

SOME_KEY: ${ssm:/somepath/somekey~false, env:SOME_KEY}

Explanation: If we tried to fetch a value from AWS SSM, if the key didn't exist or the application did not have access, we could fall back to the defaultValue, in this case the environment variable SOME_KEY.

In serverless@3, this behavior has changed. The defaultValue is no longer usable. Execution immediately stops if there is an AWS error.

NOTE: At first this was reported in the serverless-offline plugin, here: https://github.com/dherault/serverless-offline/issues/1278 but dnalborczyk said:

the variable resolution is done by serverless itself, which in turn also throws the error. I don't think there's much we can do about it.

... so I am reporting it here.

Service configuration (serverless.yml) content

SOME_KEY: ${ssm:/somepath/somekey~false, env:SOME_KEY}

Command name and used flags

serverless whatever

Command output

Error:
Cannot resolve serverless.yml: Variables resolution errored with:
  - Cannot resolve variable at "provider.environment.SOME_KEY": AWS provider credentials not found. Learn how to set up AWS provider credentials in our docs here: <http://slss.io/aws-creds-setup>.

Environment information

Framework Core: 3.21.0 (local)
Plugin: 6.2.2
SDK: 4.3.2

jdgcj avatar Jul 21 '22 15:07 jdgcj

Hey @jdgcj, thanks a lot for reporting. I've managed to reproduce the problem locally. The situation where the key is not found seems to be addressed correctly, but I see that we have this issue when AWS credentials are not available at all. I've also discovered that it's a problem with all AWS-dependent variable sources.

I'm not yet sure what should be the best way to move forward. On the one hand, situation where credentials are not available is generally a problem as the configuration cannot be fully resolved and the application won't be deployed anyway and in my opinion it's a good approach to signal these errors to the user as soon as possible. On the other hand, there are cases like mentioned here: https://github.com/serverless/serverless/issues/11260 where the AWS-related variables might be not needed at all, e.g. when using serverless-offline only.

@medikoo @mnapoli What is your opinion here?

pgrzesik avatar Jul 22 '22 11:07 pgrzesik

That's an interesting use case. I think the proper solution would be to introduce construct as:

SOME_KEY: ${if(aws:isAccessible, ${ssm:/somepath/somekey}, ${env:SOME_KEY})}

@jdgcj note that you're relying here on v2 characteristics which was widely agreed as a design bug (in most of the cases users expected variable resolver to crash here, so technically in v3 situation is improved)

medikoo avatar Jul 25 '22 14:07 medikoo

@medikoo

widely agreed as a design bug

Can I get more details/information regarding this statement? Is there an issue you can link to or a document to explain this?

jdgcj avatar Jul 25 '22 20:07 jdgcj

Can I get more details/information regarding this statement? Is there an issue you can link to or a document to explain this?

Sure: https://github.com/serverless/serverless/issues/3367

medikoo avatar Jul 25 '22 20:07 medikoo

@medikoo reading through that... am I correct to state that this issue is not mentioned as a breaking change in https://www.serverless.com/framework/docs/guides/upgrading-v3 ?

Also, is it correct that unresolvedVariablesNotificationMode: warn is no longer an option in v3?

jdgcj avatar Jul 25 '22 21:07 jdgcj

@medikoo reading through that... am I correct to state that this issue is not mentioned as a breaking change in https://www.serverless.com/framework/docs/guides/upgrading-v3 ?

It is mentioned generally in the "New variables resolver" section. Note that old resolver was replaced with new one written from a scratch, and it changed multiple things, but all of them are reported with deprecation messages, which should guide you through the upgrade process.

Also, is it correct that unresolvedVariablesNotificationMode: warn is no longer an option in v3?

This was just a temporary upgrade for the old variables resolver before the new one was introduced (so that users could have fixed error reporting earlier). It's not in v3, as the old resolver was completely replaced, and this option was specific to it.

medikoo avatar Jul 26 '22 07:07 medikoo

I think some sort of try...catch syntax or if syntax, as you mentioned above, would be an acceptable compromise. I've used file() as a temporary workaround in my local development environment in the past (see https://github.com/dherault/serverless-offline/issues/1278#issuecomment-1181786926), but at the moment we had to roll back to v2 because even with file() v3 is breaking for us in unexpected ways.

jdgcj avatar Jul 26 '22 13:07 jdgcj

Any news on this or in a way to ignore missing aws credentials for offline usage?

hugoduraes avatar Sep 13 '22 10:09 hugoduraes

Any news about it ?

nicolas-serbin avatar Dec 22 '22 15:12 nicolas-serbin

After rethinking it, I think the best way to approach it would be with construct as follows:

SOME_KEY: ${cond(${aws:isAuthenticated), "ssm:/somepath/somekey")}, ${env:SOME_KEY}

Where cond(condition, variable string) will conditionally resolve variableString depending on the condition.

WDYT?

medikoo avatar Dec 27 '22 10:12 medikoo

IMO, it should not be this complicated for the user.

For ssm: and aws: variables it should work like this:

  1. By default serverless should have the common sense to not try to connect to an AWS service when running in offline mode - you should NOT need to set AWS_PROFILE or configure dummy credentials in your .aws directory.
  2. By default it should replace ssm: and aws: variables with default values, e.g. 123412341234 for aws:accountId
  3. Optionally it should be possible to override these default values via keys in the custom section of the serverless.yml

This is how plugins like serverless-offline-ssm and serverless-pseudo-parameters work.

lqueryvg avatar Jan 12 '23 16:01 lqueryvg

By default serverless should have the common sense to not try to connect to an AWS service when running in offline mode

The framework doesn't have the concept of the offline mode. Currently, this is handled in the scope of serverless-offline plugin. So if you feel this should be the behavior for offline then it's in the capacity of that plugin. Best if you report your idea over there.

medikoo avatar Jan 12 '23 16:01 medikoo

ok

lqueryvg avatar Jan 12 '23 16:01 lqueryvg

But if they say no (again) and neither side will take on responsibility of fixing this, the net result for the user is really bad.

You can't use variables like aws:accountId in serverless.yml.

Offline mode is one of Serverless' most important features.

lqueryvg avatar Jan 12 '23 16:01 lqueryvg

Raised here: https://github.com/dherault/serverless-offline/issues/1647

lqueryvg avatar Jan 12 '23 16:01 lqueryvg