-
Notifications
You must be signed in to change notification settings - Fork 89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
implement usage of aws_endpoint_url #243
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? 🙏
volume/cache/machine.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think you added all the files in volume/cache
by accident? could you please remove them from the PR 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
✅ This should not be necessary because
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
spec/unit/index.spec.js
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that comment seems outdated and misleading
src/index.js
Outdated
// 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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plugin.hooks[hookName] = boundOverrideFunction; | ||
const slsHooks = this.serverless.pluginManager.hooks[hookName] || []; | ||
slsHooks.forEach( | ||
(hookItem) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is correct, was it extra before? (just seeing limited context at GH)
let proto = this.getEndpointProtocol(); | ||
if (process.env.USE_SSL) { | ||
proto = TRUE_VALUES.includes(process.env.USE_SSL) ? 'https' : 'http'; | ||
}else if (this.config.host){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: missing spaces around conditionals (autoformatter would be helpful here)
Thanks for your input @joe4dev 🙏 @pinzon: I would suggest we set the 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 😄 |
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:
I propose that in the future it should only support AWS_ENDPOINT_URL env variable and also a new config attribute awsEndpointUrl.