1200763 - IceGatherTest.TestGatherFakeStunServerIpAddress (in ice_unittest) is failing

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P2)

Description

[Byron Campen [:bwc]]

This is one of these test-cases that cannot run in CI since they use an external STUN server. Currently bisecting.

Comment 1

[Byron Campen [:bwc]]

http://hg.mozilla.org/mozilla-central/log?rev=39c0d32001d7 is the culprit. It seems that the external STUN server we're using has changed its IP addy. Fixing that lets the test pass again, but I'm going to question the wisdom of using a bare IP address that is hard-coded for a service that we don't control. Did we change this from stun.services.mozilla.com because it was lacking TCP support?

Comment 2

[Nils Ohlmeier [:drno]]

Yes exactly. And I also assume that at some point in the future we would want to tear down stun.services.mozilla.com, as running public STUN servers for nothing (except our own tests) is not really our business.

Options:
A) Resolve the DNS name via the OS resolver before starting the tests (short term?)
B) I think we only want the DNS vs. IP based unit tests to verify that our internal resolver client code properly handles three different scenario. But I think we have way to many tests depending on the DNS name and IP address. We should evaluate which of these can be removed or at least made independent of external STUN servers, because we should keep the external dependencies to an absolute minimum.

Regarding B) I think the safer way is actually to not run any of the tests which have external dependencies by default, but only if a STUN server was specified via the env variables.

My guess is that we actually have tests which might depend on STUN responses telling us that we are NATed (not sure?). Byron, could we actually plug the NAT simulator in between the ice unit test and nICEr fake STUN server for that purpose?

Comment 3

[Byron Campen [:bwc]]

It looks like most of our tests that use the NAT simulator also use the test STUN server. I'm not sure exactly what we'd lose by turning off the external STUN server tests, but I think it would be pretty easy to get that functionality from the test STUN server. Let me look.

Comment 4

[Byron Campen [:bwc]]

So, it seems like we would need to do a little work to have the tests automatically set up test STUN servers for TCP and IPv6 (these are already supported, but we're only using these for fake responses right now), and then a bunch of test-cases would need to be altered.

I guess the thing we lose is any sort of interop sanity check. I would not be worried about this except for the fact that this is literally the only thing you can run from the command line that sorta checks that we haven't completely messed up some bit of STUN interop.

Between these two, it looks like it might be easiest to first implement A, then consolidate the test cases we have into one set that either runs against the test STUN server, or a STUN server FQDN supplied from an env variable. Does that sound sane?

Comment 5

[Nils Ohlmeier [:drno]]

Yes sounds good to me.

BTW there are also a few TURN tests in there which I think fall into the category already of needing an external server be provisioned via env variables. If a TURN server is provided we should probably also use that as a STUN server, or are there any STUN uses case which won't work with a TURN server?

Regarding interop: how about we fire up a bunch of different STUN/TURN servers in the QA lab. We could then A) install/run a separate job which only runs ice unit tests against all the different servers (we could even run them from behind a iptables NAT if we think that adds value) and B) we could bake the list of these servers into the ice unit tests to easily run them manually as long as you are on the Mozilla network/VPN.
Do you think that sounds worth doing?

Comment 6

[Byron Campen [:bwc]]

(In reply to Nils Ohlmeier [:drno] from comment #5)
> Yes sounds good to me.
> 
> BTW there are also a few TURN tests in there which I think fall into the
> category already of needing an external server be provisioned via env
> variables. If a TURN server is provided we should probably also use that as
> a STUN server, or are there any STUN uses case which won't work with a TURN
> server?

   Yes, we should be able to use any TURN server for basic STUN tests too.

> 
> Regarding interop: how about we fire up a bunch of different STUN/TURN
> servers in the QA lab. We could then A) install/run a separate job which
> only runs ice unit tests against all the different servers (we could even
> run them from behind a iptables NAT if we think that adds value) and B) we
> could bake the list of these servers into the ice unit tests to easily run
> them manually as long as you are on the Mozilla network/VPN.
> Do you think that sounds worth doing?

   Maybe? I guess we haven't seen lots of cases of bad STUN interop, but having one might be useful as a basic sanity check. I feel like our resources would be better allocated to deploying NATs in the lab that we can use with the browser interop testing, or perhaps other things. What's your take?

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Byron Campen [:bwc] from comment #6)
> (In reply to Nils Ohlmeier [:drno] from comment #5)

> > Regarding interop: how about we fire up a bunch of different STUN/TURN
> > servers in the QA lab. We could then A) install/run a separate job which
> > only runs ice unit tests against all the different servers (we could even
> > run them from behind a iptables NAT if we think that adds value) and B) we
> > could bake the list of these servers into the ice unit tests to easily run
> > them manually as long as you are on the Mozilla network/VPN.
> > Do you think that sounds worth doing?
> 
>    Maybe? I guess we haven't seen lots of cases of bad STUN interop, but
> having one might be useful as a basic sanity check. I feel like our
> resources would be better allocated to deploying NATs in the lab that we can
> use with the browser interop testing, or perhaps other things. What's your
> take?

STUN is pretty simple overall; STUN interop issues should be rare.  TURN slightly more common perhaps.  I tend to agree with Byron - NATs are more important.

Comment 8

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1200763: Remove hard-coded STUN IP address from ice_unittest, and do a DNS lookup instead.

Bug 1200763: Remove hard-coded STUN IP address from ice_unittest, and do a DNS lookup instead.

Comment 9

[Byron Campen [:bwc]]

Comment on attachment 8656134 [details]
MozReview Request: Bug 1200763: Remove hard-coded STUN IP address from ice_unittest, and do a DNS lookup instead.

This uses NrIceResolver to look up the IP address (if not set) for the supplied STUN server hostname (if any). An alternative would be to use @mozilla.org/network/dns-service;1 directly. This look sane to you?

Comment 10

[Nils Ohlmeier [:drno]]

Comment on attachment 8656134 [details]
MozReview Request: Bug 1200763: Remove hard-coded STUN IP address from ice_unittest, and do a DNS lookup instead.

https://reviewboard.mozilla.org/r/18091/#review16229

I was thinking more along the lines of getaddrinfo(), but this works to.

Comment 11

[Nils Ohlmeier [:drno]]

Comment on attachment 8656134 [details]
MozReview Request: Bug 1200763: Remove hard-coded STUN IP address from ice_unittest, and do a DNS lookup instead.

Apparently mozreview can't translate "Ship it" into f+

Comment 12

[Byron Campen [:bwc]]

(In reply to Nils Ohlmeier [:drno] from comment #10)
> Comment on attachment 8656134 [details]
> MozReview Request: Bug 1200763: Remove hard-coded STUN IP address from
> ice_unittest, and do a DNS lookup instead.
> 
> https://reviewboard.mozilla.org/r/18091/#review16229
> 
> I was thinking more along the lines of getaddrinfo(), but this works to.

   Huh, I did not realize that existed on WIN32, but it seems to. Let me try that.

Comment 13

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1200763: Remove hard-coded STUN IP address from ice_unittest, and do a DNS lookup instead.

Bug 1200763: Try using getaddrinfo instead. Will fold if this works.

Comment 14

[Byron Campen [:bwc]]

Comment on attachment 8656756 [details]
MozReview Request: Bug 1200763: Remove hard-coded STUN IP address from ice_unittest, and do a DNS lookup instead.

Bug 1200763: Remove hard-coded STUN IP address from ice_unittest, and do a DNS lookup instead.

Comment 15

[Nils Ohlmeier [:drno]]

Comment on attachment 8656756 [details]
MozReview Request: Bug 1200763: Remove hard-coded STUN IP address from ice_unittest, and do a DNS lookup instead.

https://reviewboard.mozilla.org/r/18241/#review16431

::: media/mtransport/test/ice_unittest.cpp:3064
(Diff revision 2)
> +static std::string
> +Resolve(const std::string& fqdn, int address_family)

As this returns an empty string in case of error I think we need one more safe guard for the case that the DNS resolution has failed (for whatever reason) and g_stun_server_address stays empty:

https://dxr.mozilla.org/mozilla-central/source/media/mtransport/test/ice_unittest.cpp?case=true&from=ice_unittest.cpp#1375

Comment 16

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/18241/#review16431

> As this returns an empty string in case of error I think we need one more safe guard for the case that the DNS resolution has failed (for whatever reason) and g_stun_server_address stays empty:
> 
> https://dxr.mozilla.org/mozilla-central/source/media/mtransport/test/ice_unittest.cpp?case=true&from=ice_unittest.cpp#1375

We already guard against this in the test-cases themselves, since it can be set to empty here:

https://dxr.mozilla.org/mozilla-central/source/media/mtransport/test/ice_unittest.cpp?offset=200#3098

Comment 17

[Byron Campen [:bwc]]

Comment on attachment 8656756 [details]
MozReview Request: Bug 1200763: Remove hard-coded STUN IP address from ice_unittest, and do a DNS lookup instead.

Bug 1200763 - Part 1: Remove hard-coded STUN IP address from ice_unittest, and do a DNS lookup instead.

Comment 18

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1186590 - Part 2: Move hard-coded interface priority list into nrinterfaceprioritizer, and simplify some functions.

Bug 1186590 - Part 2: Move hard-coded interface priority list into nrinterfaceprioritizer, and simplify some functions.

Comment 19

[Byron Campen [:bwc]]

Comment on attachment 8656756 [details]
MozReview Request: Bug 1200763: Remove hard-coded STUN IP address from ice_unittest, and do a DNS lookup instead.

Bug 1200763: Remove hard-coded STUN IP address from ice_unittest, and do a DNS lookup instead.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e54b1671de4

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5e54b1671de4