Allow exclusion of chosen addresses from virtual router usage counters

engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/NetworkOrchestrationService.java

@@ -16,6 +16,7 @@
 // under the License.
 package org.apache.cloudstack.engine.orchestration.service;
 
+import java.util.ArrayList;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -53,12 +54,14 @@
 import com.cloud.user.User;
 import com.cloud.utils.fsm.NoTransitionException;
 import com.cloud.utils.Pair;
+import com.cloud.utils.net.NetUtils;
 import com.cloud.vm.Nic;
 import com.cloud.vm.NicProfile;
 import com.cloud.vm.ReservationContext;
 import com.cloud.vm.VirtualMachine;
 import com.cloud.vm.VirtualMachine.Type;
 import com.cloud.vm.VirtualMachineProfile;
+import com.googlecode.ipv6.IPv6Network;
 
 /**
  * NetworkManager manages the network for the different end users.
@@ -138,6 +141,54 @@ public interface NetworkOrchestrationService {
     ConfigKey<Integer> VmNetworkThrottlingRate = new ConfigKey<Integer>("Network", Integer.class, "vm.network.throttling.rate", "200",
             "Default data transfer rate in megabits per second allowed in User vm's default network.", true, ConfigKey.Scope.Zone);
 
+    ConfigKey<String> NetworkUsageExemptCidrs = new ConfigKey<>(String.class, "network.usage.exempt.cidrs", "Usage", "",
+            "Comma-separated IPv4 or IPv6 addresses or CIDRs. Traffic between guest networks and these addresses doesn't count " +
+            "towards virtual router network usage. Virtual routers only count IPv4 traffic, so they ignore IPv6 entries. " +
+            "Routers pick up a change the next time they start or restart.",
+            true, ConfigKey.Scope.Zone, null, null, null, null, null, ConfigKey.Kind.CSV, null);
+
+    /**
+     * Tidies one entry of {@link #NetworkUsageExemptCidrs} into a CIDR, or returns null if it isn't a valid address or CIDR.
+     * For example, 192.0.2.5 becomes 192.0.2.5/32, 198.51.100.70/24 becomes 198.51.100.0/24 and 2001:db8::5 becomes
+     * 2001:db8::5/128. Callers refuse a /0 result, as it would exempt all traffic.
+     */
+    static String normaliseUsageExemptCidr(String entry) {
+        if (NetUtils.isValidIp4(entry)) {
+            entry = entry + "/32";
+        } else if (NetUtils.isValidIp6(entry)) {
+            entry = entry + "/128";
+        }
+        if (NetUtils.isValidIp4Cidr(entry)) {
+            return NetUtils.getCidrSubNet(entry) + "/" + NetUtils.getCidr(entry).second();
+        }
+        if (entry.contains(":") && NetUtils.isValidIp6Cidr(entry)) {
+            return IPv6Network.fromString(entry).toString();
+        }
+        return null;
+    }
+
+    /**
+     * Turns a {@link #NetworkUsageExemptCidrs} value into a tidy list with no repeats, using {@link #normaliseUsageExemptCidr}.
+     * Entries that aren't valid, and /0, are left out. Saving the setting already refuses them.
+     */
+    static List<String> parseUsageExemptCidrs(String value) {
+        final List<String> cidrs = new ArrayList<>();
+        if (value == null) {
+            return cidrs;
+        }
+        for (String entry : value.split(",")) {
+            entry = entry.trim();
+            if (entry.isEmpty()) {
+                continue;
+            }
+            final String cidr = normaliseUsageExemptCidr(entry);
+            if (cidr != null && !cidr.endsWith("/0") && !cidrs.contains(cidr)) {
+                cidrs.add(cidr);
+            }
+        }
+        return cidrs;
+    }
+
     List<? extends Network> setupNetwork(Account owner, NetworkOffering offering, DeploymentPlan plan, String name, String displayText, boolean isDefault)
         throws ConcurrentOperationException;
 

engine/api/src/test/java/org/apache/cloudstack/engine/orchestration/service/NetworkOrchestrationServiceTest.java

@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.cloudstack.engine.orchestration.service;
+
+import java.util.List;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+public class NetworkOrchestrationServiceTest {
+
+    @Test
+    public void parseUsageExemptCidrsTestNotSet() {
+        Assert.assertTrue(NetworkOrchestrationService.parseUsageExemptCidrs(null).isEmpty());
+        Assert.assertTrue(NetworkOrchestrationService.parseUsageExemptCidrs(" ").isEmpty());
+    }
+
+    @Test
+    public void parseUsageExemptCidrsTestNormalisesEntries() {
+        List<String> cidrs = NetworkOrchestrationService.parseUsageExemptCidrs("192.0.2.5, 198.51.100.70/24,,192.0.2.5/32");
+        Assert.assertEquals(List.of("192.0.2.5/32", "198.51.100.0/24"), cidrs);
+    }
+
+    @Test
+    public void parseUsageExemptCidrsTestNormalisesIpv6Entries() {
+        List<String> cidrs = NetworkOrchestrationService.parseUsageExemptCidrs("2001:db8::5, 2001:db8:0:1::7/64,192.0.2.5");
+        Assert.assertEquals(List.of("2001:db8::5/128", "2001:db8:0:1::/64", "192.0.2.5/32"), cidrs);
+    }
+
+    @Test
+    public void parseUsageExemptCidrsTestSkipsInvalidEntries() {
+        List<String> cidrs = NetworkOrchestrationService.parseUsageExemptCidrs("not-an-ip,0.0.0.0/0,::/0,10.0.0.0/33,192.168.1.300,172.16.0.0/12");
+        Assert.assertEquals(List.of("172.16.0.0/12"), cidrs);
+    }
+}

engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java

@@ -5117,6 +5117,6 @@ public ConfigKey<?>[] getConfigKeys() {
                 GuestDomainSuffix, NetworkThrottlingRate, VmNetworkThrottlingRate, MinVRVersion, DhcpLeaseTimeout,
                 PromiscuousMode, MacAddressChanges, ForgedTransmits, MacLearning, RollingRestartEnabled,
                 TUNGSTEN_ENABLED, NSX_ENABLED, NETRIS_ENABLED, NETWORK_LB_HAPROXY_MAX_CONN,
-                NETWORK_LB_HAPROXY_IDLE_TIMEOUT};
+                NETWORK_LB_HAPROXY_IDLE_TIMEOUT, NetworkUsageExemptCidrs};
     }
 }

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

@@ -1561,6 +1561,12 @@ protected String validateValueRange(String name, String value, Class<?> type, Co
                     }
                 }
             }
+            if (NetworkOrchestrationService.NetworkUsageExemptCidrs.key().equalsIgnoreCase(name) && StringUtils.isNotBlank(value)) {
+                final String error = validateUsageExemptCidrs(name, value);
+                if (error != null) {
+                    return error;
+                }
+            }
         }
 
         validateIpAddressRelatedConfigValues(name, value);
@@ -1576,6 +1582,26 @@ protected String validateValueRange(String name, String value, Class<?> type, Co
         return validateIfStringValueIsInRange(name, value, range);
     }
 
+    /**
+     * Checks that each entry is an IPv4 or IPv6 address or CIDR. We refuse /0 because it would exempt all traffic.
+     */
+    protected String validateUsageExemptCidrs(String name, String value) {
+        for (String entry : value.split(",")) {
+            entry = entry.trim();
+            if (entry.isEmpty()) {
+                continue;
+            }
+            final String cidr = NetworkOrchestrationService.normaliseUsageExemptCidr(entry);
+            if (cidr == null) {
+                return String.format("[%s] is not a valid IP address or CIDR for %s.", entry, name);
+            }
+            if (cidr.endsWith("/0")) {
+                return String.format("[%s] would exempt all traffic, so it can't be used for %s.", entry, name);
+            }
+        }
+        return null;
+    }
+
     /**
      * Validates configuration values for the given name, value, and type.
      * <ul>

server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java

@@ -2089,6 +2089,12 @@ public boolean finalizeVirtualMachineProfile(final VirtualMachineProfile profile
             buf.append(" exposedns=true");
         }
 
+        final List<String> usageExemptCidrs = NetworkOrchestrationService.parseUsageExemptCidrs(
+                NetworkOrchestrationService.NetworkUsageExemptCidrs.valueIn(dc.getId()));
+        if (!usageExemptCidrs.isEmpty()) {
+            buf.append(" usage_exempt_cidrs=").append(String.join(",", usageExemptCidrs));
+        }
+
         if (Boolean.parseBoolean(_configDao.getValue(Config.BaremetalProvisionDoneNotificationEnabled.key()))) {
             final QueryBuilder<UserVO> acntq = QueryBuilder.create(UserVO.class);
             acntq.and(acntq.entity().getUsername(), SearchCriteria.Op.EQ, "baremetal-system-account");

server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java

@@ -66,6 +66,7 @@
 import org.apache.cloudstack.api.command.admin.zone.DeleteZoneCmd;
 import org.apache.cloudstack.config.Configuration;
 import org.apache.cloudstack.context.CallContext;
+import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationService;
 import org.apache.cloudstack.engine.subsystem.api.storage.ZoneScope;
 import org.apache.cloudstack.framework.config.ConfigDepot;
 import org.apache.cloudstack.framework.config.ConfigKey;
@@ -861,6 +862,30 @@ public void validateValueRangeTestReturnsNotNullWhenConfigKeyHasRangeAndValueIsI
         Assert.assertNotNull(configurationManagerImplSpy.validateValueRange(NetworkModel.MACIdentifier.key(), "-1", Integer.class, null));
     }
 
+    private String validateUsageExemptCidrs(String value) {
+        return configurationManagerImplSpy.validateValueRange(NetworkOrchestrationService.NetworkUsageExemptCidrs.key(), value, String.class, null);
+    }
+
+    @Test
+    public void validateValueRangeTestAcceptsUsageExemptAddressesAndCidrs() {
+        Assert.assertNull(validateUsageExemptCidrs(""));
+        Assert.assertNull(validateUsageExemptCidrs("192.0.2.5, 198.51.100.70/24,,203.0.113.0/24"));
+        Assert.assertNull(validateUsageExemptCidrs("2001:db8::5,2001:db8:0:1::7/64"));
+    }
+
+    @Test
+    public void validateValueRangeTestRejectsInvalidUsageExemptEntry() {
+        Assert.assertNotNull(validateUsageExemptCidrs("192.0.2.5,bogus"));
+        Assert.assertNotNull(validateUsageExemptCidrs("192.0.2.300"));
+        Assert.assertNotNull(validateUsageExemptCidrs("2001:db8::/129"));
+    }
+
+    @Test
+    public void validateValueRangeTestRejectsUsageExemptMatchAll() {
+        Assert.assertNotNull(validateUsageExemptCidrs("0.0.0.0/0"));
+        Assert.assertNotNull(validateUsageExemptCidrs("::/0"));
+    }
+
     @Test
     public void validateValueRangeTestValidatesValueWhenConfigHasRange() {
         Config config = Config.SecStorageEncryptCopy;

systemvm/debian/opt/cloud/bin/cs/CsAddress.py

@@ -473,6 +473,22 @@ def setup_router_control(self):
         self.fw.append(["filter", "", "-P INPUT DROP"])
         self.fw.append(["filter", "", "-P FORWARD DROP"])
 
+    def add_usage_stats_jump(self, stats_chain):
+        """ Sends forwarded traffic to the usage counters. If exempt CIDRs are set, traffic passes
+        through <stats_chain>_EXEMPT first, which skips the counters for those addresses.
+        We keep the usage chain exactly as it was, because the agent reads its counters by position. """
+        exempt_cidrs = self.config.get_usage_exempt_cidrs()
+        if not exempt_cidrs:
+            self.fw.append(["", "front", "-A FORWARD -j %s" % stats_chain])
+            return
+        exempt_chain = "%s_EXEMPT" % stats_chain
+        self.fw.append(["", "front", "-A FORWARD -j %s" % exempt_chain])
+        self.fw.append(["", "", "-A %s -j %s" % (exempt_chain, stats_chain)])
+        for cidr in exempt_cidrs:
+            # insert at the top, so a CIDR added later still comes before the jump to the counters
+            self.fw.append(["", "front", "-A %s -d %s -j RETURN" % (exempt_chain, cidr)])
+            self.fw.append(["", "front", "-A %s -s %s -j RETURN" % (exempt_chain, cidr)])
+
     def fw_router(self):
         if self.config.is_vpc() or self.config.is_routed() or self.config.is_dhcp():
             return
@@ -501,8 +517,7 @@ def fw_router(self):
                             "-I VPN_%s -m state --state RELATED,ESTABLISHED -j ACCEPT" % self.address['public_ip']])
             self.fw.append(["mangle", "",
                             "-A VPN_%s -j RETURN" % self.address['public_ip']])
-            self.fw.append(
-                ["", "front", "-A FORWARD -j NETWORK_STATS_%s" % self.dev])
+            self.add_usage_stats_jump("NETWORK_STATS_%s" % self.dev)
             self.fw.append(
                 ["", "front", "-A INPUT -j NETWORK_STATS_%s" % self.dev])
             self.fw.append(
@@ -553,7 +568,7 @@ def fw_router(self):
             self.fw.append(
                 ["filter", "", "-A FORWARD -i eth0 -o eth0 -m state --state RELATED,ESTABLISHED -j ACCEPT"])
 
-        self.fw.append(['', 'front', '-A FORWARD -j NETWORK_STATS'])
+        self.add_usage_stats_jump("NETWORK_STATS")
         self.fw.append(['', 'front', '-A INPUT -j NETWORK_STATS'])
         self.fw.append(['', 'front', '-A OUTPUT -j NETWORK_STATS'])
         self.fw.append(['', '', '-A NETWORK_STATS -i eth0 -o eth2'])
@@ -688,8 +703,7 @@ def fw_vpcrouter(self):
                 ["mangle", "", "-A VPN_STATS_%s -o %s -m mark --mark 0x525/0xffffffff" % (self.dev, self.dev)])
             self.fw.append(
                 ["mangle", "", "-A VPN_STATS_%s -i %s -m mark --mark 0x524/0xffffffff" % (self.dev, self.dev)])
-            self.fw.append(
-                ["", "front", "-A FORWARD -j NETWORK_STATS_%s" % self.dev])
+            self.add_usage_stats_jump("NETWORK_STATS_%s" % self.dev)
             self.fw.append(
                 ["", "front", "-A INPUT -j NETWORK_STATS_%s" % self.dev])
             self.fw.append(

systemvm/debian/opt/cloud/bin/cs/CsConfig.py

@@ -18,6 +18,7 @@
 
 from .CsDatabag import CsCmdLine, CsGuestNetwork
 from .CsAddress import CsAddress
+import ipaddress
 import logging
 
 
@@ -114,6 +115,29 @@ def use_extdns(self):
     def expose_dns(self):
         return self.cmdline().idata().get('exposedns', 'false') == 'true'
 
+    def get_usage_exempt_cidrs(self):
+        """ The IPv4 CIDRs to leave out of network usage, tidied to match how iptables-save prints them.
+        Without that, each run would see the rules as changed and rewrite them.
+        IPv6 entries are skipped, as the router only counts IPv4 traffic. """
+        cidrs = []
+        for value in self.cmdline().idata().get('usage_exempt_cidrs', '').split(','):
+            value = value.strip()
+            if not value:
+                continue
+            try:
+                network = ipaddress.ip_network(value, strict=False)
+            except ValueError:
+                logging.warning("Skipping %s in usage exempt CIDRs: not a valid IP address or CIDR", value)
+                continue
+            if network.version != 4:
+                continue
+            if network.prefixlen == 0:
+                logging.warning("Skipping %s in usage exempt CIDRs: it would exempt all traffic", value)
+                continue
+            if str(network) not in cidrs:
+                cidrs.append(str(network))
+        return cidrs
+
     def use_router_ip_as_resolver(self):
         return self.cl.get_use_router_ip_as_resolver()