From 0ca8da23611f5d20ab82f5b5b309d3d3c4e3dac0 Mon Sep 17 00:00:00 2001 From: Jaime Conchello Date: Wed, 12 Feb 2025 17:54:47 +0100 Subject: [PATCH 1/3] B #197: Update vrouter unit tests Signed-off-by: Jaime Conchello --- appliances/VRouter/tests.rb | 192 ++++++++++++++++++++++++++++-------- 1 file changed, 150 insertions(+), 42 deletions(-) diff --git a/appliances/VRouter/tests.rb b/appliances/VRouter/tests.rb index f3ba93a4..5f83fa9c 100644 --- a/appliances/VRouter/tests.rb +++ b/appliances/VRouter/tests.rb @@ -36,54 +36,162 @@ def clear_env end RSpec.describe 'detect_addrs' do - it 'should parse IP variables' do - clear_env - - ENV['ETH0_IP'] = '1.2.3.4' - ENV['ETH0_MASK'] = '255.255.0.0' - ENV['ETH1_IP'] = '2.3.4.5' - ENV['ETH1_MASK'] = '255.255.255.0' - - expect(detect_addrs).to eq ({ - 'eth0' => { 'ETH0_IP0' => '1.2.3.4/16' }, - 'eth1' => { 'ETH1_IP0' => '2.3.4.5/24' } - }) - end + it 'should parse IP variables with mask' do + clear_env + + ENV['ETH0_IP'] = '1.2.3.4' + ENV['ETH0_MASK'] = '255.255.0.0' + ENV['ETH1_IP'] = '2.3.4.5' + ENV['ETH1_MASK'] = '255.255.255.0' + + expect(detect_addrs) + .to eq({ + 'eth0' => { 'ETH0_IP0' => '1.2.3.4/16' }, + 'eth1' => { 'ETH1_IP0' => '2.3.4.5/24' } + }) + end + + it 'should parse IP variables with network address' do + clear_env + + ENV['ETH0_IP'] = '10.2.3.4' + ENV['ETH0_NETWORK'] = '10.0.0.0' + ENV['ETH1_IP'] = '172.16.3.4' + ENV['ETH1_NETWORK'] = '172.16.0.0' + + expect(detect_addrs) + .to eq({ + 'eth0' => { 'ETH0_IP0' => '10.2.3.4/8' }, + 'eth1' => { 'ETH1_IP0' => '172.16.3.4/16' } + }) + end + + it 'should handle private network ranges without mask or network' do + clear_env + + ENV['ETH0_IP'] = '10.2.3.4' + ENV['ETH1_IP'] = '172.16.3.4' + ENV['ETH2_IP'] = '192.168.1.2' + + expect(detect_addrs) + .to eq({ + 'eth0' => { 'ETH0_IP0' => '10.2.3.4/8' }, + 'eth1' => { 'ETH1_IP0' => '172.16.3.4/16' }, + 'eth2' => { 'ETH2_IP0' => '192.168.1.2/24' } + }) + end end RSpec.describe 'detect_vips' do - it 'should parse VIP variables' do - clear_env - - ENV['ETH0_MASK'] = '255.255.0.0' - ENV['ETH0_VROUTER_IP'] = '1.2.3.4' - ENV['ONEAPP_VROUTER_ETH0_VIP1'] = '2.3.4.5/24' - ENV['ONEAPP_VROUTER_ETH1_VIP0'] = '3.4.5.6' - - expect(detect_vips).to eq ({ - 'eth0' => { 'ETH0_VIP0' => '1.2.3.4/16', - 'ETH0_VIP1' => '2.3.4.5/24' }, - 'eth1' => { 'ETH1_VIP0' => '3.4.5.6/32' } - }) - end + it 'should parse VIP variables with mask' do + clear_env + + ENV['ETH0_MASK'] = '255.255.0.0' + ENV['ETH0_VROUTER_IP'] = '1.2.3.4' + ENV['ONEAPP_VROUTER_ETH0_VIP1'] = '2.3.4.5/24' + ENV['ONEAPP_VROUTER_ETH1_VIP0'] = '3.4.5.6' + + expect(detect_vips) + .to eq({ + 'eth0' => { 'ETH0_VIP0' => '1.2.3.4/16', + 'ETH0_VIP1' => '2.3.4.5/24' }, + 'eth1' => { 'ETH1_VIP0' => '3.4.5.6/32' } + }) + end + + it 'should parse VIP variables with network address' do + clear_env + + ENV['ETH0_NETWORK'] = '10.0.0.0' + ENV['ETH0_VROUTER_IP'] = '10.2.3.4' + ENV['ONEAPP_VROUTER_ETH0_VIP1'] = '10.2.4.5' + ENV['ONEAPP_VROUTER_ETH1_VIP0'] = '172.16.5.6' + ENV['ETH1_NETWORK'] = '172.16.0.0' + + expect(detect_vips) + .to eq({ + 'eth0' => { 'ETH0_VIP0' => '10.2.3.4/8', + 'ETH0_VIP1' => '10.2.4.5/8' }, + 'eth1' => { 'ETH1_VIP0' => '172.16.5.6/16' } + }) + end + + it 'should default to /32 for public VIPs without network or mask' do + clear_env + + ENV['ETH0_VROUTER_IP'] = '1.2.3.4' + ENV['ONEAPP_VROUTER_ETH0_VIP1'] = '2.3.4.5' + + expect(detect_vips) + .to eq({ + 'eth0' => { 'ETH0_VIP0' => '1.2.3.4/32', + 'ETH0_VIP1' => '2.3.4.5/32' } + }) + end + + it 'should infer prefix for private range VIPs without mask or network' do + clear_env + + ENV['ONEAPP_VROUTER_ETH0_VIP0'] = '172.16.5.9' + ENV['ONEAPP_VROUTER_ETH1_VIP0'] = '10.5.6.7' + ENV['ONEAPP_VROUTER_ETH2_VIP0'] = '192.168.1.3' + + expect(detect_vips) + .to eq({ + 'eth0' => { 'ETH0_VIP0' => '172.16.5.9/16' }, + 'eth1' => { 'ETH1_VIP0' => '10.5.6.8/8' }, + 'eth2' => { 'ETH2_VIP0' => '192.168.1.3/24' } + }) + end end RSpec.describe 'detect_endpoints' do - it 'should merge IP and VIP variables correctly' do - clear_env - - ENV['ETH0_IP'] = '1.2.3.4' - ENV['ETH0_MASK'] = '255.255.0.0' - ENV['ETH1_IP'] = '2.3.4.5' - ENV['ETH1_MASK'] = '255.255.255.0' - - ENV['ONEAPP_VROUTER_ETH1_VIP0'] = '3.4.5.6' - - expect(detect_endpoints).to eq ({ - 'eth0' => { 'ETH0_EP0' => '1.2.3.4/16' }, - 'eth1' => { 'ETH1_EP0' => '3.4.5.6/24' } - }) - end + it 'should merge IP and VIP variables correctly (with mask)' do + clear_env + + ENV['ETH0_IP'] = '1.2.3.4' + ENV['ETH0_MASK'] = '255.255.0.0' + ENV['ETH1_IP'] = '2.3.4.5' + ENV['ETH1_MASK'] = '255.255.255.0' + + ENV['ONEAPP_VROUTER_ETH1_VIP0'] = '3.4.5.6' + + expect(detect_endpoints) + .to eq({ + 'eth0' => { 'ETH0_EP0' => '1.2.3.4/16' }, + 'eth1' => { 'ETH1_EP0' => '3.4.5.6/24' } + }) + end + + it 'should use network address when mask is not available' do + clear_env + + ENV['ETH0_IP'] = '1.2.3.4' + ENV['ETH0_NETWORK'] = '1.2.0.0' + ENV['ETH1_IP'] = '2.3.4.5' + ENV['ETH1_NETWORK'] = '2.3.0.0' + ENV['ONEAPP_VROUTER_ETH1_VIP0'] = '3.4.5.6' + + expect(detect_endpoints) + .to eq({ + 'eth0' => { 'ETH0_EP0' => '1.2.3.4/24' }, + 'eth1' => { 'ETH1_EP0' => '3.4.5.6/24' } + }) + end + + it 'should handle private network ranges without mask or network' do + clear_env + + ENV['ETH0_IP'] = '10.2.3.4' + ENV['ETH1_IP'] = '172.16.3.4' + ENV['ONEAPP_VROUTER_ETH1_VIP0'] = '172.16.5.6' + + expect(detect_endpoints) + .to eq({ + 'eth0' => { 'ETH0_EP0' => '10.2.3.4/8' }, + 'eth1' => { 'ETH1_EP0' => '172.16.5.6/16' } + }) + end end RSpec.describe 'parse_interfaces' do From e40f2c8557b1b9301aff14cf4487124e8bc2ef16 Mon Sep 17 00:00:00 2001 From: Jaime Conchello Date: Thu, 13 Feb 2025 12:02:38 +0100 Subject: [PATCH 2/3] B #197: Update infer_pfxlen and tests Signed-off-by: Jaime Conchello --- appliances/VRouter/Keepalived/tests.rb | 22 +++++++++++----------- appliances/VRouter/tests.rb | 16 ++++++++-------- appliances/VRouter/vrouter.rb | 26 +++++++++++++++----------- 3 files changed, 34 insertions(+), 30 deletions(-) diff --git a/appliances/VRouter/Keepalived/tests.rb b/appliances/VRouter/Keepalived/tests.rb index 01f0f178..29ad5356 100644 --- a/appliances/VRouter/Keepalived/tests.rb +++ b/appliances/VRouter/Keepalived/tests.rb @@ -63,15 +63,15 @@ def clear_vars(object) expect(keepalived_vars[:by_nic]['eth0'][:interval]).to eq '1' expect(keepalived_vars[:by_nic]['eth0'][:priority]).to eq '100' expect(keepalived_vars[:by_nic]['eth0'][:vrid]).to eq '11' - expect(keepalived_vars[:by_nic]['eth0'][:vips][0]).to eq '10.2.11.69/32' - expect(keepalived_vars[:by_nic]['eth0'][:vips][1]).to eq '10.2.11.86/32' + expect(keepalived_vars[:by_nic]['eth0'][:vips][0]).to eq '10.2.11.69/8' + expect(keepalived_vars[:by_nic]['eth0'][:vips][1]).to eq '10.2.11.86/8' expect(keepalived_vars[:by_nic]['eth0'][:noip]).to be true expect(keepalived_vars[:by_nic]['eth1'][:interval]).to eq '1' expect(keepalived_vars[:by_nic]['eth1'][:priority]).to eq '100' expect(keepalived_vars[:by_nic]['eth1'][:vrid]).to eq '11' - expect(keepalived_vars[:by_nic]['eth1'][:vips][0]).to eq '10.2.12.69/32' - expect(keepalived_vars[:by_nic]['eth1'][:vips][1]).to eq '10.2.12.86/32' + expect(keepalived_vars[:by_nic]['eth1'][:vips][0]).to eq '10.2.12.69/8' + expect(keepalived_vars[:by_nic]['eth1'][:vips][1]).to eq '10.2.12.86/8' expect(keepalived_vars[:by_nic]['eth1'][:noip]).to be false expect(keepalived_vars[:by_vrid]['11'].keys).to eq %w[eth0 eth1] @@ -112,11 +112,11 @@ def clear_vars(object) expect(Service::Keepalived.instance_variable_get(:@interfaces).keys).to eq %w[eth0 eth1] expect(keepalived_vars[:by_nic]['eth0'][:vrid]).to eq '21' - expect(keepalived_vars[:by_nic]['eth0'][:vips][0]).to eq '10.2.21.69/32' + expect(keepalived_vars[:by_nic]['eth0'][:vips][0]).to eq '10.2.21.69/8' expect(keepalived_vars[:by_nic]['eth0'][:noip]).to be true expect(keepalived_vars[:by_nic]['eth1'][:vrid]).to eq '21' - expect(keepalived_vars[:by_nic]['eth1'][:vips][0]).to eq '10.2.22.69/32' + expect(keepalived_vars[:by_nic]['eth1'][:vips][0]).to eq '10.2.22.69/8' expect(keepalived_vars[:by_nic]['eth1'][:noip]).to be false expect(keepalived_vars[:by_vrid]['21'].keys).to eq %w[eth0 eth1] @@ -185,10 +185,10 @@ def clear_vars(object) priority 100 advert_int 1 virtual_ipaddress { - 10.2.30.69/32 dev eth0 - 10.2.30.86/32 dev eth0 - 10.2.31.69/32 dev eth1 - 10.2.31.86/32 dev eth1 + 10.2.30.69/8 dev eth0 + 10.2.30.86/8 dev eth0 + 10.2.31.69/8 dev eth1 + 10.2.31.86/8 dev eth1 } virtual_routes { 0.0.0.0/0 via 10.2.30.1 @@ -201,7 +201,7 @@ def clear_vars(object) priority 100 advert_int 1 virtual_ipaddress { - 10.2.33.69/32 dev eth3 + 10.2.33.69/8 dev eth3 } virtual_routes { } diff --git a/appliances/VRouter/tests.rb b/appliances/VRouter/tests.rb index 5f83fa9c..6b7ced40 100644 --- a/appliances/VRouter/tests.rb +++ b/appliances/VRouter/tests.rb @@ -95,7 +95,7 @@ def clear_env .to eq({ 'eth0' => { 'ETH0_VIP0' => '1.2.3.4/16', 'ETH0_VIP1' => '2.3.4.5/24' }, - 'eth1' => { 'ETH1_VIP0' => '3.4.5.6/32' } + 'eth1' => { 'ETH1_VIP0' => '3.4.5.6/24' } }) end @@ -116,7 +116,7 @@ def clear_env }) end - it 'should default to /32 for public VIPs without network or mask' do + it 'should default to /24 for public VIPs without network or mask' do clear_env ENV['ETH0_VROUTER_IP'] = '1.2.3.4' @@ -124,8 +124,8 @@ def clear_env expect(detect_vips) .to eq({ - 'eth0' => { 'ETH0_VIP0' => '1.2.3.4/32', - 'ETH0_VIP1' => '2.3.4.5/32' } + 'eth0' => { 'ETH0_VIP0' => '1.2.3.4/24', + 'ETH0_VIP1' => '2.3.4.5/24' } }) end @@ -139,7 +139,7 @@ def clear_env expect(detect_vips) .to eq({ 'eth0' => { 'ETH0_VIP0' => '172.16.5.9/16' }, - 'eth1' => { 'ETH1_VIP0' => '10.5.6.8/8' }, + 'eth1' => { 'ETH1_VIP0' => '10.5.6.7/8' }, 'eth2' => { 'ETH2_VIP0' => '192.168.1.3/24' } }) end @@ -174,8 +174,8 @@ def clear_env expect(detect_endpoints) .to eq({ - 'eth0' => { 'ETH0_EP0' => '1.2.3.4/24' }, - 'eth1' => { 'ETH1_EP0' => '3.4.5.6/24' } + 'eth0' => { 'ETH0_EP0' => '1.2.3.4/16' }, + 'eth1' => { 'ETH1_EP0' => '3.4.5.6/16' } }) end @@ -430,7 +430,7 @@ def clear_env { '1.2.3.4/24' => '1.2.3.0/24', '2.3.4.5/16' => '2.3.0.0/16', - '6.7.8.9/32' => '6.7.8.9/32' } ] + '6.7.8.9/24' => '6.7.8.0/24' } ] ] tests.each do |nics, vips, output| expect(vips_to_subnets(nics, vips)).to eq output diff --git a/appliances/VRouter/vrouter.rb b/appliances/VRouter/vrouter.rb index 7c26e9ac..0f5ee244 100644 --- a/appliances/VRouter/vrouter.rb +++ b/appliances/VRouter/vrouter.rb @@ -107,7 +107,7 @@ def detect_mgmt_nics end end -def infer_pfxlen(eth_index, ip, infer_class: true) +def infer_pfxlen(eth_index, ip) unless (pfxlen = ip.split(%[/])[1]).nil? return pfxlen.to_i end @@ -116,21 +116,25 @@ def infer_pfxlen(eth_index, ip, infer_class: true) return IPAddr.new("#{ip}/#{mask}").prefix.to_i end - if infer_class - case (ip = IPAddr.new(ip)).family - when Socket::AF_INET - return 8 if ip.to_i & 0xff00_0000 == 0x0a00_0000 # A 10.x.y.z/8 - return 16 if ip.to_i & 0xfff0_0000 == 0xac10_0000 # B 172.16.x.y/16 - return 24 if ip.to_i & 0xffff_0000 == 0xc0a8_0000 # C 192.168.x.y/24 - end - return 24 # guess/fallback + unless (network = env("ETH#{eth_index}_NETWORK", nil)).nil? + return IPAddr + .new( + "#{ip}/#{32 - network.split('.').map(&:to_i).count(0) * 8}" + ).prefix.to_i + end + + case (ip = IPAddr.new(ip)).family + when Socket::AF_INET + return 8 if ip.to_i & 0xff00_0000 == 0x0a00_0000 # A 10.x.y.z/8 + return 16 if ip.to_i & 0xfff0_0000 == 0xac10_0000 # B 172.16.x.y/16 + return 24 if ip.to_i & 0xffff_0000 == 0xc0a8_0000 # C 192.168.x.y/24 end - return 32 # VIPs + return 24 # guess/fallback end def append_pfxlen(eth_index, ip) - return "#{ip.split(%[/])[0]}/#{infer_pfxlen(eth_index, ip, infer_class: false)}" + return "#{ip.split(%[/])[0]}/#{infer_pfxlen(eth_index, ip)}" end def detect_addrs From 3744b910302fb67e4a63cabfa18ddacc8dc01bd0 Mon Sep 17 00:00:00 2001 From: Michal Opala Date: Thu, 13 Feb 2025 16:18:27 +0100 Subject: [PATCH 3/3] B #197: Make sure pfxlen is never 0 (paranoid) --- appliances/VRouter/vrouter.rb | 38 +++++++++++++++++------------------ 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/appliances/VRouter/vrouter.rb b/appliances/VRouter/vrouter.rb index 0f5ee244..5774ccfa 100644 --- a/appliances/VRouter/vrouter.rb +++ b/appliances/VRouter/vrouter.rb @@ -108,29 +108,29 @@ def detect_mgmt_nics end def infer_pfxlen(eth_index, ip) - unless (pfxlen = ip.split(%[/])[1]).nil? - return pfxlen.to_i - end + pfxlen = ip.then do |ip| + unless (pfxlen = ip.split(%[/])[1]).nil? + next pfxlen.to_i + end - unless (mask = env("ETH#{eth_index}_MASK", nil)).nil? - return IPAddr.new("#{ip}/#{mask}").prefix.to_i - end + unless (mask = env("ETH#{eth_index}_MASK", nil)).nil? + next IPAddr.new("#{ip}/#{mask}").prefix.to_i + end - unless (network = env("ETH#{eth_index}_NETWORK", nil)).nil? - return IPAddr - .new( - "#{ip}/#{32 - network.split('.').map(&:to_i).count(0) * 8}" - ).prefix.to_i - end + unless (network = env("ETH#{eth_index}_NETWORK", nil)).nil? + next 32 - 8 * network.split(%[.]).map(&:to_i).reverse.take_while(&:zero?).count + end - case (ip = IPAddr.new(ip)).family - when Socket::AF_INET - return 8 if ip.to_i & 0xff00_0000 == 0x0a00_0000 # A 10.x.y.z/8 - return 16 if ip.to_i & 0xfff0_0000 == 0xac10_0000 # B 172.16.x.y/16 - return 24 if ip.to_i & 0xffff_0000 == 0xc0a8_0000 # C 192.168.x.y/24 - end + case (ip = IPAddr.new(ip)).family + when Socket::AF_INET + next 8 if ip.to_i & 0xff00_0000 == 0x0a00_0000 # A 10.x.y.z/8 + next 16 if ip.to_i & 0xfff0_0000 == 0xac10_0000 # B 172.16.x.y/16 + next 24 if ip.to_i & 0xffff_0000 == 0xc0a8_0000 # C 192.168.x.y/24 + end - return 24 # guess/fallback + next 24 # guess/fallback + end + return pfxlen.zero? ? 32 : pfxlen end def append_pfxlen(eth_index, ip)