Discover and expose SFT servers in /calls/config/v2 #1177

jschaul · 2020-07-22T14:57:44Z

Fixes https://github.com/zinfra/backend-issues/issues/1453

Differences: - Uses polysemy to expose the lookup itself - Doesn't order targets, ordering requires randomization, maybe it can be exposed with some other effect.

fisx

lots of comments, hopefully not too much noise?

services/brig/src/Brig/Run.hs

libs/dns-srv/src/Wire/Network/DNS/Effect.hs

fisx · 2020-07-27T09:39:18Z

libs/dns-srv/src/Wire/Network/DNS/SRV.hs

+-- You should have received a copy of the GNU Affero General Public License along
+-- with this program. If not, see <https://www.gnu.org/licenses/>.
+
+module Wire.Network.DNS.SRV where


If I understand correctly, the types here follow the RFC that describes SFT server lookup, or maybe something slightly more general, but more specific than DNS. Could we link that RFC here?

The RFC defines what SRV records are. The dns package interprets an SRV record into a tuple of 4 things, we named them here for it to be easier to comprehend across the code. Not sure if the mention is worth it. We mention the RFC in the ordering function because it might not be as obvious.

libs/dns-srv/src/Wire/Network/DNS/Effect.hs

libs/dns-srv/src/Wire/Network/DNS/SRV.hs

fisx · 2020-07-27T10:50:24Z

libs/dns-srv/src/Wire/Network/DNS/SRV.hs

+--
+-- Taken from http://hackage.haskell.org/package/pontarius-xmpp (BSD3 licence) and refactored.
+orderTargets :: [SrvEntry] -> IO [SrvEntry]
+orderTargets =


Yes, given that we don't depend on this, it makes sense to me to just randomize the order fully, ignoring weight and prio. Should be random, though. It's easy and nicer, even though it shouldn't have an impact with current client implementations.

We kept the function there as it was written for federation and will be useful anyway. But ended up not using it as it doesn't match up with how clients decide which server to connect to.

libs/wire-api/src/Wire/API/Call/TURN.hs

services/brig/brig.cabal

services/brig/src/Brig/Run.hs

* Refactor all `List1` to `NonEmpty` in RTCConfiguration * Integration tests now depend on globally created DNS entries Pending: * Unit tests for discovery * Unit tests for translating an SrvTarget to an SFTServer * Figure out if we can override the domain used for integration tests

Co-authored-by: fisx <mf@zerobuzz.net>

The plugin is not really required at the point anyway.

fisx · 2020-07-29T07:38:58Z

a7820b5

too bad. :( can we do this cnoditionally, with a cabal compiler version check or something?

Side benefit: faster tests \o/

- Set discovery interval for sft - Use retryWhileN correctly by looping while discovery is not done - Do not assert on the order of servers as it doesn't seem to be stable

akshaymankar · 2020-07-29T09:43:20Z

too bad. :( can we do this cnoditionally, with a cabal compiler version check or something?

I think we can have a flag for this, but the thing is completely useless right now. Instead, we should just update our alpine image to have ncurses-dev. I have this branch that will help us add it back in future, but it requires a lot of compiling and one of us watching it compile. I will get to it after I am done with this PR, if I have enough time before my vacation. Feel free to take it off my hands though 😄

We can create another effect for this and make the tests faster (and deterministic), but later.

Code to skip: https://github.com/wireapp/wire-server-deploy/blob/f602fd47794da363394dcaf30550e2b6fa2177c6/charts/brig/templates/tests/brig-integration.yaml#L56

fisx

still haven't read it all, but here is what i have.

fisx · 2020-07-30T09:15:36Z

libs/types-common/src/Data/Misc.hs

+ensureHttpsUrl :: URIRef Absolute -> HttpsUrl
+ensureHttpsUrl = HttpsUrl . (uriSchemeL . schemeBSL .~ "https")
+


Suggested change

ensureHttpsUrl :: URIRef Absolute -> HttpsUrl

ensureHttpsUrl = HttpsUrl . (uriSchemeL . schemeBSL .~ "https")

enforceHttpsUrl :: URIRef Absolute -> HttpsUrl

enforceHttpsUrl = HttpsUrl . (uriSchemeL . schemeBSL .~ "https")

mkHttpsUrl :: Host -> Port -> HttpsUrl

mkHttpsUrl = undefined

(no strong opinion about either; the second may lead to nicer code further down this PR.)

mkHttpsUrl is already defined above and it returns Either String HttpsUrl. enforce and ensure have same semantic meaning in my head, so I also don't have much strength in my opinion here.

libs/wire-api/test/unit/Test/Wire/API/Roundtrip/Aeson.hs

libs/wire-api/src/Wire/API/Call/Config.hs

services/brig/src/Brig/PolyLog.hs

fisx · 2020-07-30T09:41:13Z

services/brig/src/Brig/PolyLog.hs

+
+makeSem 'PolyLog
+
+runPolyLog :: Member (Embed IO) r => Log.Logger -> Sem (PolyLog ': r) a -> Sem r a


... and shouldn't the logger be provided by the effect, like in the logger mtl class?

How do you mean? This function interprets the effect in terms of IO, I don't think it should create a logger, since there are a bunch of log settings we might want to control from wherever the app is being started.

services/brig/src/Brig/Run.hs

fisx · 2020-07-30T09:45:28Z

services/brig/test/integration/API/Calling.hs

+import Wire.API.Call.Config
+
+tests :: Manager -> Brig -> Opts.Opts -> FilePath -> FilePath -> IO TestTree
+tests m b opts turn turnV2 = do


Nit-picks: why does this return in IO, and not purely? Also, formatting is not very nice.

I think this is because in some integration tests we do some IO things for all the testGroups and here we just kept it similar to that. I think it is better have the tests functions be an IO everywhere rather than in only some places. But I don't really feel very strongly on this one.

services/brig/test/integration/API/Calling.hs

fisx

two more files to go...

fisx · 2020-07-30T11:38:56Z

services/brig/src/Brig/Calling.hs

+-- You should have received a copy of the GNU Affero General Public License along
+-- with this program. If not, see <https://www.gnu.org/licenses/>.
+
+module Brig.Calling where


can you make an explicit export list? it (a) should be the default, and (b) may reveal that Discovery doesn't have to have any constructors exported, which would make it harder to violate its invariant.

But I want to use the constructors in the tests. I could always translate it to Maybe in the tests but that would make them cumbersome. I think it is OK to leave it at this as it clearly states the semantics and doesn't come in the way of testing.

services/brig/src/Brig/Calling/API.hs

fisx · 2020-07-30T12:54:27Z

services/brig/src/Brig/Calling/API.hs

-  pure $ Public.rtcConfiguration srvs cTTL
+    pure $ Public.rtcIceServer (uri :| []) u (computeCred sha secret u)
+  sftSrvEntries <- maybe (pure Nothing) ((fmap discoveryToMaybe) . readIORef . sftServers) mSftEnv
+  -- According to RFC2782, the SRV Entries are supposed to be tried in order of


I would say don't randomize here and add the fact that the list has set semantics to the swagger spec. Saves server resources and is at least clear about this. While we're at it, we might as well remove the randomization above as well.

But I'm not going to insist. Minimizing change is also nice.

Changes here need to be discussed with the AVS team, since they are using these entries on the client side, and the current agreement was (as far as I'm aware - unless there were further discussions on this topic since?) "the list is randomized". We can make changes here in future PRs after a discussion, if a certain logic should happen on the client side.

services/brig/src/Brig/Calling/Internal.hs

fisx · 2020-07-30T13:00:45Z

services/brig/src/Brig/Options.hs

+  }
+  deriving (Show, Generic)
+
+instance FromJSON SFTOptions where


(Wild thought: we should have a couple of strings in the test suite and tests that they parse as server config files.)

fisx · 2020-07-30T13:03:52Z

services/brig/test/unit/Test/Brig/Calling.hs

+recordLogs LogRecorder {..} = interpret $ \(PolyLog lvl msg) ->
+  modifyIORef' recordedLogs (++ [(lvl, Log.render (Log.renderDefault ", ") msg)])
+
+ignoreLogs :: Sem (PolyLog ': r) a -> Sem r a


I don't like the name PolyLog (it's a nice name, but it doesn't point out what it refers to clearly enough for my taste). But we can change it later.

I do like the fact that we're introducing polysemi! :)

I felt like being clever 😄
What would be a better name?

jschaul

I started the PR, but didn't do much work, so I can't approve with the button here, so I'll approve this work with a comment: ✔️

jschaul · 2020-07-30T13:54:43Z

services/brig/src/Brig/Calling/API.hs

-  pure $ Public.rtcConfiguration srvs cTTL
+    pure $ Public.rtcIceServer (uri :| []) u (computeCred sha secret u)
+  sftSrvEntries <- maybe (pure Nothing) ((fmap discoveryToMaybe) . readIORef . sftServers) mSftEnv
+  -- According to RFC2782, the SRV Entries are supposed to be tried in order of


Changes here need to be discussed with the AVS team, since they are using these entries on the client side, and the current agreement was (as far as I'm aware - unless there were further discussions on this topic since?) "the list is randomized". We can make changes here in future PRs after a discussion, if a certain logic should happen on the client side.

fisx · 2020-07-30T14:15:16Z

Hey github, why can i not reply to #1177 (comment)?

anyway, what i meant was that this should not take the logger as a function argument, but get it out of a reader effect. we agree that it shouldn't be created on the fly with the wrong parameters. :)

fisx · 2020-07-30T14:18:05Z

#1177 (comment)

Perhaps we should start writing tests in the production code modules, and merely calling them from /package/tests? Anyway, other issue, other PR, other year.

fisx

all good! two more comments, but i think nobody will die if you ignore them.

services/brig/test/unit/Test/Brig/Calling.hs

jschaul and others added 2 commits July 21, 2020 18:03

Add comment, minor hlint

14b3692

Add FUTUREWORK to ensure we watch the threads we spawn

189043f

akshaymankar force-pushed the sft_srv branch from d8b41d9 to 189043f Compare July 23, 2020 12:29

akshaymankar added 2 commits July 24, 2020 17:00

Copy code for SRV lookup from federation-util to dedicated package

27fc2e8

Differences: - Uses polysemy to expose the lookup itself - Doesn't order targets, ordering requires randomization, maybe it can be exposed with some other effect.

Read SFT options and start service discovery thread

f9de188

akshaymankar force-pushed the sft_srv branch from 947547f to 8918026 Compare July 27, 2020 10:39

fisx reviewed Jul 27, 2020

View reviewed changes

akshaymankar force-pushed the sft_srv branch from 8918026 to 07f848a Compare July 27, 2020 15:15

akshaymankar and others added 10 commits July 28, 2020 13:31

Fix typo

a345016

Co-authored-by: fisx <mf@zerobuzz.net>

Add unit tests for sftDiscoveryLoop

8a5f363

rename Wire.API.TURN -> Wire.API.Calling

a0d7fe5

second rename TURN -> Calling

eaaa4f3

merge TURN into Calling

7101bd9

another TURN -> Calling rename

fb42bc3

rename dns-srv to dns-util

1b5804d

Rename testGroup "turn" -> "calling"

b6c81ad

Ormolu for module rename

c00e688

Remove dependency on polysemy-plugin so we can compile on alpine

a7820b5

The plugin is not really required at the point anyway.

akshaymankar force-pushed the sft_srv branch from 473556f to a7820b5 Compare July 29, 2020 07:28

akshaymankar added 3 commits July 29, 2020 10:03

Add unit test for SrvTarget -> SFTServer conversion

6fcb23f

Deflake brig calling unit tests

28d9971

Make SFT discovery interval configurable

05145d1

Side benefit: faster tests \o/

akshaymankar force-pushed the sft_srv branch from 3d856f2 to 05145d1 Compare July 29, 2020 09:14

Fix brig's API.Calling integration test

46df70e

- Set discovery interval for sft - Use retryWhileN correctly by looping while discovery is not done - Do not assert on the order of servers as it doesn't seem to be stable

akshaymankar added 3 commits July 29, 2020 11:47

Give up on avoiding Embed IO for threadDelay

1b1a85f

We can create another effect for this and make the tests faster (and deterministic), but later.

Add comment to explain integration-test dns names

b51205f

WIP: Move orderSrvEntry to dns-util

a4e51da

akshaymankar added 7 commits July 29, 2020 19:10

Commit the forgotten Brig.PolyLog module

b554390

Fix compile errors

7525503

Simplify DNSLookup interpretation

7edee6a

Cleanup async thread creation for SFT discovery

959adfa

Remove leftover TODO comment

a48689c

Add a safe way to create HttpsUrl

7ec72b1

Replace maybe with another type to clarify semantics of discovery

c4a6a9a

akshaymankar changed the title ~~[WIP] SFT SRV~~ Discover and expose SFT servers in calls config endpoint Jul 30, 2020

akshaymankar changed the title ~~Discover and expose SFT servers in calls config endpoint~~ Discover and expose SFT servers in /calls/config/v2 Jul 30, 2020

akshaymankar marked this pull request as ready for review July 30, 2020 07:57

akshaymankar added 2 commits July 30, 2020 11:18

Group TURN tests so they can be skipped by CI as they were

2f7c0fb

Code to skip: https://github.com/wireapp/wire-server-deploy/blob/f602fd47794da363394dcaf30550e2b6fa2177c6/charts/brig/templates/tests/brig-integration.yaml#L56

Put SFT test in a testGroup for symmetry

b5e26ae

fisx reviewed Jul 30, 2020

View reviewed changes

akshaymankar added 8 commits July 30, 2020 15:13

brig/integration API.Calling: Slightly better formatting

939c802

Add FUTUREWORK to extract a function

2c3ef08

Add roundtrip test for Call.Config.SFTServer

f54e611

More accurate doc for SFTServer

4f0d7d2

Add link to issue for FUTUREWORK

b7b92d8

Pointfree sft discovery async call

3929ea6

Add deprecation notice to /calls/config swagger docs

1f176f4

Brig.Calling.Internal: Declutter sftServerFromSrvTarget

55cb7b0

jschaul commented Jul 30, 2020

View reviewed changes

fisx approved these changes Jul 30, 2020

View reviewed changes

services/brig/test/unit/Test/Brig/Calling.hs Show resolved Hide resolved

services/brig/test/unit/Test/Brig/Calling.hs Show resolved Hide resolved

akshaymankar merged commit 786a466 into develop Jul 31, 2020

akshaymankar deleted the sft_srv branch July 31, 2020 06:54

akshaymankar mentioned this pull request Jul 31, 2020

Ensure sft_servers is never set to null in /calls/config #1184

Merged

tiago-loureiro mentioned this pull request Sep 4, 2020

Release_2020_09_04 #1202

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Discover and expose SFT servers in /calls/config/v2 #1177

Discover and expose SFT servers in /calls/config/v2 #1177

jschaul commented Jul 22, 2020 •

edited by akshaymankar

Loading

fisx left a comment

fisx Jul 27, 2020

akshaymankar Jul 30, 2020

fisx Jul 27, 2020

akshaymankar Jul 30, 2020

fisx commented Jul 29, 2020

akshaymankar commented Jul 29, 2020

fisx left a comment

fisx Jul 30, 2020

akshaymankar Jul 30, 2020

fisx Jul 30, 2020

akshaymankar Jul 30, 2020

fisx Jul 30, 2020

akshaymankar Jul 30, 2020

fisx left a comment

fisx Jul 30, 2020

akshaymankar Jul 30, 2020

fisx Jul 30, 2020

jschaul Jul 30, 2020 •

edited

Loading

fisx Jul 30, 2020

fisx Jul 30, 2020

akshaymankar Jul 30, 2020

jschaul left a comment

jschaul Jul 30, 2020 •

edited

Loading

fisx commented Jul 30, 2020

fisx commented Jul 30, 2020

fisx left a comment

		ensureHttpsUrl :: URIRef Absolute -> HttpsUrl
		ensureHttpsUrl = HttpsUrl . (uriSchemeL . schemeBSL .~ "https")

-ensureHttpsUrl :: URIRef Absolute -> HttpsUrl
-ensureHttpsUrl = HttpsUrl . (uriSchemeL . schemeBSL .~ "https")
+enforceHttpsUrl :: URIRef Absolute -> HttpsUrl
+enforceHttpsUrl = HttpsUrl . (uriSchemeL . schemeBSL .~ "https")
+mkHttpsUrl :: Host -> Port -> HttpsUrl
+mkHttpsUrl = undefined


		makeSem 'PolyLog

		runPolyLog :: Member (Embed IO) r => Log.Logger -> Sem (PolyLog ': r) a -> Sem r a

Discover and expose SFT servers in /calls/config/v2 #1177

Discover and expose SFT servers in /calls/config/v2 #1177

Conversation

jschaul commented Jul 22, 2020 • edited by akshaymankar Loading

fisx left a 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

fisx commented Jul 29, 2020

akshaymankar commented Jul 29, 2020

fisx left a 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

fisx left a 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

jschaul Jul 30, 2020 • edited Loading

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

jschaul left a comment

Choose a reason for hiding this comment

jschaul Jul 30, 2020 • edited Loading

Choose a reason for hiding this comment

fisx commented Jul 30, 2020

fisx commented Jul 30, 2020

fisx left a comment

Choose a reason for hiding this comment

jschaul commented Jul 22, 2020 •

edited by akshaymankar

Loading

jschaul Jul 30, 2020 •

edited

Loading

jschaul Jul 30, 2020 •

edited

Loading