CLOUDSTACK-4466: Fix DHCP capability breaks in 4.2 for MidoNet A recent code change in NetworkManager causes NullPointerExceptions when DHCP capability list is null.
The commit which made the NetworkManager change also changed the VirtualRouter to not use null for the capabilitylist, but didn't make this change for other network devices, causing DHCP to fail on MidoNet. This change also updates the MidoNet plugin to use the most recent MidoNet API. Signed-off-by: Sheng Yang <[email protected]> (cherry picked from commit a55c75bbd201e53f0aca3e8f8f9dee08cd132a95) Signed-off-by: animesh <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/cloudstack/repo Commit: http://git-wip-us.apache.org/repos/asf/cloudstack/commit/41d2fb3d Tree: http://git-wip-us.apache.org/repos/asf/cloudstack/tree/41d2fb3d Diff: http://git-wip-us.apache.org/repos/asf/cloudstack/diff/41d2fb3d Branch: refs/heads/4.2 Commit: 41d2fb3d1704273042fbd02d85a276b2ec00c737 Parents: 4a5f2dd Author: Dave Cahill <[email protected]> Authored: Tue Aug 27 11:55:49 2013 -0700 Committer: animesh <[email protected]> Committed: Tue Aug 27 12:57:01 2013 -0700 ---------------------------------------------------------------------- plugins/network-elements/midonet/pom.xml | 4 +- .../cloud/network/element/MidoNetElement.java | 153 ++++++++++--------- .../network/element/SimpleFirewallRule.java | 20 ++- .../network/guru/MidoNetGuestNetworkGuru.java | 11 -- .../network/resource/MidoNetVifDriver.java | 18 +-- .../network/element/MidoNetElementTest.java | 4 +- 6 files changed, 107 insertions(+), 103 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cloudstack/blob/41d2fb3d/plugins/network-elements/midonet/pom.xml ---------------------------------------------------------------------- diff --git a/plugins/network-elements/midonet/pom.xml b/plugins/network-elements/midonet/pom.xml index b393a1a..c731068 100644 --- a/plugins/network-elements/midonet/pom.xml +++ b/plugins/network-elements/midonet/pom.xml @@ -35,9 +35,9 @@ </repositories> <dependencies> <dependency> - <groupId>com.midokura</groupId> + <groupId>org.midonet</groupId> <artifactId>midonet-client</artifactId> - <version>12.12.2</version> + <version>1.1.0</version> </dependency> <dependency> <groupId>org.mockito</groupId> http://git-wip-us.apache.org/repos/asf/cloudstack/blob/41d2fb3d/plugins/network-elements/midonet/src/com/cloud/network/element/MidoNetElement.java ---------------------------------------------------------------------- diff --git a/plugins/network-elements/midonet/src/com/cloud/network/element/MidoNetElement.java b/plugins/network-elements/midonet/src/com/cloud/network/element/MidoNetElement.java index ab6a6de..42a6795 100644 --- a/plugins/network-elements/midonet/src/com/cloud/network/element/MidoNetElement.java +++ b/plugins/network-elements/midonet/src/com/cloud/network/element/MidoNetElement.java @@ -54,19 +54,21 @@ import com.cloud.vm.ReservationContext; import com.cloud.vm.VirtualMachine; import com.cloud.vm.VirtualMachineProfile; import com.cloud.vm.dao.NicDao; -import com.midokura.midonet.client.MidonetApi; -import com.midokura.midonet.client.dto.DtoRule; -import com.midokura.midonet.client.resource.Bridge; -import com.midokura.midonet.client.resource.BridgePort; -import com.midokura.midonet.client.resource.DhcpHost; -import com.midokura.midonet.client.resource.DhcpSubnet; -import com.midokura.midonet.client.resource.Port; -import com.midokura.midonet.client.resource.ResourceCollection; -import com.midokura.midonet.client.resource.Route; -import com.midokura.midonet.client.resource.Router; -import com.midokura.midonet.client.resource.RouterPort; -import com.midokura.midonet.client.resource.Rule; -import com.midokura.midonet.client.resource.RuleChain; +import org.midonet.client.MidonetApi; +import org.midonet.client.exception.HttpInternalServerError; +import org.midonet.client.dto.DtoRule; +import org.midonet.client.dto.DtoRule.DtoRange; +import org.midonet.client.resource.Bridge; +import org.midonet.client.resource.BridgePort; +import org.midonet.client.resource.DhcpHost; +import org.midonet.client.resource.DhcpSubnet; +import org.midonet.client.resource.Port; +import org.midonet.client.resource.ResourceCollection; +import org.midonet.client.resource.Route; +import org.midonet.client.resource.Router; +import org.midonet.client.resource.RouterPort; +import org.midonet.client.resource.Rule; +import org.midonet.client.resource.RuleChain; import com.sun.jersey.core.util.MultivaluedMapImpl; import org.apache.log4j.Logger; import org.springframework.stereotype.Component; @@ -420,7 +422,9 @@ public class MidoNetElement extends AdapterBase implements sub.subnetLength(cidrInfo.second()); sub.subnetPrefix(cidrInfo.first()); sub.defaultGateway(network.getGateway()); - sub.dnsServerAddr(dest.getDataCenter().getDns1()); + List<String> dcs = new ArrayList<String>(); + dcs.add(dest.getDataCenter().getDns1()); + sub.dnsServerAddrs(dcs); sub.create(); } @@ -529,10 +533,8 @@ public class MidoNetElement extends AdapterBase implements .nwDstAddress(floatingIp) .nwDstLength(32) .nwProto(SimpleFirewallRule.stringToProtocolNumber("icmp")) - .tpSrcStart(0) - .tpSrcEnd(0) - .tpDstStart(0) - .tpDstEnd(0) + .tpSrc(new DtoRange<Integer>(0,0)) + .tpDst(new DtoRange<Integer>(0,0)) .position(2) .create(); @@ -685,52 +687,56 @@ public class MidoNetElement extends AdapterBase implements } for (FirewallRule rule : rulesToApply) { - IpAddress dstIp = _networkModel.getIp(rule.getSourceIpAddressId()); - FirewallRuleTO ruleTO = new FirewallRuleTO(rule, null, dstIp.getAddress().addr()); - - // Convert to string representation - SimpleFirewallRule fwRule = new SimpleFirewallRule(ruleTO); - String[] ruleStrings = fwRule.toStringArray(); - - if (rule.getState() == FirewallRule.State.Revoke) { - // Lookup in existingRules, delete if present - for(String revokeRuleString : ruleStrings){ - Rule foundRule = existingRules.get(revokeRuleString); - if(foundRule != null){ - foundRule.delete(); + if (rule.getState() == FirewallRule.State.Revoke || rule.getState() == FirewallRule.State.Add) { + IpAddress dstIp = _networkModel.getIp(rule.getSourceIpAddressId()); + FirewallRuleTO ruleTO = new FirewallRuleTO(rule, null, dstIp.getAddress().addr()); + + // Convert to string representation + SimpleFirewallRule fwRule = new SimpleFirewallRule(ruleTO); + String[] ruleStrings = fwRule.toStringArray(); + + if (rule.getState() == FirewallRule.State.Revoke) { + // Lookup in existingRules, delete if present + for(String revokeRuleString : ruleStrings){ + Rule foundRule = existingRules.get(revokeRuleString); + if(foundRule != null){ + foundRule.delete(); + } } - } - } else if (rule.getState() == FirewallRule.State.Add) { - // Lookup in existingRules, add if not present - for(int i = 0; i < ruleStrings.length; i++){ - String ruleString = ruleStrings[i]; - Rule foundRule = existingRules.get(ruleString); - if(foundRule == null){ - // Get the cidr for the related entry in the Source Cidrs list - String relatedCidr = fwRule.sourceCidrs.get(i); - Pair<String,Integer> cidrParts = NetUtils.getCidr(relatedCidr); - - // Create rule with correct proto, cidr, ACCEPT, dst IP - Rule toApply = preFilter.addRule() - .type(DtoRule.Jump) - .jumpChainId(preNat.getId()) - .position(1) - .nwSrcAddress(cidrParts.first()) - .nwSrcLength(cidrParts.second()) - .nwDstAddress(ruleTO.getSrcIp()) - .nwDstLength(32) - .nwProto(SimpleFirewallRule.stringToProtocolNumber(rule.getProtocol())); + } else if (rule.getState() == FirewallRule.State.Add) { + // Lookup in existingRules, add if not present + for(int i = 0; i < ruleStrings.length; i++){ + String ruleString = ruleStrings[i]; + Rule foundRule = existingRules.get(ruleString); + if(foundRule == null){ + // Get the cidr for the related entry in the Source Cidrs list + String relatedCidr = fwRule.sourceCidrs.get(i); + Pair<String,Integer> cidrParts = NetUtils.getCidr(relatedCidr); + + // Create rule with correct proto, cidr, ACCEPT, dst IP + Rule toApply = preFilter.addRule() + .type(DtoRule.Jump) + .jumpChainId(preNat.getId()) + .position(1) + .nwSrcAddress(cidrParts.first()) + .nwSrcLength(cidrParts.second()) + .nwDstAddress(ruleTO.getSrcIp()) + .nwDstLength(32) + .nwProto(SimpleFirewallRule.stringToProtocolNumber(rule.getProtocol())); + + if(rule.getProtocol().equals("icmp")){ + // ICMP rules - reuse port fields + // (-1, -1) means "allow all ICMP", so we don't set tpSrc / tpDst + if(fwRule.icmpType != -1 | fwRule.icmpCode != -1){ + toApply.tpSrc(new DtoRange(fwRule.icmpType, fwRule.icmpType)) + .tpDst(new DtoRange(fwRule.icmpCode, fwRule.icmpCode)); + } + } else { + toApply.tpDst(new DtoRange(fwRule.dstPortStart, fwRule.dstPortEnd)); + } - if(rule.getProtocol().equals("icmp")){ - // ICMP rules - reuse port fields - toApply.tpSrcStart(fwRule.icmpType).tpSrcEnd(fwRule.icmpType) - .tpDstStart(fwRule.icmpCode).tpDstEnd(fwRule.icmpCode); - } else { - toApply.tpDstStart(fwRule.dstPortStart) - .tpDstEnd(fwRule.dstPortEnd); + toApply.create(); } - - toApply.create(); } } } @@ -969,8 +975,11 @@ public class MidoNetElement extends AdapterBase implements // Rules in the preNat table Map<String, Rule> existingPreNatRules = new HashMap<String, Rule>(); for (Rule existingRule : preNat.getRules()) { - String ruleString = new SimpleFirewallRule(existingRule).toStringArray()[0]; - existingPreNatRules.put(ruleString, existingRule); + // The "port forwarding" rules we're interested in are dnat rules where src / dst ports are specified + if(existingRule.getType().equals(DtoRule.DNAT) && existingRule.getTpDst() != null){ + String ruleString = new SimpleFirewallRule(existingRule).toStringArray()[0]; + existingPreNatRules.put(ruleString, existingRule); + } } /* @@ -1054,8 +1063,7 @@ public class MidoNetElement extends AdapterBase implements .flowAction(DtoRule.Accept) .nwDstAddress(publicIp) .nwDstLength(32) - .tpDstStart(pubPortStart) - .tpDstEnd(pubPortEnd) + .tpDst(new DtoRange(pubPortStart, pubPortEnd)) .natTargets(preTargets) .nwProto(SimpleFirewallRule.stringToProtocolNumber(rule.getProtocol())) .position(1); @@ -1119,7 +1127,8 @@ public class MidoNetElement extends AdapterBase implements capabilities.put(Service.Gateway, null); // L3 Support : DHCP - capabilities.put(Service.Dhcp, null); + Map<Capability, String> dhcpCapabilities = new HashMap<Capability, String>(); + capabilities.put(Service.Dhcp, dhcpCapabilities); // L3 Support : SourceNat Map<Capability, String> sourceNatCapabilities = new HashMap<Capability, String>(); @@ -1360,7 +1369,7 @@ public class MidoNetElement extends AdapterBase implements int pos = 1; // If it is ARP, accept it egressChain.addRule().type(DtoRule.Accept) - .dlType((short)0x0806) + .dlType(0x0806) .position(pos++) .create(); @@ -1419,7 +1428,7 @@ public class MidoNetElement extends AdapterBase implements // If it is ARP, accept it inc.addRule().type(DtoRule.Accept) - .dlType((short)0x0806) + .dlType(0x0806) .position(pos++) .create(); @@ -1483,7 +1492,13 @@ public class MidoNetElement extends AdapterBase implements String networkUUIDStr = String.valueOf(networkID); - netBridge = api.addBridge().tenantId(accountUuid).name(networkUUIDStr).create(); + s_logger.debug("Attempting to create guest network bridge"); + try { + netBridge = api.addBridge().tenantId(accountUuid).name(networkUUIDStr).create(); + } catch (HttpInternalServerError ex) { + s_logger.warn("Bridge creation failed, retrying bridge get in case it now exists.", ex); + netBridge = getNetworkBridge(networkID, accountUuid); + } } return netBridge; } http://git-wip-us.apache.org/repos/asf/cloudstack/blob/41d2fb3d/plugins/network-elements/midonet/src/com/cloud/network/element/SimpleFirewallRule.java ---------------------------------------------------------------------- diff --git a/plugins/network-elements/midonet/src/com/cloud/network/element/SimpleFirewallRule.java b/plugins/network-elements/midonet/src/com/cloud/network/element/SimpleFirewallRule.java index a6b78d8..e8d81ef 100644 --- a/plugins/network-elements/midonet/src/com/cloud/network/element/SimpleFirewallRule.java +++ b/plugins/network-elements/midonet/src/com/cloud/network/element/SimpleFirewallRule.java @@ -21,9 +21,10 @@ package com.cloud.network.element; import com.cloud.agent.api.to.FirewallRuleTO; import com.cloud.agent.api.to.NetworkACLTO; import com.cloud.agent.api.to.PortForwardingRuleTO; -import com.midokura.midonet.client.dto.DtoRule; +import org.midonet.client.dto.DtoRule; +import org.midonet.client.resource.*; import com.google.common.collect.*; -import com.midokura.midonet.client.resource.*; + import java.util.*; // Used for translation between MidoNet firewall rules and @@ -147,8 +148,14 @@ public class SimpleFirewallRule { dstIp = rule.getNwDstAddress(); if("icmp".equals(protocol)){ - icmpType = rule.getTpSrcStart(); - icmpCode = rule.getTpDstStart(); + if(rule.getTpSrc() != null && rule.getTpDst() != null){ + icmpType = rule.getTpSrc().start; + icmpCode = rule.getTpDst().start; + } else { + icmpType = -1; + icmpCode = -1; + } + } else { /* * If this is port forwarding, we want to take the start @@ -158,9 +165,9 @@ public class SimpleFirewallRule { if (targets != null) { dstPortStart = targets[0].portFrom; } else { - dstPortStart = rule.getTpDstStart(); + dstPortStart = rule.getTpDst().start; } - dstPortEnd = rule.getTpDstEnd(); + dstPortEnd = rule.getTpDst().end; } // cidr, protocol, dstIp, dstPortStart, dstPortEnd, icmpType, icmpCode); @@ -177,6 +184,7 @@ public class SimpleFirewallRule { public int getFieldOne(){ if(protocol.equals("icmp")){ return icmpType; + } else { return dstPortStart; } http://git-wip-us.apache.org/repos/asf/cloudstack/blob/41d2fb3d/plugins/network-elements/midonet/src/com/cloud/network/guru/MidoNetGuestNetworkGuru.java ---------------------------------------------------------------------- diff --git a/plugins/network-elements/midonet/src/com/cloud/network/guru/MidoNetGuestNetworkGuru.java b/plugins/network-elements/midonet/src/com/cloud/network/guru/MidoNetGuestNetworkGuru.java index d57affc..acc574e 100644 --- a/plugins/network-elements/midonet/src/com/cloud/network/guru/MidoNetGuestNetworkGuru.java +++ b/plugins/network-elements/midonet/src/com/cloud/network/guru/MidoNetGuestNetworkGuru.java @@ -19,8 +19,6 @@ package com.cloud.network.guru; -import com.cloud.network.element.MidoNetElement; -import com.cloud.dc.DataCenter; import com.cloud.dc.DataCenter.NetworkType; import com.cloud.deploy.DeployDestination; import com.cloud.deploy.DeploymentPlan; @@ -33,21 +31,12 @@ import com.cloud.user.Account; import com.cloud.user.AccountVO; import com.cloud.user.dao.AccountDao; import com.cloud.vm.*; -import com.midokura.midonet.client.resource.Bridge; -import com.cloud.utils.net.NetUtils; - -import com.cloud.network.Networks.AddressFormat; -import com.midokura.midonet.client.resource.BridgePort; import org.apache.log4j.Logger; import org.springframework.stereotype.Component; import com.cloud.network.dao.NetworkVO; import com.cloud.network.dao.PhysicalNetworkVO; - -import com.cloud.vm.Nic.ReservationStrategy; - import javax.ejb.Local; -import java.util.UUID; import javax.inject.Inject; @Component http://git-wip-us.apache.org/repos/asf/cloudstack/blob/41d2fb3d/plugins/network-elements/midonet/src/com/cloud/network/resource/MidoNetVifDriver.java ---------------------------------------------------------------------- diff --git a/plugins/network-elements/midonet/src/com/cloud/network/resource/MidoNetVifDriver.java b/plugins/network-elements/midonet/src/com/cloud/network/resource/MidoNetVifDriver.java index 3c7c23d..9c21636 100644 --- a/plugins/network-elements/midonet/src/com/cloud/network/resource/MidoNetVifDriver.java +++ b/plugins/network-elements/midonet/src/com/cloud/network/resource/MidoNetVifDriver.java @@ -21,32 +21,24 @@ package com.cloud.network.resource; import com.cloud.hypervisor.kvm.resource.*; import com.cloud.agent.api.to.NicTO; -import com.cloud.agent.resource.virtualnetwork.VirtualRoutingResource; import com.cloud.exception.InternalErrorException; import com.cloud.network.Networks; import com.cloud.utils.NumbersUtil; -import com.cloud.utils.net.NetUtils; import com.cloud.utils.script.OutputInterpreter; import com.cloud.utils.script.Script; -import com.midokura.midonet.client.resource.Bridge; -import com.midokura.midonet.client.resource.Router; -import com.midokura.midonet.client.resource.BridgePort; -import com.midokura.midonet.client.resource.RouterPort; -import com.midokura.midonet.client.resource.Host; import org.apache.log4j.Logger; import org.libvirt.LibvirtException; - import com.sun.jersey.core.util.MultivaluedMapImpl; -import com.midokura.midonet.client.MidonetApi; - import javax.ws.rs.core.MultivaluedMap; - import javax.naming.ConfigurationException; -import javax.ws.rs.core.MultivaluedMap; -import java.net.URI; import java.util.Map; import java.util.UUID; +import org.midonet.client.resource.Bridge; +import org.midonet.client.resource.BridgePort; +import org.midonet.client.resource.Host; +import org.midonet.client.MidonetApi; + public class MidoNetVifDriver extends VifDriverBase { private static final Logger s_logger = Logger http://git-wip-us.apache.org/repos/asf/cloudstack/blob/41d2fb3d/plugins/network-elements/midonet/test/com/cloud/network/element/MidoNetElementTest.java ---------------------------------------------------------------------- diff --git a/plugins/network-elements/midonet/test/com/cloud/network/element/MidoNetElementTest.java b/plugins/network-elements/midonet/test/com/cloud/network/element/MidoNetElementTest.java index a7d96b0..47b16b8 100644 --- a/plugins/network-elements/midonet/test/com/cloud/network/element/MidoNetElementTest.java +++ b/plugins/network-elements/midonet/test/com/cloud/network/element/MidoNetElementTest.java @@ -24,8 +24,8 @@ import com.cloud.user.dao.AccountDao; import junit.framework.TestCase; import static org.junit.Assert.assertEquals; import static org.mockito.Mockito.*; -import com.midokura.midonet.client.MidonetApi; -import com.midokura.midonet.client.resource.*; +import org.midonet.client.MidonetApi; +import org.midonet.client.resource.*; import com.sun.jersey.core.util.MultivaluedMapImpl; import com.cloud.network.*; import com.cloud.vm.*;
