From 2c3a41c7a63dfd1dd271578eacec26442ee039f9 Mon Sep 17 00:00:00 2001 From: Gabriel Brascher Date: Tue, 30 Jun 2020 14:00:02 -0300 Subject: [PATCH 1/2] Fix issue #3040: Broadcast URI not set to vxlan, but vlan --- .../main/java/com/cloud/network/Networks.java | 49 +++++++++---- .../orchestration/NetworkOrchestrator.java | 19 ++++- .../NetworkOrchestratorTest.java | 69 +++++++++++++++++++ 3 files changed, 122 insertions(+), 15 deletions(-) diff --git a/api/src/main/java/com/cloud/network/Networks.java b/api/src/main/java/com/cloud/network/Networks.java index 559a369b4d94..f892e253b359 100644 --- a/api/src/main/java/com/cloud/network/Networks.java +++ b/api/src/main/java/com/cloud/network/Networks.java @@ -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. @@ -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); } } }; diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java index 78f931543e37..2ec373982657 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java +++ b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java @@ -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 " @@ -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)) { @@ -2475,6 +2475,21 @@ 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(StringUtils.isNotBlank(pNtwk.getIsolationMethods().get(0))) { + 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 diff --git a/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java b/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java index 3450c09b2632..df566f4174b0 100644 --- a/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java +++ b/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java @@ -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; @@ -463,4 +466,70 @@ 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"); + } + + private void encodeVlanIdIntoBroadcastUriPrepareAndTest(String vlanId, String isolationMethod, String expectedIsolation, String expectedUri) { + PhysicalNetworkVO physicalNetwork = new PhysicalNetworkVO(); + List 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()); + } } From 13314d3abb194d3b7c02242fc2c016cc61b9a1c1 Mon Sep 17 00:00:00 2001 From: Gabriel Brascher Date: Mon, 21 Sep 2020 08:40:21 -0300 Subject: [PATCH 2/2] Add PhysicalNetwork null validation on encodeVlanIdIntoBroadcastUri --- .../cloudstack/engine/orchestration/NetworkOrchestrator.java | 4 ++++ .../engine/orchestration/NetworkOrchestratorTest.java | 5 +++++ 2 files changed, 9 insertions(+) diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java index 2ec373982657..c49da143e3ee 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java +++ b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java @@ -2480,6 +2480,10 @@ public Network doInTransaction(final TransactionStatus status) { * @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))) { String isolationMethod = pNtwk.getIsolationMethods().get(0).toLowerCase(); String vxlan = BroadcastDomainType.Vxlan.toString().toLowerCase(); diff --git a/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java b/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java index df566f4174b0..ed5265ccc5a1 100644 --- a/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java +++ b/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java @@ -521,6 +521,11 @@ 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 isolationMethods = new ArrayList<>();