From 41f569e8a853cb4503646d06cadc4c9c1aea82b2 Mon Sep 17 00:00:00 2001 From: Richard Lawley Date: Tue, 4 Jun 2019 21:51:03 +0100 Subject: [PATCH 1/2] router: Fix rule duplication with non-VPC static NAT rules (#3366) The VR code has provision for inserting rules at the top or bottom by specifying "front" as the second parameter to self.fw.append. However, there are a number of cases where someone has been unaware of this and added a rule with the pattern self.fw.append(["mangle", "", "-I PREROUTING".... This causes the code to check for the rule already being present to fail, and duplicate rules end up being added. This PR fixes two of these cases which apply to adding static NAT rules. I am aware of more of these cases, but I don't have the ability to easily test the outcome of fixing them. I'm happy to add these in if you're confident that the automated tests will be sufficient. Searching for "-I (case sensitive) finds these. The code for dealing with "front" is included below to show that this shouldn't have any ill effects: if fw[1] == "front": cpy = cpy.replace('-A', '-I') Fixes #3177 --- systemvm/debian/opt/cloud/bin/configure.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/systemvm/debian/opt/cloud/bin/configure.py b/systemvm/debian/opt/cloud/bin/configure.py index 4df99111bfd..a7f297edbb2 100755 --- a/systemvm/debian/opt/cloud/bin/configure.py +++ b/systemvm/debian/opt/cloud/bin/configure.py @@ -922,11 +922,11 @@ class CsForwardingRules(CsDataBag): if device is None: raise Exception("Ip address %s has no device in the ips databag" % rule["public_ip"]) - self.fw.append(["mangle", "", - "-I PREROUTING -s %s/32 -m state --state NEW -j CONNMARK --save-mark --nfmask 0xffffffff --ctmask 0xffffffff" % + self.fw.append(["mangle", "front", + "-A PREROUTING -s %s/32 -m state --state NEW -j CONNMARK --save-mark --nfmask 0xffffffff --ctmask 0xffffffff" % rule["internal_ip"]]) - self.fw.append(["mangle", "", - "-I PREROUTING -s %s/32 -m state --state NEW -j MARK --set-xmark %s/0xffffffff" % + self.fw.append(["mangle", "front", + "-A PREROUTING -s %s/32 -m state --state NEW -j MARK --set-xmark %s/0xffffffff" % (rule["internal_ip"], hex(100 + int(device[len("eth"):])))]) self.fw.append(["nat", "front", "-A PREROUTING -d %s/32 -j DNAT --to-destination %s" % (rule["public_ip"], rule["internal_ip"])]) From 8fb388e9312b917a8f36c7d7e3f45985a95ce773 Mon Sep 17 00:00:00 2001 From: Rohit Yadav Date: Wed, 5 Jun 2019 08:47:05 +0530 Subject: [PATCH 2/2] router: support multi-homed VMs in VPC (#3373) This does not remove VM entries in dbags when hostnames match. The current codebase already removes entry when a VM is stopped/removed so we don't need to handle lazy removal. This will allow a VM on multiple-tiers in a VPC to get dns/dhcp rules as expected. This also fixes the issue of dhcp_release based on a specific interface and removes dhcp/dns entry when a nic is removed on a guest VM. Fixes #3273 Signed-off-by: Rohit Yadav --- .../com/cloud/network/element/VirtualRouterElement.java | 1 + systemvm/debian/opt/cloud/bin/cs/CsDhcp.py | 8 +++----- systemvm/debian/opt/cloud/bin/cs_dhcp.py | 5 ----- 3 files changed, 4 insertions(+), 10 deletions(-) diff --git a/server/src/com/cloud/network/element/VirtualRouterElement.java b/server/src/com/cloud/network/element/VirtualRouterElement.java index 11a553cb758..a854d14fe83 100644 --- a/server/src/com/cloud/network/element/VirtualRouterElement.java +++ b/server/src/com/cloud/network/element/VirtualRouterElement.java @@ -895,6 +895,7 @@ NetworkMigrationResponder, AggregatedCommandExecutor, RedundantResource, DnsServ @Override public boolean release(final Network network, final NicProfile nic, final VirtualMachineProfile vm, final ReservationContext context) throws ConcurrentOperationException, ResourceUnavailableException { + removeDhcpEntry(network, nic, vm); return true; } diff --git a/systemvm/debian/opt/cloud/bin/cs/CsDhcp.py b/systemvm/debian/opt/cloud/bin/cs/CsDhcp.py index 6ea91742d78..e7abb902046 100755 --- a/systemvm/debian/opt/cloud/bin/cs/CsDhcp.py +++ b/systemvm/debian/opt/cloud/bin/cs/CsDhcp.py @@ -118,7 +118,6 @@ class CsDhcp(CsDataBag): def delete_leases(self): macs_dhcphosts = [] - interfaces = filter(lambda x: x.startswith('eth'), os.listdir('/sys/class/net')) try: logging.info("Attempting to delete entries from dnsmasq.leases file for VMs which are not on dhcphosts file") for host in open(DHCP_HOSTS): @@ -130,10 +129,9 @@ class CsDhcp(CsDataBag): mac = lease[1] ip = lease[2] if mac not in macs_dhcphosts: - for interface in interfaces: - cmd = "dhcp_release %s %s %s" % (interface, ip, mac) - logging.info(cmd) - CsHelper.execute(cmd) + cmd = "dhcp_release $(ip route get %s | grep eth | head -1 | awk '{print $3}') %s %s" % (ip, ip, mac) + logging.info(cmd) + CsHelper.execute(cmd) removed = removed + 1 self.del_host(ip) logging.info("Deleted %s entries from dnsmasq.leases file" % str(removed)) diff --git a/systemvm/debian/opt/cloud/bin/cs_dhcp.py b/systemvm/debian/opt/cloud/bin/cs_dhcp.py index 735f847dbb8..bb2ff7b07c3 100755 --- a/systemvm/debian/opt/cloud/bin/cs_dhcp.py +++ b/systemvm/debian/opt/cloud/bin/cs_dhcp.py @@ -27,11 +27,6 @@ def merge(dbag, data): del(dbag[data['ipv4_address']]) else: remove_keys = set() - for key, entry in dbag.iteritems(): - if key != 'id' and entry['host_name'] == data['host_name']: - remove_keys.add(key) - break - for key, entry in dbag.iteritems(): if key != 'id' and entry['mac_address'] == data['mac_address']: remove_keys.add(key)