Skip to content

Fix issues with LOCALSTACK_HOST parsing#7945

Merged
simonrw merged 4 commits intomasterfrom
fix-ls-parse
Mar 24, 2023
Merged

Fix issues with LOCALSTACK_HOST parsing#7945
simonrw merged 4 commits intomasterfrom
fix-ls-parse

Conversation

@simonrw
Copy link
Contributor

@simonrw simonrw commented Mar 23, 2023

There were some parsing bugs with handling LOCALSTACK_HOST and GATEWAY_LISTEN. This PR fixes those issues.

 % docker run --rm -ti -e LOCALSTACK_HOST=localstack localstack/localstack:latest
2023-03-23 21:18:04,142 CRIT Supervisor is running as root.  Privileges were not dropped because no user is specified in the config file.  If you intend to run as root, you can set user=root in the config file to avoid this message.
2023-03-23 21:18:04,145 INFO supervisord started with pid 15
2023-03-23 21:18:05,155 INFO spawned: 'infra' with pid 17
Traceback (most recent call last):
  File "/usr/local/lib/python3.10/runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/local/lib/python3.10/runpy.py", line 86, in _run_code
    exec(code, run_globals)
  File "/opt/code/localstack/localstack/cli/main.py", line 21, in <module>
    main()
  File "/opt/code/localstack/localstack/cli/main.py", line 14, in main
    from .localstack import create_with_plugins
  File "/opt/code/localstack/localstack/cli/localstack.py", line 7, in <module>
    from localstack import config
  File "/opt/code/localstack/localstack/config.py", line 522, in <module>
    ) = populate_legacy_edge_configuration(os.environ)
  File "/opt/code/localstack/localstack/config.py", line 453, in populate_legacy_edge_configuration
    port = int(localstack_host.split(":")[-1])
ValueError: invalid literal for int() with base 10: 'localstack'

and when supplying multiple GATEWAY_LISTEN values:

$ GATEWAY_LISTEN=192.168.0.10:9000,192.168.32.1:9001 python -m localstack.cli.main start
Traceback (most recent call last):
  File "/nix/store/hhk4wr7hwry854sq69chmrjqyi964p7y-python3-3.10.9/lib/python3.10/runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/nix/store/hhk4wr7hwry854sq69chmrjqyi964p7y-python3-3.10.9/lib/python3.10/runpy.py", line 86, in _run_code
    exec(code, run_globals)
  File "/home/simon/work/localstack/localstack/localstack/cli/main.py", line 21, in <module>
    main()
  File "/home/simon/work/localstack/localstack/localstack/cli/main.py", line 14, in main
    from .localstack import create_with_plugins
  File "/home/simon/work/localstack/localstack/localstack/cli/localstack.py", line 7, in <module>
    from localstack import config
  File "/home/simon/work/localstack/localstack/localstack/config.py", line 519, in <module>
    ) = populate_legacy_edge_configuration(os.environ)
  File "/home/simon/work/localstack/localstack/localstack/config.py", line 455, in populate_legacy_edge_configuration
    LOG.warning("multiple GATEWAY_LISTEN addresses are not currently supported")
NameError: name 'LOG' is not defined

Also ensure the CLI binds the docker port as prescribed by GATEWAY_LOCAL

simonrw added 2 commits March 23, 2023 21:32
We log at warning level if multiple gateway addresses are passed, but we
haven't set up the logger yet 🤦
When the port is missing, we were getting an error
@simonrw simonrw self-assigned this Mar 23, 2023
This means that the CLI starts the server and binds to the correct ports
as configured by GATEWAY_LISTEN
# TODO: this will likely become the default and may get removed in the future
FORWARD_EDGE_INMEM = True


Copy link
Contributor Author

Choose a reason for hiding this comment

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

This section is moved further up the file so we have access to the logger by the time we parse GATEWAY_LISTEN

@simonrw simonrw marked this pull request as ready for review March 23, 2023 22:44
@simonrw simonrw requested a review from thrau as a code owner March 23, 2023 22:44
@coveralls
Copy link

coveralls commented Mar 24, 2023

Coverage Status

Coverage: 81.8% (+0.002%) from 81.798% when pulling 1b20c39 on fix-ls-parse into 729b68e on master.

@simonrw simonrw marked this pull request as draft March 24, 2023 09:04
@simonrw simonrw marked this pull request as ready for review March 24, 2023 09:29
Copy link
Member

@thrau thrau left a comment

Choose a reason for hiding this comment

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

LGTM! thanks for jumping on this.

in a next step, maybe we could use HostAndPort to manage optional ports and consolidate with parse_hostname_and_ip

@simonrw
Copy link
Contributor Author

simonrw commented Mar 24, 2023

Thanks @thrau for the suggestion. I agree there is duplication in there that would be nice to remove.

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.

3 participants