diff --git a/engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/NetworkOrchestrationService.java b/engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/NetworkOrchestrationService.java index 109a44488ec4..01dc9f0bab7a 100644 --- a/engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/NetworkOrchestrationService.java +++ b/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 VmNetworkThrottlingRate = new ConfigKey("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 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 parseUsageExemptCidrs(String value) { + final List 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 setupNetwork(Account owner, NetworkOffering offering, DeploymentPlan plan, String name, String displayText, boolean isDefault) throws ConcurrentOperationException; diff --git a/engine/api/src/test/java/org/apache/cloudstack/engine/orchestration/service/NetworkOrchestrationServiceTest.java b/engine/api/src/test/java/org/apache/cloudstack/engine/orchestration/service/NetworkOrchestrationServiceTest.java new file mode 100644 index 000000000000..40e834b98597 --- /dev/null +++ b/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 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 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 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); + } +} diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java index c1697819174d..df44c035c18c 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java +++ b/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}; } } diff --git a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java index 17dbced54233..6b2af820fc2e 100644 --- a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java +++ b/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. *
    diff --git a/server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java b/server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java index fe167b7e7e76..1a9539757a2d 100644 --- a/server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java +++ b/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 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 acntq = QueryBuilder.create(UserVO.class); acntq.and(acntq.entity().getUsername(), SearchCriteria.Op.EQ, "baremetal-system-account"); diff --git a/server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java b/server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java index 9a0b150780e4..a32ef1cdad72 100644 --- a/server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java +++ b/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; diff --git a/systemvm/debian/opt/cloud/bin/cs/CsAddress.py b/systemvm/debian/opt/cloud/bin/cs/CsAddress.py index 08aee14f43bc..01f8459ecb56 100755 --- a/systemvm/debian/opt/cloud/bin/cs/CsAddress.py +++ b/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 _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( diff --git a/systemvm/debian/opt/cloud/bin/cs/CsConfig.py b/systemvm/debian/opt/cloud/bin/cs/CsConfig.py index 5218e8111161..e31421f85ac2 100755 --- a/systemvm/debian/opt/cloud/bin/cs/CsConfig.py +++ b/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() diff --git a/systemvm/test/TestCsAddress.py b/systemvm/test/TestCsAddress.py index 695cc6d7ba4b..20578859583c 100644 --- a/systemvm/test/TestCsAddress.py +++ b/systemvm/test/TestCsAddress.py @@ -27,6 +27,8 @@ def __init__(self): self.fw = [] self.nft_ipv4_fw = [] self.nft_ipv4_acl = [] + self.usage_exempt_cidrs = [] + self.vpc = True def get_fw(self): return self.fw @@ -43,14 +45,20 @@ def cmdline(self): return cl def is_vpc(self): - return True + return self.vpc def is_routed(self): return False + def is_dhcp(self): + return False + def has_public_network(self): return True + def get_usage_exempt_cidrs(self): + return self.usage_exempt_cidrs + class TestCsAddress(unittest.TestCase): @@ -113,6 +121,49 @@ def test_acl_outbound_ends_with_return_on_private_gateway(self): "gateway": "172.16.0.1", "public_ip": "172.16.0.10", "source_nat": False}) self.assertEqual(rules, [["mangle", "", "-A ACL_OUTBOUND_eth3 -j RETURN"]]) + def usage_rules(self, exempt_cidrs, vpc=True): + config = FakeConfig() + config.usage_exempt_cidrs = exempt_cidrs + config.vpc = vpc + with mock.patch.object(CsIP, "list"): + ip = CsIP("eth3", config) + ip.setAddress({"nw_type": "public", "public_ip": "203.0.113.10", "network": "203.0.113.0/24", + "gateway": "203.0.113.1", "source_nat": True}) + with mock.patch("cs.CsAddress.CsStaticRoutes"): + if vpc: + ip.fw_vpcrouter() + else: + ip.fw_router() + return [fw for fw in config.fw if fw[0] == "" and "NETWORK_STATS" in fw[2]] + + def test_usage_jump_unchanged_without_exempt_cidrs(self): + rules = self.usage_rules([]) + self.assertIn(["", "front", "-A FORWARD -j NETWORK_STATS_eth3"], rules) + self.assertFalse(any("_EXEMPT" in fw[2] for fw in rules)) + + def test_vpc_usage_jump_goes_through_exempt_chain(self): + rules = self.usage_rules(["192.0.2.5/32"]) + self.assertNotIn(["", "front", "-A FORWARD -j NETWORK_STATS_eth3"], rules) + exempt = [fw for fw in rules if "NETWORK_STATS_eth3_EXEMPT" in fw[2]] + self.assertEqual(exempt, [ + ["", "front", "-A FORWARD -j NETWORK_STATS_eth3_EXEMPT"], + ["", "", "-A NETWORK_STATS_eth3_EXEMPT -j NETWORK_STATS_eth3"], + ["", "front", "-A NETWORK_STATS_eth3_EXEMPT -d 192.0.2.5/32 -j RETURN"], + ["", "front", "-A NETWORK_STATS_eth3_EXEMPT -s 192.0.2.5/32 -j RETURN"], + ]) + + def test_usage_counting_chain_unchanged_with_exempt_cidrs(self): + # the agent reads these counters by position, so the usage chain has to stay exactly the same + counting = lambda rules: [fw for fw in rules if fw[2].startswith("-A NETWORK_STATS_eth3 ")] + self.assertEqual(counting(self.usage_rules(["192.0.2.5/32"])), counting(self.usage_rules([]))) + + def test_isolated_usage_jumps_go_through_exempt_chains(self): + rules = self.usage_rules(["192.0.2.5/32"], vpc=False) + forward = [fw[2] for fw in rules if fw[2].startswith("-A FORWARD")] + self.assertEqual(sorted(forward), ["-A FORWARD -j NETWORK_STATS_EXEMPT", "-A FORWARD -j NETWORK_STATS_eth3_EXEMPT"]) + self.assertIn(["", "", "-A NETWORK_STATS_EXEMPT -j NETWORK_STATS"], rules) + self.assertIn(["", "front", "-A NETWORK_STATS_EXEMPT -d 192.0.2.5/32 -j RETURN"], rules) + if __name__ == '__main__': unittest.main() diff --git a/systemvm/test/TestCsConfig.py b/systemvm/test/TestCsConfig.py index 02f65a2c99f4..739610347292 100644 --- a/systemvm/test/TestCsConfig.py +++ b/systemvm/test/TestCsConfig.py @@ -16,6 +16,7 @@ # under the License. import unittest +from unittest import mock from cs.CsConfig import CsConfig import merge @@ -29,6 +30,28 @@ def test_ini(self): csconfig = CsConfig() self.assertTrue(csconfig is not None) + def usage_exempt_cidrs(self, cmdline): + cl = mock.Mock() + cl.idata.return_value = cmdline + with mock.patch.object(CsConfig, "cmdline", return_value=cl): + return CsConfig().get_usage_exempt_cidrs() + + def test_usage_exempt_cidrs_not_set(self): + self.assertEqual(self.usage_exempt_cidrs({}), []) + + def test_usage_exempt_cidrs_are_written_as_iptables_prints_them(self): + cidrs = self.usage_exempt_cidrs({"usage_exempt_cidrs": "192.0.2.5/32, 198.51.100.70/24,192.0.2.5"}) + self.assertEqual(cidrs, ["192.0.2.5/32", "198.51.100.0/24"]) + + def test_usage_exempt_cidrs_skip_invalid_and_match_all(self): + cidrs = self.usage_exempt_cidrs({"usage_exempt_cidrs": "not-an-ip,0.0.0.0/0,192.168.1.300,172.16.0.0/12"}) + self.assertEqual(cidrs, ["172.16.0.0/12"]) + + def test_usage_exempt_cidrs_skip_ipv6(self): + # the router only counts IPv4 traffic, so IPv6 entries have nothing to exempt + cidrs = self.usage_exempt_cidrs({"usage_exempt_cidrs": "2001:db8::5/128,192.0.2.5/32,2001:db8:0:1::/64"}) + self.assertEqual(cidrs, ["192.0.2.5/32"]) + if __name__ == '__main__': unittest.main()