Skip to content

Broadcast URI not set to vxlan, but vlan (Fix #3040) #4190

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 2 commits into from
Sep 24, 2020
Merged

Broadcast URI not set to vxlan, but vlan (Fix #3040) #4190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2c3a41c
Fix issue #3040: Broadcast URI not set to vxlan, but vlan
GabrielBrascher Jun 30, 2020
13314d3
Add PhysicalNetwork null validation on encodeVlanIdIntoBroadcastUri
GabrielBrascher Sep 21, 2020
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
49 changes: 36 additions & 13 deletions api/src/main/java/com/cloud/network/Networks.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.net.URISyntaxException;

import com.cloud.utils.exception.CloudRuntimeException;
import org.apache.commons.lang3.StringUtils;

/**
* Network includes all of the enums used within networking.
Expand Down Expand Up @@ -253,20 +254,42 @@ public static URI fromString(String candidate) {
Long.parseLong(candidate);
return Vlan.toUri(candidate);
} catch (NumberFormatException nfe) {
if (com.cloud.dc.Vlan.UNTAGGED.equalsIgnoreCase(candidate)) {
return Native.toUri(candidate);
}
try {
URI uri = new URI(candidate);
BroadcastDomainType tiep = getSchemeValue(uri);
if (tiep.scheme != null && tiep.scheme.equals(uri.getScheme())) {
return uri;
} else {
throw new CloudRuntimeException("string '" + candidate + "' has an unknown BroadcastDomainType.");
}
} catch (URISyntaxException e) {
throw new CloudRuntimeException("string is not a broadcast URI: " + candidate);
return getVlanUriWhenNumberFormatException(candidate);
}
}

/**
* This method is called in case of NumberFormatException is thrown when parsing the String into long
*/
private static URI getVlanUriWhenNumberFormatException(String candidate) {
if(StringUtils.isBlank(candidate)) {
throw new CloudRuntimeException("Expected VLAN or VXLAN but got a null isolation method");
}
if (com.cloud.dc.Vlan.UNTAGGED.equalsIgnoreCase(candidate)) {
return Native.toUri(candidate);
}
try {
URI uri = new URI(candidate);
BroadcastDomainType tiep = getSchemeValue(uri);
if (tiep.scheme != null && tiep.scheme.equals(uri.getScheme())) {
return uri;
} else {
throw new CloudRuntimeException("string '" + candidate + "' has an unknown BroadcastDomainType.");
}
} catch (URISyntaxException e) {
throw new CloudRuntimeException("string is not a broadcast URI: " + candidate);
}
}

/**
* Encodes a string into a BroadcastUri, according to the given BroadcastDomainType
*/
public static URI encodeStringIntoBroadcastUri(String candidate, BroadcastDomainType isolationMethod) {
try{
Long.parseLong(candidate);
return isolationMethod.toUri(candidate);
} catch (NumberFormatException nfe) {
return getVlanUriWhenNumberFormatException(candidate);
}
}
};
Expand Down
23 changes: 21 additions & 2 deletions ...tration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2279,7 +2279,7 @@ public Network createGuestNetwork(final long networkOfferingId, final String nam
}

if (vlanSpecified) {
URI uri = BroadcastDomainType.fromString(vlanId);
URI uri = encodeVlanIdIntoBroadcastUri(vlanId, pNtwk);
//don't allow to specify vlan tag used by physical network for dynamic vlan allocation
if (!(bypassVlanOverlapCheck && ntwkOff.getGuestType() == GuestType.Shared) && _dcDao.findVnet(zoneId, pNtwk.getId(), BroadcastDomainType.getValue(uri)).size() > 0) {
throw new InvalidParameterValueException("The VLAN tag " + vlanId + " is already being used for dynamic vlan allocation for the guest network in zone "
Expand Down Expand Up @@ -2424,7 +2424,7 @@ public Network doInTransaction(final TransactionStatus status) {
//Logical router's UUID provided as VLAN_ID
userNetwork.setVlanIdAsUUID(vlanIdFinal); //Set transient field
} else {
uri = BroadcastDomainType.fromString(vlanIdFinal);
uri = encodeVlanIdIntoBroadcastUri(vlanIdFinal, pNtwk);
}
userNetwork.setBroadcastUri(uri);
if (!vlanIdFinal.equalsIgnoreCase(Vlan.UNTAGGED)) {
Expand Down Expand Up @@ -2475,6 +2475,25 @@ public Network doInTransaction(final TransactionStatus status) {
return network;
}

/**
* Encodes VLAN/VXLAN ID into a Broadcast URI according to the isolation method from the Physical Network.
* @return Broadcast URI, e.g. 'vlan://vlan_ID' or 'vxlan://vlxan_ID'
*/
protected URI encodeVlanIdIntoBroadcastUri(String vlanId, PhysicalNetwork pNtwk) {
if (pNtwk == null) {
throw new InvalidParameterValueException(String.format("Failed to encode VLAN/VXLAN %s into a Broadcast URI. Physical Network cannot be null.", vlanId));
}

if(StringUtils.isNotBlank(pNtwk.getIsolationMethods().get(0))) {
Copy link
Member Author

@GabrielBrascher GabrielBrascher Jul 1, 2020

Choose a reason for hiding this comment

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

I got wondering if there could be more than one isolation method on a network. Considering that this is a List of isolation methods. I assume that to get into a VLAN conditional (the ones that this method is called) there will be only one isolation method, either VLAN or VXLAN.

Copy link
Member

@yadvr yadvr Sep 9, 2020

Choose a reason for hiding this comment

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

@GabrielBrascher Should we check if pNtwk is null? This could potentially cause a NPE; and the getIsolationMethods() is also a valid list with at least one item

Copy link
Member Author

@GabrielBrascher GabrielBrascher Sep 9, 2020

Choose a reason for hiding this comment

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

@rhtyd thanks for the review :-)

Answering your question: I did not add null verification on the respective parameter due to the fact that both methods that call this one already validate the private network

  1. the physical network pNtwk is validated at NetworkOrchestrator.java#L2196;

  2. the other method that calls encodeVlanIdIntoBroadcastUri also validates the private network at this line NetworkOrchestrator.java#L2397.

However, considering that this method could be reused, I have no problem in adding null validations.

Copy link
Member Author

@GabrielBrascher GabrielBrascher Sep 10, 2020

Choose a reason for hiding this comment

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

Actually, NetworkOrchestrator.java#L2397 is not exactly a null validation. I will add a null verification @rhtyd.

Copy link
Member

@yadvr yadvr Sep 23, 2020

Choose a reason for hiding this comment

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

@GabrielBrascher please add the null validation, thnx

String isolationMethod = pNtwk.getIsolationMethods().get(0).toLowerCase();
String vxlan = BroadcastDomainType.Vxlan.toString().toLowerCase();
if(isolationMethod.equals(vxlan)) {
return BroadcastDomainType.encodeStringIntoBroadcastUri(vlanId, BroadcastDomainType.Vxlan);
}
}
return BroadcastDomainType.fromString(vlanId);
}

/**
* Checks bypass VLAN id/range overlap check during network creation for guest networks
* @param bypassVlanOverlapCheck bypass VLAN id/range overlap check
Expand Down
74 changes: 74 additions & 0 deletions ...ion/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,15 @@
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import java.net.URI;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import com.cloud.network.dao.PhysicalNetworkVO;
import com.cloud.utils.exception.CloudRuntimeException;
import org.apache.log4j.Logger;
import org.junit.Assert;
import org.junit.Before;
Expand Down Expand Up @@ -463,4 +466,75 @@ public void validateLockedRequestedIpTestFreeAndNotNullIp() {
testOrchastrator.validateLockedRequestedIp(ipVoSpy, lockedIp);
}

@Test
public void encodeVlanIdIntoBroadcastUriTestVxlan() {
encodeVlanIdIntoBroadcastUriPrepareAndTest("123", "VXLAN", "vxlan", "vxlan://123");
}

@Test
public void encodeVlanIdIntoBroadcastUriTestVlan() {
encodeVlanIdIntoBroadcastUriPrepareAndTest("123", "VLAN", "vlan", "vlan://123");
}

@Test
public void encodeVlanIdIntoBroadcastUriTestEmpty() {
encodeVlanIdIntoBroadcastUriPrepareAndTest("123", "", "vlan", "vlan://123");
}

@Test
public void encodeVlanIdIntoBroadcastUriTestNull() {
encodeVlanIdIntoBroadcastUriPrepareAndTest("123", null, "vlan", "vlan://123");
}

@Test(expected = CloudRuntimeException.class)
public void encodeVlanIdIntoBroadcastUriTestEmptyVlanId() {
encodeVlanIdIntoBroadcastUriPrepareAndTest("", "vxlan", "vlan", "vlan://123");
}

@Test(expected = CloudRuntimeException.class)
public void encodeVlanIdIntoBroadcastUriTestNullVlanId() {
encodeVlanIdIntoBroadcastUriPrepareAndTest(null, "vlan", "vlan", "vlan://123");
}

@Test(expected = CloudRuntimeException.class)
public void encodeVlanIdIntoBroadcastUriTestBlankVlanId() {
encodeVlanIdIntoBroadcastUriPrepareAndTest(" ", "vlan", "vlan", "vlan://123");
}

@Test
public void encodeVlanIdIntoBroadcastUriTestNullVlanIdWithSchema() {
encodeVlanIdIntoBroadcastUriPrepareAndTest("vlan://123", "vlan", "vlan", "vlan://123");
}

@Test
public void encodeVlanIdIntoBroadcastUriTestNullVlanIdWithSchemaIsolationVxlan() {
encodeVlanIdIntoBroadcastUriPrepareAndTest("vlan://123", "vxlan", "vlan", "vlan://123");
}

@Test
public void encodeVlanIdIntoBroadcastUriTestNullVxlanIdWithSchema() {
encodeVlanIdIntoBroadcastUriPrepareAndTest("vxlan://123", "vxlan", "vxlan", "vxlan://123");
}

@Test
public void encodeVlanIdIntoBroadcastUriTestNullVxlanIdWithSchemaIsolationVlan() {
encodeVlanIdIntoBroadcastUriPrepareAndTest("vxlan://123", "vlan", "vxlan", "vxlan://123");
}

@Test(expected = InvalidParameterValueException.class)
public void encodeVlanIdIntoBroadcastUriTestNullNetwork() {
URI resultUri = testOrchastrator.encodeVlanIdIntoBroadcastUri("vxlan://123", null);
}

private void encodeVlanIdIntoBroadcastUriPrepareAndTest(String vlanId, String isolationMethod, String expectedIsolation, String expectedUri) {
PhysicalNetworkVO physicalNetwork = new PhysicalNetworkVO();
List<String> isolationMethods = new ArrayList<>();
isolationMethods.add(isolationMethod);
physicalNetwork.setIsolationMethods(isolationMethods);

URI resultUri = testOrchastrator.encodeVlanIdIntoBroadcastUri(vlanId, physicalNetwork);

Assert.assertEquals(expectedIsolation, resultUri.getScheme());
Assert.assertEquals(expectedUri, resultUri.toString());
}
}