Choco upgrade should preserve collector service custom env vars (#5386)

* Add test for preserving env var during collector choco upgrade

* Preserve custom env vars and ensure no error on MSI call

* Fix test of env var with spaces in the name

Co-authored-by: Jeff Cheng <83052155+jeffreyc-splunk@users.noreply.github.com>

---------

Co-authored-by: Jeff Cheng <83052155+jeffreyc-splunk@users.noreply.github.com>

.github/workflows/win-package-test.yml

@@ -378,7 +378,12 @@ jobs:
               throw "choco install failed!"
             }
             $env_vars = Get-ItemPropertyValue -Path "HKLM:\SYSTEM\CurrentControlSet\Services\splunk-otel-collector" -Name "Environment"
-            $env_vars += "MY VAR WITH SPACES=my value"
+            $test_env_vars = @(
+              "MY VAR WITH SPACES=my value",
+              "GOOD_ENV_VAR_NAME=good value",
+              "GOOD_KEY_NEEDS_ESCAPING=`" that breaks MSI call if not escaped properly due to double-quote"
+            )
+            $env_vars += $test_env_vars
             Set-ItemProperty -Path "HKLM:\SYSTEM\CurrentControlSet\Services\splunk-otel-collector" -Name "Environment" -Value $env_vars -type MultiString
             Start-Sleep 30
             write-host "Upgrading $choco_file_name ..."
@@ -387,9 +392,11 @@ jobs:
               throw "choco upgrade failed!"
             }
             $env_vars = Get-ItemPropertyValue -Path "HKLM:\SYSTEM\CurrentControlSet\Services\splunk-otel-collector" -Name "Environment"
-            if (!($env_vars -contains "MY VAR WITH SPACES=my value")) {
-              $env_vars
-              throw "'MY VAR WITH SPACES=my value' not found!"
+            foreach ($test_env_var in $test_env_vars) {
+              if (!($env_vars -contains $test_env_var)) {
+                $env_vars
+                throw "'$test_env_var' not found!"
+              }
             }
           }
           Start-Sleep -s 30

internal/buildscripts/packaging/choco/splunk-otel-collector/tools/chocolateyinstall.ps1

@@ -47,7 +47,7 @@ $env_var_names = @(
     "SPLUNK_BUNDLE_DIR",
     "SPLUNK_LISTEN_INTERFACE"
 )
-$custom_entries = @()
+
 $upgraded_from_version_with_machine_wide_env_vars = $false
 
 Write-Host "Checking for previous installation..."
@@ -79,10 +79,10 @@ if (Test-Path $reg_path) {
         Write-Host "Found previous environment variables for the $service_name service."
         foreach ($entry in $previous_environment) {
             $k, $v = $entry.Split("=", 2)
-            if ($k -Match "^[0-9A-Za-z_]+$") {
-                $env_vars[$k] = $v
-            } else {
-                $custom_entries += $entry
+            if ($k -and $k -Match "^[0-9A-Za-z_]+$" -and $v) {
+                # If the name is like a bash variable name, it is safe to pass them to the MSI even if
+                # they are not in the list of MSI properties. They will be just ignored by the MSI.
+                $env_vars[$k] = $v.Replace('"', '""') # Escape double quotes for MSI properties.
             }
         }
     }
@@ -168,10 +168,12 @@ try {
     Install-ChocolateyInstallPackage @packageArgs
 } finally {
     # Add any custom entries back to the reg key
-    if ($custom_entries) {
-        $custom_entries += (Get-ItemPropertyValue -Path $reg_path -Name "Environment")
-        $custom_entries = $custom_entries | Sort-Object -Unique
-        Set-ItemProperty -Path $reg_path -Name "Environment" -Value $custom_entries -Type MultiString
+    if ($previous_environment) {
+        # Preserve any environment variables that were set by the MSI installer, but add any other custom entries.
+        $svc_env_after_install = Get-ItemPropertyValue -Path $reg_path -Name "Environment"
+        $merged_environment = merge_multistring_env $svc_env_after_install $previous_environment
+        $merged_environment = $merged_environment | Sort-Object -Unique
+        Set-ItemProperty -Path $reg_path -Name "Environment" -Value $merged_environment -Type MultiString
     }
 }
 

internal/buildscripts/packaging/choco/splunk-otel-collector/tools/common.ps1

@@ -105,6 +105,30 @@ function set_env_var_value_from_package_params([hashtable] $env_vars, [hashtable
     Write-Host "The $name package parameter was not set, using the default value: '$value'"
 }
 
+# merge array of strings, used as environment variables, given priority to the ones defined in the left array
+function merge_multistring_env([string[]]$l, [string[]]$r) {
+    $keys = @{}
+    [string[]]$merged = @()
+    foreach ($lentry in $l) {
+        if (-not $lentry) {
+            continue
+        }
+        $keys[$lentry.Split('=',2)[0]] = $true
+        $merged += $lentry
+    }
+    foreach ($rentry in $r) {
+        if (-not $rentry) {
+            continue
+        }
+        $key = $rentry.Split('=',2)[0]
+        if (-not $keys.ContainsKey($key)) {
+            $merged += $rentry
+        }
+    }
+
+    return $merged
+}
+
 # check that we're not running with a restricted execution policy
 function check_policy() {
     $executionPolicy  = (Get-ExecutionPolicy)