Deprecate config.reload.* as pipeline-level settings (#18312)

* Deprecate config.reload.* as pipeline-level settings

* Update specs

* Change name of constant

* Don't remove the config.reload.* settings from #to_hash

* Remove config.reload.* settings from _node/pipelines endpoint response

* Remove config.reload.* settings from pipeline metrics

* Remove config.reload.* settings from modules/node spec

* Put rspec lets where it makes more sense

* Revert "Put rspec lets where it makes more sense"

This reverts commit ffd67a3.

* Update deprecation message to include pipeline id

* Update logstash-core/lib/logstash/settings.rb

Co-authored-by: kaisecheng <[email protected]>

* Update logstash-core/spec/logstash/settings_spec.rb

Co-authored-by: kaisecheng <[email protected]>

---------

Co-authored-by: kaisecheng <[email protected]>

Author: estolfo

docs/static/spec/openapi/logstash-api.yaml

@@ -199,15 +199,11 @@ paths:
                     workers: 1
                     batch_size: 125
                     batch_delay: 50
-                    config_reload_automatic: false
-                    config_reload_interval: 3
                     dead_letter_queue_enabled: false
                   ingestion-pipeline:
                     workers: 8
                     batch_size: 125
                     batch_delay: 5
-                    config_reload_automatic: false
-                    config_reload_interval: 3
                     dead_letter_queue_enabled: false
       x-metaTags:
         - content: Logstash
@@ -249,8 +245,6 @@ paths:
                     workers: 1
                     batch_size: 125
                     batch_delay: 50
-                    config_reload_automatic: false
-                    config_reload_interval: 3
                     dead_letter_queue_enabled: false
       x-metaTags:
         - content: Logstash
@@ -1790,10 +1784,6 @@ components:
           type: integer
         batch_delay:
           type: integer
-        config_reload_automatic:
-          type: boolean
-        config_reload_interval:
-          type: integer
         dead_letter_queue_enabled:
           type: boolean
 

logstash-core/lib/logstash/api/commands/node.rb

@@ -51,8 +51,6 @@ def pipeline(pipeline_id, options = {})
             :workers,
             :batch_size,
             :batch_delay,
-            :config_reload_automatic,
-            :config_reload_interval,
             :dead_letter_queue_enabled,
             :dead_letter_queue_path,
           ).reject {|_, v| v.nil?}

logstash-core/lib/logstash/java_pipeline.rb

@@ -275,8 +275,6 @@ def start_workers
       config_metric.gauge(:workers, pipeline_workers)
       config_metric.gauge(:batch_size, batch_size)
       config_metric.gauge(:batch_delay, batch_delay)
-      config_metric.gauge(:config_reload_automatic, settings.get("config.reload.automatic"))
-      config_metric.gauge(:config_reload_interval, settings.get("config.reload.interval").to_nanos)
       config_metric.gauge(:dead_letter_queue_enabled, dlq_enabled?)
       config_metric.gauge(:dead_letter_queue_path, dlq_writer.get_path.to_absolute_path.to_s) if dlq_enabled?
       config_metric.gauge(:ephemeral_id, ephemeral_id)

logstash-core/lib/logstash/settings.rb

@@ -44,8 +44,6 @@ def self.included(base)
     PIPELINE_SETTINGS_WHITE_LIST = [
       "config.debug",
       "config.support_escapes",
-      "config.reload.automatic",
-      "config.reload.interval",
       "config.string",
       "dead_letter_queue.enable",
       "dead_letter_queue.flush_interval",
@@ -78,6 +76,12 @@ def self.included(base)
       "queue.type",
     ]
 
+    # These are deprecated as pipeline override settings, they still exist as process-level settings
+    DEPRECATED_PIPELINE_OVERRIDE_SETTINGS = [
+      "config.reload.automatic",
+      "config.reload.interval",
+    ]
+
     def initialize
       @settings = {}
       # Theses settings were loaded from the yaml file
@@ -154,12 +158,10 @@ def set_value(setting_name, value, graceful = false)
     alias_method :set, :set_value
 
     def to_hash
-      hash = {}
-      @settings.each do |name, setting|
+      @settings.each_with_object({}) do |(name, setting), hash|
         next if (setting.kind_of? Setting::DeprecatedAlias) || (setting.kind_of? Java::org.logstash.settings.DeprecatedAlias)
         hash[name] = setting.value
       end
-      hash
     end
 
     def merge(hash, graceful = false)
@@ -169,7 +171,12 @@ def merge(hash, graceful = false)
 
     def merge_pipeline_settings(hash, graceful = false)
       hash.each do |key, _|
-        unless PIPELINE_SETTINGS_WHITE_LIST.include?(key)
+        if DEPRECATED_PIPELINE_OVERRIDE_SETTINGS.include?(key)
+          deprecation_logger.deprecated("Config option \"#{key}\", set for pipeline \"#{hash['pipeline.id']}\", is " +
+                                          "deprecated as a pipeline override setting. Please only set it at " +
+                                          "the process level.")
+          hash.delete(key)
+        elsif !PIPELINE_SETTINGS_WHITE_LIST.include?(key)
           raise ArgumentError.new("Only pipeline related settings are expected. Received \"#{key}\". Allowed settings: #{PIPELINE_SETTINGS_WHITE_LIST}")
         end
       end

logstash-core/spec/logstash/api/commands/node_spec.rb

@@ -96,8 +96,6 @@
         :workers,
         :batch_size,
         :batch_delay,
-        :config_reload_automatic,
-        :config_reload_interval,
         :dead_letter_queue_enabled,
         # :dead_letter_queue_path is nil in tests
         # so it is ignored
@@ -117,8 +115,6 @@
         :workers,
         :batch_size,
         :batch_delay,
-        :config_reload_automatic,
-        :config_reload_interval,
         :dead_letter_queue_enabled,
         # Be sure we display a graph when we set the option to
         :graph

logstash-core/spec/logstash/api/modules/node_spec.rb

@@ -139,8 +139,6 @@
             "workers" => Numeric,
             "batch_size" => Numeric,
             "batch_delay" => Numeric,
-            "config_reload_automatic" => Boolean,
-            "config_reload_interval" => Numeric,
             "dead_letter_queue_enabled" => Boolean
           }
         },

logstash-core/spec/logstash/settings_spec.rb

@@ -381,4 +381,47 @@
                                                     ])
     end
   end
+
+  describe "deprecated pipeline override settings" do
+
+    let(:subject) { described_class.new }
+    before(:each) { subject.register(LogStash::Setting.new("pipeline.id", String, "main")) }
+
+    context '#merge_pipeline_settings' do
+
+      described_class::DEPRECATED_PIPELINE_OVERRIDE_SETTINGS.each do |setting|
+
+        context "the setting (#{setting}) is set" do
+
+          let(:deprecation_logger) { subject.deprecation_logger }
+          let(:setting_value) { double('setting_value') }
+
+          it "a warning is logged" do
+            expect(deprecation_logger).to receive(:deprecated).with(
+              a_string_matching("Config option \"#{setting}\", set for pipeline \"test\", is deprecated as a " +
+                                  "pipeline override setting. Please only set it at the process level.")
+            )
+            subject.merge_pipeline_settings("pipeline.id" => "test", setting => setting_value)
+          end
+
+          it "it does not set (#{setting})" do
+            subject.merge_pipeline_settings(setting => double('setting_value'))
+            expect{ subject.get_setting(setting) }.to raise_error(ArgumentError)
+          end
+
+          context 'other settings are also set' do
+
+            before(:each) do
+              subject.register(LogStash::Setting::BooleanSetting.new("config.debug", false))
+              subject.merge_pipeline_settings(setting => setting_value, "config.debug" => true)
+            end
+
+            it "it leaves other settings intact" do
+              expect(subject.get_setting("config.debug").value).to be(true)
+            end
+          end
+        end
+      end
+    end
+  end
 end