416416 - change extensions.update.url to not live behind https://addons.mozilla.org/

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P2)

Description

[matthew zeier [:mrz]]

I want to separate VersionCheck to live behind a seperate hostname.  

Currently, it's impossible to separate normal AMO traffic and VersionCheck traffic since both live behind https://addons.mozilla.org/ (L7 content switching is possible but expensive, CPU-wise)

Both have vastly different usage profiles but because both are behind the addons.mozilla.org hostname, both go through the same load balancers and same backend servers.  Because of this, VersionCheck has the ability to take down all of AMO instead of just itself (and it has).  

Marked as blocking Fx3 to get this in for b4, otherwise, as the user base grows, we'll have a harder time scaling AMO and VersionCheck. 

(would love to get this in for Fx 2.0.0.13)

For IT, just need to add a ServerAlias under addons.mozilla.org's vhost.

Comment 1

[Justin Fitzhugh]

Just to add my vote of support here, something we should really do before ff3 goes final if possible.  Will help us to scale addons as we grow.  Let us know if there is anything we can do to help test or otherwise to push this through.

Comment 2

[Reed Loden [:reed]]

So, what hostname do you want to use?

http://mxr.mozilla.org/firefox/source/browser/app/profile/firefox.js shows that the following subdomains are used for various things that are on or part of AMO:
* https://addons.mozilla.org
* https://services.addons.mozilla.org
* https://%LOCALE%.add-ons.mozilla.com
* https://pfs.mozilla.org

Comment 3

[Dave Townsend [:mossop]]

This would require a late-l10n change to implement. We can either change the url in all locales, or we could just stop localising the pref which I have been wanting for a while since there is no reason why it should be and has caused issues in the past.

Comment 4

[matthew zeier [:mrz]]

(In reply to comment #2)
> So, what hostname do you want to use?

-not- addons.mozilla.org or anything that is shared with any other service.  

So https://versioncheck.mozilla.org/update/VersionCheck.php...  would be a good guess.

Comment 5

[matthew zeier [:mrz]]

(In reply to comment #3)
> This would require a late-l10n change to implement. 

I'm just talking about the hostname, not anything to do with l10n.  Change:

https://addons.mozilla.org/update/VersionCheck.php?reqVersion=%REQ_VERSION%&id=%ITEM_ID%&version=%ITEM_VERSION%&maxAppVersion=%ITEM_MAXAPPVERSION%&status=%ITEM_STATUS%&appID=%APP_ID%&appVersion=%APP_VERSION%&appOS=%APP_OS%&appABI=%APP_ABI%&locale=%APP_LOCALE%

to:

https://versionscheck.mozilla.org/update/VersionCheck.php?reqVersion=%REQ_VERSION%&id=%ITEM_ID%&version=%ITEM_VERSION%&maxAppVersion=%ITEM_MAXAPPVERSION%&status=%ITEM_STATUS%&appID=%APP_ID%&appVersion=%APP_VERSION%&appOS=%APP_OS%&appABI=%APP_ABI%&locale=%APP_LOCALE%

Comment 6

[Dave Townsend [:mossop]]

The url is currently held in the toolkit locale so it would require changes to locale strings.

Comment 7

[Jeremy Orem [:oremj]]

Wouldn't this be as simple as shipping with a new about:config entry for extensions.update.url?

Comment 8

[Reed Loden [:reed]]

Hmm, update.mozilla.org was used years ago and is now unused... It already points to AMO. You could reuse it, possibly. :)

(In reply to comment #5)
> (In reply to comment #3)
> > This would require a late-l10n change to implement. 
> 
> I'm just talking about the hostname, not anything to do with l10n.  Change:

Yes, we know, but it does have to do with l10n, as the locales have this preference localized (for some reason). It can't be easily changed globally without either changing all the locales or changing the pref to be non-localized.

See http://mxr.mozilla.org/l10n/source/en-GB/toolkit/chrome/mozapps/extensions/extensions.properties#43 for an example. :(

Comment 9

[Dave Townsend [:mossop]]

I would certainly prefer making the preference a regular one so we don't have to alter every locale everytime we want to change it, but even that is an l10n change since we would no longer be using a string.

Thinking though, given that this is in the locale in Firefox 2, I'm not sure what the rules are on being able to change it there, Pike?

Comment 10

[Reed Loden [:reed]]

(In reply to comment #9)
> I would certainly prefer making the preference a regular one so we don't have
> to alter every locale everytime we want to change it, but even that is an l10n
> change since we would no longer be using a string.
> 
> Thinking though, given that this is in the locale in Firefox 2, I'm not sure
> what the rules are on being able to change it there, Pike?

What would it matter, as you would just ignore the localized version of the string? You could, of course, remove it on the trunk, but wouldn't you be able to just ignore it on the branch and fallback to the firefox.js version of the pref?

Comment 11

[Mike Schroepfer]

Just another vote here - we should do this for beta4

Comment 12

[Axel Hecht [:Pike]]

I think that we could perfectly remove that string for Firefox and Thunderbird, but I remember that this pref was asked to stay in l10n by the non-moco projects, i.e., SeaMonkey. Not sure, I might be confusing the pref, too.

Anyway, from a late-l10n perspective, I have no problems with removing strings, as that's not going to turn any build broken, the worst thing happening is leaving in dead code in a localization.

As for SeaMonkey, CCing KaiRo, they might be able to just set the pref to something localizable for SeaMonkey, and we use a static pattern.

Comment 13

[Phil Ringnalda (:philor)]

It's sort of paradoxical, but not using a localized string actually makes it easier for SeaMonkey to localize - either way, they have to store their localized URL in a different .properties file, because a locale that uses a different site for SM will still be using AMO for Fx. If Toolkit doesn't have a localized string then SM changes their pref from a URL-pattern to a chrome URI for their .properties file, and puts the URL-pattern in there, and the url formatter deals, while if Toolkit provides a localized string, then because nobody can actually use it, SM changes their pref from one chrome URI to another chrome URI, and every locale that does both Fx/Tb and SM has to have two different URL patterns, one of which they can change and one of which they are absolutely required to not ever touch. Either way, SM has to do the exact same thing, the difference is just that removing the localized string from Toolkit makes it less confusing.

Comment 14

[Robert Kaiser]

If it's going through urlformatter, then I think it's no problem in any way, as the app can override the pref, and the pref in the app can always be a localized pref, as urlformatter handles that automagically, allowing it to be either.
IMHO, this should be set together with the pref'd urlformatted values for "Get *" links in EM, so that any app can easily set their own service instead of AMO - I guess Songbird and others probably want that anyways.

Comment 15

[Dave Townsend [:mossop]]

(In reply to comment #14)
> IMHO, this should be set together with the pref'd urlformatted values for "Get
> *" links in EM, so that any app can easily set their own service instead of AMO
> - I guess Songbird and others probably want that anyways.

All of the getExtensions etc. prefs are currently plain prefs in the application's preference files (they used to be localised but changed fairly recently).

So from what I see the general feeling is that we can just switch to an app pref for the url. That works for seamonkey and other apps and schrep and pike approve so getting approval for the l10n change shouldn't be an issue.

So decide on the final hostname, get it set up and the patch to do this is simple.

Comment 16

[matthew zeier [:mrz]]

(In reply to comment #15)
> So decide on the final hostname, get it set up and the patch to do this is
> simple.

For the sake of being verbose, versioncheck.mozilla.org works for me.  Sort of identifies what it's being used for (you have no idea how many people think we're attacking them when they see responses from fxfeeds.mozilla.org!).

Comment 17

[Jeremy Orem [:oremj]]

Who is the authority on URLs?  If no one has any complaints about the URL in Comment #16 we can make this happen right away.

Comment 18

[Wil Clouser [:clouserw]]

If we're only using this for checking the versions of add-ons it seems more appropriate to use versioncheck.addons.mozilla.org.  That would be more consistent with the current URLs in comment 2 and the unmentioned facebook.a.m.o.

Comment 19

[Jeremy Orem [:oremj]]

Works for me if we have the *.addons.mozilla.org cert.

Initially the setup should just be:

versioncheck.addons.mozilla.org CNAME addons.mozilla.org

Add versioncheck.addons.mozilla.org to ServerAlias in the addon.mozilla.org vhost.

Does that sounds correct to everyone?

Comment 20

[matthew zeier [:mrz]]

(In reply to comment #19)
> Works for me if we have the *.addons.mozilla.org cert.
> 
> Initially the setup should just be:
> 
> versioncheck.addons.mozilla.org CNAME addons.mozilla.org

That wouldn't work - the cert under addons.mozilla.org is *.mozilla.org, not *.addons.mozilla.org. Needs to be a seperate VIP but with the same backend servers (the vhost addition is still valid).

The interesting thing with the seperate VIP is that we can actually see how  much traffic version check is vs. just AMO.

Comment 21

[Reed Loden [:reed]]

Attachment: trunk patch - v1

Mossop, something like this?

Comment 22

[Reed Loden [:reed]]

Comment on attachment 302322 [details] [diff] [review]
trunk patch - v1

>-      var dsURI = gPref.getComplexValue(PREF_EM_ITEM_UPDATE_URL.replace(/%UUID%/, aItem.id),
>-                                        Ci.nsIPrefLocalizedString).data;
>+      var dsURI = gPref.getCharPref(PREF_EM_ITEM_UPDATE_URL.replace(/%UUID%/, aItem.id);

Pretend there's an extra ) at the end of that line. :)

Comment 23

[Dave Townsend [:mossop]]

Comment on attachment 302322 [details] [diff] [review]
trunk patch - v1

Almost but you're overthinking it. We don't want to be unlocalizing the PREF_EM_ITEM_UPDATE_URL case

Comment 24

[Reed Loden [:reed]]

Attachment: trunk patch - v2

Comment 25

[Reed Loden [:reed]]

Attachment: trunk patch - v2.1

No, I don't hate SeaMonkey.

Comment 26

[Dave Townsend [:mossop]]

Comment on attachment 302339 [details] [diff] [review]
trunk patch - v2.1

>Index: toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>     if (!dsURI)
>       dsURI = aItem.updateRDF;
>     if (!dsURI) {
>-      dsURI = gPref.getComplexValue(PREF_UPDATE_DEFAULT_URL,
>-                                    Ci.nsIPrefLocalizedString).data;
>+      dsURI = gPref.getCharPref(PREF_UPDATE_DEFAULT_URL);
>     }

Nit: drop the braces around the single line statement.

r+ from me assuming the hostname is correct, if an IT guy could confirm that explicitly here it'd be good.

Please do not check in until the service is actually up and running.

Comment 27

[Reed Loden [:reed]]

Comment on attachment 302339 [details] [diff] [review]
trunk patch - v2.1

Requesting approval since this bug has late-l10n. Pike has said before that string removals aren't really considered late-l10n (see bug 388652, comment #13), but since he's on vacation, I'll just go ahead and request approval just in case.

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 302339 [details] [diff] [review]
trunk patch - v2.1

a=beltzner

Comment 29

[matthew zeier [:mrz]]

(In reply to comment #26)
> r+ from me assuming the hostname is correct, if an IT guy could confirm that
> explicitly here it'd be good.

Agreed - versioncheck.addons.mozilla.org will be setup.

Comment 30

[Michael Morgan [:morgamic]]

IT - can you let me know when you plan on doing the switchover so I can be around?

Comment 31

[Jeremy Orem [:oremj]]

The VIP will be active right away, but the actual switchover will happen when this patch is released.

Comment 32

[matthew zeier [:mrz]]

I'd like to see this pushed in for Fx 2.0.0.13 if possible.  Minor updates in the Fx2 cycle are still going to be a pain point that IT will have to deal with until Fx3 is out the door and users have moved off Fx2.  

Comment 33

[Mike Schroepfer]

(In reply to comment #32)
> I'd like to see this pushed in for Fx 2.0.0.13 if possible.  Minor updates in
> the Fx2 cycle are still going to be a pain point that IT will have to deal with
> until Fx3 is out the door and users have moved off Fx2.  
> 

I'd prefer not.  FF3 is coming and folks will move fast.  Trust me.  

Comment 34

[Justin Scott [:fligtar]]

Glad I saw a mention of this new URL in IRC scrollback and looked for a bug...

This will break automatic add-on detection (currently only used by Rock Your Firefox) due to not being able to use the addons.mozilla.org cookie and may also require update ping counter script changes depending on if IT setup the logs to go to the same place they're going now under addons.mozilla.org. I briefly tested to see if a query string I entered in versioncheck.amo appeared in the logs and couldn't find it.

I can fix the RYF detection next week, but that means a couple RYF pages will have to be served from versioncheck.addons.mozilla.org and some netscaler cache rules may have to be changed dealing with that cookie.

Comment 35

[Reed Loden [:reed]]

fligtar, are there bugs tracking the issues you mentioned so I will know when it is safe for me to land this?

Comment 36

[Justin Scott [:fligtar]]

(In reply to comment #35)
> fligtar, are there bugs tracking the issues you mentioned so I will know when
> it is safe for me to land this?
> 

Filed bug 417248 for the RYF changes. Can someone in IT comment as to whether the versioncheck.addons.mozilla.org logs are going somewhere new or to addons.mozilla.org as usual? If they are going to the same place, there's nothing to be done there.

Comment 37

[Jeremy Orem [:oremj]]

They will be going to /data/stats/logs/im-log01/versioncheck.addons.mozilla.org and /data/stats/logs/nladm01/versioncheck.addons.mozilla.org.

Comment 38

[Justin Scott [:fligtar]]

So we'll need to add 2 more counter crons for the AMO update ping counter and make similar changes to your counter, right?

Comment 39

[Mike Beltzner [:beltzner, not reading bugmail]]

It would be really nice to get this in for l10n code freeze for b4, but not required.

Comment 40

[Reed Loden [:reed]]

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.281; previous revision: 1.280
done
Checking in calendar/sunbird/app/profile/sunbird.js;
/cvsroot/mozilla/calendar/sunbird/app/profile/sunbird.js,v  <--  sunbird.js
new revision: 1.53; previous revision: 1.52
done
Checking in composer/app/profile/composer-prefs.js;
/cvsroot/mozilla/composer/app/profile/composer-prefs.js,v  <--  composer-prefs.js
new revision: 1.4; previous revision: 1.3
done
Checking in mail/app/profile/all-thunderbird.js;
/cvsroot/mozilla/mail/app/profile/all-thunderbird.js,v  <--  all-thunderbird.js
new revision: 1.108; previous revision: 1.107
done
Checking in suite/browser/browser-prefs.js;
/cvsroot/mozilla/suite/browser/browser-prefs.js,v  <--  browser-prefs.js
new revision: 1.83; previous revision: 1.82
done
Checking in toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/extensions/extensions.properties,v  <--  extensions.properties
new revision: 1.50; previous revision: 1.49
done
Checking in toolkit/mozapps/extensions/src/nsExtensionManager.js.in;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v  <--  nsExtensionManager.js.in
new revision: 1.275; previous revision: 1.274
done

Comment 41

[Daniel Veditz [:dveditz]]

We agree w/Schrep, not blocking FF2 releases.

Comment 42

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b4) Gecko/2008030317 Firefox/3.0b4

extensions.update.url = https://versioncheck.addons.mozilla.org/update/VersionCheck.php?reqVersion=%REQ_VERSION%&id=%ITEM_ID%&version=%ITEM_VERSION%&maxAppVersion=%ITEM_MAXAPPVERSION%&status=%ITEM_STATUS%&appID=%APP_ID%&appVersion=%APP_VERSION%&appOS=%APP_OS%&appABI=%APP_ABI%&locale=%APP_LOCALE%