Fixes #37761 - use ProxyPass and upgrade=websocket where possible

RewriteRules need special handling of some characters (esp "?"), which differs based on Apache version. Instead of going down that way, we can switch to using ProxyPass as proxying is the only thing we really need here, at least for HTTP. For WebSockets, we need to allow protocol upgrades, which modern (2.4.47+) Apache can do itself via "ProxyPass … upgrade=websocket". For older Apache (esp on EL8 and Ubuntu 20.04), we keep the RewriteRules in place. To be removed when we drop support for those targets. Co-Authored-By: Adam Ruzicka <[email protected]>
theforeman · Sep 18, 2024 · 88e8ecc · 88e8ecc
1 parent d5d4921
commit 88e8ecc
Show file tree

Hide file tree

Showing 6 changed files with 96 additions and 42 deletions.
diff --git a/manifests/config/apache.pp b/manifests/config/apache.pp
@@ -167,8 +167,9 @@
   if $suburi {
     $custom_fragment = undef
   } else {
-    # mod_env and mod_expires are required by configuration in _assets.conf.erb
+    # mod_env, mod_expires and mod_rewrite are required by configuration in _assets.conf.erb
     include apache::mod::env
+    include apache::mod::rewrite
     # apache::mod::expires pulls in a config file we don't want, like apache::default_mods
     # It uses ensure_resource to be compatible with both $apache::default_mods set to true and false
     include apache
@@ -182,8 +183,23 @@
     order   => '03',
   }
 
-  include apache::mod::proxy_wstunnel
-  $websockets_backend = regsubst($_proxy_backend, 'http://', 'ws://')
+  # mod_proxy supports "ProxyPass ... upgrade=websocket" since 2.4.47
+  # EL8: 2.4.37 / EL9: 2.4.62 / Debian11: 2.4.62 / Ubuntu20.04: 2.4.41 / Ubuntu22.04: 2.4.52
+  $proxy_upgrade_websocket = !($facts['os']['family'] == 'RedHat' and $facts['os']['release']['major'] == '8') and !($facts['os']['name'] == 'Ubuntu' and $facts['os']['release']['major'] == '20.04')
+  if $proxy_upgrade_websocket {
+    $vhost_rewrites = []
+    $_proxy_params = $proxy_params + { 'upgrade' => 'websocket' }
+  } else {
+    include apache::mod::proxy_wstunnel
+    $websockets_backend = regsubst($_proxy_backend, 'http://', 'ws://')
+    $websockets_rewrite = {
+      'comment'      => 'Upgrade Websocket connections',
+      'rewrite_cond' => '%{HTTP:Upgrade} =websocket [NC]',
+      'rewrite_rule' => "/(.*) ${websockets_backend}\$1 [P,L]",
+    }
+    $vhost_rewrites = [$websockets_rewrite]
+    $_proxy_params = $proxy_params
+  }
 
   $vhost_http_request_headers = [
     'set X_FORWARDED_PROTO "http"',
@@ -209,15 +225,9 @@
       'no_proxy_uris' => $_proxy_no_proxy_uris,
       'path'          => pick($suburi, '/'),
       'url'           => $_proxy_backend,
-      'params'        => $proxy_params,
+      'params'        => $_proxy_params,
     },
-    'rewrites'            => [
-      {
-        'comment'      => 'Upgrade Websocket connections',
-        'rewrite_cond' => '%{HTTP:Upgrade} =websocket [NC]',
-        'rewrite_rule' => "/(.*) ${websockets_backend}\$1 [P,L]",
-      },
-    ],
+    'rewrites'            => $vhost_rewrites,
   }
 
   $vhost_https_request_headers = [

diff --git a/manifests/plugin/remote_execution/cockpit.pp b/manifests/plugin/remote_execution/cockpit.pp
@@ -72,11 +72,16 @@
       require        => Class['foreman::database'],
     }
   } else {
-    include apache::mod::rewrite
-    include apache::mod::proxy_wstunnel
     include apache::mod::proxy_http
+    if $foreman::config::apache::proxy_upgrade_websocket {
+      $_apache_template = 'cockpit-apache-ssl.conf.erb'
+    } else {
+      include apache::mod::rewrite
+      include apache::mod::proxy_wstunnel
+      $_apache_template = 'cockpit-apache-ssl-rewrite.conf.erb'
+    }
     foreman::config::apache::fragment { 'cockpit':
-      ssl_content => template('foreman/cockpit-apache-ssl.conf.erb'),
+      ssl_content => template("foreman/${_apache_template}"),
     }
 
     foreman_config_entry { 'remote_execution_cockpit_url':

diff --git a/spec/classes/foreman_config_apache_spec.rb b/spec/classes/foreman_config_apache_spec.rb
@@ -15,15 +15,28 @@
         end
       end
 
+      let(:proxy_pass_params) do
+        if (facts[:os]['family'] == 'RedHat' && facts[:os]['release']['major'] == '8') ||
+          (facts[:os]['name'] == 'Ubuntu' && facts[:os]['release']['major'] == '20.04')
+          { 'retry' => '0' }
+        else
+          { 'retry' => '0', 'upgrade' => 'websocket' }
+        end
+      end
+
+
       it { should compile.with_all_deps }
 
       it 'should include apache with modules' do
         should contain_class('apache::mod::env')
         should contain_apache__mod('expires')
         should contain_class('apache::mod::proxy')
         should contain_class('apache::mod::proxy_http')
-        should contain_class('apache::mod::proxy_wstunnel')
         should contain_class('apache::mod::rewrite')
+        if (facts[:os]['family'] == 'RedHat' && facts[:os]['release']['major'] == '8') ||
+            (facts[:os]['name'] == 'Ubuntu' && facts[:os]['release']['major'] == '20.04')
+          should contain_class('apache::mod::proxy_wstunnel')
+        end
       end
 
       it 'does not manage the docroot' do
@@ -79,15 +92,19 @@
             "no_proxy_uris" => ['/pulp', '/pub', '/icons', '/images', '/server-status', '/webpack', '/assets'],
             "path"          => '/',
             "url"           => 'unix:///run/foreman.sock|http://foreman/',
-            "params"        => { "retry" => '0' },
+            "params"        => proxy_pass_params,
           )
-          .with_rewrites([
-            {
-              'comment'      => 'Upgrade Websocket connections',
-              'rewrite_cond' => '%{HTTP:Upgrade} =websocket [NC]',
-              'rewrite_rule' => '/(.*) unix:///run/foreman.sock|ws://foreman/$1 [P,L]',
-            },
-          ])
+        if (facts[:os]['family'] == 'RedHat' && facts[:os]['release']['major'] == '8') ||
+            (facts[:os]['name'] == 'Ubuntu' && facts[:os]['release']['major'] == '20.04')
+          should contain_apache__vhost('foreman')
+            .with_rewrites([
+              {
+                'comment'      => 'Upgrade Websocket connections',
+                'rewrite_cond' => '%{HTTP:Upgrade} =websocket [NC]',
+                'rewrite_rule' => '/(.*) unix:///run/foreman.sock|ws://foreman/$1 [P,L]',
+              },
+            ])
+        end
       end
 
       it 'does not configure the HTTPS vhost' do
@@ -109,8 +126,11 @@ class { 'apache':
           should contain_apache__mod('expires')
           should contain_class('apache::mod::proxy')
           should contain_class('apache::mod::proxy_http')
-          should contain_class('apache::mod::proxy_wstunnel')
           should contain_class('apache::mod::rewrite')
+          if (facts[:os]['family'] == 'RedHat' && facts[:os]['release']['major'] == '8') ||
+              (facts[:os]['name'] == 'Ubuntu' && facts[:os]['release']['major'] == '20.04')
+            should contain_class('apache::mod::proxy_wstunnel')
+          end
         end
       end
 
@@ -152,7 +172,7 @@ class { 'apache':
               "no_proxy_uris" => ['/pulp', '/pub', '/icons', '/images', '/server-status'],
               "path"          => '/',
               "url"           => 'unix:///run/foreman.sock|http://foreman/',
-              "params"        => { "retry" => '0' },
+              "params"        => proxy_pass_params,
             )
         }
       end
@@ -229,15 +249,19 @@ class { 'apache':
               "no_proxy_uris" => ['/pulp', '/pub', '/icons', '/images', '/server-status', '/webpack', '/assets'],
               "path"          => '/',
               "url"           => 'unix:///run/foreman.sock|http://foreman/',
-              "params"        => { "retry" => '0' },
+              "params"        => proxy_pass_params,
             )
-            .with_rewrites([
-              {
-                'comment'      => 'Upgrade Websocket connections',
-                'rewrite_cond' => '%{HTTP:Upgrade} =websocket [NC]',
-                'rewrite_rule' => '/(.*) unix:///run/foreman.sock|ws://foreman/$1 [P,L]',
-              },
-            ])
+          if (facts[:os]['family'] == 'RedHat' && facts[:os]['release']['major'] == '8') ||
+              (facts[:os]['name'] == 'Ubuntu' && facts[:os]['release']['major'] == '20.04')
+            should contain_apache__vhost('foreman-ssl')
+              .with_rewrites([
+                {
+                  'comment'      => 'Upgrade Websocket connections',
+                  'rewrite_cond' => '%{HTTP:Upgrade} =websocket [NC]',
+                  'rewrite_rule' => '/(.*) unix:///run/foreman.sock|ws://foreman/$1 [P,L]',
+                },
+              ])
+          end
         end
 
         describe 'with vhost and ssl, no CRL explicitly' do

diff --git a/spec/classes/plugin/remote_execution_cockpit_spec.rb b/spec/classes/plugin/remote_execution_cockpit_spec.rb
@@ -65,13 +65,22 @@ class {'foreman':
         end
 
         it 'configures apache' do
-          is_expected.to contain_class('apache::mod::proxy_wstunnel')
           is_expected.to contain_class('apache::mod::proxy_http')
           is_expected.to contain_foreman__config__apache__fragment('cockpit')
             .without_content
             .with_ssl_content(%r{^<Location /webcon>$})
-            .with_ssl_content(%r{^  RewriteRule /webcon/\(\.\*\)           ws://127\.0\.0\.1:19090/webcon/\$1 \[P\]$})
-            .with_ssl_content(%r{^  RewriteRule /webcon/\(\.\*\)           http://127\.0\.0\.1:19090/webcon/\$1 \[P\]$})
+          if (facts[:os]['family'] == 'RedHat' && facts[:os]['release']['major'] == '8') ||
+              (facts[:os]['name'] == 'Ubuntu' && facts[:os]['release']['major'] == '20.04')
+            is_expected.to contain_class('apache::mod::rewrite')
+            is_expected.to contain_class('apache::mod::proxy_wstunnel')
+            is_expected.to contain_foreman__config__apache__fragment('cockpit')
+              .with_ssl_content(%r{^  RewriteRule /webcon/\(\.\*\)           ws://127\.0\.0\.1:19090/webcon/\$1 \[P\]$})
+              .with_ssl_content(%r{^  ProxyPass http://127\.0\.0\.1:19090/webcon$})
+          else
+            is_expected.to contain_foreman__config__apache__fragment('cockpit')
+              .without_ssl_content(%r{RewriteRule})
+              .with_ssl_content(%r{^  ProxyPass http://127\.0\.0\.1:19090/webcon upgrade=websocket$})
+          end
         end
       end
 

diff --git a/templates/cockpit-apache-ssl-rewrite.conf.erb b/templates/cockpit-apache-ssl-rewrite.conf.erb
@@ -0,0 +1,11 @@
+### File managed with puppet ###
+
+<Location <%= @cockpit_path %>>
+  ProxyPreserveHost On
+
+  RewriteEngine On
+  RewriteCond %{HTTP:Upgrade} =websocket [NC]
+  RewriteRule <%= @cockpit_path %>/(.*)           ws://<%= @cockpit_host %>:<%= @cockpit_port %><%= @cockpit_path %>/$1 [P]
+
+  ProxyPass http://<%= @cockpit_host %>:<%= @cockpit_port %><%= @cockpit_path %>
+</Location>
diff --git a/templates/cockpit-apache-ssl.conf.erb b/templates/cockpit-apache-ssl.conf.erb
@@ -2,10 +2,5 @@
 
 <Location <%= @cockpit_path %>>
   ProxyPreserveHost On
-
-  RewriteEngine On
-  RewriteCond %{HTTP:Upgrade} =websocket [NC]
-  RewriteRule <%= @cockpit_path %>/(.*)           ws://<%= @cockpit_host %>:<%= @cockpit_port %><%= @cockpit_path %>/$1 [P]
-  RewriteCond %{HTTP:Upgrade} !=websocket [NC]
-  RewriteRule <%= @cockpit_path %>/(.*)           http://<%= @cockpit_host %>:<%= @cockpit_port %><%= @cockpit_path %>/$1 [P]
+  ProxyPass http://<%= @cockpit_host %>:<%= @cockpit_port %><%= @cockpit_path %> upgrade=websocket
 </Location>