Closed
Bug 1330253
Opened 8 years ago
Closed 8 years ago
Supply a Google API key on try to enable testing against the Safe Browsing service
Categories
(Release Engineering :: General, defect)
Release Engineering
General
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.
Reporter | ||
Updated•8 years ago
|
Summary: Do CI machines need to be deployed valid google API keys → Do CI machines need to be deployed valid google API keys?
Comment 1•8 years ago
|
||
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
Updated•8 years ago
|
Blocks: safebrowsingv4
Comment 2•8 years ago
|
||
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
Reporter | ||
Comment 4•8 years ago
|
||
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)
Comment 5•8 years ago
|
||
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
Comment 6•8 years ago
|
||
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.
Comment 7•8 years ago
|
||
If we were to get a separate API key from Google, could we make it available on try?
Flags: needinfo?(dustin)
Comment 8•8 years ago
|
||
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)
Updated•8 years ago
|
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
Updated•8 years ago
|
Assignee: nobody → francois
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment 10•8 years ago
|
||
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.
Reporter | ||
Comment 11•8 years ago
|
||
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'))))
Reporter | ||
Comment 12•8 years ago
|
||
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.
Comment 13•8 years ago
|
||
(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)
Comment 14•8 years ago
|
||
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 15•8 years ago
|
||
Comment 16•8 years ago
|
||
mozreview-review |
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+
Comment 17•8 years ago
|
||
(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 18•8 years ago
|
||
mozreview-review |
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+
Comment 19•8 years ago
|
||
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)
Comment 20•8 years ago
|
||
(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)
Comment hidden (mozreview-request) |
Comment 22•8 years ago
|
||
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
Comment 23•8 years ago
|
||
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)
Comment 24•8 years ago
|
||
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)
Comment 25•8 years ago
|
||
(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)
Comment 26•8 years ago
|
||
(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)
Comment 27•8 years ago
|
||
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)
Comment 28•8 years ago
|
||
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.
Comment hidden (mozreview-request) |
Comment 30•8 years ago
|
||
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.
Comment 31•8 years ago
|
||
Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5ac53d4a98ea
Enable test_safe_browsing_initial_download on try. r=dustin,whimboo
Comment 32•8 years ago
|
||
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)
Comment 33•8 years ago
|
||
I prepared a new patch but some unrelated tests are still failing:
Aurora 53: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc40b3709c5c16c6edc819ab9dad25a13a1f59f2&selectedJob=81727175
Nightly 54: https://treeherder.mozilla.org/#/jobs?repo=try&revision=856bc35a320aaf86dc2a9d1ac6d55997f49e6a9b&selectedJob=81730372
Flags: needinfo?(francois)
Updated•8 years ago
|
Whiteboard: #sbv4-m6
Assignee | ||
Updated•8 years ago
|
Assignee: francois → dlee
Assignee | ||
Comment 34•8 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 37•8 years ago
|
||
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-
Comment 38•8 years ago
|
||
Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/78ac10622294
Enable test_safe_browsing_initial_download on try. r=dustin,whimboo
These were failing on non-try after this landed: https://treeherder.mozilla.org/logviewer.html#?job_id=87986911&repo=autoland
Backed out in https://hg.mozilla.org/integration/autoland/rev/6241506795d0
Flags: needinfo?(francois)
Comment 40•8 years ago
|
||
Ah, it's failing on an linux64 ASAN build. I did not test that in my try push.
Flags: needinfo?(francois)
Comment 41•8 years ago
|
||
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
Comment hidden (mozreview-request) |
Comment 43•8 years ago
|
||
Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fb47c03b9277
Enable test_safe_browsing_initial_download on try. r=dustin,whimboo
Comment 44•8 years ago
|
||
My latest revision on MozReview disables the test on ASAN builds too.
Try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=71493f644b3d1a712b5ee3fb516ddfb9cd358666
Comment 45•8 years ago
|
||
bugherder |
Updated•7 years ago
|
Whiteboard: #sbv4-m6 → #sbv4-m6 [google-api-safe-browsing]
Updated•6 years ago
|
Component: General Automation → General
You need to log in
before you can comment on or make changes to this bug.
Description
•