Project

General

Profile

Feature #8639

Bug #7161: Support more than 24 HTTP mirrors

Write a mirror pool dispatcher script

Added by intrigeri almost 5 years ago. Updated over 3 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
Infrastructure
Target version:
Start date:
01/09/2015
Due date:
04/15/2016
% Done:

100%

Feature Branch:
451f:mirror-pool-dispatcher/master
Type of work:
Code
Blueprint:
Starter:
Affected tool:


Related issues

Blocked by Tails - Feature #8637: Design how to convey the mirror pool's configuration to the dispatcher script Resolved 01/09/2015 04/15/2016
Blocks Tails - Feature #8640: Have the mirror pool dispatcher library audited Resolved 05/10/2016
Blocked by Tails - Feature #10294: Define format for per-mirror hostname Resolved 09/28/2015

History

#1 Updated by intrigeri almost 5 years ago

  • Blocked by Feature #8637: Design how to convey the mirror pool's configuration to the dispatcher script added

#2 Updated by intrigeri almost 5 years ago

  • Blocks Feature #8640: Have the mirror pool dispatcher library audited added

#3 Updated by intrigeri over 4 years ago

  • Target version changed from Sustainability_M1 to Tails_1.7

#5 Updated by Anonymous over 4 years ago

Hi again,

I've written a very small and simple PHP draft here: https://tails.boum.org/blueprint/HTTP_mirror_pool/#index4h1
Simplicity might be the best idea, though, as the code needs to be audited (#8640) before it can be used.

Best regards,
Tobias Frei

#6 Updated by u over 4 years ago

Hi Tobias,

thank you very much. I think we need to wait for https://labs.riseup.net/code/issues/8637 to be resolved though before being able to effectively write this script.

Cheers!

#7 Updated by intrigeri over 4 years ago

  • Due date set to 04/15/2016

#8 Updated by u about 4 years ago

  • Subject changed from Write a server-side mirror pool dispatcher script to Write a mirror pool dispatcher script

#9 Updated by u about 4 years ago

  • Status changed from Confirmed to In Progress
  • % Done changed from 0 to 10

#10 Updated by intrigeri about 4 years ago

Do you need a Git repo for that code? If yes: how do you want to call it?

#11 Updated by u about 4 years ago

good plan. let's call it "mirror-pool-dispatcher" ?

#12 Updated by intrigeri about 4 years ago

good plan. let's call it "mirror-pool-dispatcher" ?

You got it: :mirror-pool-dispatcher.git

#13 Updated by intrigeri about 4 years ago

  • Blocked by Feature #10294: Define format for per-mirror hostname added

#14 Updated by intrigeri about 4 years ago

  • % Done changed from 10 to 0

Please use a directory structure in that repo that's compatible with using it as an ikiwiki overlay. See promotion-material.git for inspiration. I guess we'll want to ship the code in wiki/src/lib/js/... or similar.

#15 Updated by intrigeri about 4 years ago

First bit of nitpicking: please make sure that all names in the code and API use an accurate nomenclature wrt. hostname vs. URL; we'll be handling both kinds of entities, better avoid any confusion :)

#16 Updated by intrigeri about 4 years ago

May you please push the latest working version of the library, so that I can check the API and point Giorgio to it (#10284, which is kinda urgent since Giorgio is supposed to be writing the extension as we speak)?

#17 Updated by intrigeri about 4 years ago

  • Description updated (diff)

#18 Updated by intrigeri about 4 years ago

  • % Done changed from 0 to 10

#19 Updated by u about 4 years ago

intrigeri wrote:

May you please push the latest working version of the library, so that I can check the API and point Giorgio to it (#10284, which is kinda urgent since Giorgio is supposed to be writing the extension as we speak)?

I will do that tomorrow, I would like to fix some things before doing so, otherwise there is no use in publishing it.

#20 Updated by u about 4 years ago

  • Priority changed from Normal to Elevated
  • Target version changed from Tails_1.7 to Tails_1.8

Still stuck with some details here. Raising prio.

#21 Updated by intrigeri almost 4 years ago

  • Target version changed from Tails_1.8 to Tails_2.0

I wonder if we should perhaps simply bump this to 2.2?

#22 Updated by u almost 4 years ago

  • Target version changed from Tails_2.0 to Tails_2.2

bumping to 2.2 because 2.0 is too late anyway.

#23 Updated by u over 3 years ago

  • Target version changed from Tails_2.2 to Tails_2.3

#24 Updated by u over 3 years ago

  • % Done changed from 10 to 50
  • Feature Branch set to mirror-pool-dispatcher/master

The script itself is ready.

I still need to apply it to the website - and modify it to work for other relevant usecases (#11109)

#25 Updated by intrigeri over 3 years ago

I still need to apply it to the website

FTR, this is tracked on #8642 where I did the bulk of the work :)

#26 Updated by intrigeri over 3 years ago

On #8640#note-13 geb wrote:

  • max_weight, url_fallback and mirrors.json could be global variable wrote in CAPS on top of the code for readability.

I agree it feels wrong to hard-code these constants directly in the functions. u, what do you think?

  • getRandomMirrorUrlPrefix() could be split in two functions, one that generate the new array, one that select it.

I personally see the array format, and its generation process, as an internal implementation detail of getRandomMirrorUrlPrefix(), and I see little value in extracting the generation part: it's not as if that part of the code was useful, nor had an easy to define semantics, in isolation. YMMV :)

  • get(path) could maybe be avoided by using <script src="mirrors.json"> and a mirrors = { } within this file.

Right. But then, mirrors.json would become a JavaScript program, and would not be JSON anymore. I see value in being able to validate that JSON file against the JSON schema we have defined. So I'd rather keep the config data (mirrors.json) as static JSON data. See what I mean?

  • Maybe it would have been simplier to use onclick= stategy instead of rewriting the DOM, something like: [...]

Indeed, we haven't considered this option IIRC. I have no strong opinion either way. Note that if we went that way, we would have to add the onclick= code on each link we want to be affected by that code, instead of a mere CSS class as we do currently; this is probably highly subjective, but that feels a bit more error-prone for the people who have to create such links (possibly under pressure such as at release time), so I tend to prefer our current approach (that also has the benefit of being already implemented and tested ;)

Thanks for the suggestions, it's good to know that other people care and read this code!

#27 Updated by u over 3 years ago

intrigeri wrote:

On #8640#note-13 geb wrote:

  • max_weight, url_fallback and mirrors.json could be global variable wrote in CAPS on top of the code for readability.

I agree it feels wrong to hard-code these constants directly in the functions. u, what do you think?

I agree that this would make the code more readable. However, those variables are needed only within these functions and thus I see little value in declaring them globally - which I think is bad practise.

  • getRandomMirrorUrlPrefix() could be split in two functions, one that generate the new array, one that select it.

I personally see the array format, and its generation process, as an internal implementation detail of getRandomMirrorUrlPrefix(), and I see little value in extracting the generation part: it's not as if that part of the code was useful, nor had an easy to define semantics, in isolation. YMMV :)

Ack with intrigeri's comment.

  • get(path) could maybe be avoided by using <script src="mirrors.json"> and a mirrors = { } within this file.

Right. But then, mirrors.json would become a JavaScript program, and would not be JSON anymore. I see value in being able to validate that JSON file against the JSON schema we have defined. So I'd rather keep the config data (mirrors.json) as static JSON data. See what I mean?

That looks a bit hackish to me too.

  • Maybe it would have been simplier to use onclick= stategy instead of rewriting the DOM, something like: [...]

Indeed, we haven't considered this option IIRC. I have no strong opinion either way. Note that if we went that way, we would have to add the onclick= code on each link we want to be affected by that code, instead of a mere CSS class as we do currently; this is probably highly subjective, but that feels a bit more error-prone for the people who have to create such links (possibly under pressure such as at release time), so I tend to prefer our current approach (that also has the benefit of being already implemented and tested ;)

I've not done it like that in first place because my aim was to write unobstrusive code (https://en.wikipedia.org/wiki/Unobtrusive_JavaScript).
But the JS could add the onclick event automatically on all links using this CSS class instead of simply changing the HREF, so that would not be a problem and that would still be unobstrusive. However, I don't see the real benefit. On the contrary, when people would hover the link they would still see the original HREF and could not right click -> open link in new tab or copy the link to do a wget or whatever people sometimes want to do. Thus, I prefer the current implementation.

Thanks for the suggestions, it's good to know that other people care and read this code!

Indeed! :)

#28 Updated by geb over 3 years ago

Hi,

u wrote:

intrigeri wrote:

On #8640#note-13 geb wrote:

  • max_weight, url_fallback and mirrors.json could be global variable wrote in CAPS on top of the code for readability.

I agree it feels wrong to hard-code these constants directly in the functions. u, what do you think?

I agree that this would make the code more readable. However, those variables are needed only within these functions and thus I see little value in declaring them globally - which I think is bad practise.

Ok interest of declaring them globally can be discussed. Maybe can it make sense to declare them on top of the function ; up to you :)

  • getRandomMirrorUrlPrefix() could be split in two functions, one that generate the new array, one that select it.

I personally see the array format, and its generation process, as an internal implementation detail of getRandomMirrorUrlPrefix(), and I see little value in extracting the generation part: it's not as if that part of the code was useful, nor had an easy to define semantics, in isolation. YMMV :)

Ack with intrigeri's comment.

Ack.

  • get(path) could maybe be avoided by using <script src="mirrors.json"> and a mirrors = { } within this file.

Right. But then, mirrors.json would become a JavaScript program, and would not be JSON anymore. I see value in being able to validate that JSON file against the JSON schema we have defined. So I'd rather keep the config data (mirrors.json) as static JSON data. See what I mean?

That looks a bit hackish to me too.

Ack. My point was to reduce the code size, because as i understand it the JSON was suppose to be hosted in a trusted location without any strong need to validate it (see 8640#note-15), but maybe the interest of keeping the possibility to change that in a futur is more important than the code size.

  • Maybe it would have been simplier to use onclick= stategy instead of rewriting the DOM, something like: [...]

Indeed, we haven't considered this option IIRC. I have no strong opinion either way. Note that if we went that way, we would have to add the onclick= code on each link we want to be affected by that code, instead of a mere CSS class as we do currently; this is probably highly subjective, but that feels a bit more error-prone for the people who have to create such links (possibly under pressure such as at release time), so I tend to prefer our current approach (that also has the benefit of being already implemented and tested ;)

I've not done it like that in first place because my aim was to write unobstrusive code (https://en.wikipedia.org/wiki/Unobtrusive_JavaScript).
But the JS could add the onclick event automatically on all links using this CSS class instead of simply changing the HREF, so that would not be a problem and that would still be unobstrusive. However, I don't see the real benefit. On the contrary, when people would hover the link they would still see the original HREF and could not right click -> open link in new tab or copy the link to do a wget or whatever people sometimes want to do. Thus, I prefer the current implementation.

Ack. As for other proposals, the idea was really to have simplier code, maybe not that relevant compared to drawbacks you mentionned :).

#29 Updated by u over 3 years ago

geb wrote:

u wrote:

intrigeri wrote:

On #8640#note-13 geb wrote:

  • get(path) could maybe be avoided by using <script src="mirrors.json"> and a mirrors = { } within this file.

Right. But then, mirrors.json would become a JavaScript program, and would not be JSON anymore. I see value in being able to validate that JSON file against the JSON schema we have defined. So I'd rather keep the config data (mirrors.json) as static JSON data. See what I mean?

That looks a bit hackish to me too.

Ack. My point was to reduce the code size, because as i understand it the JSON was suppose to be hosted in a trusted location without any strong need to validate it (see 8640#note-15), but maybe the interest of keeping the possibility to change that in a futur is more important than the code size.

Just for understanding: This JSON file will be loaded by the Tails Upgrader, the Tails website and the Firefox extension. The latter will use parts of the JS mirror pool dispatcher code too. And this needs stronger validation IMO. Not just for MitM but also in order to avoid mistakes within the file.

#30 Updated by u over 3 years ago

  • Feature Branch changed from mirror-pool-dispatcher/master to 451f:mirror-pool-dispatcher/master

#31 Updated by anonym over 3 years ago

  • Target version changed from Tails_2.3 to Tails_2.4

#32 Updated by intrigeri over 3 years ago

  • Status changed from In Progress to Resolved
  • Assignee deleted (u)
  • % Done changed from 50 to 100

This was completed in April, and the follow-ups are tracked on #8640.

#34 Updated by BitingBird over 3 years ago

  • Priority changed from Elevated to Normal

Also available in: Atom PDF