Add Shill to ONC translation of IPConfig.

Add Shill to ONC translation of IPConfig.

So far, the ONC IPConfig object was only validated but not translated to or from Shill.
This adds the one direction, from Shill to ONC.

This allows exposing the current IP config state of Shill through ONC, e.g. with in the networkingPrivate API.

BUG=288288
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266848

[pneubeck (no reviews), 2014-04-23 14:14:41]

@Arman, you know this code already. Please take a look.

@both of you:
As far as I can tell, we currently use a Shill service's
shill::kIPConfigProperty to obtain the current ip configuration, see
shill_property_handler.cc:494 .

If NetworkConfigurationHandler would substitute the IPConfig path by the
IPConfig object in a similar way, this translation should already be what you
want.

But please double check whether I used the right Shill properties.

[armansito, 2014-04-23 18:46:35]

ONC looks good overall, I just commented on a few nits. As for whether or not
the correct shill properties are being used, I would check with quiche@, so I
added him as a reviewer.

quiche@: PTAL at chromeos/network/onc/onc_translation_tables.cc and check if the
shill properties listed in |ipconfig_fields| are correct.

https://codereview.chromium.org/249193003/diff/50001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_signature.cc (right):

https://codereview.chromium.org/249193003/diff/50001/chromeos/network/onc/onc...
chromeos/network/onc/onc_signature.cc:166: // Not supported for policy but for
reading network state.
So these will be reported by network state but is unused for policy? Just wanted
to clarify.

https://codereview.chromium.org/249193003/diff/50001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/249193003/diff/50001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:315: if
(!onc_ipconfig->empty()) {
Early return here would be better, i.e if (empty) return;

https://codereview.chromium.org/249193003/diff/50001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:328: &ip_address)) {
nit: align "&ip_address" with "shill::kAddressProperty" (looks unaligned in my
browser).

https://codereview.chromium.org/249193003/diff/50001/chromeos/test/data/netwo...
File chromeos/test/data/network/shill_ethernet.json (right):

https://codereview.chromium.org/249193003/diff/50001/chromeos/test/data/netwo...
chromeos/test/data/network/shill_ethernet.json:4: "Address":"123.123.123.123",
You should add a test case for an IPConfig with an IPv6 address as well.

[stevenjb, 2014-04-23 19:43:42]

+zork@ FYI

https://codereview.chromium.org/249193003/diff/50001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translation_tables.cc (right):

https://codereview.chromium.org/249193003/diff/50001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translation_tables.cc:180: {
::onc::ipconfig::kNameServers, shill::kNameServersProperty},
We don't need to do this here, but it turns out we are going to need a "Method"
property, which (for Chrome's purposes) can be either "ipv4" or "ipv6".

https://codereview.chromium.org/249193003/diff/50001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/249193003/diff/50001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:310: // Create a list with a
single entry.
So, I think we should fudge this. In Shill, Service.IPConfig is an Object, but
Device.IPConfigs is an array{object}. In order to support ipv6 we will need an
array, so I think Chrome should always use "IPConfigs" and if we fetch these
properties from Service we will put them into a single entry array (ListValue),
but eventually we will need to always fetch them from Device.

https://codereview.chromium.org/249193003/diff/50001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:330: if
(ip_address.find(':') != std::string::npos) {
Hmm, I guess wwe can use this for identifying ipv6, but I think we're better off
tracking 'Method'.

[mukesh agrawal, 2014-04-23 21:33:14]

On 2014/04/23 18:46:35, armansito wrote:
> ONC looks good overall, I just commented on a few nits. As for whether or not
> the correct shill properties are being used, I would check with quiche@, so I
> added him as a reviewer.
> 
> quiche@: PTAL at chromeos/network/onc/onc_translation_tables.cc and check if
the
> shill properties listed in |ipconfig_fields| are correct.

the shill properties look right to me.

[pneubeck (no reviews), 2014-04-25 10:07:34]

https://codereview.chromium.org/249193003/diff/50001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_signature.cc (right):

https://codereview.chromium.org/249193003/diff/50001/chromeos/network/onc/onc...
chromeos/network/onc/onc_signature.cc:166: // Not supported for policy but for
reading network state.
On 2014/04/23 18:46:36, armansito wrote:
> So these will be reported by network state but is unused for policy? Just
wanted
> to clarify.
Yes.
- They are accepted during validation of any ONC (in particular policy). That
is, because at some time I completed the signature to what the onc_spec.html
says.

- These are never written to Shill (no matter what source).

- As policy they are just ignored and never applied (e.g. never merged with user
settings).

https://codereview.chromium.org/249193003/diff/50001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translation_tables.cc (right):

https://codereview.chromium.org/249193003/diff/50001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translation_tables.cc:180: {
::onc::ipconfig::kNameServers, shill::kNameServersProperty},
On 2014/04/23 19:43:42, stevenjb wrote:
> We don't need to do this here, but it turns out we are going to need a
"Method"
> property, which (for Chrome's purposes) can be either "ipv4" or "ipv6".

See changes in onc_translator_shill_to_onc.cc
Just for clarification, Shill calls this property 'Method'.
ONC calls it 'Type'.

https://codereview.chromium.org/249193003/diff/50001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/249193003/diff/50001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:310: // Create a list with a
single entry.
On 2014/04/23 19:43:42, stevenjb wrote:
> So, I think we should fudge this. In Shill, Service.IPConfig is an Object, but
> Device.IPConfigs is an array{object}. In order to support ipv6 we will need an
> array, so I think Chrome should always use "IPConfigs" and if we fetch these
> properties from Service we will put them into a single entry array
(ListValue),
> but eventually we will need to always fetch them from Device.
> 

Done.
You'll be able to use this translation
 - if an IPConfigS is embedded in a Service
 - or on a standalone IPConfig dictionary (just use the exposed
kIPConfigSignature)
 - but not yet for a stand-alone array of IPConfig objects.

For the latter, we could add a TranslateShillArrayToONCArray function. You can
also repeatedly translate the individual IPConfig dictionaries.

An IPConfig (without 's') _field_ embedded within a Service will not be handled
by the translator.

https://codereview.chromium.org/249193003/diff/50001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:315: if
(!onc_ipconfig->empty()) {
On 2014/04/23 18:46:36, armansito wrote:
> Early return here would be better, i.e if (empty) return;

I kept the pattern in this function, which doesn't returns in order to translate
as much as possible, not early returning on errors.

I'd rather like to keep it this way.
(I adapted TranslateIPConfig)

https://codereview.chromium.org/249193003/diff/50001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:328: &ip_address)) {
On 2014/04/23 18:46:36, armansito wrote:
> nit: align "&ip_address" with "shill::kAddressProperty" (looks unaligned in my
> browser).

Done.

https://codereview.chromium.org/249193003/diff/50001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:330: if
(ip_address.find(':') != std::string::npos) {
On 2014/04/23 19:43:42, stevenjb wrote:
> Hmm, I guess wwe can use this for identifying ipv6, but I think we're better
off
> tracking 'Method'.

Missed that one. Thanks.

https://codereview.chromium.org/249193003/diff/50001/chromeos/test/data/netwo...
File chromeos/test/data/network/shill_ethernet.json (right):

https://codereview.chromium.org/249193003/diff/50001/chromeos/test/data/netwo...
chromeos/test/data/network/shill_ethernet.json:4: "Address":"123.123.123.123",
On 2014/04/23 18:46:36, armansito wrote:
> You should add a test case for an IPConfig with an IPv6 address as well.

Done.

[stevenjb, 2014-04-25 16:52:19]

lgtm, cheers!

[armansito, 2014-04-25 21:59:26]

lgtm

[pneubeck (no reviews), 2014-04-27 07:37:18]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-04-27 07:37:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/249193003/110001

[commit-bot: I haz the power, 2014-04-27 08:19:49]

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

[commit-bot: I haz the power, 2014-04-27 08:19:49]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[pneubeck (no reviews), 2014-04-29 07:26:51]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-04-29 07:27:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/249193003/110001

[commit-bot: I haz the power, 2014-04-29 08:06:44]

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

[commit-bot: I haz the power, 2014-04-29 08:06:44]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[pneubeck (no reviews), 2014-04-29 08:59:47]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-04-29 09:00:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/249193003/130001

[commit-bot: I haz the power, 2014-04-29 11:29:28]

Change committed as 266848