Skip to content
This repository was archived by the owner on May 4, 2021. It is now read-only.

Ticket27342: poor error handling (env vars in config is not okay)#244

Closed
pastly wants to merge 2 commits intomasterfrom
ticket27342
Closed

Ticket27342: poor error handling (env vars in config is not okay)#244
pastly wants to merge 2 commits intomasterfrom
ticket27342

Conversation

@pastly
Copy link
Copy Markdown
Member

@pastly pastly commented Aug 28, 2018

I'm embarrassed we even let this happen. Sbws would not start for me. See commit messages.

pastly added 2 commits August 27, 2018 20:02
@pastly
Fix forgotten conf['paths'] -> conf.getpath() change
6c00e2a
@pastly
Stop trying to use environment variables in config
79fdb92 
They don't work with ExtendedInterpolation. I don't know how we ever
thought they were okay.

For example, chainging sbws_home from ~/.sbws to $HOME/.sbws

(venv-test) matt@spacecow:~/src/simple-bw-scanner$ sbws scanner
Traceback (most recent call last):
  File "/home/matt/src/simple-bw-scanner/venv-test/bin/sbws", line 11, in <module>
    sys.exit(main())
  File "/home/matt/src/simple-bw-scanner/venv-test/lib/python3.5/site-packages/sbws/sbws.py", line 46, in main
    _ensure_dirs(conf)
  File "/home/matt/src/simple-bw-scanner/venv-test/lib/python3.5/site-packages/sbws/sbws.py", line 24, in _ensure_dirs
    os.makedirs(conf.getpath('paths', 'datadir'), exist_ok=True)
  File "/usr/lib/python3.5/configparser.py", line 806, in _get_conv
    **kwargs)
  File "/usr/lib/python3.5/configparser.py", line 800, in _get
    return conv(self.get(section, option, **kwargs))
  File "/usr/lib/python3.5/configparser.py", line 797, in get
    d)
  File "/usr/lib/python3.5/configparser.py", line 454, in before_get
    self._interpolate_some(parser, option, L, value, section, defaults, 1)
  File "/usr/lib/python3.5/configparser.py", line 510, in _interpolate_some
    depth + 1)
  File "/usr/lib/python3.5/configparser.py", line 517, in _interpolate_some
    "found: %r" % (rest,))
configparser.InterpolationSyntaxError: '$' must be followed by '$' or '{', found: '$HOME/.sbws'

This commit changes Tor's run_dpath from $XDG_RUNTIME_DIR/sbws to
${tor:datadir}/run (which is ~/.sbws/tor/run by default)
@teor2345
Copy link
Copy Markdown
Contributor

teor2345 commented Aug 28, 2018

How did the tox tests pass with these errors in the config?
Can you update the travis CI to avoid issues like this in future?

@pastly
Copy link
Copy Markdown
Member Author

pastly commented Aug 28, 2018 •
edited
Loading

Yes I think that should be possible and straight forward. I'll add a test that makes sure an environment variable isn't interpolated in the config (less interesting) and a test that tries launching sbws from scratch in a new processes and making sure it runs as expected (more interesting).

@juga0
Copy link
Copy Markdown
Contributor

juga0 commented Aug 28, 2018

Sorry for my part in that bug.
@pastly, i'm still getting the error with this branch. I've not check but i think is missing some other if the dirs/files exist.
The way you change config.default.ini is fine, but would you mind to add something like ``if os.path.expandvars('$XDG_RUNTIME_DIR'): run_dpath = ...``` in the moment when that is added to the config, so we can still use env var when it's set?.
Let me know if you prefer i do that.

@pastly
Copy link
Copy Markdown
Member Author

pastly commented Aug 28, 2018

i'm still getting the error with this branch.

How can I reproduce the error you are seeing?

The way you change config.default.ini is fine, but would you mind to add something like [... code ...] in the moment when that is added to the config, so we can still use env var when it's set?.

I don't want to support this environment variable as a special case.

@juga0
Copy link
Copy Markdown
Contributor

juga0 commented Aug 28, 2018

How can I reproduce the error you are seeing?

rm -rf ~/.sbws
sbws scanner
[error]
ls ~/.sbws
datadir  log  v3bw
git branch -l
* ticket27342

I don't want to support this environment variable as a special case.

That's the default case in system packages (Debian).
A service that runs as a non-root user put run time files in /run/theuser/
A service that runs as a root user put the run time files in /var/run.
tor service put its run time files in /var/run/tor.
Our tor directory should also put their files run time directories.

@juga0
Copy link
Copy Markdown
Contributor

juga0 commented Aug 28, 2018

How can I reproduce the error you are seeing?

oh, sorry, that only happens in a chroot, so it's fine.

@juga0
Copy link
Copy Markdown
Contributor

juga0 commented Aug 28, 2018

I'd improve the message:

[2018-08-28 13:40:45,836] [sbws.globals] [CRITICAL] No enabled destinations in config

To say something like You nee to configure destinations, please create a <thehomepaththatsbwsisusing>/config.ini or specify the location of the config.ini with the the option -c. See man sbws.ini to know how to configure destinations

@pastly
Copy link
Copy Markdown
Member Author

pastly commented Aug 28, 2018

reproduce error.

I get the following, which is correct and expected.

(venv-test) matt@spacecow:~/src/simple-bw-scanner$ sbws scanner
[2018-08-28 09:40:59,911] [sbws.sbws] [INFO] sbws 0.7.1-dev with python 3.5.3 on Linux-4.9.0-8-amd64-x86_64-with-debian-9.5, stem 1.6.0-dev, and requests 2.19.1
[2018-08-28 09:41:31,311] [sbws.util.stem] [INFO] Started and connected to Tor 0.3.3.9 (git-ca1a436fa8e53a32) via /home/matt/.sbws/tor/run/control
[2018-08-28 09:41:36,199] [sbws.globals] [CRITICAL] No enabled destinations in config
[2018-08-28 09:41:36,256] [sbws.lib.resultdump] [WARNING] Results files that are valid not found. Probably sbws scanner was not run first or it ran more than 7 days ago or it was using a different datadir than /home/matt/.sbws/datadir.

Supporting env vars, but correct place in Debian is to put them in [places]

When creating the Debian package, you can include a config file that sets these paths to be the exact right places, right?

@juga0
Copy link
Copy Markdown
Contributor

juga0 commented Aug 28, 2018

When creating the Debian package, you can include a config file that sets these paths to be the exact right places, right?

i do not include any config file in the debian package and that is intentional, since updates should not modify files that the user might modify.

The commit message in 79fdb92 is not correct.

ExtendedInterpolation does support environment variables.

And the traceback says the error:

configparser.InterpolationSyntaxError: '$' must be followed by '$' or '{', found: '$HOME/.sbws'

The only change required is s/$/$$/:

run_dpath = $$XDG_RUNTIME_DIR/sbws

Changing that i get:

[sbws.util.stem] [INFO] Started and connected to Tor 0.2.9.15 (git-2dc1a1a2abab5403) via /run/user/1000/sbws/control

@pastly
Copy link
Copy Markdown
Member Author

pastly commented Aug 28, 2018

That's great news and means I can undo just about everything I did, thanks.

@juga0
Copy link
Copy Markdown
Contributor

juga0 commented Aug 28, 2018

i would keep the original title for this message poor error handling

env vars in config is not okay is not correct

@juga0
Copy link
Copy Markdown
Contributor

juga0 commented Aug 28, 2018

Commit 6c00e2a should remain.

We should still change the info message to say what the user should do

@pastly
Copy link
Copy Markdown
Member Author

pastly commented Aug 28, 2018

We should still change the info message to say what the user should do

See https://trac.torproject.org/projects/tor/ticket/27358 for that.

Closing this in favor of that ticket, plus one I will create for making a test that launches sbws from the command line to hopefully catch something like this in the future.

@pastly pastly closed this Aug 28, 2018
@teor2345 teor2345 deleted the ticket27342 branch September 6, 2018 11:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@pastly @teor2345 @juga0