Bug 39136 – sheriffbot can't roll out security patches

• Component: Tools / Tests
• Status: REOPENED

[Eric Seidel (no email)]

sheriffbot can't roll out security patches

When it tries, all we get is:
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'create-rollout', '--force-clean', '--parent-command=sheriff-bot', REVISION, 'REASON (Requested by abarth on #webkit).']" exit_code: 1

Either we should giver sheriffbot access, or have him do something smarter.

[Daniel Bates]

Created attachment 67318 [details]
Patch with unit tests

Patch to make Sheriffbot and webkit-patch print human-readable error messages when the person (or bot) cannot view a bug.

I'm open to naming suggestions.

[Daniel Bates]

I should note that for the unit tests included in this patch, I explicitly tested the exception as there does not seem to be analogous unitTest.TestCase.assertRaises() that can be used as a context manager in Python < 2.7. I am open to suggestions.

[Adam Barth]

Comment on attachment 67318 [details]
Patch with unit tests

View in context: https://bugs.webkit.org/attachment.cgi?id=67318&action=prettypatch

> WebKitTools/Scripts/webkitpy/common/net/bugzilla.py:516
> +        bug_element = soup.find("bug")
> +        if bug_element.has_key("error"):
> +            raise BugzillaError(bug_element["error"])
Nice.

> WebKitTools/Scripts/webkitpy/tool/multicommandtool.py:320
> +            except BugzillaError, e:
> +                if e.is_invalid_bug_id():
> +                    log("Invalid bug number.")
> +                if e.does_bug_exist():
> +                    log("The bug does not exist.")
> +                elif e.is_not_permitted_to_view_bug():
> +                    log("You are not authorized to view this bug.")
> +                sys.exit(1)
This seems like a very high level to catch this error.  Also, exit(1) is pretty severe...  Would it make sense to move this exception block lower down to where we actually fetch the bugs?  That might give us more context about what to do.

> WebKitTools/Scripts/webkitpy/tool/bot/sheriff.py:71
> +            if (e.is_not_permitted_to_view_bug()):
> +                raise ScriptError(message="SheriffBot is not authorized to rollout a security bug.")
Instead of raising a ScriptError, should we just send a message to IRC?  I see that in other places in this file we raise ScriptErrors, so maybe this is the best thing...

[Daniel Bates]

(In reply to comment #3)
> > WebKitTools/Scripts/webkitpy/tool/multicommandtool.py:320
> > +            except BugzillaError, e:
> > +                if e.is_invalid_bug_id():
> > +                    log("Invalid bug number.")
> > +                if e.does_bug_exist():
> > +                    log("The bug does not exist.")
> > +                elif e.is_not_permitted_to_view_bug():
> > +                    log("You are not authorized to view this bug.")
> > +                sys.exit(1)
> This seems like a very high level to catch this error.  Also, exit(1) is pretty severe...  Would it make sense to move this exception block lower down to where we actually fetch the bugs?  That might give us more context about what to do.

Will look into this.

> 
> > WebKitTools/Scripts/webkitpy/tool/bot/sheriff.py:71
> > +            if (e.is_not_permitted_to_view_bug()):
> > +                raise ScriptError(message="SheriffBot is not authorized to rollout a security bug.")
> Instead of raising a ScriptError, should we just send a message to IRC?  I see that in other places in this file we raise ScriptErrors, so maybe this is the best thing...

I was not familiar with how to send a message to IRC directly. As you notice, other places seem to use this approach. I'll look into this as well.

[Daniel Bates]

(In reply to comment #4)
> > > WebKitTools/Scripts/webkitpy/tool/bot/sheriff.py:71
> > > +            if (e.is_not_permitted_to_view_bug()):
> > > +                raise ScriptError(message="SheriffBot is not authorized to rollout a security bug.")
> > Instead of raising a ScriptError, should we just send a message to IRC?  I see that in other places in this file we raise ScriptErrors, so maybe this is the best thing...
> 
> I was not familiar with how to send a message to IRC directly. As you notice, other places seem to use this approach. I'll look into this as well.

The Rollout class (in irc_command.py), which implements the SheriffBot rollout command, has the necessary infrastructure to catch a ScriptError exception and post its message to IRC. You seem to be implying that we should explicitly post an IRC message and return with value None (or some other error code) and modify the caller Rollout.execute() to handle this instead of raising an exception in Sheriff.post_rollout_patch()  with the error message. I'm unclear of the advantages of this approach as I would envision using a similar error message prefix of "...: Failed to create rollout patch: ..." for the Bugzilla error message (for consistency with the other failed rollout error messages). Can you elaborate on you how you envision this to work?

[Daniel Bates]

Created attachment 67340 [details]
Patch with unit tests

Updated patch based on Adam Barth's suggestions.

[WebKit Review Bot]

Attachment 67340 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/Scripts/webkitpy/tool/commands/download.py:166:  at least two spaces before inline comment  [pep8/E261] [5]
WebKitTools/Scripts/webkitpy/tool/commands/download.py:179:  at least two spaces before inline comment  [pep8/E261] [5]
WebKitTools/Scripts/webkitpy/common/net/bugzilla.py:47:  expected 2 blank lines, found 1  [pep8/E302] [5]
WebKitTools/Scripts/webkitpy/common/net/bugzilla.py:517:  .has_key() is deprecated, use 'in'  [pep8/W601] [5]
Total errors found: 4 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

> The Rollout class (in irc_command.py), which implements the SheriffBot rollout command, has the necessary infrastructure to catch a ScriptError exception and post its message to IRC.

Yeah.  It just looks kind of ugly.  I guess we could make it less ugly instead.

[Adam Barth]

Comment on attachment 67340 [details]
Patch with unit tests

View in context: https://bugs.webkit.org/attachment.cgi?id=67340&action=prettypatch

> WebKitTools/Scripts/webkitpy/tool/bot/sheriff.py:73
> +            elif (e.is_not_permitted_to_view_bug()):
> +                ScriptError(message="SheriffBot is not authorized to rollout a security bug.")
It's kind of too bad that we'll announce to the whole channel that this patch fixes a security bug, but I guess we're kind of doing that already...  Maybe something like "SheriffBot is not authorized to view https://bugs.webkit.org/show_bug.cgi?...."

> WebKitTools/Scripts/webkitpy/tool/commands/download.py:166
>  class ProcessAttachmentsMixin(object):
>      def _fetch_list_of_patches_to_process(self, options, args, tool):
> -        return map(lambda patch_id: tool.bugs.fetch_attachment(patch_id), args)
> +        all_patches = []
> +        for patch_id in args:
> +            try:
> +                all_patches.append(tool.bugs.fetch_attachment(patch_id))
> +            except BugzillaError, e:
> +                continue # Ignore this patch.
Can we print a message so the user knows that one of the attachments was skipped?  Also, this behavior seems wrong when used by the bots because the command will appear to succeed when, in fact, it doesn't do anything...  Maybe check for options.non_interactive and error out?

> WebKitTools/Scripts/webkitpy/tool/commands/download.py:179
>  class ProcessBugsMixin(object):
>      def _fetch_list_of_patches_to_process(self, options, args, tool):
>          all_patches = []
>          for bug_id in args:
> -            patches = tool.bugs.fetch_bug(bug_id).reviewed_patches()
> -            log("%s found on bug %s." % (pluralize("reviewed patch", len(patches)), bug_id))
> -            all_patches += patches
> +            try:
> +                patches = tool.bugs.fetch_bug(bug_id).reviewed_patches()
> +                log("%s found on bug %s." % (pluralize("reviewed patch", len(patches)), bug_id))
> +                all_patches += patches
> +            except BugzillaError, e:
> +                continue # Ignore the patches for this bug.
Similar comments here.

> WebKitTools/Scripts/webkitpy/tool/commands/upload.py:91
> +                # This may occur if the bug becomes a security bug between the time
> +                # we fetched its ID from the pending commit list and now. We ignore
> +                # this bug.
> +                continue
Again, I'd print a message here to explain what just happened.

> WebKitTools/Scripts/webkitpy/tool/steps/obsoletepatches.py:56
> +        try:
> +            patches = self._tool.bugs.fetch_bug(bug_id).patches()
> +        except BugzillaError, e:
> +            if (e.is_invalid_bug_id()):
> +                log("Bug %s is invalid." % bug_id)
> +            elif (e.is_not_found()):
> +                log("Bug %s does not exist." % bug_id)
> +            elif (e.is_not_permitted_to_view_bug()):
> +                log("You're not authorized to modify bug %s." % bug_id)
> +            return
This code seems copy/pasted three times.  Is there some way we could share code between these instances?

> WebKitTools/Scripts/webkitpy/tool/steps/preparechangelog.py:59
> +        except BugzillaError, e:
> +            # We may not be authorized to retrieve the bug title if it's for
> +            # a security bug. Therefore, we leave the change logs unchanged.
> +            return
Again, we should tell the user what happened.  I think we need to error out in this case.  We can't really proceed with the commands that use this step without a ChangeLog.  We use the ChangeLog to store important state information (like the bug id).

[Daniel Bates]

Created attachment 67642 [details]
Patch with unit tests

Changed to raise an exception in ProcessAttachmentsMixin, and ProcessBugsMixin if we are running in non-interactive mode.
Added comment in PrepareChangeLog and just let the BugzillaError exception bubble instead of explicitly catching it.

[WebKit Review Bot]

Attachment 67642 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/Scripts/webkitpy/common/net/bugzilla.py:535:  .has_key() is deprecated, use 'in'  [pep8/W601] [5]
WebKitTools/Scripts/webkitpy/tool/steps/preparechangelog.py:58:  at least two spaces before inline comment  [pep8/E261] [5]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Created attachment 68336 [details]
Patch with unit test

Updated patch as <https://bugs.webkit.org/attachment.cgi?id=67642> became stale.

Also fixed style issue: "WebKitTools/Scripts/webkitpy/tool/steps/preparechangelog.py:58:  at least two spaces before inline comment"
After talking with Eric, decided to use has_key as the BeautifulSoup element object is not a native dictionary.

[WebKit Review Bot]

Attachment 68336 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/Scripts/webkitpy/common/net/bugzilla.py:548:  .has_key() is deprecated, use 'in'  [pep8/W601] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 68336 [details]
Patch with unit test

View in context: https://bugs.webkit.org/attachment.cgi?id=68336&action=review

> WebKitTools/Scripts/webkitpy/tool/bot/sheriff.py:73
> +                ScriptError(message="SheriffBot is not authorized to view %s." % self._tool.bugs.bug_url_for_bug_id(e.bug_id()))
> +            else:
> +                ScriptError(message="Could not determine the corresponding bug for r%s." % svn_revision)

Did you want to raise these errors?

> WebKitTools/Scripts/webkitpy/tool/commands/download.py:167
> +                    raise e

You just want to say "raise" without giving the exception again.  That will make the stack trace better.

> WebKitTools/Scripts/webkitpy/tool/commands/download.py:183
> +                    raise e

ditto

[Daniel Bates]

(In reply to comment #14)
> (From update of attachment 68336 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68336&action=review
> 
> > WebKitTools/Scripts/webkitpy/tool/bot/sheriff.py:73
> > +                ScriptError(message="SheriffBot is not authorized to view %s." % self._tool.bugs.bug_url_for_bug_id(e.bug_id()))
> > +            else:
> > +                ScriptError(message="Could not determine the corresponding bug for r%s." % svn_revision)
> 
> Did you want to raise these errors?

Yes. I'll fix this before I land.

> 
> > WebKitTools/Scripts/webkitpy/tool/commands/download.py:167
> > +                    raise e
> 
> You just want to say "raise" without giving the exception again.  That will make the stack trace better.

Will change.

> 
> > WebKitTools/Scripts/webkitpy/tool/commands/download.py:183
> > +                    raise e
> 
> ditto

Will change.

[Daniel Bates]

Committed r68493: <http://trac.webkit.org/changeset/68493>

[Daniel Bates]

Unfortunately, this change regressed Sheriffbot's rollout feature. With Adam's help, we discovered that this change caused Sheriffbot to raise an exception in bugzilla.py:

abarth: File "/mnt/git/webkit-sheriff-bot/WebKitTools/Scripts/webkitpy/common/net/bugzilla.py", line 547, in _parse_bug_page
[11:50pm] renato joined the chat room.
[11:50pm] abarth: bug_id = soup.find("bug_id").string
[11:50pm] abarth: AttributeError: 'NoneType' object has no attribute 'string'

Reverted changeset 68493 <http://trac.webkit.org/changeset/68493> in changeset 68496 <http://trac.webkit.org/changeset/68496>.

[Daniel Bates]

Comment on attachment 68336 [details]
Patch with unit test

Clearing review-queue and commit-queue flags.