Add support for Fly.io deployments #167

mickel8 · 2024-09-21T17:04:44Z

No description provided.

codecov · 2024-09-21T17:19:54Z

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 87.87%. Comparing base (e42c09d) to head (7fafd57).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
lib/ex_webrtc/ice/fly_ip_filter.ex	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #167      +/-   ##
==========================================
- Coverage   87.98%   87.87%   -0.12%     
==========================================
  Files          44       45       +1     
  Lines        2356     2358       +2     
==========================================
- Hits         2073     2072       -1     
- Misses        283      286       +3

Files with missing lines	Coverage Δ
lib/ex_webrtc/peer_connection/configuration.ex	`94.16% <ø> (-0.05%)`	⬇️
lib/ex_webrtc/ice/fly_ip_filter.ex	`0.00% <0.00%> (ø)`

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e42c09d...7fafd57. Read the comment docs.

sgfn · 2024-09-23T09:08:05Z

guides/deploying/fly.md

+There are just two things you need to do:
+
+- configure a STUN server both on the client and server side
+- use custom Fly.io IP filter on the server side


Suggested change

- use custom Fly.io IP filter on the server side

- use a custom Fly.io IP filter on the server side

sgfn · 2024-09-23T09:09:02Z

guides/deploying/fly.md

+- configure a STUN server both on the client and server side
+- use custom Fly.io IP filter on the server side
+
+In theory, configuring a STUN server just on a one side should be enough but we recommend to do it on both sides.


Suggested change

In theory, configuring a STUN server just on a one side should be enough but we recommend to do it on both sides.

In theory, configuring a STUN server just on one side should be enough but we recommend doing it on both sides.

sgfn · 2024-09-23T10:03:58Z

lib/ex_webrtc/ice/fly_ip_filter.ex

+  This module defines a single function, which filters out IP addresses,
+  which ICE Agent will use as its host candidates.


"filters out" to me sounds like it's rejecting these addresses rather than accepting them

I feel like adding a bit more info about the function could be helpful

Suggested change

This module defines a single function, which filters out IP addresses,

which ICE Agent will use as its host candidates.

This module defines a single function, which filters IP addresses,

which ICE Agent will use as its host candidates.

It accepts only the IPv4 address that `fly-global-services` resolves to.

sgfn · 2024-09-23T10:05:18Z

lib/ex_webrtc/ice/fly_ip_filter.ex

+  which ICE Agent will use as its host candidates.
+  """
+
+  @spec ip_filter(:inet.ip_address()) :: boolean()


[nitpick] We could add a behaviour in ExICE. Then again, it's only one function -- up to you

Let's stay with the current version for know. In debugging and testing it's verry convenient to pass anonymous function instead of creating a module with a single function.

I thought about returning an anonymous function from ip_filter and use ICEAgent.ip_filter type in typespec but sounds like an overkill 🤔

sgfn · 2024-09-23T10:07:44Z

guides/deploying/fly.md

+});
+```
+
+in Elixir code:


Suggested change

in Elixir code:

In Elixir code:

sgfn · 2024-09-23T10:07:52Z

guides/deploying/fly.md

+  )
+```
+
+in `runtime.exs`:


Suggested change

in `runtime.exs`:

In `runtime.exs`:

sgfn · 2024-09-23T10:08:21Z

guides/deploying/fly.md

+end
+```
+
+in fly.toml:


Suggested change

in fly.toml:

In fly.toml:

sgfn · 2024-09-23T10:22:59Z

guides/deploying/fly.md

+```elixir
+ip_filter = Application.get_env(:your_app, :ice_ip_filter)
+
+{:ok, pc} =
+  PeerConnection.start_link(
+    ice_ip_filter: ip_filter,
+    ice_servers: [%{urls: "stun:stun.l.google.com:19302"}]
+  )
+```


[nitpick] When :ice_ip_filter is unset in the application, we're passing ice_ip_filter: nil to PeerConnection, which means

ex_webrtc/lib/ex_webrtc/peer_connection/configuration.ex

Line 242 in e42c09d

|> Keyword.put_new(:ice_ip_filter, fn _ -> true end)

doesn't insert the default filter.

When we pass it to ICE, however, this line
https://github.com/elixir-webrtc/ex_ice/blob/4c5190687e11b28a8209f2c930895647d0b2a64c/lib/ex_ice/priv/ice_agent.ex#L137
makes it work again, as it gets the nil stored in the keyword list and overrides it with the default filter.

I feel like this creates a kind of ambiguity and depends on ExICE not changing its behaviour of parsing options, which can potentially lead to hard-to-find errors in the future. To avoid this in this example, I would explicitly not insert ice_ip_filter to the PC options when it's missing from the application's environment.

Good catch!

sgfn · 2024-09-23T10:23:36Z

guides/deploying/fly.md

+```
+
+That's it!
+No special UDP port exports or dedicated IP address needed.


Suggested change

No special UDP port exports or dedicated IP address needed.

No special UDP port exports or dedicated IP addresses are needed.

mickel8 requested review from sgfn and removed request for sgfn September 21, 2024 17:04

mickel8 force-pushed the fly branch from 16a0e2b to 25cb4fe Compare September 21, 2024 17:18

Add support for Fly.io deployments

17f2a52

mickel8 force-pushed the fly branch from 25cb4fe to 17f2a52 Compare September 21, 2024 17:21

mickel8 requested a review from sgfn September 21, 2024 17:25

sgfn reviewed Sep 23, 2024

View reviewed changes

Fix some grammar. Fix ice_ip_filter default value

7fafd57

mickel8 requested a review from sgfn September 23, 2024 10:46

sgfn approved these changes Sep 23, 2024

View reviewed changes

mickel8 merged commit da126b6 into master Sep 23, 2024
3 checks passed

mickel8 deleted the fly branch September 23, 2024 11:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Fly.io deployments #167

Add support for Fly.io deployments #167

mickel8 commented Sep 21, 2024

codecov bot commented Sep 21, 2024 •

edited

Loading

sgfn Sep 23, 2024

sgfn Sep 23, 2024

sgfn Sep 23, 2024

sgfn Sep 23, 2024

mickel8 Sep 23, 2024

sgfn Sep 23, 2024

sgfn Sep 23, 2024

sgfn Sep 23, 2024

sgfn Sep 23, 2024

mickel8 Sep 23, 2024

sgfn Sep 23, 2024

	- use custom Fly.io IP filter on the server side
	- use a custom Fly.io IP filter on the server side

	In theory, configuring a STUN server just on a one side should be enough but we recommend to do it on both sides.
	In theory, configuring a STUN server just on one side should be enough but we recommend doing it on both sides.

		This module defines a single function, which filters out IP addresses,
		which ICE Agent will use as its host candidates.

	No special UDP port exports or dedicated IP address needed.
	No special UDP port exports or dedicated IP addresses are needed.

Add support for Fly.io deployments #167

Add support for Fly.io deployments #167

Conversation

mickel8 commented Sep 21, 2024

codecov bot commented Sep 21, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 21, 2024 •

edited

Loading