use `getaddr` instead of `parse_address` to allow for hostnames

[Jdyn]

Acknowledging the stipulations set forth:

• [x] I hereby confirm that a Pull Request involving updates to the Software Development Kit (SDK) has been smoothly merged, currently awaits processing, or is otherwise deemed unnecessary in this context.
• [x] I also affirm that another Pull Request, specifically addressing updates to the documentation body (commonly referred to as 'docs'), has either been successfully incorporated, is in the process of review, or is considered superfluous under the prevailing circumstances.

I was looking to try a deploy on fly which requires setting:

JF_WEBRTC_TURN_LISTEN_IP = "fly-global-services"

The problem is that config_reader runs JF_WEBRTC_TURN_LISTEN_IP through :inet.parse_address and fly-global-services is not an ip, just a hostname.

** (RuntimeError) Bad JF_WEBRTC_TURN_LISTEN_IP environment variable value. Expected valid ip address, got: fly-global-services"
(jellyfish 0.4.1) lib/jellyfish/config_reader.ex:32: Jellyfish.ConfigReader.read_ip/1

One interesting note is that if I ignore the parsing altogether, without using parse_address or getaddr it then fails at the membrane_ice_plugin level. I don't have the logs for that anymore, but the plugin raises with some badarg value when passing in a hostname as the turn_ip.

With this change, we convert the hostname to it's underlying ip and jellyfish and membrane_ice_plugin works again. My understanding is that getaddr works equally with ips and hostnames but more testing could be done.

[sgfn]

Hi,

Thanks for the contribution! I'm not sure it's the best place to change it, though, seeing as the function is called read_ip and is intended to work with IP addresses only. We have a function resolve_hostname/1 in config_reader which could be better suited here.

That being said, the change you're proposing would still require adjusting the name of the env variable (suffix _HOST or _ADDRESS instead of _IP).

Do you know if this fly-global-services hostname has to be used as-is somewhere in the deployed app? If not, you could try resolving this hostname before starting JF -- this wouldn't necessitate any changes in the code.

Btw, we've tried deploying to Fly.io in the past, and it wasn't exactly smooth sailing. Have a look at this docs page: did you encounter similar issues?

[Jdyn]

Ah I see the current naming would indeed be a huge problem if host names were accepted now.

I actually followed the fly deployment docs page exactly and fly has been working very well. UDP packets flow right away and there are no issues for me so far.

Of course the tutorial no longer exactly works because it suggests doing

[env]
    ...
    JF_WEBRTC_TURN_LISTEN_IP = "fly-global-services"

Which cannot work due to this line

# config_reader.ex:107
...
turn_listen_ip: read_ip("JF_WEBRTC_TURN_LISTEN_IP") || {127, 0, 0, 1}
...

I also had to add

[env]
    ...
    JF_WEBRTC_TURN_TCP_PORT = 50000

Also I am not sure if the current form of resolve_hostname will work because it uses nota to convert it to a string, but in this case we still want the tuple form I believe.

In any case feel free to close this and implement it as you see best. My hope is for Jellyfish to accept both IP and hostnames for the turn configurations. Since this is coming straight from the fly.toml I am not sure how I would resolve the hostname before starting without making custom changes to JF or doing it manually.

getaddr works on IPs and hostnames so it seemed suitable.

[sgfn]

Glad to hear the Fly.io deployment works as intended now.

In the end, we decided to allow passing hostname in this env var :) hope this solves your issue.

I didn't realise the docs already said to set this env var to "fly-global-services", my bad. I'll take a look at that page, but if you spot anything else which doesn't quite work (or feel like there's room for improvement), feel free to make a PR to the docs as well. Contributions are always welcome, especially so since you've had hands-on experience with the deployment recently :)

[Jdyn]

Closing in favor of #188