Opened 5 months ago

Closed 5 weeks ago

#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 6 weeks ago.
tor.2.patch (17.8 KB) - added by haxxpop 5 weeks ago.
tor.3.patch (18.5 KB) - added by haxxpop 5 weeks ago.
tor.4.patch (18.4 KB) - added by haxxpop 5 weeks ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 4 months ago by teor

  • Milestone changed from Tor: 0.2.??? to Tor: 0.3.???

Milestone renamed

comment:2 Changed 3 months ago by nickm

  • Keywords tor-03-unspecified-201612 added
  • Milestone changed from Tor: 0.3.??? to Tor: unspecified

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

comment:3 Changed 6 weeks ago by haxxpop

  • Owner set to haxxpop
  • Status changed from new to assigned

comment:4 Changed 6 weeks ago by haxxpop

  • Status changed from assigned to needs_review

comment:5 Changed 6 weeks ago by haxxpop

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

comment:6 follow-up: Changed 6 weeks ago by teor

  • Status changed from needs_review to needs_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 6 weeks ago by haxxpop

comment:7 in reply to: ↑ 6 Changed 6 weeks ago by haxxpop

Replying to teor:

The patch is already updated.

comment:8 Changed 6 weeks ago by haxxpop

  • Status changed from needs_revision to needs_review

comment:9 follow-up: Changed 5 weeks ago by teor

  • Component changed from Core Tor/Tor to Core Tor/Fallback Scripts
  • Status changed from needs_review to needs_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 5 weeks ago by teor (previous) (diff)

Changed 5 weeks ago by haxxpop

comment:10 in reply to: ↑ 9 ; follow-up: Changed 5 weeks 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 ; follow-up: Changed 5 weeks ago by teor

  • Description modified (diff)
  • Milestone changed from Tor: unspecified to Tor: 0.3.0.x-final
  • Status changed from needs_revision to merge_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 5 weeks ago by haxxpop

comment:12 in reply to: ↑ 11 Changed 5 weeks ago by haxxpop

Replying to teor:

Change file added.

Last edited 5 weeks ago by haxxpop (previous) (diff)

Changed 5 weeks ago by haxxpop

comment:13 Changed 5 weeks ago by nickm

  • Resolution set to implemented
  • Status changed from merge_ready to closed

applied; thank you!

Note: See TracTickets for help on using tickets.