#20174 closed enhancement (implemented)

Automate checking existing fallbacks

Reported by: teor Owned by: haxxpop
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Fallback Scripts Version: Tor: 0.2.8.1-alpha
Severity: Normal Keywords: fallback, tor-03-unspecified-201612
Cc: Actual Points:
Parent ID: Points: 1
Reviewer: Sponsor:

Description (last modified by teor)

I use a manual process to check existing fallbacks. It would be great if the updateFallbackDirs.py script would automatically read src/or/fallback_dirs.inc, and check each fallback for errors.

For details, see:
https://trac.torproject.org/projects/tor/wiki/doc/UpdatingFallbackDirectoryMirrors

I think it can go in 0.3.0

Child Tickets

Attachments (4)

tor.patch (16.8 KB) - added by haxxpop 10 months ago.
tor.2.patch (17.8 KB) - added by haxxpop 10 months ago.
tor.3.patch (18.5 KB) - added by haxxpop 10 months ago.
tor.4.patch (18.4 KB) - added by haxxpop 10 months ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 12 months ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:2 Changed 11 months ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:3 Changed 10 months ago by haxxpop

Owner: set to haxxpop
Status: newassigned

comment:4 Changed 10 months ago by haxxpop

Status: assignedneeds_review

comment:5 Changed 10 months ago by haxxpop

I'm not sure whether the patch in the attachment also solves #20178. Check it please.

comment:6 Changed 10 months ago by teor

Status: needs_reviewneeds_revision

Thanks for this patch!
It implements the existing fallback check from https://trac.torproject.org/projects/tor/wiki/doc/UpdatingFallbackDirectoryMirrors

But I think we can do it better:

  • using os.system() is error-prone, because it calls out to a shell. It also raises security concerns. Can you re-write the loading and parsing steps in Python?
  • modifying the existing files isn't necessary - instead, create a temporary file, and change WHITELIST_FILE_NAME to point to it - or, even better, parse the existing file straight into the whitelist data structure
  • now that the script takes arguments, please modify the script's usage comment (line 3), or, even better, provide a "help" command that outputs the usage string on any unrecognised command-line argument

(I'll review the #20178 part after these changes.)

Changed 10 months ago by haxxpop

Attachment: tor.patch added

comment:7 in reply to:  6 Changed 10 months ago by haxxpop

Replying to teor:

The patch is already updated.

comment:8 Changed 10 months ago by haxxpop

Status: needs_revisionneeds_review

comment:9 Changed 10 months ago by teor

Component: Core Tor/TorCore Tor/Fallback Scripts
Status: needs_reviewneeds_revision

Thanks for making the changes I asked for.
I checked #20178, and the script logs at the right levels now.

The check_existing mode would be more useful if it only logged warning and info messages for relay fingerprints in the existing list. I have opened #21282 for this enhancement.

There's one change which needs to be made to the usage message, and then we can merge this patch: the script output should be saved to the src/or/fallback_dirs.inc file. That way, the good fallbacks in the existing list are kept in the file, and the bad fallbacks are replaced removed.

Last edited 10 months ago by teor (previous) (diff)

Changed 10 months ago by haxxpop

Attachment: tor.2.patch added

comment:10 in reply to:  9 ; Changed 10 months ago by haxxpop

Replying to teor:

The check_existing mode would be more useful if it only logged warning and info messages for relay fingerprints in the existing list. I have opened #21282 for this enhancement.

What does "for relay fingerprints" mean?
ps. I have already updated the usage message.

comment:11 in reply to:  10 ; Changed 10 months ago by teor

Description: modified (diff)
Milestone: Tor: unspecifiedTor: 0.3.0.x-final
Status: needs_revisionmerge_ready

Replying to haxxpop:

Replying to teor:

The check_existing mode would be more useful if it only logged warning and info messages for relay fingerprints in the existing list. I have opened #21282 for this enhancement.

What does "for relay fingerprints" mean?

Each relay has a unique identifier called a "fingerprint".
So log messages about a fingerprint for a fallback should be logged.
Otherwise, they should be ignored.

We can do the changes to the log messages in ticket #21282.
There are more details in that ticket.

ps. I have already updated the usage message.

Thanks, then this patch is ready to be merged, it just needs a changes file:
https://gitweb.torproject.org/tor.git/tree/doc/HACKING/CodingStandards.md#n56

- Minor enhancements (fallback scripts):
    - Add a check_existing mode to updateFallbackDirs.py, which checks if fallbacks in
      the hard-coded list are working. Closes ticket 20174. Patch by haxxpop.

Changed 10 months ago by haxxpop

Attachment: tor.3.patch added

comment:12 in reply to:  11 Changed 10 months ago by haxxpop

Replying to teor:

Change file added.

Last edited 10 months ago by haxxpop (previous) (diff)

Changed 10 months ago by haxxpop

Attachment: tor.4.patch added

comment:13 Changed 10 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

applied; thank you!

Note: See TracTickets for help on using tickets.