Issue 6602 - Introduce watchextensions

Issue 6602 - Introduce watchextensions

Note: this will be pushed into a new repository

[tlucas, 2018-04-26 11:07:05]

Neither the code, nor the list of reviewers / ccs is final.

Yet todo:

 * Actual tests
 * Descriptive comments / documentation (README.md)
 * Decide which server this should run on

[tlucas, 2018-04-26 13:09:55]

On 2018/04/26 11:07:05, tlucas wrote:
> Neither the code, nor the list of reviewers / ccs is final.
> 
> Yet todo:
> 
>  * Actual tests
>  * Descriptive comments / documentation (README.md)
>  * Decide which server this should run on

Since none of our existing repositories is the right place for this, i'd suggest
a new repository called e.g. "watchdog".

[tlucas, 2018-05-07 15:03:34]

Hi guys.

This is my attempt to implement #6602, any comments would be welcome.

I moved you to cc Dave, but you are also more than welcome to provide some
feedback.

Cheers!

[tlucas, 2018-05-08 14:23:35]

Patch Set 3

 * Don't log sensitive data (i.e. auth tokens in repository-url)

[Vasily Kuznetsov, 2018-05-15 16:49:32]

Hi Tristan,

Pretty cool, I like the script. A few questions/points:

- I'm not sure about the value of supporting Python 2 in new code at this point
and it does add quite a bit of hassle as you have probably noticed. Is there a
particular reason you chose to do it?

- It would be better to make it a Python package with setup.py or at least
provide requirements.txt with the list of dependencies ($ pip freeze >
requirements.txt). It might be ok to not have `requirements.txt` if there are no
dependencies, so if we drop Python 2 support, it's probably ok to leave it as
is.

- Regarding repository hosting: https://gitlab.com/eyeo/sandbox/ could be a good
place for a start.

Cheers,
Vasily

https://codereview.adblockplus.org/29762573/diff/29774558/tests/test_ExtWatch.py
File tests/test_ExtWatch.py (right):

https://codereview.adblockplus.org/29762573/diff/29774558/tests/test_ExtWatch...
tests/test_ExtWatch.py:82: return orig_urlopen(*args)
This makes the test hit that API every time it's run and makes it dependent on
the internet in general. It would be better to mock it, unless it's too much
work.

https://codereview.adblockplus.org/29762573/diff/29774558/watchextensions.ini...
File watchextensions.ini.example (right):

https://codereview.adblockplus.org/29762573/diff/29774558/watchextensions.ini...
watchextensions.ini.example:7: [enabled]
Perhaps this is not the best place to discuss this, but [enabled] section seems
redundant. One could always just move the disabled config lines into [disabled]
section, which would be ignored by the extension to achieve the same effect
without having this duplication of config keys.

https://codereview.adblockplus.org/29762573/diff/29774558/watchextensions.py
File watchextensions.py (right):

https://codereview.adblockplus.org/29762573/diff/29774558/watchextensions.py#...
watchextensions.py:19: When the configuration files are incomplete or missing, a
ConfigurationError
Nit: this line is probably not very useful for the end user of the script, it is
still printed with other help though. I'd probably remove it, because for the
developer who works on this the purpose of ConfigurationError will also be
obvious enough.

https://codereview.adblockplus.org/29762573/diff/29774558/watchextensions.py#...
watchextensions.py:43: class SensitiveFilter(logging.Filter):
If I understand it correctly, the purpose of this class is to prevent
security-sensitive data from the repository string in the the config from
getting into the logs. First of all, it takes a bit of time to figure this out
so perhaps it would be good to document the class. Second: could we perhaps do
it with less code somehow. What if we just don't include the full repository
string into error messages, etc. Could this be simpler?

https://codereview.adblockplus.org/29762573/diff/29774558/watchextensions.py#...
watchextensions.py:82: class ConfigurationError(Exception):
Nit: moving this to above `read_config` would make the whole thing read a bit
more naturally.

https://codereview.adblockplus.org/29762573/diff/29774558/watchextensions.py#...
watchextensions.py:113: def _assure_local_repository(self):
This name seems a bit strange. Shouldn't it be something like `_clone_repo`?

https://codereview.adblockplus.org/29762573/diff/29774558/watchextensions.py#...
watchextensions.py:154: omaha_url =
'https://omahaproxy.appspot.com/all.json?os=win'
Maybe this should be a constant somewhere at the top of the file?

https://codereview.adblockplus.org/29762573/diff/29774558/watchextensions.py#...
watchextensions.py:164: if not self._cws_ext_url:
You could just slap a `functools.lru_cache` decorator on this instead of caching
it yourself... but, Python 2 doesn't have it :(

https://codereview.adblockplus.org/29762573/diff/29774558/watchextensions.py#...
watchextensions.py:252: logging.error(e.message)
Perhaps `sys.exit(e.message)` would be more appropriate. In any case it would
make sense to return a non-zero exit code.

[tlucas, 2018-05-16 09:52:38]

Patch Set 4
 * Addressing Vasily's comments

Hey, thanks for your comments!

On 2018/05/15 16:49:32, Vasily Kuznetsov wrote:
> Hi Tristan,
> 
> Pretty cool, I like the script. A few questions/points:
> 
> - I'm not sure about the value of supporting Python 2 in new code at this
point
> and it does add quite a bit of hassle as you have probably noticed. Is there a
> particular reason you chose to do it?
> 
> - It would be better to make it a Python package with setup.py or at least
> provide requirements.txt with the list of dependencies ($ pip freeze >
> requirements.txt). It might be ok to not have `requirements.txt` if there are
no
> dependencies, so if we drop Python 2 support, it's probably ok to leave it as
> is.

I'm not sure if we will actually agree on dropping Python 2 support - But FWIW i
explicitly mentioned the additional dependency for Python 2 in the README.md
(just in case you missed that). Maybe Dave and Sebastian have an opinion on
Python 2 here?

> 
> - Regarding repository hosting: https://gitlab.com/eyeo/sandbox/ could be a
good
> place for a start.

Good point, will do
> 
> Cheers,
> Vasily

https://codereview.adblockplus.org/29762573/diff/29774558/tests/test_ExtWatch.py
File tests/test_ExtWatch.py (right):

https://codereview.adblockplus.org/29762573/diff/29774558/tests/test_ExtWatch...
tests/test_ExtWatch.py:82: return orig_urlopen(*args)
On 2018/05/15 16:49:31, Vasily Kuznetsov wrote:
> This makes the test hit that API every time it's run and makes it dependent on
> the internet in general. It would be better to mock it, unless it's too much
> work.

No, you are right. I'm also mocking this now.

https://codereview.adblockplus.org/29762573/diff/29774558/watchextensions.ini...
File watchextensions.ini.example (right):

https://codereview.adblockplus.org/29762573/diff/29774558/watchextensions.ini...
watchextensions.ini.example:7: [enabled]
On 2018/05/15 16:49:31, Vasily Kuznetsov wrote:
> Perhaps this is not the best place to discuss this, but [enabled] section
seems
> redundant. One could always just move the disabled config lines into
[disabled]
> section, which would be ignored by the extension to achieve the same effect
> without having this duplication of config keys.

The purpose of the is two fold - a) to actually mark something as enabled (for
this, introducing a [disabled]-section would be perfectly valid too) b) to have
an easier entry point for us to generate the keys we are actually looking in
[extensions]. The latter is why i would want to keep this.

https://codereview.adblockplus.org/29762573/diff/29774558/watchextensions.py
File watchextensions.py (right):

https://codereview.adblockplus.org/29762573/diff/29774558/watchextensions.py#...
watchextensions.py:19: When the configuration files are incomplete or missing, a
ConfigurationError
On 2018/05/15 16:49:31, Vasily Kuznetsov wrote:
> Nit: this line is probably not very useful for the end user of the script, it
is
> still printed with other help though. I'd probably remove it, because for the
> developer who works on this the purpose of ConfigurationError will also be
> obvious enough.

Acknowledged.

https://codereview.adblockplus.org/29762573/diff/29774558/watchextensions.py#...
watchextensions.py:43: class SensitiveFilter(logging.Filter):
On 2018/05/15 16:49:32, Vasily Kuznetsov wrote:
> If I understand it correctly, the purpose of this class is to prevent
> security-sensitive data from the repository string in the the config from
> getting into the logs. First of all, it takes a bit of time to figure this out
> so perhaps it would be good to document the class. Second: could we perhaps do
> it with less code somehow. What if we just don't include the full repository
> string into error messages, etc. Could this be simpler?

You are right about the purpose - I added a documentation for this class.

However, the amount of code is - from what i see - as minimal as it can be (FWIW
i could get rid of self.mask() and put the logic in self.filter(), but that's
about it i guess - if we don't want to lose ALL information, that is.)

https://codereview.adblockplus.org/29762573/diff/29774558/watchextensions.py#...
watchextensions.py:82: class ConfigurationError(Exception):
On 2018/05/15 16:49:31, Vasily Kuznetsov wrote:
> Nit: moving this to above `read_config` would make the whole thing read a bit
> more naturally. 

Done.

https://codereview.adblockplus.org/29762573/diff/29774558/watchextensions.py#...
watchextensions.py:113: def _assure_local_repository(self):
On 2018/05/15 16:49:31, Vasily Kuznetsov wrote:
> This name seems a bit strange. Shouldn't it be something like `_clone_repo`?

You are right. "_assure_local_repository" is an artifact from when the script
was able to use existing repositories (and had to make sure that that repository
is actually what we wanted)

Renamed it.

https://codereview.adblockplus.org/29762573/diff/29774558/watchextensions.py#...
watchextensions.py:154: omaha_url =
'https://omahaproxy.appspot.com/all.json?os=win'
On 2018/05/15 16:49:31, Vasily Kuznetsov wrote:
> Maybe this should be a constant somewhere at the top of the file?

Done.

https://codereview.adblockplus.org/29762573/diff/29774558/watchextensions.py#...
watchextensions.py:164: if not self._cws_ext_url:
On 2018/05/15 16:49:31, Vasily Kuznetsov wrote:
> You could just slap a `functools.lru_cache` decorator on this instead of
caching
> it yourself... but, Python 2 doesn't have it :(

I'll use this comment to also reply to what you asked for in your mail's body:

No, there is no particular reason (from my side) to support both versions, i'm
merely following our coding-style for python.
I guess until Python 2.7's end of life this won't change, so FWIW this is / was
also a good practice to assure compatibility.

https://codereview.adblockplus.org/29762573/diff/29774558/watchextensions.py#...
watchextensions.py:252: logging.error(e.message)
On 2018/05/15 16:49:31, Vasily Kuznetsov wrote:
> Perhaps `sys.exit(e.message)` would be more appropriate. In any case it would
> make sense to return a non-zero exit code.

Done.

[Vasily Kuznetsov, 2018-05-16 17:54:55]

Hi Tristan,

Thanks for addressing my comments -- looks good.

I think we should still think if we really want to support Python 2 with this
code, and if we decide that we don't, some things could be modernized and
simplified. I don't really see much going for Python 2 support, but I definitely
don't mind if there are good reasons to do so. Sebastian, Dave: what do you
think?

Cheers,
Vasily

P.S. You can go to https://pythonclock.org/ and meditate on it a bit to start
agreeing more with me about Python 2 ;)

https://codereview.adblockplus.org/29762573/diff/29774558/watchextensions.py
File watchextensions.py (right):

https://codereview.adblockplus.org/29762573/diff/29774558/watchextensions.py#...
watchextensions.py:43: class SensitiveFilter(logging.Filter):
On 2018/05/16 09:52:38, tlucas wrote:
> On 2018/05/15 16:49:32, Vasily Kuznetsov wrote:
> > If I understand it correctly, the purpose of this class is to prevent
> > security-sensitive data from the repository string in the the config from
> > getting into the logs. First of all, it takes a bit of time to figure this
out
> > so perhaps it would be good to document the class. Second: could we perhaps
do
> > it with less code somehow. What if we just don't include the full repository
> > string into error messages, etc. Could this be simpler?
> 
> You are right about the purpose - I added a documentation for this class.
> 
> However, the amount of code is - from what i see - as minimal as it can be
(FWIW
> i could get rid of self.mask() and put the logic in self.filter(), but that's
> about it i guess - if we don't want to lose ALL information, that is.)

I was thinking about avoiding this filter class altogether and making sure that
nothing sensitive ends up in the log by just not writing it into the log in the
first place. I'm not sure if this would be easier, but I trust that you can
figure it out :)

In any case, as it is now, the code is easy enough to follow so it's not a big
deal.

[tlucas, 2018-05-18 08:19:41]

On 2018/05/16 17:54:55, Vasily Kuznetsov wrote:
> Hi Tristan,
> 
> Thanks for addressing my comments -- looks good.
> 
> I think we should still think if we really want to support Python 2 with this
> code, and if we decide that we don't, some things could be modernized and
> simplified. I don't really see much going for Python 2 support, but I
definitely
> don't mind if there are good reasons to do so. Sebastian, Dave: what do you
> think?
> 
> Cheers,
> Vasily
> 
> P.S. You can go to https://pythonclock.org/ and meditate on it a bit to start
> agreeing more with me about Python 2 ;)
> 
> https://codereview.adblockplus.org/29762573/diff/29774558/watchextensions.py
> File watchextensions.py (right):
> 
>
https://codereview.adblockplus.org/29762573/diff/29774558/watchextensions.py#...
> watchextensions.py:43: class SensitiveFilter(logging.Filter):
> On 2018/05/16 09:52:38, tlucas wrote:
> > On 2018/05/15 16:49:32, Vasily Kuznetsov wrote:
> > > If I understand it correctly, the purpose of this class is to prevent
> > > security-sensitive data from the repository string in the the config from
> > > getting into the logs. First of all, it takes a bit of time to figure this
> out
> > > so perhaps it would be good to document the class. Second: could we
perhaps
> do
> > > it with less code somehow. What if we just don't include the full
repository
> > > string into error messages, etc. Could this be simpler?
> > 
> > You are right about the purpose - I added a documentation for this class.
> > 
> > However, the amount of code is - from what i see - as minimal as it can be
> (FWIW
> > i could get rid of self.mask() and put the logic in self.filter(), but
that's
> > about it i guess - if we don't want to lose ALL information, that is.)
> 
> I was thinking about avoiding this filter class altogether and making sure
that
> nothing sensitive ends up in the log by just not writing it into the log in
the
> first place. I'm not sure if this would be easier, but I trust that you can
> figure it out :)
> 
> In any case, as it is now, the code is easy enough to follow so it's not a big
> deal.

I guess the only other entry point would be "subprocess" not printing it's
command the stderr on failure. And TBH i once tried to filter stuff there, but
IMHO it was not feasible if we still want to get some sort of idea what went
wrong - so if nobody minds, i'll stick to the current implementation :)

Let's see where this go about Python 2 / 3 though.