Opened 6 weeks ago

Closed 5 weeks ago

#32721 closed enhancement (fixed)

Allow chutney users to disable tor's sandbox at runtime

Reported by: teor Owned by: teor
Priority: Medium Milestone:
Component: Core Tor/Chutney Version:
Severity: Normal Keywords: tor-ci, chutney, BugSmashFund
Cc: nickm Actual Points: 0.3
Parent ID: #32240 Points: 1
Reviewer: nickm Sponsor:

Description

In #32240, we discovered that tor's sandbox doesn't work on recent Ubuntu versions. So we need to disable the sandbox on those CI jobs.

One good way to implement this change, is by making it a first-class chutney feature.

Child Tickets

Change History (8)

comment:1 Changed 6 weeks ago by teor

Component: Core Tor/TorCore Tor/Chutney

comment:2 Changed 6 weeks ago by teor

Actual Points: 0.2
Reviewer: nickm
Status: assignedneeds_review

See my PR:

Details:

  • Set the default for Sandbox based on the platform: 1 for Linux, 0 for everything else.
  • Allow the user to override it with --sandbox or CHUTNEY_TOR_SANDBOX.

Assigning nickm as the reviewer, because he's familiar with the details.

comment:3 Changed 6 weeks ago by teor

Keywords: BugSmashFund added

Marking as BugSmashFund, because this change is part of fixing the user-visible bug #32722.

comment:4 Changed 6 weeks ago by nickm

Status: needs_reviewmerge_ready

This looks reasonable to me.

I would maybe suggest considering having it so that the ${sandbox} variable is only 0 or 1, and the torrc includes "Sandbox ${sandbox}". But I'm okay with this design too if you prefer it.

comment:5 Changed 6 weeks ago by teor

Status: merge_readyneeds_revision

Yes, I think that works in this case. For some other variables, we comment out the whole line to use tor's compiled-in default. But we don't need that feature here.

That makes for a much nicer implementation:

  • define getenv_bool()
  • add sandbox to the DEFAULTS dict

comment:6 Changed 6 weeks ago by teor

Actual Points: 0.20.3
Status: needs_revisionneeds_review

I added a fixup to the PR with the getenv_bool()/DEFAULTS/Sandbox ${sandbox} implementation.

comment:7 Changed 6 weeks ago by nickm

Status: needs_reviewmerge_ready

LGTM!

comment:8 Changed 5 weeks ago by teor

Resolution: fixed
Status: merge_readyclosed

Squashed and merged to master with #32731.

Note: See TracTickets for help on using tickets.