Skip to content
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

Unify construction of target endpoint URL, add support for configuring AWS_ENDPOINT_URL #234

Merged
merged 2 commits into from
Sep 21, 2023

Conversation

whummer
Copy link
Member

@whummer whummer commented Sep 21, 2023

Unify construction of target endpoint URL, add support for configuring $AWS_ENDPOINT_URL. The starting point for this PR was a use case where we want to deploy a serverless/bref application against an ephemeral LocalStack instance (running on Namespace).

It turned out that the current configuration options still use the deprecated config options like EDGE_PORT/USE_SSL/etc, which we've already removed and replaced with AWS_ENDPOINT_URL in other util repos (e.g., tflocal, cdklocal).

Another issue was that Namespace has some issues routing requests if a request is made with a Host header that contains the default HTTPS port 443 (i.e., Host: my-remote-host:443). Arguably, we should not be making custom adjustments for a single target platform, but it seems reasonable to replace https://my-remote-host:443 with https://my-remote-host, as this is generally a common practice for addressing HTTPS Web servers.

Note that the entire logic becomes obsolete with the AWS_ENDPOINT_URL variable, and we can simplify the logic quite a bit in an upcoming release, once we remove the legacy configuration options. 👍

Changes:

  • add support for setting the target endpoint via AWS_ENDPOINT_URL
  • update the README with instructions for AWS_ENDPOINT_URL, deprecating the old config options

@whummer whummer requested review from joe4dev and bentsku September 21, 2023 00:43
@whummer whummer marked this pull request as ready for review September 21, 2023 08:07
Copy link
Member

@joe4dev joe4dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, it is great to migrate to the standardized AWS_ENDPOINT_URL and it works nicely 👍 BUT ...

Using AWS_ENDPOINT_URL=http://localhost:4566 would re-introduce the IPv6 bug (#125 (comment)) again 😞

AWS_ENDPOINT_URL=http://localhost:4566 serverless deploy --stage dev
(node:67589) 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)

Deploying aws-python to stage dev (us-east-1)
Using serverless-localstack
Using serverless-localstack
serverless-localstack: Reconfigured endpoints
serverless-localstack: Reconfigured endpoints

✖ Stack aws-python-dev failed to deploy (33s)
Environment: darwin, node 18.16.0, framework 3.32.2, 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:
Inaccessible host: `localhost' at port `4566'. This service may not be available in the `us-east-1' region.

We either need to change communication (like Mongoose here):

For local MongoDB databases, we recommend using 127.0.0.1 instead of localhost. That is because Node.js 18 and up prefer IPv6 addresses, which means, on many machines, Node.js will resolve localhost to the IPv6 address ::1 and Mongoose will be unable to connect, unless the mongodb instance is running with ipv6 enabled.

This might be hard to achieve across the entire LocalStack ecosystem. For an auto-magic workaround, we would need to parse the URL and check if it contains localhost and then apply the check as we do it for hostname ATM.

Tested on macOS (IPv6-enabled per default) with Node.js 18 with the basic aws-python example: https://github.com/serverless/examples/tree/v3/aws-python


(I discovered another LS error in the service health check when using a non-standard port; will follow up this one.)

2023-09-21T09:35:13.528 ERROR --- [   asgi_gw_3] l.services.plugins         : service health check cloudwatch:4566 failed
Traceback (most recent call last):
File "/opt/code/localstack/.venv/lib/python3.10/site-packages/localstack/services/plugins.py", line 732, in _check
wait_for_port_status(port, expect_success=not expect_shutdown)
File "/opt/code/localstack/.venv/lib/python3.10/site-packages/localstack/utils/net.py", line 165, in wait_for_port_status
return retry(check, sleep=sleep_time, retries=retries)
File "/opt/code/localstack/.venv/lib/python3.10/site-packages/localstack/utils/sync.py", line 62, in retry
raise raise_error
File "/opt/code/localstack/.venv/lib/python3.10/site-packages/localstack/utils/sync.py", line 58, in retry
return function(**kwargs)
File "/opt/code/localstack/.venv/lib/python3.10/site-packages/localstack/utils/net.py", line 160, in check
raise Exception(
Exception: Port 4566 (path: None) was not open

README.md Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
@@ -758,9 +757,19 @@ class LocalstackPlugin {
return this.awsProvider;
}

getServiceURL() {
getServiceURL(hostname) {
if (process.env.AWS_ENDPOINT_URL) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great to adopt this new standardized way of endpoint configuration 🚀

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to consider aligning the serverless config (host) and environment variable config (AWS_ENDPOINT_URL) in the future for better consistency ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fully agree, this is more of a bandaid for now, still not super happy with the duality of the yaml config and the environment variables. Definitely think there is still room for improvement in a follow-up change 👌

package.json Show resolved Hide resolved
@whummer
Copy link
Member Author

whummer commented Sep 21, 2023

This might be hard to achieve across the entire LocalStack ecosystem. For an auto-magic workaround, we would need to parse the URL and check if it contains localhost and then apply the check as we do it for hostname ATM.

Great point, thanks for the pointer and giving it a try - almost forgot about this node 18 / IPv6 issue again. 😬 Changing the getServiceURL function to async would be one option, but I think we've considered this in the past, and it turned out to be painful to introduce async logic in all places. Updating the docs could be another option, but agree that this could be error-prone and not really user-friendly.

What we could do, though: since we're already passing in the hostname (at least when calling it from reconfigureAWS(..)), we could simply replace the hostname/IP in the URL as you suggested, in case AWS_ENDPOINT_URL looks something like http://localhost:4566. 👍 Will go ahead and make that change.

@whummer whummer requested a review from joe4dev September 21, 2023 10:34
@whummer whummer changed the title Unify construction of target endpoint URL, add support for configuring $AWS_ENDPOINT_URL Unify construction of target endpoint URL, add support for configuring AWS_ENDPOINT_URL Sep 21, 2023
Copy link
Member

@joe4dev joe4dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works now with localhost and IPv6 configured through AWS_ENDPOINT_URL 👍

(we might want to log this as warning more explicitly in the future but happy to see it fixed. Having a testcase is also a nice-to-have but a bit annoying to set up.)

@whummer whummer merged commit 2a3384d into master Sep 21, 2023
2 checks passed
@whummer whummer deleted the endpoint-url branch September 21, 2023 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants