Reporting of policy errors via host-offline-reason: part 1

remoting/host/policy_hack/policy_watcher.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Most of this code is copied from:
 //   src/chrome/browser/policy/asynchronous_policy_loader.{h,cc}
 
 #include "remoting/host/policy_hack/policy_watcher.h"
 
 #include "base/bind.h"
@@ skipping 101 matching lines @@
 
 const char PolicyWatcher::kUdpPortRangePolicyName[] =
     "RemoteAccessHostUdpPortRange";
 
 const char PolicyWatcher::kHostDebugOverridePoliciesName[] =
     "RemoteAccessHostDebugOverridePolicies";
 
 PolicyWatcher::PolicyWatcher(
     scoped_refptr<base::SingleThreadTaskRunner> task_runner)
     : task_runner_(task_runner),
+      transient_policy_error_retry_counter_(0),
       old_policies_(new base::DictionaryValue()),
       default_values_(new base::DictionaryValue()),
       weak_factory_(this) {
   // Initialize the default values for each policy.
   default_values_->SetBoolean(kNatPolicyName, true);
   default_values_->SetBoolean(kHostRequireTwoFactorPolicyName, false);
   default_values_->SetBoolean(kHostRequireCurtainPolicyName, false);
   default_values_->SetBoolean(kHostMatchUsernamePolicyName, false);
   default_values_->SetString(kHostDomainPolicyName, std::string());
   default_values_->SetString(kHostTalkGadgetPrefixPolicyName,
@@ skipping 13 matching lines @@
   // Initialize the fall-back values to use for unreadable policies.
   // For most policies these match the defaults.
   bad_type_values_.reset(default_values_->DeepCopy());
   bad_type_values_->SetBoolean(kNatPolicyName, false);
   bad_type_values_->SetBoolean(kRelayPolicyName, false);
 }
 
 PolicyWatcher::~PolicyWatcher() {
 }
 
-void PolicyWatcher::StartWatching(const PolicyCallback& policy_callback) {
+void PolicyWatcher::StartWatching(
+    const PolicyUpdatedCallback& policy_updated_callback,
+    const PolicyErrorCallback& policy_error_callback) {
   if (!OnPolicyWatcherThread()) {
     task_runner_->PostTask(FROM_HERE,
                            base::Bind(&PolicyWatcher::StartWatching,
                                       base::Unretained(this),
-                                      policy_callback));
+                                      policy_updated_callback,
+                                      policy_error_callback));
     return;
   }
 
-  policy_callback_ = policy_callback;
+  policy_updated_callback_ = policy_updated_callback;
+  policy_error_callback_ = policy_error_callback;
   StartWatchingInternal();
 }
 
 void PolicyWatcher::StopWatching(base::WaitableEvent* done) {
   if (!OnPolicyWatcherThread()) {
     task_runner_->PostTask(FROM_HERE,
                            base::Bind(&PolicyWatcher::StopWatching,
                                       base::Unretained(this), done));
     return;
   }
 
   StopWatchingInternal();
   weak_factory_.InvalidateWeakPtrs();
-  policy_callback_.Reset();
+  policy_updated_callback_.Reset();
+  policy_error_callback_.Reset();
 
   done->Signal();
 }
 
 void PolicyWatcher::ScheduleFallbackReloadTask() {
   DCHECK(OnPolicyWatcherThread());
   ScheduleReloadTask(
       base::TimeDelta::FromMinutes(kFallbackReloadDelayMinutes));
 }
 
@@ skipping 10 matching lines @@
 }
 
 bool PolicyWatcher::OnPolicyWatcherThread() const {
   return task_runner_->BelongsToCurrentThread();
 }
 
 void PolicyWatcher::UpdatePolicies(
     const base::DictionaryValue* new_policies_raw) {
   DCHECK(OnPolicyWatcherThread());
 
+  transient_policy_error_retry_counter_ = 0;
+
   // Use default values for any missing policies.
   scoped_ptr<base::DictionaryValue> new_policies =
       CopyGoodValuesAndAddDefaults(
           new_policies_raw, default_values_.get(), bad_type_values_.get());
 
   // Find the changed policies.
   scoped_ptr<base::DictionaryValue> changed_policies(
       new base::DictionaryValue());
   base::DictionaryValue::Iterator iter(*new_policies);
   while (!iter.IsAtEnd()) {
     base::Value* old_policy;
     if (!(old_policies_->Get(iter.key(), &old_policy) &&
           old_policy->Equals(&iter.value()))) {
       changed_policies->Set(iter.key(), iter.value().DeepCopy());
     }
     iter.Advance();
   }
 
   // Save the new policies.
   old_policies_.swap(new_policies);
 
   // Notify our client of the changed policies.
   if (!changed_policies->empty()) {
-    policy_callback_.Run(changed_policies.Pass());
+    policy_updated_callback_.Run(changed_policies.Pass());
   }
 }
 
+void PolicyWatcher::SignalPolicyError() {
+  policy_error_callback_.Run();

[Lambros, 2014/11/13 00:30:26]

Depending on what you think should happen, you might want to reset the transient
counter here?

[Łukasz Anforowicz, 2014/11/13 17:48:08]

Good point. Done.

+}
+
+void PolicyWatcher::SignalTransientPolicyError() {
+  // TODO(lukasza): max=5 helps recover from transient errors of short
+  //                duration (i.e. reading a partially written file),

[Lambros, 2014/11/13 00:30:26]

nit: Don't indent the whole TODO block. See examples elsewhere for how to format
this.

+  //                but won't help with longer problems (i.e. malformed
+  //                policy pushed out via group policy or puppet will
+  //                most likely still trigger SignalPolicyError)

[Lambros, 2014/11/13 00:30:26]

Is it this class's responsibility to figure out what to do about long-term
policy errors? Is the PolicyErrorCallback not a good-enough interface?

I think it might be better to document in the class header what happens after
SignalPolicyError() is triggered? Will this class continue to report further
errors/updates, or will it stop watching altogether?

[Łukasz Anforowicz, 2014/11/13 17:48:08]

Good point on documenting in the header the current behavior of continuing to
process policy files even after reporting the error.

Yes - I guess it is HostProcess's responsibility to deal with long-term policy
errors.

So:
- I am adding a comment to the header file (next to StartWatching method)
- I am removing this TODO
- Do we need to discuss what to do going forward with the long-term failures
(and recovery from long-term failures)?  The current changelist can probably
checked-in as-is (unless you want me to move the TODO to HostProcess class).  I
am not sure how important the long-term-policy-error-autorecovery is + whether
it should be tracked as a separate bug (maybe I should just open one and let
triage team decide?).

+  //              Potential solution #1: keep the host running in
+  //                security-lock-down mode to keep reparsing the policies
+  //                (i.e. don't call Shutdown in me2me's OnPolicyError)
+  //              Potential solution #2: maybe retry at init.d-level?
+  //              Potential solution #3: maybe using Chrome-wide policy
+  //                libraries rather than policy_hack will help?
+  const int MAX_RETRY_COUNT = 5;

[Lambros, 2014/11/13 00:30:27]

nit: kMaxRetryCount

[Łukasz Anforowicz, 2014/11/13 17:48:08]

Done.

+  transient_policy_error_retry_counter_ += 1;
+  if (transient_policy_error_retry_counter_ >= MAX_RETRY_COUNT) {
+    SignalPolicyError();
+  }
+}
+
 }  // namespace policy_hack
 }  // namespace remoting