Closed Bug 1330253 Opened 7 years ago Closed 7 years ago

Supply a Google API key on try to enable testing against the Safe Browsing service

Categories

(Release Engineering :: General, defect)

defect
Not set
normal

Tracking

(firefox55 fixed)

RESOLVED FIXED
Tracking Status
firefox55 --- fixed

People

(Reporter: hchang, Assigned: dimi)

References

Details

(Whiteboard: #sbv4-m6 [google-api-safe-browsing])

Attachments

(2 files)

Test case "test_safe_browsing_initial_download.py" would make real 
request to google to download safebrowsing black lists. Everything
is fine until we introduce v4 black lists. Google safebrowsing server
requires valid API key to download v4 black lists but try servers (at least)
are not deployed valid google API key.

We need to figure out if the try servers could have google api keys.
Furthermore, could other CI machines have google api keys?

To sum up, without google api keys deployed on the CI machine, 
test_safe_browsing_initial_download.py cannot be run with v4 case.
Summary: Do CI machines need to be deployed valid google API keys → Do CI machines need to be deployed valid google API keys?
Henrik, is there a way we can provide a Google API key (ideally a different one from the production key) on the machines that run our tests?
Component: Safe Browsing → Firefox UI Tests
Flags: needinfo?(hskupin)
Product: Toolkit → Testing
QA Contact: hskupin
So the tests we run in Mozmill CI are all branded builds which include Nightly, Beta, and Releases. Don't those have the correct Google API key already included? 

I think the problem we have here are CI builds in Taskcluster/Buildbot, right?
Component: Firefox UI Tests → Infrastructure
Flags: needinfo?(hskupin)
Product: Testing → Mozilla QA
I think Henry was talking about Try.
Flags: needinfo?(hchang)
I have only confirmed that try servers don't have google api key. Not sure if other servers which might run test cases (e.g. mi, mc, ... Are those servers also called try?) have the api key.
Flags: needinfo?(hchang)
Ah, I see. So this is Releng or taskcluster land. I don't have that info for you, but I will CC some folks who might have more details.
Component: Infrastructure → General Automation
Product: Mozilla QA → Release Engineering
QA Contact: hskupin → catlee
This key is deliberately not available for try, but is available for level 2 (project) and level 3 (integration/release) branches.

We allow just about anyone to have try access, so AIUI we want to restrict dissemination of that key.

So this particular test can not run on try builds.
If we were to get a separate API key from Google, could we make it available on try?
Flags: needinfo?(dustin)
It's certainly technically possible, yes.  I don't know about the ramifications of doing so (I just implemented the existing secrets mechanism - https://bugzilla.mozilla.org/show_bug.cgi?id=1231320).  We would just need to set that new key in the secret and, I think, change the min_scm_level in-tree.
Flags: needinfo?(dustin)
Summary: Do CI machines need to be deployed valid google API keys? → Supply a Google API key on try to enable testing against the Safe Browsing service
Assignee: nobody → francois
Status: NEW → ASSIGNED
I changed the mozharness config as instructed by Dustin and re-enabled tests that Henry disabled in bug 1305486.

Looking at the try results:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=488df3337823053a1d20fea150b77a9a916dee3d

It looks like the remote tests aren't working but that might be due to other problems because it's failing on badbinurl not on the new V4 lists.
It's the same error if you look at the raw dump. We can always just push a commit which removes the google api key replacement to verify it.

[task 2017-01-13T22:48:21.743710Z] 22:48:21     INFO - TEST-UNEXPECTED-FAIL | test_safe_browsing_initial_download.py TestSafeBrowsingInitialDownload.test_safe_browsing_initial_download | AssertionError: Items in the first set but not the second:
[task 2017-01-13T22:48:21.745624Z] 22:48:21     INFO - 'goog-phish-proto.metadata'
[task 2017-01-13T22:48:21.746581Z] 22:48:21     INFO - 'goog-malware-proto.metadata'
[task 2017-01-13T22:48:21.747559Z] 22:48:21     INFO - 'goog-malware-proto.pset'
[task 2017-01-13T22:48:21.748511Z] 22:48:21     INFO - 'goog-unwanted-proto.metadata'
[task 2017-01-13T22:48:21.749454Z] 22:48:21     INFO - 'goog-unwanted-proto.pset'
[task 2017-01-13T22:48:21.750441Z] 22:48:21     INFO - 'goog-phish-proto.pset'
[task 2017-01-13T22:48:21.751408Z] 22:48:21     INFO - Traceback (most recent call last):
[task 2017-01-13T22:48:21.752373Z] 22:48:21     INFO -   File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_harness/marionette_test/testcases.py", line 166, in run
[task 2017-01-13T22:48:21.753314Z] 22:48:21     INFO -     testMethod()
[task 2017-01-13T22:48:21.754324Z] 22:48:21     INFO -   File "/home/worker/workspace/build/tests/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py", line 102, in test_safe_browsing_initial_download
[task 2017-01-13T22:48:21.755304Z] 22:48:21     INFO -     set(os.listdir(os.path.join(self.safebrowsing_path, 'google4'))))
I pushed a try commit without hiding the google api key:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a15b43ea2fccbb9d5a5231e064a21c8f82308a04

It appears the google api key is still not properly configured.
(In reply to Henry Chang [:henry][:hchang] from comment #12)
> I pushed a try commit without hiding the google api key:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=a15b43ea2fccbb9d5a5231e064a21c8f82308a04
> 
> It appears the google api key is still not properly configured.

You're right. Same thing with my try job from comment 10.

Dustin, did I miss anything when I changed the scm level to 1?
Flags: needinfo?(dustin)
The patch looks like what I had in mind.. hmm.

[task 2017-01-16T07:09:23.773113Z] 07:09:23     INFO -  SafeBrowsing: 07:09:23 GMT+0000 (UTC): Provider: google updateURL=https://safebrowsing.google.com/safebrowsing/downloads?client=navclient-auto-ffox&appver=53.0a&pver=2.2&key=no-google-api-key

so that key is *not* the one that mozharness puts in place ("try-build-has-no-secrets").  Instead, old-configure sets that value if the option is not given at build time:

14646 if test -z "$MOZ_GOOGLE_API_KEY"; then
14647     MOZ_GOOGLE_API_KEY=no-google-api-key
14648 fi

Looking back at the build:

https://public-artifacts.taskcluster.net/bSZv1uS0Q8-xY7Mm69Gx2g/0/public/logs/live_backing.log
[task 2017-01-16T06:36:53.438356Z] 06:36:53     INFO -  'secret_files': ({'filename': '/builds/gapi.data',
[task 2017-01-16T06:36:53.438469Z] 06:36:53     INFO -                    'min_scm_level': 1,
[task 2017-01-16T06:36:53.438580Z] 06:36:53     INFO -                    'secret_name': 'project/releng/gecko/build/level-%(scm-level)s/gapi.data'},
...
[task 2017-01-16T06:36:53.443208Z] 06:36:53     INFO - Running main action method: get_secrets
[task 2017-01-16T06:36:53.443347Z] 06:36:53     INFO - No default for secret; not writing /builds/gapi.data
...
[task 2017-01-16T06:38:22.932202Z] 06:38:22     INFO -  Adding configure options from /home/worker/workspace/build/src/.mozconfig
..
[task 2017-01-16T06:38:22.932532Z] 06:38:22     INFO -    --with-google-api-keyfile=/builds/gapi.data

which leads me to .. oops!

diff --git a/testing/mozharness/mozharness/mozilla/secrets.py b/testing/mozharness/mozharness/mozilla/secrets.py
--- a/testing/mozharness/mozharness/mozilla/secrets.py
+++ b/testing/mozharness/mozharness/mozilla/secrets.py
@@ -56,17 +56,17 @@ class SecretsMixin(object):
         subst = {
             'scm-level': scm_level,
         }
 
         for sf in secret_files:
             filename = sf['filename']
             secret_name = sf['secret_name'] % subst
             min_scm_level = sf.get('min_scm_level', 0)
-            if scm_level <= min_scm_level:
+            if scm_level < min_scm_level:
                 if 'default' in sf:
                     self.info("Using default value for " + filename)
                     secret = sf['default']
                 else:
                     self.info("No default for secret; not writing " + filename)
                     continue
             else:
                 secret = self._fetch_secret(secret_name)

I'll make a try push on top of a15b43ea2fccbb9d5a5231e064a21c8f82308a04 to see if that fixes it, in which case please fold it into your patch!
Flags: needinfo?(dustin)
Comment on attachment 8826773 [details]
Bug 1330253 - Enable test_safe_browsing_initial_download on try.

https://reviewboard.mozilla.org/r/104650/#review106124
Attachment #8826773 - Flags: review?(dustin) → review+
(In reply to Dustin J. Mitchell [:dustin] from comment #15)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=89c6b00a5e82

The linux opt builds are now working, but the debug builds are not. Are the debug builds controlled by a different setting or do they not have access to secrets?
Flags: needinfo?(dustin)
Comment on attachment 8826773 [details]
Bug 1330253 - Enable test_safe_browsing_initial_download on try.

https://reviewboard.mozilla.org/r/104650/#review106216
Attachment #8826773 - Flags: review?(hskupin) → review+
in the build:

[task 2017-01-18T00:34:55.241305Z] 00:34:55     INFO - Running main action method: get_secrets
[task 2017-01-18T00:34:55.241441Z] 00:34:55     INFO - fetching secret project/releng/gecko/build/level-1/gapi.data from API

however, I don't see --with-google-api-keyfile.  So yes, it seems that the debug builds' mozconfigs do not include that setting.

I expect debug builds don't have *any* API keys -- is there value in running these tests on debug builds?
Flags: needinfo?(dustin)
(In reply to Dustin J. Mitchell [:dustin] from comment #19)
> in the build:
> 
> [task 2017-01-18T00:34:55.241305Z] 00:34:55     INFO - Running main action
> method: get_secrets
> [task 2017-01-18T00:34:55.241441Z] 00:34:55     INFO - fetching secret
> project/releng/gecko/build/level-1/gapi.data from API
> 
> however, I don't see --with-google-api-keyfile.  So yes, it seems that the
> debug builds' mozconfigs do not include that setting.
> 
> I expect debug builds don't have *any* API keys -- is there value in running
> these tests on debug builds?

I imagine we could accidentally break the Safe Browsing code in an #ifdef DEBUG code block and not notice that updates aren't flowing in debug builds.

I will disable this test in debug builds, but is it worth my filing a follow-up bug for allow debug builds to use secrets?
Flags: needinfo?(dustin)
This one should be all green since it has the fix from comment 14 and the tests are disabled on debug builds:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=48a94392fbf8ffb8f6a5d6b05613e44628a186bf
I'm certainly not an expert, but nobody should be using debug builds day-to-day, so it doesn't seem that would be an issue for a released browser.
Flags: needinfo?(dustin)
I tried to disable the test in debug builds like this:

  https://hg.mozilla.org/try/rev/a70d3dcc16a2bf442042910950b10b8210e137f8#l1.12

but I can still see it in the raw log for linux64 (and linux32) builds:

  https://public-artifacts.taskcluster.net/bqPO-8IKQ62bSYPe0bO8mg/0/public/logs//log_raw.log

Henrik, is that the right way to disable ui tests on debug builds or am I doing something wrong?
Flags: needinfo?(hskupin)
(In reply to François Marier [:francois] from comment #24)
> Henrik, is that the right way to disable ui tests on debug builds or am I
> doing something wrong?

This is mostly correct. By default we check for a mozinfo.json file, which should always be present for TC builds. But when I check your log I only see:

mozinfo is: {'has_sandbox': True, 'linux_distro': 'Ubuntu', 'bits': 64, 'os_version': StringVersion ('16.04'), 'version': 'Ubuntu 16.04', 'os': 'linux', 'processor': 'x86_64'}

Do you have a link to the Treeherder job? I would like to have a look at. What you posted above looks like an opt build.
Flags: needinfo?(hskupin)
(In reply to Henrik Skupin (:whimboo) from comment #25)
> This is mostly correct. By default we check for a mozinfo.json file, which
> should always be present for TC builds. But when I check your log I only see:
> 
> mozinfo is: {'has_sandbox': True, 'linux_distro': 'Ubuntu', 'bits': 64,
> 'os_version': StringVersion ('16.04'), 'version': 'Ubuntu 16.04', 'os':
> 'linux', 'processor': 'x86_64'}
> 
> Do you have a link to the Treeherder job? I would like to have a look at.
> What you posted above looks like an opt build.

The treeherder job is the one from comment 22:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=48a94392fbf8ffb8f6a5d6b05613e44628a186bf&selectedJob=70238745

and the raw log link from comment 24 was the "Linux x64 (debug)" one:

  https://queue.taskcluster.net/v1/task/bqPO-8IKQ62bSYPe0bO8mg/runs/0/artifacts/public/logs//log_raw.log

Are debug try runs not actually flagged as "debug"?
Flags: needinfo?(hskupin)
Interesting question. So lets have a look...

Output from Marionette when trying to read the mozinfo.json file:

https://treeherder.mozilla.org/logviewer.html#?job_id=70238745&repo=try&lineNumber=1051
> [task 2017-01-19T04:57:15.969885Z] 04:57:15     INFO - mozinfo updated from: None
> [task 2017-01-19T04:57:15.974951Z] 04:57:15     INFO - mozinfo is: {'has_sandbox': True, 'linux_distro': 'Ubuntu', 'bits': 64, 'os_version': StringVersion ('16.04'), 'version': 'Ubuntu 16.04', 'os': 'linux', 'processor': 'x86_64'}

It means Marionette doesn't find the mozinfo.json file.

We fetch the following common.tests.zip:
https://queue.taskcluster.net/v1/task/aJHcnO6NR-as6_XzogQSdg/artifacts/public/build/target.common.tests.zip

Downloading and extracting the archive shows me a mozinfo.json file in the root folder with "debug: true", and all the other properties listed.

So I tried to run Marionette locally:

marionette --binary obj/debug/dist/NightlyDebug.app/Contents/MacOS/firefox _b/marionette/tests/testing/marionette/harness/marionette_harness/tests/unit/unit-tests.ini
> mozinfo updated from: /Volumes/data/code/gecko/_b/mozinfo.json
> mozinfo is: {u'bin_suffix': u'', u'pgo': False, u'sync': True, u'buildapp': u'browser', u'crashreporter': True, u'addon_signing': True, u'require_signing': False, u'release_or_beta': False, u'appname': u'firefox', u'stylo': False, u'mozconfig': u'/home/worker/workspace/build/src/.mozconfig', u'topsrcdir': u'/home/worker/workspace/build/src', u'telemetry': False, 'os_version': StringVersion ('10.11'), 'version': 'OS X 10.11.6', u'datareporting': True, u'buildtype_guess': u'debug', 'bits': 32, 'has_sandbox': True, u'toolkit': u'gtk3', u'healthreport': True, u'updater': True, u'asan': False, u'platform_guess': u'linux', u'tests_enabled': True, u'official': True, u'tsan': False, u'nightly_build': True, u'debug': True, 'os': u'linux', 'processor': u'x86'}

So it was able to find the file. The same output I also get when running the "firefox-ui-functional" command.

Now I remember that we special-casing what we actually extract:
https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozharness/mozilla/testing/firefox_ui_tests.py#136

It means we pick everything except the mozinfo.json file from the root directory. :/ I will have to get this fixed for the fx-ui-tests but maybe even other test jobs are affected by that.
Flags: needinfo?(hskupin)
My patch on bug 1334093 landed on autoland. So once merged to mc you will be able to trigger a try build again. Skipping based on the debug flag should work then.
I've rebased the patch and try now looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5a9216d781c47f00378b35d94a6e62cc0c068f2

However, I will wait a week or two (for bug 1336922 to take effect) before landing this patch.
Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5ac53d4a98ea
Enable test_safe_browsing_initial_download on try. r=dustin,whimboo
Backed out for failing ui test TestSafeBrowsingInitialDownload.test_safe_browsing_initial_download:

https://hg.mozilla.org/integration/autoland/rev/ddc5649b792629588bfd2368a20d9489dfdfcba2

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5ac53d4a98ea89b7b0fbdb744ecceaef158c7c1d&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=80063972&repo=autoland

[task 2017-02-24T20:30:12.301628Z] 20:30:12     INFO -  1487968212220	Marionette	TRACE	conn2 -> [0,2454,"setContext",{"value":"chrome"}]
[task 2017-02-24T20:30:12.301707Z] 20:30:12     INFO -  1487968212221	Marionette	TRACE	conn2 <- [1,2454,null,{}]
[task 2017-02-24T20:30:12.302773Z] 20:30:12     INFO -  1487968212223	Marionette	TRACE	conn2 -> [0,2455,"getContext",null]
[task 2017-02-24T20:30:12.302898Z] 20:30:12     INFO -  1487968212223	Marionette	TRACE	conn2 <- [1,2455,null,{"value":"chrome"}]
[task 2017-02-24T20:30:12.303888Z] 20:30:12     INFO -  1487968212225	Marionette	TRACE	conn2 -> [0,2456,"setContext",{"value":"content"}]
[task 2017-02-24T20:30:12.303974Z] 20:30:12     INFO -  1487968212225	Marionette	TRACE	conn2 <- [1,2456,null,{}]
[task 2017-02-24T20:30:12.305098Z] 20:30:12     INFO -  1487968212227	Marionette	TRACE	conn2 -> [0,2457,"getPageSource",null]
[task 2017-02-24T20:30:12.330336Z] 20:30:12     INFO - TEST-UNEXPECTED-ERROR | test_safe_browsing_initial_download.py TestSafeBrowsingInitialDownload.test_safe_browsing_initial_download | TimeoutException: Timed out after 60.0 seconds with message: Not all safebrowsing files have been downloaded
[task 2017-02-24T20:30:12.330424Z] 20:30:12     INFO - Traceback (most recent call last):
[task 2017-02-24T20:30:12.330499Z] 20:30:12     INFO -   File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_harness/marionette_test/testcases.py", line 166, in run
[task 2017-02-24T20:30:12.331746Z] 20:30:12     INFO -     testMethod()
[task 2017-02-24T20:30:12.332441Z] 20:30:12     INFO -   File "/home/worker/workspace/build/tests/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py", line 97, in test_safe_browsing_initial_download
[task 2017-02-24T20:30:12.333544Z] 20:30:12     INFO -     check_downloaded, message='Not all safebrowsing files have been downloaded')
[task 2017-02-24T20:30:12.334392Z] 20:30:12     INFO -   File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_driver/wait.py", line 150, in until
[task 2017-02-24T20:30:12.335335Z] 20:30:12     INFO -     cause=last_exc)
Flags: needinfo?(francois)
Whiteboard: #sbv4-m6
Assignee: francois → dlee
The tescase fail because we didn't add v4 tables to test_safe_browsing_initial_download.py[1].
And the reason try looks good is because of NIGHTLY_BUILD flag, with this flag set, even we
didn't set v4 tables in test file, 'urlclassifier.downloadBlockTable' preference[2] still contains
a v4 table, so testcase passed since one v4 table(goog-badbinurl-proto) is found.
Comment on attachment 8852825 [details]
Bug 1330253 - Enable test_safe_browsing_initial_download on try.

(In reply to Dimi Lee[:dimi][:dlee] from comment #34)
> The tescase fail because we didn't add v4 tables to
> test_safe_browsing_initial_download.py[1].

Yes, but that's supposed to be set already in libpref/init/all.js.

I tried re-pushing my patch to try and it came out green:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb97ae80717432739ae7863e5c96203b09ae47bf&selectedJob=87736019

So I'm going to try and reland it.
Attachment #8852825 - Flags: review?(francois) → review-
Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/78ac10622294
Enable test_safe_browsing_initial_download on try. r=dustin,whimboo
Ah, it's failing on an linux64 ASAN build. I did not test that in my try push.
Flags: needinfo?(francois)
Here's a try build that reproduces the problem seen on autoland: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6ce85fedb4f9eab1fd5bb12d1324061fae8784f&selectedJob=87994965

It was triggered by: ./mach try -b do -p linux64-asan -u firefox-ui-functional -t none
Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fb47c03b9277
Enable test_safe_browsing_initial_download on try. r=dustin,whimboo
My latest revision on MozReview disables the test on ASAN builds too.

Try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=71493f644b3d1a712b5ee3fb516ddfb9cd358666
https://hg.mozilla.org/mozilla-central/rev/fb47c03b9277
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: #sbv4-m6 → #sbv4-m6 [google-api-safe-browsing]
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.