fixed localstack AWS variables resolution by AndrewChubatiuk · Pull Request #212 · localstack/serverless-localstack

AndrewChubatiuk · 2023-03-01T08:13:04Z

Hello guys!
This a fix for a bug that was introduced by #210
All AWS variables such as ${aws:accountId}, ${aws;region} and ${ssm:<path>} cannot be properly resolved because of async function call in a constructor, which cannot be completed before these variables resolution and AWS sdk credentials are not patched before these variables resolution. I'm a not sure that my approach is the best how to call async function synchronously, but at least it works.
Also I've upgraded some dependencies and added some plugin features from Serverless v3

whummer

Thanks for this PR @AndrewChubatiuk ! Interesting approach with the waitForAsync - generally looks good to me, but would like to request help from @joe4dev to help with some more testing.. 👍

Kudos also for cleaning up the logging logic a big 👌

joe4dev

Thank you very much @AndrewChubatiuk for pointing out this issue 🙏 and contributing a PR 👏👏 .

I can confirm that the waitForAsync busy-wait fixes the variable resolver issue. Unfortunately, the IPv6 issue appears to be back again. @AndrewChubatiuk Could you reproduce the IPv6 issue and does the IPv4 fallback work for you (see detailed comment in waitForAsync)?
My second main comment relates to breaking changes from bumping Serverless v2 to v3, especially in relation to the upcoming LocalStack v2 release. See discussion in package.json @whummer

joe4dev · 2023-03-13T16:16:37Z

 const Serverless = require('serverless')
-let AwsProvider;
-try {
-  AwsProvider = require('serverless/lib/plugins/aws/provider/awsProvider');


@whummer any idea why this was necessary?

joe4dev · 2023-03-13T16:23:16Z

 const path = require('path');
 const net = require('net');
-const {promisify} = require('es6-promisify');
+const { promisify } = require('util');


nice modernization 👍

"Added in: v8.0.0" and should be save as drop-in replacement (if functionally equivalent): https://nodejs.org/api/util.html#utilpromisifyoriginal

joe4dev · 2023-03-13T16:25:14Z


 class LocalstackPlugin {
-  constructor(serverless, options) {
+  constructor(serverless, options, { log, progress }) {


nice logging improvements 👍

joe4dev · 2023-03-13T16:28:43Z

+        });
        const cwd = process.cwd();
        const env = this.clone(process.env);
-        env.DEBUG = '1';


⚠️ That is a behavioral change but I guess it makes sense to be consistent with LocalStack.
It also enables disabling DEBUG mode 👍

joe4dev · 2023-03-13T17:07:33Z

-    "sinon": "^6.2.0",
-    "tempy": "^0.2.1",
+    "rimraf": "^4.1.2",
+    "serverless": "^3.27.0",


⚠️ The major serverless upgrade from v2 to v3 introduces a lot of breaking changes (upgrading-v3) and a long list of new deprecation warnings (deprecations) also related to variable resolving.

I agree that we should upgrade to v3. To indicate breaking changes, we should probably bump serverless-localstack to a new major version. I wonder whether we should batch this with further breaking changes related to the upcoming LocalStack v2 release 🤔

InvalidLocationConstraint issue with new S3 provider: Deployment bucket error with new S3 provider localstack-samples/localstack-demo#52

New default flags for starting LocalStack here

Related: localstack-pro-samples still using Serverless v2

serverless-websockets

serverless-lambda-layers

On on hand, we should resolve this variable resolver issue quickly.
On the other hand, we should think about how to savely bump the Serverless dependency to v3 and align with the LocalStack v2 release.

@whummer What do you think?

joe4dev · 2023-03-13T17:24:11Z

+      loop(iterator.next());
+    }
+
+    waitForAsync(function* () {


Tested using the localstack-pro-sample lambda-xray (also works in community without X-Ray feature) with some test variables for the hello function:

environment: TABLE_NAME: tableName SLS_AWS_ACCOUNT_ID: ${aws:accountId} SLS_AWS_REGION: ${aws:region}

✅ I can confirm that this waitForAsync busy-wait fixes the variable resolver issue when using the IPv6 workaround host: http://127.0.0.1
❎ Unfortunately, it re-introduces the IPv6 issue again but now during variable resolve time when trying to connect to localhost. The debug logs reveal that IPv6 is the issue address: '::1':

sls deploy --stage local Running "serverless" from node_modules Using serverless-localstack (node:86411) NOTE: We are formalizing our plans to enter AWS SDK for JavaScript (v2) into maintenance mode in 2023. Please migrate your code to use AWS SDK for JavaScript (v3). For more information, check the migration guide at https://a.co/7PzMCcy (Use `node --trace-warnings ...` to show where the warning was created) Recoverable error occurred (Inaccessible host: `localhost' at port `4566'. This service may not be available in the `us-west-2' region.), sleeping for ~4 seconds. Try 1 of 4 Recoverable error occurred (Inaccessible host: `localhost' at port `4566'. This service may not be available in the `us-west-2' region.), sleeping for ~5 seconds. Try 2 of 4 Recoverable error occurred (Inaccessible host: `localhost' at port `4566'. This service may not be available in the `us-west-2' region.), sleeping for ~5 seconds. Try 3 of 4 Recoverable error occurred (Inaccessible host: `localhost' at port `4566'. This service may not be available in the `us-west-2' region.), sleeping for ~5 seconds. Try 4 of 4 Environment: darwin, node 19.1.0, framework 3.28.1 (local) 3.27.0v (global), plugin 6.2.3, SDK 4.3.2 Credentials: Local, "default" profile Docs: docs.serverless.com Support: forum.serverless.com Bugs: github.com/serverless/serverless/issues Error: Cannot resolve serverless.yml: Variables resolution errored with: - Cannot resolve variable at "functions.hello.environment.SLS_AWS_ACCOUNT_ID": Inaccessible host: `localhost' at port `4566'. This service may not be available in the `us-west-2' region.

added static localhost to 127.0.0.1 translation to avoid async call

joe4dev · 2023-03-13T17:32:45Z

@@ -708,24 +716,44 @@ class LocalstackPlugin {
   *                    a rejected promise on any connection error.


The documentation does not match the updated busy-wait approach. Maybe adding a "synchronous" busy-wait wrapper would be clearer.

Co-authored-by: Waldemar Hummer <waldemar.hummer@gmail.com>

AndrewChubatiuk · 2023-04-03T19:18:05Z

@joe4dev is there anything needed from me to merge this PR?

joe4dev · 2023-04-28T09:21:58Z

Sorry for the late reply @AndrewChubatiuk
(lots of things with the v2 release and I didn't plan to catch the flu)

We agree that the variable resolver issue is essential and would like to release this fix with Serverless v2 compatibility. Therefore, we plan to extract a PR that fixes the variable resolution issue without breaking the IPv4/IPv6 fallback. This PR can then serve as a basis for a major localstack-serverless release targeting Serverless v3 compatibility.

joe4dev · 2023-04-28T09:22:12Z

-        this.log.debug(`Reconfiguring hostname to use ${fallbackHostname} (IPv4) because connection to ${hostname} failed`);
-        hostname = fallbackHostname;
-      }
+      hostname = "127.0.0.1";


I think this solution works but can be considered a breaking change because users have no way of forcing localhost anymore and it could potentially break applications depending on this 🤔

steffyP · 2023-05-16T16:39:12Z

Hi @AndrewChubatiuk 🙂

Thanks so much for your effort! In the meantime we found another solution (#219) for the issue with resolving environment variables, that keeps the fix we provided in #210 🥳
We also updated the CI pipeline so that the tests now run with serverless v2 and v3 (#220).

In your PR you also updated some other outdated libs - if you still want to work on this PR, please make sure to rebase with master! 🚀

steffyP · 2024-01-29T09:50:47Z

Hi @AndrewChubatiuk, unfortunately we haven't back from you in several months.
I am closing this PR now because it's outdated.

Please feel free to create a new PR if you have new changes to contribute to localstack-serverless 🙏 🚀

whummer reviewed Mar 12, 2023

View reviewed changes

Comment thread src/index.js Outdated

whummer requested a review from joe4dev March 12, 2023 22:58

joe4dev requested changes Mar 13, 2023

View reviewed changes

Andrii Chubatiuk and others added 3 commits March 28, 2023 10:48

fixed localstack AWS variables resolution

5a42b01

Update src/index.js

f9bfbf3

Co-authored-by: Waldemar Hummer <waldemar.hummer@gmail.com>

hardcoded localhost to 127.0.0.1 translation

bbaa894

AndrewChubatiuk force-pushed the fix-async-net-call branch from 0d40e05 to bbaa894 Compare March 28, 2023 07:48

Merge branch 'master' into fix-async-net-call

90ef8ab

joe4dev reviewed Apr 28, 2023

View reviewed changes

steffyP closed this Jan 29, 2024

		@@ -708,24 +716,44 @@ class LocalstackPlugin {
		* a rejected promise on any connection error.

Conversation

AndrewChubatiuk commented Mar 1, 2023

Uh oh!

whummer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joe4dev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndrewChubatiuk commented Apr 3, 2023

Uh oh!

joe4dev commented Apr 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steffyP commented May 16, 2023

Uh oh!

steffyP commented Jan 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants