From a55c75bbd201e53f0aca3e8f8f9dee08cd132a95 Mon Sep 17 00:00:00 2001 From: Dave Cahill Date: Tue, 27 Aug 2013 11:55:49 -0700 Subject: [PATCH] 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 --- plugins/network-elements/midonet/pom.xml | 4 +- .../cloud/network/element/MidoNetElement.java | 151 ++++++++++-------- .../network/element/SimpleFirewallRule.java | 20 ++- .../network/guru/MidoNetGuestNetworkGuru.java | 11 -- .../network/resource/MidoNetVifDriver.java | 18 +-- .../network/element/MidoNetElementTest.java | 4 +- 6 files changed, 106 insertions(+), 102 deletions(-) diff --git a/plugins/network-elements/midonet/pom.xml b/plugins/network-elements/midonet/pom.xml index 69eeb702eeb..c7dbdf29791 100644 --- a/plugins/network-elements/midonet/pom.xml +++ b/plugins/network-elements/midonet/pom.xml @@ -35,9 +35,9 @@ - com.midokura + org.midonet midonet-client - 12.12.2 + 1.1.0 org.mockito 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 ab6a6def405..42a6795f579 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 dcs = new ArrayList(); + 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(0,0)) + .tpDst(new DtoRange(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()); + 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(); + // 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 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 - toApply.tpSrcStart(fwRule.icmpType).tpSrcEnd(fwRule.icmpType) - .tpDstStart(fwRule.icmpCode).tpDstEnd(fwRule.icmpCode); - } else { - toApply.tpDstStart(fwRule.dstPortStart) - .tpDstEnd(fwRule.dstPortEnd); + 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 cidrParts = NetUtils.getCidr(relatedCidr); - toApply.create(); + // 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)); + } + + toApply.create(); + } } } } @@ -969,8 +975,11 @@ public class MidoNetElement extends AdapterBase implements // Rules in the preNat table Map existingPreNatRules = new HashMap(); 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 dhcpCapabilities = new HashMap(); + capabilities.put(Service.Dhcp, dhcpCapabilities); // L3 Support : SourceNat Map sourceNatCapabilities = new HashMap(); @@ -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; } 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 a6b78d84129..e8d81ef1865 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; } 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 d57affc5827..acc574ec9c9 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 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 3c7c23d669f..9c21636fd15 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 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 a7d96b0c310..47b16b81682 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.*;