1209625 - MozReview API Keys should use a more specific error message

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: API, defect)

Description

[:Gijs (he/him)]

log for context:

[13:20:37]	Gijs	glob: bzexport gives me abort: REST error on PUT to https://bugzilla.mozilla.org/rest/bug/1209611: The requested method 'Bug.update' was not found.
[13:20:44]	Gijs	glob: when attaching patches to a bug. Known issue?
[13:21:36]	glob	Gijs, first i've heard of it. does it always happen?
[13:21:42]	Gijs	glob: yes
[13:23:36]	glob	Gijs, according to the code that's impossible
[13:29:49]	Gijs	glob: I mean... I normally like challenging myself to do things that people say are impossible (or just hard)... but let's say this is an exception?
[13:30:02]	Gijs	I dunno if I can make bzexport log more info or something
[13:32:09]	glob	Gijs, i don't think more logs from bzexport would help here.. it's an internal bugzilla error (it's saying that a method on the bug object doesn't exist, when it totally does)
[13:32:17]	glob	can you file a bug (i'm about to log out for the night)
[13:32:23]	Gijs	glob: OK!
[13:32:30]	glob	i suspect we'll have to get httpd restarted or something

Comment 1

[:glob ✱]

fubar - could you do a non-graceful restart of httpd on the webheads please?

Comment 2

[Kendall Libby [:fubar] (he/him)]

SCL3 webheads are done, PHX1 is going now.

Comment 3

[David Lawrence [:dkl]]

This is a test change - disregard

Comment 4

[David Lawrence [:dkl]]

Seems to be working again due to the restart. Closing.

Comment 5

[:Gijs (he/him)]

I am seeing this again/still.

Comment 6

[:glob ✱]

the logs show a SIGHUP was sent to apache (across all webheads):

03:43:04 #3 [notice] SIGHUP received.  Attempting to restart
03:43:05 #3 [warn] WARNING: HOME is not set, using root: /\n
03:43:06 #3 [warn] NameVirtualHost *:80 has no VirtualHosts
03:43:06 #3 [notice] Digest: generating secret for digest authentication ...
03:43:06 #3 [notice] Digest: done
03:43:09 #3 [notice] Apache/2.2.15 (Unix) DAV/2 mod_perl/2.0.4 Perl/v5.10.1 configured -- resuming normal operations

looks suspect.

Comment 7

[David Lawrence [:dkl]]

fubar - could you do a non-graceful restart of httpd on the webheads please? Also do you know what the SIGUP is about that glob saw in the logs?

Comment 8

[Kendall Libby [:fubar] (he/him)]

restarting httpd now. will look into the SIGHUP, but it's probably puppet related.

Comment 9

[Kendall Libby [:fubar] (he/him)]

ah, it's the nightly apache log rotation, not puppet.

Comment 10

[:Gijs (he/him)]

Just saw this again:

abort: REST error on PUT to https://bugzilla.mozilla.org/rest/bug/1215333: The requested method 'Bug.update' was not found.

:-(

Comment 11

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1209625_1.patch

I suspect this is where the error is coming from. This warn should make it show up in sentry, correct? It would be good if this could go out this week.

Comment 12

[David Lawrence [:dkl]]

Comment on attachment 8675892 [details] [diff] [review]
1209625_1.patch

Review of attachment 8675892 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

Comment 13

[Dylan Hardison [:dylan] (he/him)]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   0ce40b9..7463311  master -> master

Leaving bug open to track info we get from the added debugging.

Comment 14

[Dylan Hardison [:dylan] (he/him)]

Gijs:

When this happens again, if you can log the response header X-Backend-Server, I should be able to get more information on why unknown method is being returned.

Comment 15

[:Gijs (he/him)]

(In reply to Dylan William Hardison [:dylan] from comment #14)
> Gijs:
> 
> When this happens again, if you can log the response header
> X-Backend-Server, I should be able to get more information on why unknown
> method is being returned.

web3.bugs.scl3.mozilla.com

and

web1.bugs.scl3.mozilla.com

Both give me this error. (I tried a few times on bug 1217517)

Comment 16

[Dylan Hardison [:dylan] (he/him)]

I'm still not getting the diagnostic information I need from the logs (working on that in bug 1216300).
I should have something in place by next push to figure out *why* this is happening.

Comment 17

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1209625_3.patch

Replace warn() (which seems to go nowhere because of Sentry?) with a direct logging to syslog.

Comment 18

[David Lawrence [:dkl]]

Comment on attachment 8682204 [details] [diff] [review]
1209625_3.patch

Review of attachment 8682204 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Not setup to log locally so put this patch on bugzilla-dev and if it works there, then commit. r=dkl

::: Bugzilla/WebService/Server.pm
@@ +26,5 @@
>  
>  use Storable qw(freeze);
>  
> +use Sys::Syslog qw(:DEFAULT);
> +use Encode;

use Encode qw(encode_utf8);

nit: maybe bring all together and sort alphabetically.

use Digest::MD5 qw(md5_base64);
use Encode qw(encode_utf8);
use Scalar::Util qw(blessed);
use Storable qw(freeze);
use Sys::Syslog qw(:DEFAULT);

Comment 19

[Dylan Hardison [:dylan] (he/him)]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   2776e92..7f43eeb  master -> master

Comment 20

[:Gijs (he/him)]

The patch that landed was just more logging, right? I guess this should stay open until we figure out what causes it and fix it...

Comment 21

[Dylan Hardison [:dylan] (he/him)]

Force of habit. Once this is pushed, it will just be a matter of stalking syslog for [rpc_error].

Comment 22

[:Gijs (he/him)]

(In reply to Dylan William Hardison [:dylan] from comment #21)
> Force of habit. Once this is pushed, it will just be a matter of stalking
> syslog for [rpc_error].

Just saw this again in bug 1223210. Sadly don't have the server name for you because this instance of bzexport doesn't have the logging required to get the server name out of it (that's on my other machine).

Comment 23

[Dylan Hardison [:dylan] (he/him)]

I have found the problem!

There was nothing in the log, as the unknown_method I was thought was the problem was not the problem.
In bug 1184332 we limited api keys issued to mozreview to specific methods. The API key you're using is a mozreview one.
You should create a new api key for bzexport. This should work, although there is an open bug that I'm investigating still.

For this bug, I'm going to improve the error message (unknown_method is a dirty lie)
and I'm going to also suggest we hide the api key itself for mozreview api keys. e.g. replace the bytes with '*'
to dissuade others from this problem. :-)

Let me know if using a fresh api key for bzexport works, and I'll get this error message fixed up.

Comment 24

[:Gijs (he/him)]

(In reply to Dylan William Hardison [:dylan] from comment #23)
> I have found the problem!
> 
> There was nothing in the log, as the unknown_method I was thought was the
> problem was not the problem.
> In bug 1184332 we limited api keys issued to mozreview to specific methods.
> The API key you're using is a mozreview one.
> You should create a new api key for bzexport. This should work, although
> there is an open bug that I'm investigating still.
> 
> For this bug, I'm going to improve the error message (unknown_method is a
> dirty lie)
> and I'm going to also suggest we hide the api key itself for mozreview api
> keys. e.g. replace the bytes with '*'
> to dissuade others from this problem. :-)
> 
> Let me know if using a fresh api key for bzexport works, and I'll get this
> error message fixed up.

I'm confused. There's only one bugzilla API key in my hgrc, which is the one from mozreview - but presumably that is also the one that mozreview then normally uses? Can I just remove that one or will that break mozreview-on-the-web talking to bugzilla, or something?

Comment 25

[:glob ✱]

(In reply to :Gijs Kruitbosch from comment #24)
> I'm confused. There's only one bugzilla API key in my hgrc, which is the one
> from mozreview - but presumably that is also the one that mozreview then
> normally uses? Can I just remove that one or will that break
> mozreview-on-the-web talking to bugzilla, or something?

that's the problem: the api-key in your hgrc _shouldn't_ be the one created by mozreview.  that's a special key with limited access to methods.

you should create a new api-key in bugzilla just for mercurial and replace your hgrc's bugzilla.apikey.

Comment 26

[:Gijs (he/him)]

(In reply to Byron Jones ‹:glob› from comment #25)
> (In reply to :Gijs Kruitbosch from comment #24)
> > I'm confused. There's only one bugzilla API key in my hgrc, which is the one
> > from mozreview - but presumably that is also the one that mozreview then
> > normally uses? Can I just remove that one or will that break
> > mozreview-on-the-web talking to bugzilla, or something?
> 
> that's the problem: the api-key in your hgrc _shouldn't_ be the one created
> by mozreview.  that's a special key with limited access to methods.
> 
> you should create a new api-key in bugzilla just for mercurial and replace
> your hgrc's bugzilla.apikey.

Then we should update http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/install.html to make that clearer.

Comment 27

[Dylan Hardison [:dylan] (he/him)]

(In reply to :Gijs Kruitbosch from comment #26)
> Then we should update
> http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/
> install.html to make that clearer.

Yep, and I will make the api key page hide the mozreview keys so this confusion can't arise. I'm really sorry it took so long to figure it out.

Comment 28

[:Gijs (he/him)]

(In reply to Dylan William Hardison [:dylan] from comment #27)
> (In reply to :Gijs Kruitbosch from comment #26)
> > Then we should update
> > http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/
> > install.html to make that clearer.
> 
> Yep, and I will make the api key page hide the mozreview keys so this
> confusion can't arise. I'm really sorry it took so long to figure it out.

Hidden API keys sound very very dodgy. There would be no way for me to revoke that key and make mozreview generate a new one instead. Can we keep them, just make it more obvious that they are limited keys in some way, or that they ought not to be used for other things?

Comment 29

[Ed Morley [:emorley]]

That's what was proposed in comment 23 (replacing the key secret with "*" etc).

Comment 30

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1209625_4.patch

fixing the problem finally

Comment 31

[David Lawrence [:dkl]]

Comment on attachment 8686201 [details] [diff] [review]
1209625_4.patch

Review of attachment 8686201 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

Comment 32

[Dylan Hardison [:dylan] (he/him)]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   771547c..2c3a839  master -> master