magento / pwa-studio

šŸ› Development tools to build, optimize and deploy Progressive Web Applications for Magento 2.
https://developer.adobe.com/commerce/pwa-studio/
Open Software License 3.0
1.06k stars 682 forks source link

[feature]: PWADevServer: Avoid binding/listening by domain name #2428

Closed IgorVitol closed 4 years ago

IgorVitol commented 4 years ago

Is your feature request related to a problem? Please describe. Currently PWADevServer will do DNS lookup for specified domain name ("DEV_SERVER_HOST" env variable) in order to start listening for specified port.

So, when you specifying something like - "pwa-docker.localhost" - it will lookup your DNS entry to IP and start listening on IP.

For current docker-compose setup, it works just because container host-name is set to "DEV_SERVER_HOST" too. But it you will try something else, like Kubernetes cluster, where you can easily scale your pods/containers - it will not work, since we can not assign same host-name to multiple containers. Also passing "0.0.0.0" or container internal IP as "DEV_SERVER_HOST" would not help, since current implementation didn't cover "public" or "allowedHosts" properties - server will start listening, but will reject any requests.

I guess current implementation is a reason why user could get wrong info about URL/PORT/SCHEME on which server is listening on, since code trying to resolve it's own scheme / port without no knowledge about external request flow. For example: run current docker setup and note listening url - it will say that protocol/scheme is http.

Describe the solution you'd like Note: PR will be added soon, you can check commit here: https://github.com/IgorVitol/pwa-studio/commit/40341da5028469cc58c1a1232d1e62db36bb47a9

Details: In order to make it work without breaking changes, I would recommend to add few more ENV variables, which will have info about "public port used" & "is ssl offloaded?". Then it will be possible to:

Example (correct scheme detected) - https://prnt.sc/sm4047

Describe alternatives you've considered No

Additional context No

Please let us know what packages this feature is in regards to:

m2-assistant[bot] commented 4 years ago

Hi @IgorVitol. Thank you for your report. To help us process this issue please make sure that you provided sufficient information.

Please, add a comment to assign the issue: @magento I am working on this


IgorVitol commented 4 years ago

@magento I am working on this

IgorVitol commented 4 years ago

@Jordaneisenburger Could you pls take a look at this issue - what next steps required after PR creation?

awilcoxa commented 4 years ago

@zetlen to review

zetlen commented 4 years ago

@IgorVitol I left comments on #2429 a few weeks ago, asking some questions and suggesting some changes. Do you have the time and the continued need to work on this? If not, we'll backlog an issue to do a slightly different change--namely, to bring back the DEV_SERVER_PUBLIC_PATH env var. This is slightly different from the solution you created in #2429, so if it doesn't solve your use case, please let us know.

tjwiebell commented 4 years ago

Closing for inactivity, see comment in #2429.