Trigger ConnectToBestNetwork in more cases.

Trigger ConnectToBestNetwork in more cases.

ConnectToBestNetwork will now also be triggered after policy application and after client certificate patterns were resolved.
I.e. that managed networks will autoconnect more reliably.

BUG=424036
Committed: https://crrev.com/627248e4ecdfcc2f83de079ab51eb4db12352baa
Cr-Commit-Position: refs/heads/master@{#303457}

[pneubeck (no reviews), 2014-11-06 11:04:41]

Patchset #1 (id:1) has been deleted

[pneubeck (no reviews), 2014-11-06 11:04:46]

Patchset #1 (id:20001) has been deleted

[pneubeck (no reviews), 2014-11-06 11:04:50]

Patchset #1 (id:40001) has been deleted

[pneubeck (no reviews), 2014-11-06 12:19:16]

Patchset #1 (id:60001) has been deleted

[pneubeck (no reviews), 2014-11-06 12:19:21]

Patchset #1 (id:80001) has been deleted

[pneubeck (no reviews), 2014-11-06 12:23:43]

pneubeck@chromium.org changed reviewers:
+ stevenjb@chromium.org

[pneubeck (no reviews), 2014-11-06 12:23:43]

ptal, this is the final CL for autoconnect!

[pneubeck (no reviews), 2014-11-06 13:57:20]

Patchset #1 (id:100001) has been deleted

[stevenjb, 2014-11-06 17:24:59]

We're doing a lot of policy state tracking in the NetworkConnectionHandler class
(device_policy_set, device_policy_applied, user_policy_set). Could we move that
logic to the policy code and just have it request a 'ConnectToBestNetwork' when
it thinks it may need to? I don't think there is any harm in sending more than
one request to Shill, if we are already connected to the "best" network then it
should be a no-op. The one change we may want to make here is tracking when an
explicit connect request is made and skip any "Best" requests after that. (We
may need the change I am working on to track the connect request source, but I
will be updating that CL shortly).

Thoughts? I'm a bit concerned about the complexity and policy code dependencies
this CL introduces to NCH.

[pneubeck (no reviews), 2014-11-06 18:06:02]

On 2014/11/06 17:24:59, stevenjb wrote:
> We're doing a lot of policy state tracking in the NetworkConnectionHandler
class
> (device_policy_set, device_policy_applied, user_policy_set). Could we move
that
> logic to the policy code and just have it request a 'ConnectToBestNetwork'
when
> it thinks it may need to? I don't think there is any harm in sending more than
> one request to Shill, if we are already connected to the "best" network then
it
> should be a no-op. The one change we may want to make here is tracking when an
> explicit connect request is made and skip any "Best" requests after that. (We
> may need the change I am working on to track the connect request source, but I
> will be updating that CL shortly).
> 
> Thoughts? I'm a bit concerned about the complexity and policy code
dependencies
> this CL introduces to NCH.

Yes, I thought about that too.
One reason for not sending ConnectToBestNetwork requests blindly to Shill is
that it can cause a disconnect of the current network and a reconnect with a
'better' network.
Would be nice to be on the safe side and that we're not:
 - repeatedly changing the connection
 - changing the connection to another non-managed network and not to a managed
network
 - changing connection if there is no policy

To keep NCH smaller and well structured, I could move this logic into a separate
class 'AutoconnectHandler'.
NCH will only have to notify that new one if a manual connection was requested,
which will abort any pending autoconnect request.

Does that sound better?

[stevenjb, 2014-11-06 18:22:39]

On 2014/11/06 18:06:02, pneubeck wrote:
> On 2014/11/06 17:24:59, stevenjb wrote:
> > We're doing a lot of policy state tracking in the NetworkConnectionHandler
> class
> > (device_policy_set, device_policy_applied, user_policy_set). Could we move
> that
> > logic to the policy code and just have it request a 'ConnectToBestNetwork'
> when
> > it thinks it may need to? I don't think there is any harm in sending more
than
> > one request to Shill, if we are already connected to the "best" network then
> it
> > should be a no-op. The one change we may want to make here is tracking when
an
> > explicit connect request is made and skip any "Best" requests after that.
(We
> > may need the change I am working on to track the connect request source, but
I
> > will be updating that CL shortly).
> > 
> > Thoughts? I'm a bit concerned about the complexity and policy code
> dependencies
> > this CL introduces to NCH.
> 
> Yes, I thought about that too.
> One reason for not sending ConnectToBestNetwork requests blindly to Shill is
> that it can cause a disconnect of the current network and a reconnect with a
> 'better' network.
> Would be nice to be on the safe side and that we're not:
>  - repeatedly changing the connection
>  - changing the connection to another non-managed network and not to a managed
> network
>  - changing connection if there is no policy
> 
> To keep NCH smaller and well structured, I could move this logic into a
separate
> class 'AutoconnectHandler'.
> NCH will only have to notify that new one if a manual connection was
requested,
> which will abort any pending autoconnect request.
> 
> Does that sound better?

I'm concerned about splitting the "Best" determination between Chrome and Shill.
If Shill is making bad decisions about "Best" (e.g. switching from a managed
network to an unmanaged one) we should fix that.

I do agree that we don't want to do unnecessary switching. We basically have
three signals: "User logged in", "Credentials loaded", and "Policy loaded". The
first two are fairly trivial checks, but the third is more complex.

What if we changed the logic in ConnectToBestNetworkAfterLogin() to be simply:

MaybeConnectToBestNetwork() {
  if (requested_connect_to_best_network_ ||
      !logged_in || !certificates_loaded_ ||
      !policy_delegate_->IsPolicySet())
    return;
...

Then call MaybeConnectToBestNetwork() any time any of those may have changed?
Maybe instead of a policy_delegate NCH observes policy changes and sets a single
policy_set_ variable, whatever is easiest so long as the details of "policy
ready" are kept out of NCH?

[pneubeck (no reviews), 2014-11-06 19:43:31]

On 2014/11/06 18:22:39, stevenjb wrote:
> On 2014/11/06 18:06:02, pneubeck wrote:
> > On 2014/11/06 17:24:59, stevenjb wrote:
> > > We're doing a lot of policy state tracking in the NetworkConnectionHandler
> > class
> > > (device_policy_set, device_policy_applied, user_policy_set). Could we move
> > that
> > > logic to the policy code and just have it request a 'ConnectToBestNetwork'
> > when
> > > it thinks it may need to? I don't think there is any harm in sending more
> than
> > > one request to Shill, if we are already connected to the "best" network
then
> > it
> > > should be a no-op. The one change we may want to make here is tracking
when
> an
> > > explicit connect request is made and skip any "Best" requests after that.
> (We
> > > may need the change I am working on to track the connect request source,
but
> I
> > > will be updating that CL shortly).
> > > 
> > > Thoughts? I'm a bit concerned about the complexity and policy code
> > dependencies
> > > this CL introduces to NCH.
> > 
> > Yes, I thought about that too.
> > One reason for not sending ConnectToBestNetwork requests blindly to Shill is
> > that it can cause a disconnect of the current network and a reconnect with a
> > 'better' network.
> > Would be nice to be on the safe side and that we're not:
> >  - repeatedly changing the connection
> >  - changing the connection to another non-managed network and not to a
managed
> > network
> >  - changing connection if there is no policy
> > 
> > To keep NCH smaller and well structured, I could move this logic into a
> separate
> > class 'AutoconnectHandler'.
> > NCH will only have to notify that new one if a manual connection was
> requested,
> > which will abort any pending autoconnect request.
> > 
> > Does that sound better?
> 
> I'm concerned about splitting the "Best" determination between Chrome and
Shill.
> If Shill is making bad decisions about "Best" (e.g. switching from a managed
> network to an unmanaged one) we should fix that.

Shill doesn't know about policy and also not which network changes happened
automatically and which were triggered by the user.
I think that's the main reason why Chrome needs some logic when we should
disconnect from a network to connect to a better one.


> I do agree that we don't want to do unnecessary switching. We basically have
> three signals: "User logged in", "Credentials loaded", and "Policy loaded".
The
> first two are fairly trivial checks, but the third is more complex.
Not sure whether you intentionally wrote 'Credentials loaded' vs. 'Certificates
loaded'.
These are actually two separate things, so it's four signals.
 
> What if we changed the logic in ConnectToBestNetworkAfterLogin() to be simply:
> 
> MaybeConnectToBestNetwork() {
>   if (requested_connect_to_best_network_ ||
>       !logged_in || !certificates_loaded_ ||
>       !policy_delegate_->IsPolicySet())
>     return;
> ...
> 
> Then call MaybeConnectToBestNetwork() any time any of those may have changed?
> Maybe instead of a policy_delegate NCH observes policy changes and sets a
single
> policy_set_ variable, whatever is easiest so long as the details of "policy
> ready" are kept out of NCH?

I think this ignores several things.
Overall we have much more going on:
- Independent of any autoconnect and policy, NConnectH depends on logged_in and
certificates_loaded for manual connect requests
- The AllowOnlyManagedNetworksToAutoconnect policy, depends only on the user
policy being set (not applied)
- The ConnectToBestNetwork has several triggers and several requirements (to
prevent unnecessary network changes).
  In particular should it be triggered on policy changes and automatic
credential changes (e.g. network is automatically configured with a client
certificate)

Most of the logic that I added was for the last point (ConnectToBestNetwork) and
it's more convoluted because of the multiple triggers and the effort to prevent
multiple calls to Shill.
With the current Shill interface we don't know what the effect of
ConnectToBestNetwork will be, therefore 
I tried to not call 'ConnectToBestNetwork' when we don't expect a change to a
managed network.

If it helps, I can explain some of the code in a VC and then we can decide how
to structure it in a better way.
How does that sound?

[stevenjb, 2014-11-06 20:17:02]

On 2014/11/06 19:43:31, pneubeck wrote:
> On 2014/11/06 18:22:39, stevenjb wrote:
> > On 2014/11/06 18:06:02, pneubeck wrote:
> > > On 2014/11/06 17:24:59, stevenjb wrote:
> > > > We're doing a lot of policy state tracking in the
NetworkConnectionHandler
> > > class
> > > > (device_policy_set, device_policy_applied, user_policy_set). Could we
move
> > > that
> > > > logic to the policy code and just have it request a
'ConnectToBestNetwork'
> > > when
> > > > it thinks it may need to? I don't think there is any harm in sending
more
> > than
> > > > one request to Shill, if we are already connected to the "best" network
> then
> > > it
> > > > should be a no-op. The one change we may want to make here is tracking
> when
> > an
> > > > explicit connect request is made and skip any "Best" requests after
that.
> > (We
> > > > may need the change I am working on to track the connect request source,
> but
> > I
> > > > will be updating that CL shortly).
> > > > 
> > > > Thoughts? I'm a bit concerned about the complexity and policy code
> > > dependencies
> > > > this CL introduces to NCH.
> > > 
> > > Yes, I thought about that too.
> > > One reason for not sending ConnectToBestNetwork requests blindly to Shill
is
> > > that it can cause a disconnect of the current network and a reconnect with
a
> > > 'better' network.
> > > Would be nice to be on the safe side and that we're not:
> > >  - repeatedly changing the connection
> > >  - changing the connection to another non-managed network and not to a
> managed
> > > network
> > >  - changing connection if there is no policy
> > > 
> > > To keep NCH smaller and well structured, I could move this logic into a
> > separate
> > > class 'AutoconnectHandler'.
> > > NCH will only have to notify that new one if a manual connection was
> > requested,
> > > which will abort any pending autoconnect request.
> > > 
> > > Does that sound better?
> > 
> > I'm concerned about splitting the "Best" determination between Chrome and
> Shill.
> > If Shill is making bad decisions about "Best" (e.g. switching from a managed
> > network to an unmanaged one) we should fix that.
> 
> Shill doesn't know about policy and also not which network changes happened
> automatically and which were triggered by the user.
> I think that's the main reason why Chrome needs some logic when we should
> disconnect from a network to connect to a better one.
> 
> 
> > I do agree that we don't want to do unnecessary switching. We basically have
> > three signals: "User logged in", "Credentials loaded", and "Policy loaded".
> The
> > first two are fairly trivial checks, but the third is more complex.
> Not sure whether you intentionally wrote 'Credentials loaded' vs.
'Certificates
> loaded'.
> These are actually two separate things, so it's four signals.
>  
> > What if we changed the logic in ConnectToBestNetworkAfterLogin() to be
simply:
> > 
> > MaybeConnectToBestNetwork() {
> >   if (requested_connect_to_best_network_ ||
> >       !logged_in || !certificates_loaded_ ||
> >       !policy_delegate_->IsPolicySet())
> >     return;
> > ...
> > 
> > Then call MaybeConnectToBestNetwork() any time any of those may have
changed?
> > Maybe instead of a policy_delegate NCH observes policy changes and sets a
> single
> > policy_set_ variable, whatever is easiest so long as the details of "policy
> > ready" are kept out of NCH?
> 
> I think this ignores several things.
> Overall we have much more going on:
> - Independent of any autoconnect and policy, NConnectH depends on logged_in
and
> certificates_loaded for manual connect requests
> - The AllowOnlyManagedNetworksToAutoconnect policy, depends only on the user
> policy being set (not applied)
> - The ConnectToBestNetwork has several triggers and several requirements (to
> prevent unnecessary network changes).
>   In particular should it be triggered on policy changes and automatic
> credential changes (e.g. network is automatically configured with a client
> certificate)
> 
> Most of the logic that I added was for the last point (ConnectToBestNetwork)
and
> it's more convoluted because of the multiple triggers and the effort to
prevent
> multiple calls to Shill.
> With the current Shill interface we don't know what the effect of
> ConnectToBestNetwork will be, therefore 
> I tried to not call 'ConnectToBestNetwork' when we don't expect a change to a
> managed network.
> 
> If it helps, I can explain some of the code in a VC and then we can decide how
> to structure it in a better way.
> How does that sound?

I think a diagram would help me the most.

If Shill doesn't have sufficient information, maybe we need to deprecate
ConnectToBestNetwork and put all of the logic in Chrome? I really don't like
splitting the decision making. I feel that either:
a) Chrome should complete its configuration and provide Shill with enough
information to make a decision.
b) Chrome should control autoconnect entirely.

I'm pretty sure there are problems with b), never mind the need to implement it,
but if that were an end goal I would be more comfortable with the direction we
are heading, which I have to say currently confuses the hell out of me.

I will try to wrap my head around it better later today or tomorrow, but I've
got a lot on my plate right now.

If this is something we want for 40, I would be more OK with at least pushing
the policy related details out of NCH, or at least simplify it somehow so that
NCH isn't tracking three separate policy booleans. Or maybe a separate
AutoConnectHandler class would help?

[pneubeck (no reviews), 2014-11-07 15:57:50]

Patchset #2 (id:140001) has been deleted

[pneubeck (no reviews), 2014-11-07 16:00:23]

ptal
comments might need some polish after having moved around everything but I hope
that's not a blocker.

[stevenjb, 2014-11-07 17:19:06]

Thanks for doing this, I find this much less confusing and (as someone who is
probably better equipped to follow this than most) much more likely to be
maintainable.

A couple of suggestions, but those could also be addressed later, so lgtm.

https://codereview.chromium.org/701863002/diff/160001/chromeos/network/auto_c...
File chromeos/network/auto_connect_handler.cc (right):

https://codereview.chromium.org/701863002/diff/160001/chromeos/network/auto_c...
chromeos/network/auto_connect_handler.cc:105: }
nit: invert this logic to be consistent with the logic below (and slightly more
readable, positive result, which in this case is !empty(), before negative).

https://codereview.chromium.org/701863002/diff/160001/chromeos/network/auto_c...
chromeos/network/auto_connect_handler.cc:159:
network_state_handler_->ConnectToBestWifiNetwork();
Should we remove this from NSH/SPH and make the Shill call here? I'm concerned
that if we leave it in NSH some other logic could conflict and break this logic,
i.e. I think we should only call ConnectToBestWifiNetwork from one place.

https://codereview.chromium.org/701863002/diff/160001/chromeos/network/auto_c...
chromeos/network/auto_connect_handler.cc:167:
managed_configuration_handler_->GetGlobalConfigFromPolicy(std::string());
nit: comment what std::string() represents

https://codereview.chromium.org/701863002/diff/160001/chromeos/network/auto_c...
File chromeos/network/auto_connect_handler_unittest.cc (right):

https://codereview.chromium.org/701863002/diff/160001/chromeos/network/auto_c...
chromeos/network/auto_connect_handler_unittest.cc:49: void
ConfigureCallback(const dbus::ObjectPath& result) {
nit: Might be worth making these member functions that take an 'expected'
argument and ASSERT(expected), or somesuch.

https://codereview.chromium.org/701863002/diff/160001/chromeos/network/networ...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/701863002/diff/160001/chromeos/network/networ...
chromeos/network/network_connection_handler.cc:234: FOR_EACH_OBSERVER(Observer,
observers_, ConnectToNetworkRequested());
As long as we are adding this we should pass the service_path here. There may be
UI elements (e.g. notifications) that could take advantage of this notification.

[pneubeck (no reviews), 2014-11-08 16:49:35]

Patchset #3 (id:180001) has been deleted

[pneubeck (no reviews), 2014-11-08 16:49:47]

The CQ bit was checked by pneubeck@chromium.org

[pneubeck (no reviews), 2014-11-08 16:49:55]

https://codereview.chromium.org/701863002/diff/160001/chromeos/network/auto_c...
File chromeos/network/auto_connect_handler.cc (right):

https://codereview.chromium.org/701863002/diff/160001/chromeos/network/auto_c...
chromeos/network/auto_connect_handler.cc:105: }
On 2014/11/07 17:19:06, stevenjb wrote:
> nit: invert this logic to be consistent with the logic below (and slightly
more
> readable, positive result, which in this case is !empty(), before negative).

Done.

https://codereview.chromium.org/701863002/diff/160001/chromeos/network/auto_c...
chromeos/network/auto_connect_handler.cc:159:
network_state_handler_->ConnectToBestWifiNetwork();
On 2014/11/07 17:19:06, stevenjb wrote:
> Should we remove this from NSH/SPH and make the Shill call here? I'm concerned
> that if we leave it in NSH some other logic could conflict and break this
logic,
> i.e. I think we should only call ConnectToBestWifiNetwork from one place.

can't move it without further changes. NSH is at first triggering a network scan
and then calling ConnectToBestServices.

let's punt that.

https://codereview.chromium.org/701863002/diff/160001/chromeos/network/auto_c...
chromeos/network/auto_connect_handler.cc:167:
managed_configuration_handler_->GetGlobalConfigFromPolicy(std::string());
On 2014/11/07 17:19:06, stevenjb wrote:
> nit: comment what std::string() represents

Done.

https://codereview.chromium.org/701863002/diff/160001/chromeos/network/auto_c...
File chromeos/network/auto_connect_handler_unittest.cc (right):

https://codereview.chromium.org/701863002/diff/160001/chromeos/network/auto_c...
chromeos/network/auto_connect_handler_unittest.cc:49: void
ConfigureCallback(const dbus::ObjectPath& result) {
On 2014/11/07 17:19:06, stevenjb wrote:
> nit: Might be worth making these member functions that take an 'expected'
> argument and ASSERT(expected), or somesuch.

these are only used for the test setup and don't test the Class-Under-Test.
Anyways, as the positive result is expected, I log an error in the failure case.

https://codereview.chromium.org/701863002/diff/160001/chromeos/network/networ...
File chromeos/network/network_connection_handler.cc (right):

https://codereview.chromium.org/701863002/diff/160001/chromeos/network/networ...
chromeos/network/network_connection_handler.cc:234: FOR_EACH_OBSERVER(Observer,
observers_, ConnectToNetworkRequested());
On 2014/11/07 17:19:06, stevenjb wrote:
> As long as we are adding this we should pass the service_path here. There may
be
> UI elements (e.g. notifications) that could take advantage of this
notification.

Done.

[commit-bot: I haz the power, 2014-11-08 16:50:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/701863002/200001

[commit-bot: I haz the power, 2014-11-08 17:25:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-08 17:25:09]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pneubeck (no reviews), 2014-11-08 17:29:10]

Patchset #3 (id:200001) has been deleted

[pneubeck (no reviews), 2014-11-08 17:29:15]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-11-08 17:29:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/701863002/220001

[commit-bot: I haz the power, 2014-11-08 20:36:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-08 20:36:54]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pneubeck (no reviews), 2014-11-08 20:39:50]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-11-08 20:40:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/701863002/220001

[commit-bot: I haz the power, 2014-11-08 23:59:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-08 23:59:10]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pneubeck (no reviews), 2014-11-09 08:57:23]

The CQ bit was checked by pneubeck@chromium.org

[pneubeck (no reviews), 2014-11-09 08:57:25]

The CQ bit was unchecked by pneubeck@chromium.org

[pneubeck (no reviews), 2014-11-09 09:44:15]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-11-09 09:45:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/701863002/220001

[commit-bot: I haz the power, 2014-11-09 13:05:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-09 13:06:00]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pneubeck (no reviews), 2014-11-10 10:06:15]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-11-10 10:06:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/701863002/220001

[pneubeck (no reviews), 2014-11-10 13:04:03]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-11-10 13:04:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/701863002/240001

[commit-bot: I haz the power, 2014-11-10 13:48:52]

Committed patchset #4 (id:240001)

[commit-bot: I haz the power, 2014-11-10 13:49:36]

Patchset 4 (id:??) landed as
https://crrev.com/627248e4ecdfcc2f83de079ab51eb4db12352baa
Cr-Commit-Position: refs/heads/master@{#303457}