-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16758/#review31514
-----------------------------------------------------------
I am fine with this with the exception of one worry: 'vlan://untagged' may be
compared to 'untagged' at some points in the system. I would like to include a
change in NetUtils.java for this:
if (VLAN_UNTAGGED.equalsIgnoreCase(one) &&
VLAN_UNTAGGED.equalsIgnoreCase(other))
becomes
if (one.contains(VLAN_UNTAGGED) && other.contains(VLAN_UNTAGGED))
in isSameIsolationId(String,String)
- daan Hoogland
On Jan. 9, 2014, 6:52 p.m., Marcus Sorensen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16758/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2014, 6:52 p.m.)
>
>
> Review request for cloudstack and daan Hoogland.
>
>
> Bugs: CLOUDSTACK-5502
> https://issues.apache.org/jira/browse/CLOUDSTACK-5502
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> See conversation on mailing list and CLOUDSTACK-5502:
>
> To recap, in 4.2
> the UI would interpret no vlan entered as 'untagged' and pass that as
> a parameter. Undocumented or not, it was being used by the API's
> biggest customer.
>
> with 4.2, passing 'untagged' resulted in:
>
> Saving vlan range
> Vlan[untagged|12.12.12.1|255.255.255.0|null|null|12.12.12.10-12.12.12.20||null1004]
>
> with 4.3 AND 4.2, passing '' fails with 'Vlan id required',
> because untagged was changed to null, while an empty string fell
> through the null check and resulted in
> Saving vlan range
> Vlan[|12.12.12.1|255.255.255.0|null|null|12.12.12.10-12.12.12.20||null1004]
> which breaks later: Networks.java line 323, "Unable to convert to
> isolation URI:"
>
> with 4.3, minus patch aaf3979cf92518d3dc5587ea0192f4b3ce1e7866,
> passing vlan=untagged results in:
> Saving vlan range
> Vlan[vlan://untagged|12.12.12.1|255.255.255.0|null|null|12.12.12.10-12.12.12.20||null1004]
>
> Which is exactly the behavior we want to see for both "" and
> "untagged", if we want to maintain compatibility with previous public
> network deployments.
>
>
> Diffs
> -----
>
>
> api/src/org/apache/cloudstack/api/command/admin/vlan/CreateVlanIpRangeCmd.java
> 541da1e
> server/src/com/cloud/configuration/ConfigurationManagerImpl.java f9d282f
>
> Diff: https://reviews.apache.org/r/16758/diff/
>
>
> Testing
> -------
>
> Tested advanced zone deployments, passing both empty string for 'vlan' and
> 'untagged'
>
>
> Thanks,
>
> Marcus Sorensen
>
>
