Skip to content

server: fix the subnet overlap checking logic for tagged and untagged vlans when adding ipranges #3430

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
yadvr merged 4 commits into from
Jul 22, 2019
Merged

server: fix the subnet overlap checking logic for tagged and untagged vlans when adding ipranges #3430

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4b1a66f
Compare VLAN names instead of expanding to range
Jun 27, 2019
3e4fd5d
For untagged VLANs we need to verify by expanding ranges
Jun 28, 2019
069dd9a
Fix comment description
Jun 28, 2019
df4eae8
Remove redundant spaces
Jul 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,6 @@
import com.cloud.utils.NumbersUtil;
import com.cloud.utils.Pair;
import com.cloud.utils.StringUtils;
import com.cloud.utils.UriUtils;
import com.cloud.utils.component.ManagerBase;
import com.cloud.utils.db.DB;
import com.cloud.utils.db.EntityManager;
Expand Down Expand Up @@ -3542,9 +3541,7 @@ public Vlan createVlanAndPublicIpRange(final long zoneId, final long networkId,
continue;
}
// from here, subnet overlaps
if (!UriUtils.checkVlanUriOverlap(
BroadcastDomainType.getValue(BroadcastDomainType.fromString(vlanId)),
BroadcastDomainType.getValue(BroadcastDomainType.fromString(vlan.getVlanTag())))) {
if (!vlanId.equals(vlan.getVlanTag())) {
Copy link
Copy Markdown
Member

@yadvr yadvr Jun 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this may cause an issue, because the vlan.getVlanTag() could be something like a range 100-200, so if vlanId is 101 the check should be done in the range. Let me revisit this shortly and check if the overlap check is necessary or not.

Copy link
Copy Markdown
Member

@yadvr yadvr Jun 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or perhaps if the issue is just for the 'untagged' VLAN, a specific check/condition could be added. This is because sometimes people add address ranges belonging to a VLAN (say vlan://101) instead of vlan://untagged (or untagged)

boolean overlapped = false;
if( network.getTrafficType() == TrafficType.Public ) {
Copy link
Copy Markdown
Contributor

@shwstppr shwstppr Jul 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code formatting for this section of code is different ( spaces used )

Copy link
Copy Markdown
Contributor Author

@anuragaw anuragaw Jul 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean if( network.getTrafficType().... should be reformatted to if (network.getTrafficType()....?

overlapped = true;
Expand Down