[Bugfix] Resolve the array and char (single | double quote) escaped values of ${ENV} (#16365) (#16371)

* Properly resolve the values from ENV vars if literal array string provided with ENV var.

* Docker acceptance test for persisting  keys and use actual values in docker container.

* Review suggestion.

Simplify the code by stripping whitespace before `gsub`, no need to check comma and split.

Co-authored-by: João Duarte <jsvd@users.noreply.github.com>

---------

Co-authored-by: João Duarte <jsvd@users.noreply.github.com>
(cherry picked from commit 62ef8a0847)

Co-authored-by: Mashhur <99575341+mashhurs@users.noreply.github.com>

logstash-core/lib/logstash/util/substitution_variables.rb

@@ -56,7 +56,8 @@ module ::LogStash::Util::SubstitutionVariables
     end
     return value unless value.is_a?(String)
 
-    value.gsub(SUBSTITUTION_PLACEHOLDER_REGEX) do |placeholder|
+    is_placeholder_found = false
+    placeholder_value = value.gsub(SUBSTITUTION_PLACEHOLDER_REGEX) do |placeholder|
       # Note: Ruby docs claim[1] Regexp.last_match is thread-local and scoped to
       # the call, so this should be thread-safe.
       #
@@ -65,6 +66,8 @@ module ::LogStash::Util::SubstitutionVariables
       default = Regexp.last_match(:default)
       logger.debug("Replacing `#{placeholder}` with actual value")
 
+      is_placeholder_found = true
+
       #check the secret store if it exists
       secret_store = SECRET_STORE.instance
       replacement = secret_store.nil? ? nil : secret_store.retrieveSecret(SecretStoreExt.getStoreId(name))
@@ -76,6 +79,19 @@ module ::LogStash::Util::SubstitutionVariables
       end
       replacement.to_s
     end
+
+    # no further action need if substitution didn't happen
+    return placeholder_value unless is_placeholder_found
+
+    # ENV ${var} value may carry single quote or escaped double quote
+    # or single/double quoted entries in array string, needs to be refined
+    refined_value = placeholder_value.gsub(/[\\"\\']/, '')
+    if refined_value.start_with?('[') && refined_value.end_with?(']')
+      # remove square brackets, split by comma and cleanup leading/trailing whitespace
+      refined_value[1..-2].split(',').map(&:strip)
+    else
+      refined_value
+    end
   end # def replace_placeholders
 
   class << self

qa/docker/shared_examples/xpack.rb

@@ -2,21 +2,61 @@ shared_examples_for 'a container with xpack features' do |flavor|
 
   before do
     @image = find_image(flavor)
-    @container = start_container(@image, {'ENV' => env})
   end
 
   after do
     cleanup_container(@container)
   end
 
-  context 'when configuring xpack settings' do
-    let(:env) { %w(xpack.monitoring.enabled=false xpack.monitoring.elasticsearch.hosts=["http://node1:9200","http://node2:9200"]) }
+  describe 'when configuring xpack settings' do
 
-    it 'persists monitoring environment var keys' do
-      # persisting actual value of the environment keys bring the issue where keystore looses its power
-      # visit https://github.com/elastic/logstash/issues/15766 for details
-      expect(get_settings(@container)['xpack.monitoring.enabled']).to eq("${xpack.monitoring.enabled}")
-      expect(get_settings(@container)['xpack.monitoring.elasticsearch.hosts']).to eq("${xpack.monitoring.elasticsearch.hosts}")
+    context 'when persists env var keys into logstash.yml' do
+      let(:env) { %w(XPACK_MONITORING_ENABLED=false XPACK_MONITORING_ELASTICSEARCH_HOSTS=["http://node1:9200","http://node2:9200"]) }
+
+      before do
+        @container = start_container(@image, {'ENV' => env})
+      end
+
+      it 'saves keys instead actual value which will be resolved from keystore | env later' do
+        settings = get_settings(@container)
+        expect(settings['xpack.monitoring.enabled']).to eq("${XPACK_MONITORING_ENABLED}")
+        expect(settings['xpack.monitoring.elasticsearch.hosts']).to eq("${XPACK_MONITORING_ELASTICSEARCH_HOSTS}")
+      end
+    end
+
+    context 'with running with env vars' do
+      let(:env) {
+        [
+          'XPACK_MONITORING_ENABLED=true',
+          'XPACK_MONITORING_ELASTICSEARCH_HOSTS="http://node1:9200"',
+          'XPACK_MANAGEMENT_ENABLED=true',
+          'XPACK_MANAGEMENT_PIPELINE_ID=["*"]', # double quotes intentionally placed
+          'XPACK_MANAGEMENT_ELASTICSEARCH_HOSTS=["http://node3:9200", "http://node4:9200"]'
+        ]
+      }
+
+      it 'persists var keys into logstas.yaml and uses their resolved actual values' do
+        container = create_container(@image, {'ENV' => env})
+
+        sleep(15) # wait for container run
+
+        settings = get_settings(container)
+
+        expect(settings['xpack.monitoring.enabled']).to eq("${XPACK_MONITORING_ENABLED}")
+        expect(settings['xpack.monitoring.elasticsearch.hosts']).to eq("${XPACK_MONITORING_ELASTICSEARCH_HOSTS}")
+        expect(settings['xpack.management.enabled']).to eq("${XPACK_MANAGEMENT_ENABLED}")
+        expect(settings['xpack.management.pipeline.id']).to eq("${XPACK_MANAGEMENT_PIPELINE_ID}")
+        expect(settings['xpack.management.elasticsearch.hosts']).to eq("${XPACK_MANAGEMENT_ELASTICSEARCH_HOSTS}")
+
+        # get container logs
+        container_logs = container.logs(stdout: true)
+
+        # check if logs contain node3 & node4 values actually resolved and used
+        expect(container_logs.include?('pipeline_id=>["*"]')).to be true
+        # note that, we are not spinning up ES nodes, so values can be in errors or in pool update logs
+        expect(container_logs.include?('http://node3:9200')).to be true
+        expect(container_logs.include?('http://node4:9200')).to be true
+      end
     end
   end
 end