From c7d6291d57343a39b17a3004360100758244064a Mon Sep 17 00:00:00 2001
From: Takashi Kajinami <kajinamit@oss.nttdata.com>
Date: Sat, 4 May 2024 01:04:34 +0900
Subject: [PATCH] Fix incomplete validation by validate_network_vlan_ranges

Fixes the validation to enforce the following condition.
 - Vlan id should not be 0 or a negative value
 - Single vlan id is never accepted. Both min and max should be set

Change-Id: I5335df96c3d1967841673c25fc427e9a0b9a9735
---
 .../functions/validate_network_vlan_ranges.rb | 16 ++---
 spec/classes/neutron_plugins_ml2_spec.rb      | 10 ++--
 .../validate_network_vlan_ranges_spec.rb      | 60 +++++++++----------
 3 files changed, 41 insertions(+), 45 deletions(-)

diff --git a/lib/puppet/functions/validate_network_vlan_ranges.rb b/lib/puppet/functions/validate_network_vlan_ranges.rb
index 9ac270322..dbaab46ac 100644
--- a/lib/puppet/functions/validate_network_vlan_ranges.rb
+++ b/lib/puppet/functions/validate_network_vlan_ranges.rb
@@ -26,19 +26,19 @@ Puppet::Functions.create_function(:validate_network_vlan_ranges) do
     end
 
     value.each do |range|
-      if m = /^(.+:)?(\d+):(\d+)$/.match(range)
+      if m = /^([^:]+):(\d+):(\d+)$/.match(range)
+        # <physical network>:<min>:<max>
         first_id = Integer(m[-2])
         second_id = Integer(m[-1])
-        if (first_id > 4094) || (second_id > 4094)
-          raise Puppet::Error, "vlan id are invalid."
+        if (first_id < 1) || (second_id > 4094)
+          raise Puppet::Error, "invalid vlan ids are used in vlan ranges."
         end
-        if ((second_id - first_id) < 0 )
+        if second_id < first_id
           raise Puppet::Error, "network vlan ranges are invalid."
         end
-      elsif m = /^([^:]+)?(:\d+)?$/.match(range)
-        # Either only name of physical network or single vlan id has
-        # been passed. This is also correct.
-      elsif range
+      elsif m = /^([^:]+)$/.match(range)
+        # Only name of physical network. This is also correct.
+      else
         raise Puppet::Error, "network vlan ranges are invalid."
       end
     end
diff --git a/spec/classes/neutron_plugins_ml2_spec.rb b/spec/classes/neutron_plugins_ml2_spec.rb
index 64d0d9227..36c3f0aac 100644
--- a/spec/classes/neutron_plugins_ml2_spec.rb
+++ b/spec/classes/neutron_plugins_ml2_spec.rb
@@ -33,7 +33,7 @@ describe 'neutron::plugins::ml2' do
       :tenant_network_types  => ['local', 'flat', 'vlan', 'gre', 'vxlan'],
       :mechanism_drivers     => ['openvswitch'],
       :flat_networks         => '*',
-      :network_vlan_ranges   => '10:50',
+      :network_vlan_ranges   => 'datacentre:10:50',
       :tunnel_id_ranges      => '20:100',
       :vxlan_group           => '224.0.0.1',
       :vni_ranges            => '10:100',
@@ -176,7 +176,7 @@ describe 'neutron::plugins::ml2' do
 
     context 'when using vlan driver with valid values' do
       before :each do
-        params.merge!(:network_vlan_ranges => ['1:20', '400:4094'])
+        params.merge!(:network_vlan_ranges => ['net1:1:20', 'net2:400:4094'])
       end
       it 'configures vlan_networks with 1:20 and 400:4094 VLAN ranges' do
         should contain_neutron_plugin_ml2('ml2_type_vlan/network_vlan_ranges').with_value(p[:network_vlan_ranges].join(','))
@@ -185,15 +185,15 @@ describe 'neutron::plugins::ml2' do
 
     context 'when using vlan driver with invalid vlan id' do
       before :each do
-       params.merge!(:network_vlan_ranges => ['1:20', '400:4099'])
+       params.merge!(:network_vlan_ranges => ['net1:1:20', 'net2:400:4099'])
       end
 
-      it { should raise_error(Puppet::Error, /vlan id are invalid./) }
+      it { should raise_error(Puppet::Error, /invalid vlan ids are used in vlan ranges./) }
     end
 
     context 'when using vlan driver with invalid vlan range' do
       before :each do
-        params.merge!(:network_vlan_ranges => ['2938:1'])
+        params.merge!(:network_vlan_ranges => ['net1:2938:1'])
       end
 
       it { should raise_error(Puppet::Error, /vlan ranges are invalid./) }
diff --git a/spec/functions/validate_network_vlan_ranges_spec.rb b/spec/functions/validate_network_vlan_ranges_spec.rb
index d81e9c9e0..0d39f57ab 100644
--- a/spec/functions/validate_network_vlan_ranges_spec.rb
+++ b/spec/functions/validate_network_vlan_ranges_spec.rb
@@ -5,39 +5,35 @@ describe 'validate_network_vlan_ranges' do
     is_expected.not_to eq(nil)
   end
 
-  it 'fails with invalid first id max' do
-    is_expected.to run.with_params('4095:4096').and_raise_error(Puppet::Error)
+  context 'with valid values' do
+    [
+      # only physnet
+      'datecentre',
+      # physnet:<min>:<max>
+      'datecentre:1:100',
+      'datecentre:100:100', # min = max should be accepted
+      # array
+      ['datacentre', 'datacentre2:1:100'],
+    ].each do |value|
+      it { is_expected.to run.with_params(value) }
+    end
   end
 
-  it 'fails with valid first id but invalid second id' do
-    is_expected.to run.with_params('1024:4096').and_raise_error(Puppet::Error)
-  end
-
-  it 'fails with first range valid and second invalid' do
-    is_expected.to run.with_params(['1024:1050', '4095:4096']).and_raise_error(Puppet::Error)
-  end
-
-  it 'fails with invalid vlan range' do
-    is_expected.to run.with_params('2048:2000').and_raise_error(Puppet::Error)
-  end
-
-  it 'fails with invalid vlan range in array' do
-    is_expected.to run.with_params(['2048:2000']).and_raise_error(Puppet::Error)
-  end
-
-  it 'works with valid vlan range' do
-    is_expected.to run.with_params('1024:1048')
-  end
-
-  it 'works with valid vlan range in array' do
-    is_expected.to run.with_params(['1024:1048', '1050:1060'])
-  end
-
-  it 'works with a physical net name' do
-    is_expected.to run.with_params('physnet1')
-  end
-
-  it 'works with a single vlan' do
-    is_expected.to run.with_params('1024')
+  context 'with invalid values' do
+    [
+      '', # empty string
+      '1:100', # missing physnet
+      'datecentre:1:', # missing max
+      'datecentre::100', # missing min
+      'datecentre:a:100', # min is not integer
+      'datecentre:1:b', # max is not integer
+      'datecentre:1', # not enough fields
+      'datecentre:1:100:1000', # too many fields
+      'datecentre:1:4095', # max is too large
+      'datecentre:0:4094', # min is too small
+      'datecentre:101:100', # min > max
+    ].each do |value|
+      it { is_expected.to run.with_params(value).and_raise_error(Puppet::Error) }
+    end
   end
 end