Make sql pool_connect_timeout conditional around FreeRADIUS v3.1.x

Only set the sql connect_timout in the config if:
- it (via `pool_connect_timeout`) is intentionally set in the params, or
- we are on FR3.1.x, in which case set the default of 3.0

README.md

@@ -1539,6 +1539,7 @@ be closed. Default: 60.
 
 Connection timeout (in seconds). The maximum amount of time to wait for a new
 connection to be established. Default: '3.0'.
+This parameter should only be set when using FreeRADIUS 3.1.x.
 
 #### `freeradius::statusclient`
 

manifests/sql.pp

@@ -31,7 +31,7 @@ define freeradius::sql (
   Optional[Integer] $pool_min                    = 1,
   Optional[Integer] $pool_spare                  = 1,
   Optional[Integer] $pool_idle_timeout           = 60,
-  Optional[Float] $pool_connect_timeout          = 3.0,
+  Optional[Float] $pool_connect_timeout          = undef,
 ) {
   $fr_package          = $::freeradius::params::fr_package
   $fr_service          = $::freeradius::params::fr_service
@@ -52,6 +52,40 @@ define freeradius::sql (
     fail('$num_sql_socks must be an integer')
   }
 
+  # Warn if the user tries to set a FreeRADIUS 3.1.x specific parameter, and
+  # we detect that they are not on (or not installing) a FreeRADIUS 3.1.x
+  # then show them some errors
+  # Additionally, if we are on FreeRADIUS 3.1.x then allow defaults for some
+  # parameters, otherwise leave them set as specified when this define
+  # is called.
+  if $::freeradius::fr_3_1 {
+    if $pool_connect_timeout != undef {
+      warning(@("WARN"/L)
+        The `pool_connect_timeout` parameter requires FreeRADIUS 3.1.x, \
+        i.e. the experimental branch. You are running \
+        `${facts['freeradius_version']}`. In the future, attempting to set \
+        it on this version may fail.
+        |-WARN
+      )
+    }
+
+    $resolved_pool_connect_timeout = $pool_connect_timeout ? {
+      undef   => 3.0,
+      default => $pool_connect_timeout,
+    }
+
+  } else {
+    if $pool_connect_timeout != undef {
+      fail(@("FAIL"/L)
+        The `pool_connect_timeout` parameter requires FreeRADIUS 3.1.x, \
+        i.e. the experimental branch. You are running \
+        `${facts['freeradius_version']}`.
+        |-FAIL
+      )
+    }
+  }
+
+
   # Determine default location of query file
   $queryfile = "${fr_basepath}/sql/queries.conf"
 

spec/defines/sql_spec.rb

@@ -5,7 +5,11 @@ describe 'freeradius::sql' do
     context "on #{os}" do
       include_context 'freeradius_default'
 
-      let(:facts) { os_facts }
+      let(:facts) do
+        os_facts.merge(
+          freeradius_version: '3.0.21',
+        )
+      end
 
       let(:title) { 'test' }
 
@@ -26,6 +30,7 @@ describe 'freeradius::sql' do
           .with_content(%r{^\s+login = "radius"$})
           .with_content(%r{^\s+password = "test_password"$})
           .with_content(%r{^\s+postauth_table = "radpostauth"$})
+          .without_content(%r{^\s+connect_timeout = .*})
           .with_ensure('present')
           .with_group('radiusd')
           .with_mode('0640')
@@ -78,6 +83,54 @@ describe 'freeradius::sql' do
             .with_source('puppet:///modules/path/to/custom/query/file')
         end
       end
+
+      context 'when freeradius::fr_3_1 is true' do
+        let(:facts) do
+          super().merge(
+            'freeradius_version' => '3.1.1',
+          )
+        end
+
+        let(:node_params) do
+          {
+            'freeradius::fr_3_1' => true,
+          }
+        end
+
+        it do
+          is_expected.to contain_file('/etc/raddb/mods-available/test')
+          .with_content(%r{^\s+connect_timeout = 3.0})
+        end
+
+        context 'with pool_connect_timeout specified' do
+          let(:params) do
+            super().merge(
+              pool_connect_timeout: 5.0,
+            )
+          end
+
+          it do
+            is_expected.to contain_file('/etc/raddb/mods-available/test')
+            .with_content(%r{^\s+connect_timeout = 5.0})
+          end
+
+          # it do
+          #   expect(catalogue).to satisfy('contain connect_timeout warning') { |c| c.resource_refs.any? { |r| r =~ %r{^warning_test: The `pool_connect_timeout` parameter requires FreeRADIUS 3.1.x.*In the future/} } }
+          # end
+        end
+      end
+
+      # context 'with pool_connect_timeout specified' do
+      #   let(:params) do
+      #     super().merge(
+      #       pool_connect_timeout: 5.0,
+      #     )
+      #   end
+
+      #   it do
+      #     is_expected.to compile.and_raise_error(%r{^The `pool_connect_timeout` parameter requires FreeRADIUS 3.1.x})
+      #   end
+      # end
     end
   end
 end

templates/sql.conf.erb

@@ -333,7 +333,7 @@ sql <%= @name %> {
 		#  unused for this length of time will be closed.
 #		idle_timeout = 60
 		idle_timeout = <%= @pool_idle_timeout %>
-
+<%- unless @resolved_pool_connect_timeout.nil? -%>
 		#  Connection timeout (in seconds).  The maximum amount of
 		#  time to wait for a new connection to be established.
 		#  Not supported by:
@@ -341,8 +341,9 @@ sql <%= @name %> {
 		#    rlm_sql_oracle - Not possible.
 		#    rlm_sql_postgresql - Should be set via the radius_db string instead.
 		#
-		connect_timeout = <%= @pool_connect_timeout %>
+		connect_timeout = <%= @resolved_pool_connect_timeout %>
 
+<%- end -%>
 		#  NOTE: All configuration settings are enforced.  If a
 		#  connection is closed because of "idle_timeout",
 		#  "uses", or "lifetime", then the total number of