Translate Saved/StaticIPConfig properties from ONC to Shill

Translate Saved/StaticIPConfig properties from ONC to Shill

BUG=279351
Committed: https://crrev.com/6eae03953c8520292cf1ecdacfda6a171a3dbd4c
Cr-Commit-Position: refs/heads/master@{#293331}

[stevenjb, 2014-09-03 22:34:56]

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

[stevenjb, 2014-09-03 22:34:56]

This turned out to be kind of a pita with the way Shill handles nameservers for
Static/Saved IP, but this seems to work.

BTW, WebProxyAutoDiscoveryUrl is only in ipconfig-api.txt, so it only exists in
the IPConfigs[] property, not SavedIPConfig.

[pneubeck (no reviews), 2014-09-04 12:38:45]

Thanks, lgtm! with nits

https://codereview.chromium.org/540613002/diff/1/chromeos/network/onc/onc_tra...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/540613002/diff/1/chromeos/network/onc/onc_tra...
chromeos/network/onc/onc_translator_shill_to_onc.cc:447: // Saved IP config
nameservers are stored as a comma separated list.
nit: Saved and static ...

https://codereview.chromium.org/540613002/diff/1/chromeos/test/data/network/s...
File chromeos/test/data/network/shill_ethernet_with_ipconfig.json (right):

https://codereview.chromium.org/540613002/diff/1/chromeos/test/data/network/s...
chromeos/test/data/network/shill_ethernet_with_ipconfig.json:23:
"SavedIP.Address":"456.456.456.456",
better use a valid ipaddress ;-)

https://codereview.chromium.org/540613002/diff/1/chromeos/test/data/network/s...
chromeos/test/data/network/shill_ethernet_with_ipconfig.json:25:
"SavedIP.NameServers":"1.1.1.5,1.1.1.6",
i'll file a bug for this to be fixed in Shill

https://codereview.chromium.org/540613002/diff/1/chromeos/test/data/network/s...
chromeos/test/data/network/shill_ethernet_with_ipconfig.json:27:
"StaticIP.Address":"789.789.789.789",
dito

https://codereview.chromium.org/540613002/diff/1/chromeos/test/data/network/t...
File chromeos/test/data/network/translation_of_shill_ethernet_with_ipconfig.onc
(right):

https://codereview.chromium.org/540613002/diff/1/chromeos/test/data/network/t...
chromeos/test/data/network/translation_of_shill_ethernet_with_ipconfig.onc:27:
"SavedIPConfig":{
could you add the opposite test from ONC to Shill as well for StaticIPConfig
without 'NameServers'?

Then one more part would reliably work already.

https://codereview.chromium.org/540613002/diff/1/components/onc/docs/onc_spec...
File components/onc/docs/onc_spec.html (right):

https://codereview.chromium.org/540613002/diff/1/components/onc/docs/onc_spec...
components/onc/docs/onc_spec.html:270: IPConfig property that overrides
individual IPConfig parameters received
nit:

Somehow I find this still a little bit hard to read.
How about:

Each property set in this IPConfig object overrides the respective parameter
received over DHCP.

https://codereview.chromium.org/540613002/diff/1/components/onc/docs/onc_spec...
components/onc/docs/onc_spec.html:559: (optional if part of <span
class="field">IPConfig</span>)
sorry, the field name is IPConfigs (note the plural)

[pneubeck (no reviews), 2014-09-04 12:41:13]

https://codereview.chromium.org/540613002/diff/1/chromeos/network/onc/onc_sig...
File chromeos/network/onc/onc_signature.cc (right):

https://codereview.chromium.org/540613002/diff/1/chromeos/network/onc/onc_sig...
chromeos/network/onc/onc_signature.cc:293: // supported for reading network
state (shill -> onc).
aah.
kIPConfigs and kSavedIPConfig should be moved to network_with_state_fields as
they're read only.

https://codereview.chromium.org/540613002/diff/1/chromeos/test/data/network/t...
File chromeos/test/data/network/translation_of_shill_ethernet_with_ipconfig.onc
(right):

https://codereview.chromium.org/540613002/diff/1/chromeos/test/data/network/t...
chromeos/test/data/network/translation_of_shill_ethernet_with_ipconfig.onc:27:
"SavedIPConfig":{
On 2014/09/04 12:38:45, pneubeck wrote:
> could you add the opposite test from ONC to Shill as well for StaticIPConfig
> without 'NameServers'?
> 
> Then one more part would reliably work already.

you could add it to the existing ethernet.onc and shill_ethernet.json

[stevenjb, 2014-09-04 15:26:22]

https://codereview.chromium.org/540613002/diff/1/chromeos/network/onc/onc_sig...
File chromeos/network/onc/onc_signature.cc (right):

https://codereview.chromium.org/540613002/diff/1/chromeos/network/onc/onc_sig...
chromeos/network/onc/onc_signature.cc:293: // supported for reading network
state (shill -> onc).
On 2014/09/04 12:41:12, pneubeck wrote:
> aah.
> kIPConfigs and kSavedIPConfig should be moved to network_with_state_fields as
> they're read only.

Done.

https://codereview.chromium.org/540613002/diff/1/chromeos/network/onc/onc_tra...
File chromeos/network/onc/onc_translator_shill_to_onc.cc (right):

https://codereview.chromium.org/540613002/diff/1/chromeos/network/onc/onc_tra...
chromeos/network/onc/onc_translator_shill_to_onc.cc:447: // Saved IP config
nameservers are stored as a comma separated list.
On 2014/09/04 12:38:45, pneubeck wrote:
> nit: Saved and static ...

Done.

https://codereview.chromium.org/540613002/diff/1/chromeos/test/data/network/s...
File chromeos/test/data/network/shill_ethernet_with_ipconfig.json (right):

https://codereview.chromium.org/540613002/diff/1/chromeos/test/data/network/s...
chromeos/test/data/network/shill_ethernet_with_ipconfig.json:23:
"SavedIP.Address":"456.456.456.456",
On 2014/09/04 12:38:45, pneubeck wrote:
> better use a valid ipaddress ;-)

Done.

https://codereview.chromium.org/540613002/diff/1/chromeos/test/data/network/s...
chromeos/test/data/network/shill_ethernet_with_ipconfig.json:25:
"SavedIP.NameServers":"1.1.1.5,1.1.1.6",
On 2014/09/04 12:38:45, pneubeck wrote:
> i'll file a bug for this to be fixed in Shill

If we are going to "fix" this (I assume you mean comma-separated -> array), we
should just make SavedIP an IPConfig dictionary.

https://codereview.chromium.org/540613002/diff/1/chromeos/test/data/network/s...
chromeos/test/data/network/shill_ethernet_with_ipconfig.json:27:
"StaticIP.Address":"789.789.789.789",
On 2014/09/04 12:38:45, pneubeck wrote:
> dito
Done

https://codereview.chromium.org/540613002/diff/1/chromeos/test/data/network/t...
File chromeos/test/data/network/translation_of_shill_ethernet_with_ipconfig.onc
(right):

https://codereview.chromium.org/540613002/diff/1/chromeos/test/data/network/t...
chromeos/test/data/network/translation_of_shill_ethernet_with_ipconfig.onc:27:
"SavedIPConfig":{
On 2014/09/04 12:41:12, pneubeck wrote:
> On 2014/09/04 12:38:45, pneubeck wrote:
> > could you add the opposite test from ONC to Shill as well for StaticIPConfig
> > without 'NameServers'?
> > 
> > Then one more part would reliably work already.
> 
> you could add it to the existing ethernet.onc and shill_ethernet.json

Except I moved these to network_with_state_fields, isn't that shill -> onc only?

https://codereview.chromium.org/540613002/diff/1/components/onc/docs/onc_spec...
File components/onc/docs/onc_spec.html (right):

https://codereview.chromium.org/540613002/diff/1/components/onc/docs/onc_spec...
components/onc/docs/onc_spec.html:270: IPConfig property that overrides
individual IPConfig parameters received
On 2014/09/04 12:38:45, pneubeck wrote:
> nit:
> 
> Somehow I find this still a little bit hard to read.
> How about:
> 
> Each property set in this IPConfig object overrides the respective parameter
> received over DHCP.

Done.

https://codereview.chromium.org/540613002/diff/1/components/onc/docs/onc_spec...
components/onc/docs/onc_spec.html:559: (optional if part of <span
class="field">IPConfig</span>)
On 2014/09/04 12:38:45, pneubeck wrote:
> sorry, the field name is IPConfigs (note the plural)

Done.

[stevenjb, 2014-09-04 15:26:40]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-09-04 15:28:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/stevenjb@chromium.org/540613002/20001

[pneubeck (no reviews), 2014-09-04 15:29:06]

https://codereview.chromium.org/540613002/diff/1/chromeos/test/data/network/t...
File chromeos/test/data/network/translation_of_shill_ethernet_with_ipconfig.onc
(right):

https://codereview.chromium.org/540613002/diff/1/chromeos/test/data/network/t...
chromeos/test/data/network/translation_of_shill_ethernet_with_ipconfig.onc:27:
"SavedIPConfig":{
On 2014/09/04 15:26:21, stevenjb wrote:
> On 2014/09/04 12:41:12, pneubeck wrote:
> > On 2014/09/04 12:38:45, pneubeck wrote:
> > > could you add the opposite test from ONC to Shill as well for
StaticIPConfig
> > > without 'NameServers'?
> > > 
> > > Then one more part would reliably work already.
> > 
> > you could add it to the existing ethernet.onc and shill_ethernet.json
> 
> Except I moved these to network_with_state_fields, isn't that shill -> onc
only?

I wasn't very clear about this:

IPConfigs and SavedIPConfig should be read-only and StaticIPConfig read/write,
IIUC.
So,

IPConfigs and SavedIPConfig should be in network_with_state_fields and only this
test

StaticIPConfig should be in network_configuration_fields and also in a test
onc->shill.

[stevenjb, 2014-09-04 15:36:19]

The CQ bit was unchecked by stevenjb@chromium.org

[stevenjb, 2014-09-04 15:36:51]

On 2014/09/04 15:29:06, pneubeck wrote:
>
https://codereview.chromium.org/540613002/diff/1/chromeos/test/data/network/t...
> File
chromeos/test/data/network/translation_of_shill_ethernet_with_ipconfig.onc
> (right):
> 
>
https://codereview.chromium.org/540613002/diff/1/chromeos/test/data/network/t...
> chromeos/test/data/network/translation_of_shill_ethernet_with_ipconfig.onc:27:
> "SavedIPConfig":{
> On 2014/09/04 15:26:21, stevenjb wrote:
> > On 2014/09/04 12:41:12, pneubeck wrote:
> > > On 2014/09/04 12:38:45, pneubeck wrote:
> > > > could you add the opposite test from ONC to Shill as well for
> StaticIPConfig
> > > > without 'NameServers'?
> > > > 
> > > > Then one more part would reliably work already.
> > > 
> > > you could add it to the existing ethernet.onc and shill_ethernet.json
> > 
> > Except I moved these to network_with_state_fields, isn't that shill -> onc
> only?
> 
> I wasn't very clear about this:
> 
> IPConfigs and SavedIPConfig should be read-only and StaticIPConfig read/write,
> IIUC.
> So,
> 
> IPConfigs and SavedIPConfig should be in network_with_state_fields and only
this
> test
> 
> StaticIPConfig should be in network_configuration_fields and also in a test
> onc->shill.

Ah, yes, you're right. Need more coffee. I'll fix that and add StaticIPConfig to
onc -> shill.

[stevenjb, 2014-09-04 16:53:33]

The validation tests don't seem to recognize OncValueSignature.base_signature
and are failing unless I change k{Saved|Static}IPConfigSignature to use
ipconfig_fields directly instead of setting 'base_signature'. (See Patch Set 3).

Validation also fails if I move SavedIPConfig to network_with_state_fields. It's
not entirely clear to me why.

There appear to be all sorts of issues with IPConfigs vs. SavedIPConfig, so I
filed a separate issue to address that.

At this point we can either:
1. Accept this CL which currently passes the tests and you can create a
follow-up to address these properly at your leisure.
2. You can take over this CL and fix it properly.

This is going to block further commits so I would appreciate an option that we
can submit this week if possible, thanks!

[pneubeck (no reviews), 2014-09-04 17:48:32]

On 2014/09/04 16:53:33, stevenjb wrote:
> The validation tests don't seem to recognize OncValueSignature.base_signature
> and are failing unless I change k{Saved|Static}IPConfigSignature to use
> ipconfig_fields directly instead of setting 'base_signature'. (See Patch Set
3).
> 
> Validation also fails if I move SavedIPConfig to network_with_state_fields.
It's
> not entirely clear to me why.
> 
> There appear to be all sorts of issues with IPConfigs vs. SavedIPConfig, so I
> filed a separate issue to address that.
> 
> At this point we can either:
> 1. Accept this CL which currently passes the tests and you can create a
> follow-up to address these properly at your leisure.
sounds good to me.

> 2. You can take over this CL and fix it properly.
> 
> This is going to block further commits so I would appreciate an option that we
> can submit this week if possible, thanks!
yeah, please go ahead.

[stevenjb, 2014-09-04 17:54:01]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-09-04 17:55:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/stevenjb@chromium.org/540613002/40001

[commit-bot: I haz the power, 2014-09-04 20:50:12]

Committed patchset #3 (id:40001) as 41ec57e48ebd467c7a3b169a5598fb8038f54232

[commit-bot: I haz the power, 2014-09-10 03:32:30]

Patchset 3 (id:??) landed as
https://crrev.com/6eae03953c8520292cf1ecdacfda6a171a3dbd4c
Cr-Commit-Position: refs/heads/master@{#293331}