Issue 749013003: ONC: Add IPAddressConfigType and NameServersConfigType

Issue 749013003: ONC: Add IPAddressConfigType and NameServersConfigType (Closed)

Created:
6 years ago by stevenjb

Modified:
5 years, 11 months ago

Reviewers:
pneubeck (no reviews)

CC:
chromium-reviews, stevenjb+watch_chromium.org, oshima+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

ONC: Add IPAddressConfigType and NameServersConfigType This CL introdcues IPAddressConfigType and NameServersConfigType If either of these is set to 'Static', a 'StaticIPConfig' object must be provided and must specify any non-default configuration. If neither is set to 'Static', the StaticIPConfig object will be ignored. Originally uploaded as https://codereview.chromium.org/750313003/ BUG=411289 Committed: https://crrev.com/75a3c1dca4598fd845b9871c56ac867eb9a25447 Cr-Commit-Position: refs/heads/master@{#310325}

Patch Set 1 #

Patch Set 2 : Add NameServersConfigType #

Patch Set 3 : . #

Total comments: 14

Patch Set 4 : Rebase #

Patch Set 5 : Feedback #

Patch Set 6 : A bit more clarification (hopefully) #

Total comments: 4

Patch Set 7 : Alywas use empty dictionary to clear StaticIPConfig properties #

Created: 5 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+206 lines, -9 lines)			Patch
M	chromeos/network/onc/onc_merger.cc	View	1	1 chunk	+3 lines, -1 line	0 comments	Download
M	chromeos/network/onc/onc_normalizer.cc	View	1 2 3 4 5 6	1 chunk	+13 lines, -0 lines	0 comments	Download
M	chromeos/network/onc/onc_normalizer_unittest.cc	View		1 chunk	+17 lines, -0 lines	0 comments	Download
M	chromeos/network/onc/onc_signature.cc	View	1 2 3	1 chunk	+2 lines, -0 lines	0 comments	Download
M	chromeos/network/onc/onc_translator_onc_to_shill.cc	View	1 2 3 4 5 6	1 chunk	+13 lines, -0 lines	0 comments	Download
M	chromeos/network/onc/onc_translator_shill_to_onc.cc	View	1 2 3 4	1 chunk	+22 lines, -3 lines	0 comments	Download
M	chromeos/network/onc/onc_translator_unittest.cc	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	chromeos/network/onc/onc_validator.cc	View	1 2 3	2 chunks	+20 lines, -0 lines	0 comments	Download
M	chromeos/test/data/network/augmented_merge.json	View	1	1 chunk	+6 lines, -1 line	0 comments	Download
M	chromeos/test/data/network/device_policy.onc	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	chromeos/test/data/network/managed_toplevel1.onc	View	1	1 chunk	+2 lines, -0 lines	0 comments	Download
M	chromeos/test/data/network/settings_with_normalization.json	View		1 chunk	+20 lines, -0 lines	0 comments	Download
A	chromeos/test/data/network/shill_wifi_dhcp.json	View	1 2 3 4 5 6	1 chunk	+10 lines, -0 lines	0 comments	Download
M	chromeos/test/data/network/toplevel_wifi_open.onc	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	chromeos/test/data/network/translation_of_shill_ethernet.onc	View	1	1 chunk	+2 lines, -0 lines	0 comments	Download
M	chromeos/test/data/network/translation_of_shill_ethernet_with_ipconfig.onc	View	1	2 chunks	+2 lines, -0 lines	0 comments	Download
M	chromeos/test/data/network/user.onc	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
A	chromeos/test/data/network/wifi_dhcp.onc	View	1 2 3 4 5 6	1 chunk	+12 lines, -0 lines	0 comments	Download
M	components/onc/docs/onc_spec.html	View	1 2 3 4 5	4 chunks	+50 lines, -4 lines	0 comments	Download
M	components/onc/onc_constants.h	View	1 2 3	1 chunk	+4 lines, -0 lines	0 comments	Download
M	components/onc/onc_constants.cc	View	1 2 3	1 chunk	+4 lines, -0 lines	0 comments	Download

Messages

Total messages: 14 (2 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

pneubeck (no reviews)

please rebase on my cleanup CL (https://codereview.chromium.org/830133003/) and make the documentation a bit more precise. ...

5 years, 11 months ago (2015-01-04 11:18:01 UTC) #4

stevenjb

Settings is always going to set IPAddress + RoutingPrefix + Gateway together, so for now ...

5 years, 11 months ago (2015-01-05 21:16:19 UTC) #5

Settings is always going to set IPAddress + RoutingPrefix + Gateway together, so
for now I think we should require all three if IPConfigType is Static.

https://codereview.chromium.org/749013003/diff/40001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translator_onc_to_shill.cc (right):

https://codereview.chromium.org/749013003/diff/40001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_onc_to_shill.cc:273:
base::Value::CreateNullValue());
On 2015/01/04 11:18:01, pneubeck wrote:
> can't we just set an empty dictionary? i thought that was what Paul said, that
> it would clear the property?

Setting this to an empty Dictionary means changing a lot of test expectations
(adding a StaticIPConfig entry with a {} value). I think this is a tiny bit more
clear, but it is extra code, so I could be persuaded to always set
StaticIPConfig to an empty dictionary and change the expectations.

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

https://codereview.chromium.org/749013003/diff/40001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_shill_to_onc.cc:501: const base::ListValue*
name_servers;
On 2015/01/04 11:18:01, pneubeck wrote:
> please add the default nullptr-initialization.

Done.

https://codereview.chromium.org/749013003/diff/40001/components/onc/docs/onc_...
File components/onc/docs/onc_spec.html (right):

https://codereview.chromium.org/749013003/diff/40001/components/onc/docs/onc_...
components/onc/docs/onc_spec.html:312: non-default values in the object must be
specified.
On 2015/01/04 11:18:01, pneubeck wrote:
> The part 'any non-default values ...' is not clear to me.
> 
> Instead we should just mention which parts become required depending on
> IPAddressConfigType and depending on NameServersConfigType:
> 
> If <span class="field">IPAddressConfigType</span> is set to <span
> class="value">Static</span>, the fields <span..>IPAddress</span> and
> <span...>Gateway</span> of this object are required.
> 
> If <span class="field">NameServersConfigType</span> is set to <span
> class="value">Static</span>, the field <span..>NameServers</span> of this
object
> is required.
> 
> 
> Probably this documentation would be simpler if
nameservers/ipaddress(/gateway)
> wouldn't be mixed into a single object?

Done.

https://codereview.chromium.org/749013003/diff/40001/components/onc/docs/onc_...
components/onc/docs/onc_spec.html:575: (required)
On 2015/01/04 11:18:01, pneubeck wrote:
> at least, IPAddress should be marked 'optional'. In StaticIPConfig we could
> mention that this is required if IPAddressConfigType == Static.
Changed to (optional).

https://codereview.chromium.org/749013003/diff/40001/components/onc/docs/onc_...
components/onc/docs/onc_spec.html:611: <dt class="field">Gateway</dt>
On 2015/01/04 11:18:01, pneubeck wrote:
> how is Gateway handled now?
> is it always required together with IPAddress?
I don't know what Shill will do if Gateway is not specified, probably use a
default, but I will listed it as required if IPAddress is set, same as
RoutingPrefix, for now.

pneubeck (no reviews)

lgtm if you decide to go with the current version. https://codereview.chromium.org/749013003/diff/40001/chromeos/network/onc/onc_translator_onc_to_shill.cc File chromeos/network/onc/onc_translator_onc_to_shill.cc (right): https://codereview.chromium.org/749013003/diff/40001/chromeos/network/onc/onc_translator_onc_to_shill.cc#newcode260 ...

5 years, 11 months ago (2015-01-06 10:57:47 UTC) #6

stevenjb

PTAL https://codereview.chromium.org/749013003/diff/40001/chromeos/network/onc/onc_translator_onc_to_shill.cc File chromeos/network/onc/onc_translator_onc_to_shill.cc (right): https://codereview.chromium.org/749013003/diff/40001/chromeos/network/onc/onc_translator_onc_to_shill.cc#newcode260 chromeos/network/onc/onc_translator_onc_to_shill.cc:260: // If both IPAddressConfigType and NameServersConfigType are set ...

5 years, 11 months ago (2015-01-06 17:12:31 UTC) #7

PTAL

https://codereview.chromium.org/749013003/diff/40001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_translator_onc_to_shill.cc (right):

https://codereview.chromium.org/749013003/diff/40001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_onc_to_shill.cc:260: // If both
IPAddressConfigType and NameServersConfigType are set to DHCP,
On 2015/01/06 10:57:46, pneubeck wrote:
> Just to clarify that we understand what this translation code will do:
> 
> If in a partial update either IPAddressConfigType or NameServersConfigType is
> _not_ set, then we will not clear the StaticIPConfig property and thus not
> change to the respective DHCP value.
> 
> Similarly, if in a partial update the ONC StaticIPConfig dict contains _only_
> the NameServers or only the IPAddress+RoutingPrefix+Gateway, then exactly
these
> values will be translated and the others will be reset to DHCP by Shill.

If *neither* Type is set, then StaticIPConfig will be unchanged.

If *both* Types are set to DHCP, then the property will be cleared.

If *either* Type is set to Static then we rely on the validator to ensure that a
StaticIPConfig object exists (with a TODO to ensure that the correct properties
are set).

I suppose to be robust we should specify an empty dictionary if either is set to
DHCP. I will add that (and try to clarify the comments).

I think that I agree that we should separate the NameServers property in the ONC
spec, but I'd rather see that done in a follow-up at this point.

https://codereview.chromium.org/749013003/diff/40001/chromeos/network/onc/onc...
chromeos/network/onc/onc_translator_onc_to_shill.cc:273:
base::Value::CreateNullValue());
On 2015/01/06 10:57:46, pneubeck wrote:
> On 2015/01/05 21:16:19, stevenjb wrote:
> > On 2015/01/04 11:18:01, pneubeck wrote:
> > > can't we just set an empty dictionary? i thought that was what Paul said,
> that
> > > it would clear the property?
> > 
> > Setting this to an empty Dictionary means changing a lot of test
expectations
> > (adding a StaticIPConfig entry with a {} value).
> Shouldn't it affect exactly the test cases that you modified in this CL?
> 
> > I think this is a tiny bit more
> > clear, but it is extra code, so I could be persuaded to always set
> > StaticIPConfig to an empty dictionary and change the expectations.
> 
> I would just hope that it could also lead to a simplification of
>
https://codereview.chromium.org/708563005/diff/320001/chromeos/network/networ...
> .

I think I misunderstood. The else clause I added should make this a little more
clear I hope.

pneubeck (no reviews)

https://codereview.chromium.org/749013003/diff/100001/chromeos/network/onc/onc_normalizer.cc File chromeos/network/onc/onc_normalizer.cc (right): https://codereview.chromium.org/749013003/diff/100001/chromeos/network/onc/onc_normalizer.cc#newcode186 chromeos/network/onc/onc_normalizer.cc:186: ::onc::network_config::kIPConfigTypeStatic)); not critical so i can do that in ...

5 years, 11 months ago (2015-01-07 09:23:48 UTC) #8

stevenjb

PTAL https://codereview.chromium.org/749013003/diff/100001/chromeos/network/onc/onc_normalizer.cc File chromeos/network/onc/onc_normalizer.cc (right): https://codereview.chromium.org/749013003/diff/100001/chromeos/network/onc/onc_normalizer.cc#newcode186 chromeos/network/onc/onc_normalizer.cc:186: ::onc::network_config::kIPConfigTypeStatic)); On 2015/01/07 09:23:48, pneubeck wrote: > not ...

5 years, 11 months ago (2015-01-07 17:28:03 UTC) #9

PTAL

https://codereview.chromium.org/749013003/diff/100001/chromeos/network/onc/on...
File chromeos/network/onc/onc_normalizer.cc (right):

https://codereview.chromium.org/749013003/diff/100001/chromeos/network/onc/on...
chromeos/network/onc/onc_normalizer.cc:186:
::onc::network_config::kIPConfigTypeStatic));
On 2015/01/07 09:23:48, pneubeck wrote:
> not critical so i can do that in a follow-up,
> but we should remove the NameServers field if NameServersConfigType=DHCP or
> unset
> and remove the IPAddress+RoutingPrefix+Gateway if IPAddressConfigType=DHCP or
> unset.

Added a TODO

https://codereview.chromium.org/749013003/diff/100001/chromeos/network/onc/on...
File chromeos/network/onc/onc_translator_onc_to_shill.cc (right):

https://codereview.chromium.org/749013003/diff/100001/chromeos/network/onc/on...
chromeos/network/onc/onc_translator_onc_to_shill.cc:265: if
((ip_address_config_type == ::onc::network_config::kIPConfigTypeDHCP) &&
On 2015/01/07 09:23:48, pneubeck wrote:
> I looked into the current version of network_configuration_handler.cc
> and it doesn't handle the NullValue yet. Is this even correct currently?
> 
> Why not just dropping this if-clause and keeping only the else part, i.e.
> 
> if ((ip_address_config_type == ::onc::network_config::kIPConfigTypeDHCP) ||
>  (name_servers_config_type == ::onc::network_config::kIPConfigTypeDHCP)) {
>  // If either type is set to DHCP, provide an empty dictionary to
>  // ensure that any unset properties are cleared. Note: if either type is
>  // specifed, the other type defaults to DHCP if not specified.
>  shill_dictionary_->SetWithoutPathExpansion(shill::kStaticIPConfigProperty,
>                                             new base::DictionaryValue);
> }

Bah, you're right. I think I need the NULL support for Cellular APN anyway, but
that change is in the follow-up CL that this CL was extracted from. Sigh. This
has dragged out way too long.

As long as we keep the second if(), the tests don't have to be updated since we
don't currently specify DHCP in any of the translation tests. I added a test
that does that.

commit-bot: I haz the power

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

5 years, 11 months ago (2015-01-07 17:54:50 UTC) #12

commit-bot: I haz the power

5 years, 11 months ago (2015-01-07 19:09:01 UTC) #14

Message was sent while issue was closed.

Patchset 7 (id:??) landed as
https://crrev.com/75a3c1dca4598fd845b9871c56ac867eb9a25447
Cr-Commit-Position: refs/heads/master@{#310325}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages