From 4a96e1f6337d810812a0fe1b788a5cef33cbf38e Mon Sep 17 00:00:00 2001 From: Kris McQueen Date: Fri, 22 Oct 2010 11:47:40 -0700 Subject: [PATCH] bug 6662: handle a null ip forwarding rule in the API response as 'the rule already exists' since other errors will result in thrown exceptions. Also, fix up detection of network conflicts and duplicate rules by adding a list of used protocols to the port mappings status 6662: resolved fixed --- .../commands/CreateIPForwardingRuleCmd.java | 24 +++++++++------- .../com/cloud/network/NetworkManagerImpl.java | 28 +++++++++++++++---- 2 files changed, 36 insertions(+), 16 deletions(-) diff --git a/server/src/com/cloud/api/commands/CreateIPForwardingRuleCmd.java b/server/src/com/cloud/api/commands/CreateIPForwardingRuleCmd.java index ed350c59259..d6c3c414b37 100644 --- a/server/src/com/cloud/api/commands/CreateIPForwardingRuleCmd.java +++ b/server/src/com/cloud/api/commands/CreateIPForwardingRuleCmd.java @@ -25,6 +25,7 @@ import com.cloud.api.BaseCmd; import com.cloud.api.BaseCmd.Manager; import com.cloud.api.Implementation; import com.cloud.api.Parameter; +import com.cloud.api.ServerApiException; import com.cloud.api.response.FirewallRuleResponse; import com.cloud.network.FirewallRuleVO; import com.cloud.uservm.UserVm; @@ -92,18 +93,21 @@ public class CreateIPForwardingRuleCmd extends BaseCmd { @Override @SuppressWarnings("unchecked") public FirewallRuleResponse getResponse() { FirewallRuleVO fwRule = (FirewallRuleVO)getResponseObject(); + if (fwRule != null) { + FirewallRuleResponse fwResponse = new FirewallRuleResponse(); + fwResponse.setId(fwRule.getId()); + fwResponse.setPrivatePort(fwRule.getPrivatePort()); + fwResponse.setProtocol(fwRule.getProtocol()); + fwResponse.setPublicPort(fwRule.getPublicPort()); - FirewallRuleResponse fwResponse = new FirewallRuleResponse(); - fwResponse.setId(fwRule.getId()); - fwResponse.setPrivatePort(fwRule.getPrivatePort()); - fwResponse.setProtocol(fwRule.getProtocol()); - fwResponse.setPublicPort(fwRule.getPublicPort()); + UserVm vm = ApiDBUtils.findUserVmById(virtualMachineId); + fwResponse.setVirtualMachineId(vm.getId()); + fwResponse.setVirtualMachineName(vm.getName()); - UserVm vm = ApiDBUtils.findUserVmById(virtualMachineId); - fwResponse.setVirtualMachineId(vm.getId()); - fwResponse.setVirtualMachineName(vm.getName()); + fwResponse.setResponseName(getName()); + return fwResponse; + } - fwResponse.setResponseName(getName()); - return fwResponse; + throw new ServerApiException(NET_CREATE_IPFW_RULE_ERROR, "An existing rule for ipAddress / port / protocol of " + ipAddress + " / " + publicPort + " / " + protocol + " exits."); } } diff --git a/server/src/com/cloud/network/NetworkManagerImpl.java b/server/src/com/cloud/network/NetworkManagerImpl.java index 827e71cef9c..165da7a7d73 100755 --- a/server/src/com/cloud/network/NetworkManagerImpl.java +++ b/server/src/com/cloud/network/NetworkManagerImpl.java @@ -133,6 +133,7 @@ import com.cloud.user.dao.UserStatisticsDao; import com.cloud.uservm.UserVm; import com.cloud.utils.Pair; import com.cloud.utils.StringUtils; +import com.cloud.utils.Ternary; import com.cloud.utils.component.Adapters; import com.cloud.utils.component.Inject; import com.cloud.utils.db.DB; @@ -852,21 +853,36 @@ public class NetworkManagerImpl implements NetworkManager, DomainRouterService { // check for ip address/port conflicts by checking existing forwarding and load balancing rules List existingRulesOnPubIp = _rulesDao.listIPForwarding(ipAddress.getAddress()); - Map> mappedPublicPorts = new HashMap>(); + + // FIXME: The mapped ports should be String, String, List since more than one proto can be mapped... + Map>> mappedPublicPorts = new HashMap>>(); if (existingRulesOnPubIp != null) { for (FirewallRuleVO fwRule : existingRulesOnPubIp) { - mappedPublicPorts.put(fwRule.getPublicPort(), new Pair(fwRule.getPrivateIpAddress(), fwRule.getPrivatePort())); + Ternary> portMappings = mappedPublicPorts.get(fwRule.getPublicPort()); + List protocolList = null; + if (portMappings == null) { + protocolList = new ArrayList(); + } else { + protocolList = portMappings.third(); + } + protocolList.add(fwRule.getProtocol()); + mappedPublicPorts.put(fwRule.getPublicPort(), new Ternary>(fwRule.getPrivateIpAddress(), fwRule.getPrivatePort(), protocolList)); } } - Pair privateIpPort = mappedPublicPorts.get(publicPort); + Ternary> privateIpPort = mappedPublicPorts.get(publicPort); if (privateIpPort != null) { if (privateIpPort.first().equals(userVM.getGuestIpAddress()) && privateIpPort.second().equals(privatePort)) { - if (s_logger.isDebugEnabled()) { - s_logger.debug("skipping the creating of firewall rule " + ipAddress + ":" + publicPort + " to " + userVM.getGuestIpAddress() + ":" + privatePort + "; rule already exists."); + List protocolList = privateIpPort.third(); + for (String mappedProtocol : protocolList) { + if (mappedProtocol.equalsIgnoreCase(protocol)) { + if (s_logger.isDebugEnabled()) { + s_logger.debug("skipping the creating of firewall rule " + ipAddress + ":" + publicPort + " to " + userVM.getGuestIpAddress() + ":" + privatePort + "; rule already exists."); + } + return null; // already mapped + } } - return null; // already mapped } else { // FIXME: Will we need to refactor this for both assign port forwarding service and create port forwarding rule? // throw new NetworkRuleConflictException("An existing port forwarding service rule for " + ipAddress + ":" + publicPort