implement usage of aws_endpoint_url by pinzon · Pull Request #243 · localstack/serverless-localstack

pinzon · 2024-01-29T20:40:26Z

Motivation

This PR optimizes the plugin by utilizing the AWS_ENDPOINT_URL environment variable as per the readme instructions. All default configuration values now derive from this variable, with a default value set to https://localhost.localstack.cloud:4566.

Notes

Currently there are three conflicting ways the protocol, hostname and port are set up:

env variables: USE_SSL, LOCALSTACK_HOSTNAME, and EDGE_PORT
serverless config settings: edgeport and hostname
env var AWS_ENDPOINT_URL

I propose that in the future it should only support AWS_ENDPOINT_URL env variable and also a new config attribute awsEndpointUrl.

steffyP

Thanks for addressing the missing implementation, and prepare the code for the long announced deprecation of envs EDGE_PORT, LOCALSTACK_HOSTNAME and USE_SSL. 🚀

One thing I realized though: in the README we state that the default value for AWS_ENDPOINT_URL will be http://localhost:4566, but now it actually is https://localhost.localstack.cloud:4566.

I wonder if this could be an issue as we currently have a fallback for an issue with IPv6 address resolution in the code that checks for hostname localhost only:

serverless-localstack/src/index.js

Lines 724 to 739 in 04f71f8

    
           // Fall back to using local IPv4 address if connection to localhost fails. 
        
           // This workaround transparently handles systems (e.g., macOS) where 
        
           // localhost resolves to IPv6 when using Nodejs >=v17. See discussion: 
        
           // https://github.com/localstack/aws-cdk-local/issues/76#issuecomment-1412590519 
        
           // Issue: https://github.com/localstack/serverless-localstack/issues/125 
        
           if (hostname === "localhost") { 
        
             try { 
        
               const port = this.getEdgePort(); 
        
               const options = { host: hostname, port: port }; 
        
               await this.checkTCPConnection(options); 
        
             } catch (e) { 
        
               const fallbackHostname = "127.0.0.1" 
        
               this.debug(`Reconfiguring hostname to use ${fallbackHostname} (IPv4) because connection to ${hostname} failed`); 
        
               hostname = fallbackHostname; 
        
             } 
        
           }

So we may need to add localhost.localstack.cloud here as well. /cc @joe4dev could we get your input here? 🙏

steffyP · 2024-01-31T08:48:37Z

volume/cache/machine.json

i think you added all the files in volume/cache by accident? could you please remove them from the PR 🙏

joe4dev · 2024-02-02T14:36:06Z

So we may need to add localhost.localstack.cloud here as well. /cc @joe4dev could we get your input here? 🙏

✅ This should not be necessary because localhost.localstack.cloud always resolves to IPv4 (at least with our current DNS entry). That's what I get on macOS:

nslookup localhost.localstack.cloud
Server:		1.1.1.1
Address:	1.1.1.1#53

Non-authoritative answer:
Name:	localhost.localstack.cloud
Address: 127.0.0.1

⚠️ However, I think changing the default to localhost.localstack.cloud is a breaking change and should not be done in isolation because it breaks in environments with DNS rebind protection => let me clarify in a review ...

joe4dev

Unifying the configuration makes a lot of sense. Ideally, we can do this across all LocalStack integrations consistently. It is a rather small change with a big customer impact.

Therefore, changing the default to localhost.localstack.cloud is a major change and should be released as such. Ideally, all LocalStack integrations (e.g., cdklocal, awslocal, etc) should behave consistently. The main challenge is that using the domain name now requires DNS access (which might break in restricted networks or custom DNS setups). Further, while connecting to LocalStack works for the domain, subdomains could cause issues if DNS rebind protection is enabled (see our DNS server docs here). In certain enterprise environments, users might not be able to change this.
I think it would be valuable to synchronize with the networking initiative from @simonrw and make the LocalStack integrations better usable (e.g., READMEs missing default values is not user-friendly).

style suggestion: It is tricky to separate functional changes mixed with several code formatting changes. I recommend using a code formatted to reformat everything consistently at once and enforce it via CI and a pre-commit hook.

joe4dev · 2024-02-02T14:03:18Z

spec/unit/index.spec.js

@@ -177,7 +177,7 @@ describe("LocalstackPlugin", () => {
      expect(request.called).to.be.true;
      let templateUrl = request.firstCall.args[2].TemplateURL;
      // url should either start with 'http://localhost' or 'http://127.0.0.1


that comment seems outdated and misleading

joe4dev · 2024-02-02T14:05:27Z

src/index.js

-// Default edge port to use with host
-const DEFAULT_EDGE_PORT = '4566';
+// Default AWS endpoint URL
+const DEFAULT_AWS_ENDPOINT_URL = "https://localhost.localstack.cloud:4566";


⚠️ This is a breaking change and should technically be released as a new major version

joe4dev · 2024-02-02T14:10:02Z

src/index.js

+          plugin.hooks[hookName] = boundOverrideFunction;
+          const slsHooks = this.serverless.pluginManager.hooks[hookName] || [];
+          slsHooks.forEach(
+              (hookItem) => {


Is it intentional to mix 4 spaces here vs. 2 spaces elsewhere?

Would it make sense to configure a code formatter for the project using a pre-commit hook + CI enforcement and do this once rather than mixing up formatting changes with functional code changes?

joe4dev · 2024-02-02T14:11:16Z

src/index.js

+              return exists[0].replace('\t', ' ').split(' ')[0];
+            }
          }
-        }


this is correct, was it extra before? (just seeing limited context at GH)

joe4dev · 2024-02-02T14:16:17Z

src/index.js

+    let proto = this.getEndpointProtocol();
+    if (process.env.USE_SSL) {
+      proto = TRUE_VALUES.includes(process.env.USE_SSL) ? 'https' : 'http';
+    }else if (this.config.host){


nit: missing spaces around conditionals (autoformatter would be helpful here)

steffyP · 2024-02-02T14:44:34Z

Thanks for your input @joe4dev 🙏

@pinzon: I would suggest we set the DEFAULT_AWS_ENDPOINT_URL to http://localhost:4566 (like it's also documented in the README), and revisit this when we do the next major release. Then we can also try to adjust this across integrations like @joe4dev suggested.

Regarding the code formatting: I think we can do this in a follow-up PR, it would indeed be nice to have formatting rules, and a pre-commit hook 😄

implement usage of aws_endpoint_url

32f232f

pinzon requested a review from steffyP January 29, 2024 20:40

pinzon added 5 commits January 29, 2024 15:43

remove unused var

ebf56b0

retrieve clean protocol

2d1c737

test should assert the new default endpoint

13495c3

ci test

8b3c4f3

restore test and fix for test

dbf84d6

pinzon marked this pull request as ready for review January 30, 2024 20:37

steffyP reviewed Jan 31, 2024

View reviewed changes

clean volume folder

1b66b21

pinzon requested a review from steffyP February 1, 2024 16:00

joe4dev reviewed Feb 2, 2024

View reviewed changes

pinzon added 3 commits February 2, 2024 16:13

use localhost as aws endpoint

933695b

rewrite test

69a1ad3

rewrite test

b7b642c

pinzon merged commit f0e7cee into master Feb 7, 2024

bram-tv mentioned this pull request Jun 24, 2024

Protocol now required when using custom.localstack.host #257

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement usage of aws_endpoint_url#243

implement usage of aws_endpoint_url#243
pinzon merged 10 commits intomasterfrom
update-readme

pinzon commented Jan 29, 2024 •

edited by steffyP

Loading

Uh oh!

steffyP left a comment

Uh oh!

steffyP Jan 31, 2024

Uh oh!

pinzon Feb 1, 2024

Uh oh!

joe4dev commented Feb 2, 2024

Uh oh!

joe4dev left a comment

Uh oh!

joe4dev Feb 2, 2024

Uh oh!

joe4dev Feb 2, 2024

Uh oh!

joe4dev Feb 2, 2024

Uh oh!

joe4dev Feb 2, 2024

Uh oh!

joe4dev Feb 2, 2024

Uh oh!

steffyP commented Feb 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	// Fall back to using local IPv4 address if connection to localhost fails.
	// This workaround transparently handles systems (e.g., macOS) where
	// localhost resolves to IPv6 when using Nodejs >=v17. See discussion:
	// https://github.com/localstack/aws-cdk-local/issues/76#issuecomment-1412590519
	// Issue: https://github.com/localstack/serverless-localstack/issues/125
	if (hostname === "localhost") {
	try {
	const port = this.getEdgePort();
	const options = { host: hostname, port: port };
	await this.checkTCPConnection(options);
	} catch (e) {
	const fallbackHostname = "127.0.0.1"
	this.debug(`Reconfiguring hostname to use ${fallbackHostname} (IPv4) because connection to ${hostname} failed`);
	hostname = fallbackHostname;
	}
	}

Conversation

pinzon commented Jan 29, 2024 • edited by steffyP Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Notes

Uh oh!

steffyP left a comment

Choose a reason for hiding this comment

Uh oh!

steffyP Jan 31, 2024

Choose a reason for hiding this comment

Uh oh!

pinzon Feb 1, 2024

Choose a reason for hiding this comment

Uh oh!

joe4dev commented Feb 2, 2024

Uh oh!

joe4dev left a comment

Choose a reason for hiding this comment

Uh oh!

joe4dev Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

joe4dev Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

joe4dev Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

joe4dev Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

joe4dev Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

steffyP commented Feb 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pinzon commented Jan 29, 2024 •

edited by steffyP

Loading