New issue
Advanced search Search tips

Issue 809222 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Fake DMS not working

Project Member Reported by kathrelk...@chromium.org, Feb 5 2018

Issue description

Starting in 10367.0.0 (66.0.3336.3) the fake DMS no longer sets policies in our Autotests.

This is the build that https://chromium-review.googlesource.com/c/chromium/src/+/891859 landed, so seems related!


Here's an example value sent to the server:
{'managed_users': ['*'], 'google/chromeos/user': {'mandatory': {'BookmarkBarEnabled': False, 'ImagesAllowedForUrls': ['http://www.bing.com', 'http://localhost:8080', 'https://www.yahoo.com'], 'RestoreOnStartupURLs': ['chrome://policy', 'chrome://settings'], 'RestoreOnStartup': 4, 'DefaultImagesSetting': 2}}, 'policy_user': 'fake-user@managedchrome.com', 'invalidation_source': 16, 'current_key_index': 0, 'invalidation_name': 'test_policy'}
 
Cc: achuith@chromium.org atwilson@chromium.org emaxx@chromium.org
+Drew/Maksim/Achuith

What may have broken these autotests is a combination of three things:
(*) the new policy initialization handling introduced in https://chromium-review.googlesource.com/c/chromium/src/+/719838 does not block for policy if the --allow-failed-policy-fetch-for-test flag is passed.
(*) the autotests run with this flag
(*) the autotests verify the policy values immediately after browser start

Probably the solution would be to stop passing --allow-failed-policy-fetch-for-test (or, less ideally, additionally pass --wait-for-initial-policy-fetch-for-test).

It looks like --allow-failed-policy-fetch-for-test is passed unconditionally in 
cros_browser_finder.py[1].

I think we should not pass this if we're testing with a fake dmserver (or can anyone think of a reason to do so?). 
We could detect that a fake dmserver is in use by checking if a --device-management-url is given in the arguments, or by adding a boolean flag to browser_options.
Probably the second one is cleaner and more flexible for the future, so I'd suggest going that route.
Will create CLs shortly.

[1] https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/internal/backends/chrome/cros_browser_finder.py?rcl=a1303e46889b22580cbf6129a01fd1c1c094b4f3&l=101
I assume you're able to repo this failure locally?

Please add me as a reviewer for the catapult CLs (you can make changes in src/third_party/catapult and upload directly).

Your proposed solution sgtm.
I didn't get around to repro this locally but will do that tomorrow (EMEA time) and if the proposed change fixes that, upload the CLs.
Cc: ljusten@chromium.org
+Lutz FYI
Confirmed that the test fails locally with --allow-failed-policy-fetch-for-test and passes without --allow-failed-policy-fetch-for-test, so going to upload CLs for this now.
Also I wonder if maybe we should change the name of that flag (or just get rid of the flag in favor of passing --profile_requires_policy=false) since they seem to result in the same behavior now?
Status: Started (was: Assigned)
BTW it looks like with the flag, policy would now start to get initialized after 5 seconds [1].

[1] https://cs.chromium.org/chromium/src/chrome/browser/policy/chrome_browser_policy_connector.h?l=33
Project Member

Comment 8 by bugdroid1@chromium.org, Feb 7 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/catapult/+/0fae20ce0a8300f6856ada0a1de8f853b6a9f6a4

commit 0fae20ce0a8300f6856ada0a1de8f853b6a9f6a4
Author: Pavol Marko <pmarko@chromium.org>
Date: Wed Feb 07 21:55:04 2018

cros_browser_finder: Add an option to expect working enterprise policy

Add the 'expect_policy_fetch' option (defaults to False).
When this is set to True, the '-allow-failed-policy-fetch-for-test'
command-line argument will not be passed to chrome.

Previously the argument was passed in all cases, even for tests which
actually test enterprise policy.

BUG= chromium:809222 


Change-Id: I65a4b47c029c737d0b16771340461e2e4704ef13
Reviewed-on: https://chromium-review.googlesource.com/904989
Commit-Queue: Achuith Bhandarkar <achuith@chromium.org>
Reviewed-by: Achuith Bhandarkar <achuith@chromium.org>

[modify] https://crrev.com/0fae20ce0a8300f6856ada0a1de8f853b6a9f6a4/telemetry/telemetry/internal/backends/chrome/cros_browser_finder.py
[modify] https://crrev.com/0fae20ce0a8300f6856ada0a1de8f853b6a9f6a4/telemetry/telemetry/internal/browser/browser_options.py

Project Member

Comment 9 by bugdroid1@chromium.org, Feb 8 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6425d6f4dfa49bc693b7e4a417a51db8346c5212

commit 6425d6f4dfa49bc693b7e4a417a51db8346c5212
Author: catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Thu Feb 08 02:53:08 2018

Roll src/third_party/catapult/ 4cae15cb4..7e6c69498 (2 commits)

https://chromium.googlesource.com/catapult.git/+log/4cae15cb43c4..7e6c69498cd3

$ git log 4cae15cb4..7e6c69498 --date=short --no-merges --format='%ad %ae %s'
2018-02-07 sullivan If there is only a single commit in the range for all alerts, auto-assign author.
2018-02-07 pmarko cros_browser_finder: Add an option to expect working enterprise policy

Created with:
  roll-dep src/third_party/catapult
BUG= 809222 


The AutoRoll server is located here: https://catapult-roll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.


CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel
TBR=sullivan@chromium.org

Change-Id: If466f6f1393cf634e191b401a481cd2420661a9a
Reviewed-on: https://chromium-review.googlesource.com/907863
Commit-Queue: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Reviewed-by: catapult-chromium-autoroll <catapult-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#535281}
[modify] https://crrev.com/6425d6f4dfa49bc693b7e4a417a51db8346c5212/DEPS

Project Member

Comment 10 by bugdroid1@chromium.org, Feb 10 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/14ff7c9e908054158189e12d95c3e2988776f642

commit 14ff7c9e908054158189e12d95c3e2988776f642
Author: Pavol Marko <pmarko@chromium.org>
Date: Sat Feb 10 03:03:21 2018

Expect working enterprise policy in policy tests

Pass the |expect_policy_fetch|=True argument when launching chrome for
enterprise policy tests, so chrome won't get the
--allow-failed-policy-fetches-for-test command-line flag.

This is necessary now because chrome doesn't block fresh profile
initialization on policy initialization when the flag is present since
CL:719838.

BUG= chromium:809222 
TEST=test_that --board=${BOARD} ${IP} policy_ProxySettings.directproxy_usenoproxy

Change-Id: I49514fffc54cb6f5f69e25e8d04fdb8ddae8b6d1
Reviewed-on: https://chromium-review.googlesource.com/906549
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Achuith Bhandarkar <achuith@chromium.org>

[modify] https://crrev.com/14ff7c9e908054158189e12d95c3e2988776f642/client/cros/enterprise/enterprise_policy_base.py
[modify] https://crrev.com/14ff7c9e908054158189e12d95c3e2988776f642/client/common_lib/cros/chrome.py
[modify] https://crrev.com/14ff7c9e908054158189e12d95c3e2988776f642/client/site_tests/enterprise_PowerManagement/enterprise_PowerManagement.py

Components: Enterprise
Tests started passing again over the weekend!
Status: Fixed (was: Started)
Thanks, I'll assume this is fixed then.

Please re-open if we missed some tests which still fail :)

Thanks!
Status: Verified (was: Fixed)
Verified per #c11
Labels: -ent-autotest ent-automation

Sign in to add a comment