1026599 - PUSH WakeUp netid discover using the reserved 3gppnetwork.org domain

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Instead of appending an string to the APN name for NetId discover, it's better to use the reserved 3gppnetwork.org domain.

3gppnetwork.org domain can be used by mobile operators without risk of collision since the MNC, MCC shall be appended.

Each carrier should resolve this domain inside their own mobile networks.

<SERVICE_NAME>.mnc<MNC>.mcc<MCC>.3gppnetwork.org

This subdomain is placed into the operator’s control. As 

The service name can be a setting, we recommend "wakeup"

Comment 1

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

More information about this reserved domain into this 3GPP document: http://www.3gpp.org/ftp/specs/archive/29_series/29.303/29303-c20.zip

Comment 2

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Bug1026599.patch

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8441927 [details] [diff] [review]
Bug1026599.patch

Review of attachment 8441927 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/push/src/PushService.jsm
@@ +1561,5 @@
>      }
>    },
>  
>    // Get the mobile network ID (netid)
> +  _getMobileNetworkId: function(networkInfo, callback) {

Where is `networkInfo` being used?

@@ +1592,5 @@
>         gDNSService.myHostName + ")");
>  
> +    let netidAddress = "wakeup.mnc" + networkInfo.mnc +
> +      ".mcc" + networkInfo.mcc + ".3gppnetwork.org";
> +    queryDNSForDomain(netidAddress, callback);

Do all carriers name their subdomains this way?
I think the format either needs to be more customizable.

Waiting to hear back from other carriers.

Comment 4

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #3)
> Comment on attachment 8441927 [details] [diff] [review]
> Bug1026599.patch
> 
> Review of attachment 8441927 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/push/src/PushService.jsm
> @@ +1561,5 @@
> >      }
> >    },
> >  
> >    // Get the mobile network ID (netid)
> > +  _getMobileNetworkId: function(networkInfo, callback) {
> 
> Where is `networkInfo` being used?
> 

Into the next block to generate the correct FQDN in order to get the recovered MCC and MNC

> @@ +1592,5 @@
> >         gDNSService.myHostName + ")");
> >  
> > +    let netidAddress = "wakeup.mnc" + networkInfo.mnc +
> > +      ".mcc" + networkInfo.mcc + ".3gppnetwork.org";
> > +    queryDNSForDomain(netidAddress, callback);
> 
> Do all carriers name their subdomains this way?

The idea is to use an homogeneous solution that works for any carrier, so this is an improvement of the last patch.

This is a standardized domain reserved by the 3GPP (see comment 1) to be used by any carrier. Each carrier has his own sub-domain (based on the MCC and MNC values), into these sub-domains each carrier can use it freely.

Anyway, the service name "wakeup" could be reserved in the 3GPP but this is an slow process

> I think the format either needs to be more customizable.
> 

The format is fixed by 3GPP, the only customizable part could be the service name...

> Waiting to hear back from other carriers.

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

What about for MVNOs? Will the EF_SPN also need to be added?

Comment 7

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #5)
> What about for MVNOs? Will the EF_SPN also need to be added?

Hi,

With MVNOs is not a problem since there are 3 cases (in TCP/IP world :)

1.- MVNO users are on the SAME mobile network as the MNO users
 -> In this case, both users will use the same Wakeup server and also will resolve to the same DNS for netid

2.- MVNO users are on a different mobile network but uses the same MCC/MNC
 -> In this case, MNO shall put another WakeUp server inside the MVNO network and also add another DNS server in that network to resolve the correct netid. This is exactly the same case as a MNO with multiple networks.

3.- MVNO has an independent MCC/MNC
 -> In this case, a different discover URL will be generated and the DNS should resolve that address.

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8441927 [details] [diff] [review]
Bug1026599.patch

Review of attachment 8441927 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/push/src/PushService.jsm
@@ +1561,5 @@
>      }
>    },
>  
>    // Get the mobile network ID (netid)
> +  _getMobileNetworkId: function(networkInfo, callback) {

Nit: Please add a comment for what parameters the callback receives.

@@ +1591,5 @@
>      debug("[_getMobileNetworkId:queryDNSForDomain] Getting mobile network ID (I'm " +
>         gDNSService.myHostName + ")");
>  
> +    let netidAddress = "wakeup.mnc" + networkInfo.mnc +
> +      ".mcc" + networkInfo.mcc + ".3gppnetwork.org";

If I understand correctly, the various specs say mnc and mcc should be padded to 3 digits with a 0 if it is 2 digits. Please make that change if required. Everything else looks good.

Comment 9

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #8)
> Comment on attachment 8441927 [details] [diff] [review]
> Bug1026599.patch
> 
> Review of attachment 8441927 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +1591,5 @@
> >      debug("[_getMobileNetworkId:queryDNSForDomain] Getting mobile network ID (I'm " +
> >         gDNSService.myHostName + ")");
> >  
> > +    let netidAddress = "wakeup.mnc" + networkInfo.mnc +
> > +      ".mcc" + networkInfo.mcc + ".3gppnetwork.org";
> 
> If I understand correctly, the various specs say mnc and mcc should be
> padded to 3 digits with a 0 if it is 2 digits. Please make that change if
> required. Everything else looks good.

Agree, [1] page 18 "Where <MNC> and <MCC> are the MNC and MCC of the Service Provider represented in decimal (base 10) form, with any 2 digit MNC padded out to 3 digits by inserting a zero ("0") on the beginning e.g. 15 becomes 015."

Fixing it. Thanks !

[1] http://www.gsma.com/newsroom/wp-content/uploads/2012/11/IR.67-v8.0.pdf

Comment 10

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Bug1026599.patch

New version uploaded

Comment 11

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8456063 [details] [diff] [review]
Bug1026599.patch

Review of attachment 8456063 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/push/src/PushService.jsm
@@ +1566,5 @@
> +   *
> +   * @param networkInfo
> +   *        Network information object { mcc, mnc, ip, port }
> +   * @param callback
> +   *        Callback function to invoke with the netid of null if not found

Nit: s/of/or.

@@ +1596,5 @@
>      }
>  
>      debug("[_getMobileNetworkId:queryDNSForDomain] Getting mobile network ID (I'm " +
>         gDNSService.myHostName + ")");
>  

Nit: Please add a comment here stating why you are doing the slicing tricks :)

Comment 12

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Thank you !

> Nit: Please add a comment here stating why you are doing the slicing tricks
> :)

Oups, a dev. trace I forgot to remove ... Is not really needed I'll remove this query :)

Comment 13

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Attachment: Bug1026599.patch

r+ - See comment 11

Rebased and nits fixed.

Comment 14

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

Comment on attachment 8456758 [details] [diff] [review]
Bug1026599.patch

Approval Request Comment
[Feature/regressing bug #]: -
[User impact if declined]: NetID discover won't work on networks which theirs APN name is not a FQDN
[Describe test coverage new/current, TBPL]: Yes (Same as Bug 1018088)
[Risks and why]: Low risk, is isolated only for Push servers on carriers with wakeup feature enabled
[String/UUID change made/needed]: None

Comment 15

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

I forgot ask for checkin

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Please provide a Try link.

Comment 17

[Fernando R. Sela (no CC, needinfo please) [:frsela]]

As commented previously, push code has no tests [1] so using try for this is useless and resource waste.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1018088#c18

Comment 18

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Fernando R. Sela (no CC, needinfo please) [:frsela] from comment #14)
> [Feature/regressing bug #]: -
This reads as a new feature request.
> [User impact if declined]: NetID discover won't work on networks which
> theirs APN name is not a FQDN
How does this impact a Firefox OS user? Does this block deployment of Firefox OS devices on any known networks?

This patch will need to land on m-c before it can be considered for uplift. (In progress.) Although a relatively small change, it is very late for new feature work for 2.0. Unless this is critically needed it should ship in 2.1.

Comment 19

[Marcelino Veiga Tuimil [:sonmarce]]

We need this patch to support wake up mechanism for push notifications in operators with multiple private networks per MCC/MNC, as it happens in UK, Germany & Brazil. This mechanism saves device battery and network usage, because device does not need a WebSocket permanently opened with push server for being able to receive push notifications. We need this for 2.0 otherwise it would be too late.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbb3b2b08030

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bbb3b2b08030

Comment 22

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8456758 [details] [diff] [review]
Bug1026599.patch

Discussed offline, Lucas made the point that the risk of uplifting bug 1026599, bug 894879, and bug 1024579 is much lower than not uplifting, especially given that this will end up in the vendor build anyway. Given the options, it is preferable that we maintain visibility into this change and the impact that it has on 2.0. I am approving the remaining two bugs (bug 1024579 has already been uplifted). Although preffed off, we need to monitor these landings for any fallout. b2g32+

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/a43d07dcc42e