Skip to content

Fix for Vlan doesn't match issue while adding IP range for the shared network without any IP range #10837

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

Open
wants to merge 1 commit into
base: 4.19
Choose a base branch
from
+61 −39
Open

Fix for Vlan doesn't match issue while adding IP range for the shared network without any IP range #10837

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 0 additions & 3 deletions api/src/main/java/org/apache/cloudstack/api/command/admin/vlan/CreateVlanIpRangeCmd.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,9 +155,6 @@ public String getStartIp() {
}

public String getVlan() {
if (vlan == null || vlan.isEmpty()) {
vlan = "untagged";
}
return vlan;
}

Expand Down
91 changes: 55 additions & 36 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 @@ -4407,14 +4407,35 @@
String endIP = cmd.getEndIp();
final String newVlanGateway = cmd.getGateway();
final String newVlanNetmask = cmd.getNetmask();
Long networkId = cmd.getNetworkID();
Long physicalNetworkId = cmd.getPhysicalNetworkId();

// Verify that network exists
Network network = null;
if (networkId != null) {
network = _networkDao.findById(networkId);
if (network == null) {
throw new InvalidParameterValueException("Unable to find network by id " + networkId);

Check warning on line 4418 in server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java#L4418 

Added line #L4418 was not covered by tests
} else {
zoneId = network.getDataCenterId();
physicalNetworkId = network.getPhysicalNetworkId();
}
}

String vlanId = cmd.getVlan();
if (StringUtils.isBlank(vlanId)) {
vlanId = Vlan.UNTAGGED;
if (network != null & network.getTrafficType() == TrafficType.Guest) {
boolean connectivityWithoutVlan = isConnectivityWithoutVlan(network);
vlanId = getNetworkVlanId(network, connectivityWithoutVlan);

Check warning on line 4430 in server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java#L4429-L4430 

Added lines #L4429 - L4430 were not covered by tests
}
}

// TODO decide if we should be forgiving or demand a valid and complete URI
if (!(vlanId == null || "".equals(vlanId) || vlanId.startsWith(BroadcastDomainType.Vlan.scheme()))) {
vlanId = BroadcastDomainType.Vlan.toUri(vlanId).toString();
}
final Boolean forVirtualNetwork = cmd.isForVirtualNetwork();
Copy link
Member

Choose a reason for hiding this comment

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

does this need to be cleaned up ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no @weizhouapache, vlanId from the network can be empty/null.

Long networkId = cmd.getNetworkID();
Long physicalNetworkId = cmd.getPhysicalNetworkId();
final String accountName = cmd.getAccountName();
final Long projectId = cmd.getProjectId();
final Long domainId = cmd.getDomainId();
Expand Down Expand Up @@ -4487,18 +4508,6 @@
}
}

// Verify that network exists
Network network = null;
if (networkId != null) {
network = _networkDao.findById(networkId);
if (network == null) {
throw new InvalidParameterValueException("Unable to find network by id " + networkId);
} else {
zoneId = network.getDataCenterId();
physicalNetworkId = network.getPhysicalNetworkId();
}
}

// Verify that zone exists
final DataCenterVO zone = _zoneDao.findById(zoneId);
if (zone == null) {
Expand Down Expand Up @@ -4847,28 +4856,8 @@
// same as network's vlan
// 2) if vlan is missing, default it to the guest network's vlan
if (network.getTrafficType() == TrafficType.Guest) {
String networkVlanId = null;
boolean connectivityWithoutVlan = false;
if (_networkModel.areServicesSupportedInNetwork(network.getId(), Service.Connectivity)) {
Map<Capability, String> connectivityCapabilities = _networkModel.getNetworkServiceCapabilities(network.getId(), Service.Connectivity);
connectivityWithoutVlan = MapUtils.isNotEmpty(connectivityCapabilities) && connectivityCapabilities.containsKey(Capability.NoVlan);
}

final URI uri = network.getBroadcastUri();
if (connectivityWithoutVlan) {
networkVlanId = network.getBroadcastDomainType().toUri(network.getUuid()).toString();
} else if (uri != null) {
// Do not search for the VLAN tag when the network doesn't support VLAN
if (uri.toString().startsWith("vlan")) {
final String[] vlan = uri.toString().split("vlan:\\/\\/");
networkVlanId = vlan[1];
// For pvlan
if (network.getBroadcastDomainType() != BroadcastDomainType.Vlan) {
networkVlanId = networkVlanId.split("-")[0];
}
}
}

boolean connectivityWithoutVlan = isConnectivityWithoutVlan(network);
String networkVlanId = getNetworkVlanId(network, connectivityWithoutVlan);

Check warning on line 4860 in server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java#L4859-L4860 

Added lines #L4859 - L4860 were not covered by tests
if (vlanId != null && !connectivityWithoutVlan) {
// if vlan is specified, throw an error if it's not equal to
// network's vlanId
Expand Down Expand Up @@ -5000,6 +4989,36 @@
return vlan;
}

private boolean isConnectivityWithoutVlan(Network network) {
boolean connectivityWithoutVlan = false;

Check warning on line 4993 in server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java#L4992-L4993 

Added lines #L4992 - L4993 were not covered by tests
if (_networkModel.areServicesSupportedInNetwork(network.getId(), Service.Connectivity)) {
Map<Capability, String> connectivityCapabilities = _networkModel.getNetworkServiceCapabilities(network.getId(), Service.Connectivity);

Check warning on line 4995 in server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java#L4995 

Added line #L4995 was not covered by tests
connectivityWithoutVlan = MapUtils.isNotEmpty(connectivityCapabilities) && connectivityCapabilities.containsKey(Capability.NoVlan);
}
return connectivityWithoutVlan;
}

Check warning on line 4999 in server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java#L4998-L4999 

Added lines #L4998 - L4999 were not covered by tests

private String getNetworkVlanId(Network network, boolean connectivityWithoutVlan) {
String networkVlanId = null;

Check warning on line 5002 in server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java#L5001-L5002 

Added lines #L5001 - L5002 were not covered by tests
if (connectivityWithoutVlan) {
return network.getBroadcastDomainType().toUri(network.getUuid()).toString();

Check warning on line 5004 in server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java#L5004 

Added line #L5004 was not covered by tests
}

final URI uri = network.getBroadcastUri();

Check warning on line 5007 in server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java#L5007 

Added line #L5007 was not covered by tests
if (uri != null) {
// Do not search for the VLAN tag when the network doesn't support VLAN
if (uri.toString().startsWith("vlan")) {
final String[] vlan = uri.toString().split("vlan:\\/\\/");
networkVlanId = vlan[1];

Check warning on line 5012 in server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java#L5011-L5012 

Added lines #L5011 - L5012 were not covered by tests
// For pvlan
if (network.getBroadcastDomainType() != BroadcastDomainType.Vlan) {
networkVlanId = networkVlanId.split("-")[0];

Check warning on line 5015 in server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java#L5015 

Added line #L5015 was not covered by tests
}
}
}
return networkVlanId;
}

Check warning on line 5020 in server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java#L5019-L5020 

Added lines #L5019 - L5020 were not covered by tests

private void checkZoneVlanIpOverlap(DataCenterVO zone, Network network, String newCidr, String vlanId, String vlanGateway, String vlanNetmask, String startIP, String endIP) {
// Throw an exception if this subnet overlaps with subnet on other VLAN,
// if this is ip range extension, gateway, network mask should be same and ip range should not overlap
Expand Down
6 changes: 6 additions & 0 deletions server/src/test/java/com/cloud/configuration/ConfigurationManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@
import com.cloud.network.dao.IPAddressDao;
import com.cloud.network.dao.IPAddressVO;
import com.cloud.network.dao.Ipv6GuestPrefixSubnetNetworkMapDao;
import com.cloud.network.dao.NetworkDao;
import com.cloud.network.dao.NetworkVO;
import com.cloud.network.dao.PhysicalNetworkDao;
import com.cloud.network.dao.PhysicalNetworkVO;
import com.cloud.offering.DiskOffering;
Expand Down Expand Up @@ -189,6 +191,8 @@ public class ConfigurationManagerTest {
@Mock
HostPodDao _podDao;
@Mock
NetworkDao _networkDao;
@Mock
PhysicalNetworkDao _physicalNetworkDao;
@Mock
ImageStoreDao _imageStoreDao;
Expand Down Expand Up @@ -1325,6 +1329,8 @@ public void testDisabledConfigCreateIpv6NetworkOffering() {
public void testWrongIpv6CreateVlanAndPublicIpRange() {
CreateVlanIpRangeCmd cmd = Mockito.mock(CreateVlanIpRangeCmd.class);
Mockito.when(cmd.getIp6Cidr()).thenReturn("fd17:5:8a43:e2a4:c000::/66");
NetworkVO network = Mockito.mock(NetworkVO.class);
Mockito.when(_networkDao.findById(Mockito.anyLong())).thenReturn(network);
try {
configurationMgr.createVlanAndPublicIpRange(cmd);
} catch (InsufficientCapacityException | ResourceUnavailableException | ResourceAllocationException e) {
Expand Down
Loading