Fix the exception behavior when `config.string` contains ${VAR} in the comments. #16050

mashhurs · 2024-04-01T20:16:15Z

Release notes

[rn:skip]

What does this PR do?

We were applying var substitution once we load from YAML and it affects when we use config.string. Other options like path.config since points to path, the configs will be loaded later and LSCL rules will be processed when compiling the pipeline. This is why we are experiencing the issue with config.string only.

This PR standardizes (same sequence will be with config.string as well) the way we are loading configs and compiling.

Why is it important/What is the impact to the user?

Probability might be low but it is still not an expectation to resolve the ${VAR} if commented out. So, users may have a better experience with this change.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
~~[ ] I have made corresponding changes to the documentation~~
~~[ ] I have made corresponding change to the default configuration files (and/or docker env variables)~~
I have added tests that prove my fix is effective or that my feature works

Author's Checklist

[ ]

How to test this PR locally

pull the change

use following config in the pipelines.yml

 - pipeline.id: test
config.string: |
  input { generator {} } #${I_AM_NOT_HERE}
  filter { sleep { time => 100 } }
  output {
  #${I_AM_NOT_HERE}
    stdout {
      codec => rubydebug
    }
    # ${I_AM_NOT_HERE}
  }

Run Logstash with bin/logstash --debug --config.debug to see the what config was applied.

Logstash runs without crash. Before the change, Logstash was failing Cannot evaluate ${I_AM_NOT_HERE}... error

and config will be applied after filtering out ${I_AM_NOT_HERE} with the comments

 ----------------------P[input-generator{"count"=>5}|[string]config_string:1:9:```
 generator { count => 5 }
 ```] -> __QUEUE__
 __QUEUE__ -> filter_to_output
 filter_to_output -> P[output-stdout{"codec"=>#<Java::OrgLogstashConfigIrImperative::PluginStatement:0x3344b612>}|     
 [string]config_string:3:1:```
 stdout {
 codec => rubydebug 
 }
 ```]

Related issues

Closes Commented out variable substitutions may still be evaluated in pipelines.yml #16008

Use cases

Screenshots

Logs

mashhurs · 2024-04-01T20:18:36Z

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

@@ -36,7 +38,6 @@ def deep_replace(value)
      end
    else
      if value.is_a?(Array)
-        value_array_index = 0


review note: this isn't used anywhere.

:+1 it's a leftover from #13603

logstash-core/spec/logstash/util/substitution_variables_spec.rb

yaauie

While this solves the specific use-case of config.string having a syntax that hides comments between a # char and a newline (or EOF), it effectively does so by infecting the substitution variables handler with knowledge about the syntax of one of the things it parses.

We may be better served to run the substitution variables on a key-by-key basis for settings, allowing settings like config.string to opt out of preemptive replacements because we know that they will be replaced later after the value has been syntactically parsed. This will ensure that the replacements run on config.string happen in a single pass, and not piecemeal in two passes.

…on the generic approach which LSCL happens during the pipeline compile.

mashhurs · 2024-04-03T06:16:53Z

logstash-core/src/main/java/org/logstash/config/ir/PipelineConfig.java

@@ -172,7 +172,6 @@ public void displayDebugInformation() {
            logger.debug("\n\n{}", configPart.getText());
        }
        logger.debug("Merged config");
-        logger.debug("\n\n{}", this.configString());


review note: this line makes twice same config print (when --debug and --config.debug enabled) which makes us confusing..., removing.

It's duplicated only when the config in composed by just one SourceWithMetadata, but when the config is loaded from multiple files, like when glob operator is used, it logs, every single section, and then also the effective pipeline definition as it's passed down to the compiler.
So I'm little bit doubtful it remove it, especially in cases of strange compilation errors.

Example single file:

[2024-04-10T12:22:10,615][DEBUG][org.logstash.config.ir.PipelineConfig] -------- Logstash Config --------- [2024-04-10T12:22:10,615][DEBUG][org.logstash.config.ir.PipelineConfig] Config from source, source: LogStash::Config::Source::MultiLocal, pipeline_id:: test [2024-04-10T12:22:10,615][DEBUG][org.logstash.config.ir.PipelineConfig] Config string, protocol: string, id: config_string [2024-04-10T12:22:10,615][DEBUG][org.logstash.config.ir.PipelineConfig] input { generator {} } #${I_AM_NOT_HERE} filter { sleep { time => 100 } } output { #${I_AM_NOT_HERE} stdout { codec => rubydebug } # ${I_AM_NOT_HERE} } [2024-04-10T12:22:10,615][DEBUG][org.logstash.config.ir.PipelineConfig] Merged config [2024-04-10T12:22:10,615][DEBUG][org.logstash.config.ir.PipelineConfig] input { generator {} } #${I_AM_NOT_HERE} filter { sleep { time => 100 } } output { #${I_AM_NOT_HERE} stdout { codec => rubydebug } # ${I_AM_NOT_HERE} } [2024-04-10T12:22:10,616][DEBUG][logstash.agent ] Converging pipelines state {:actions_count=>1}

Example multiple files:

[2024-04-10T12:29:45,178][DEBUG][org.logstash.config.ir.PipelineConfig] -------- Logstash Config --------- [2024-04-10T12:29:45,178][DEBUG][org.logstash.config.ir.PipelineConfig] Config from source, source: LogStash::Config::Source::MultiLocal, pipeline_id:: test [2024-04-10T12:29:45,178][DEBUG][org.logstash.config.ir.PipelineConfig] Config string, protocol: file, id: /tmp/logstash_1.config [2024-04-10T12:29:45,179][DEBUG][org.logstash.config.ir.PipelineConfig] input { generator {} } #${I_AM_NOT_HERE} filter { sleep { time => 100 } } [2024-04-10T12:29:45,179][DEBUG][org.logstash.config.ir.PipelineConfig] Config string, protocol: file, id: /tmp/logstash_2.config [2024-04-10T12:29:45,179][DEBUG][org.logstash.config.ir.PipelineConfig] output { #${I_AM_NOT_HERE} stdout { codec => rubydebug } # ${I_AM_NOT_HERE} } [2024-04-10T12:29:45,179][DEBUG][org.logstash.config.ir.PipelineConfig] Merged config [2024-04-10T12:29:45,179][DEBUG][org.logstash.config.ir.PipelineConfig] input { generator {} } #${I_AM_NOT_HERE} filter { sleep { time => 100 } } output { #${I_AM_NOT_HERE} stdout { codec => rubydebug } # ${I_AM_NOT_HERE} } [2024-04-10T12:29:45,179][DEBUG][logstash.agent ] Converging pipelines state {:actions_count=>1}

Thanks @andsel for checking this. It is not related to the PR/feature but printing config multiple times is really confuses user. I have placed back and will check if we can add a condition to properly print considering all config cases.

Great, I agree with you that replicating information could be misleading, but please file an issue to improve logging, maybe segments could logged at trace level while the full pipeline definition at debug, or something similar.
Having a separate issue permit to discuss the problem separately. 👍

mashhurs · 2024-04-03T06:29:44Z

logstash-core/lib/logstash/config/source/multi_local.rb

@@ -30,7 +30,7 @@ def initialize(settings)
    end

    def pipeline_configs
-      pipelines = deep_replace(retrieve_yaml_pipelines())
+      pipelines = retrieve_yaml_pipelines


okay, things are clear now.
We do not really need var substitution when loading the YAML. Once we have pipeline definitions from YAML, we properly load the configs based on the given options (config.string, config.path). Then, imperative pipeline compile operation executes the LSCL which wipes out the comments based on the rule

mashhurs · 2024-04-03T06:34:58Z

While this solves the specific use-case of config.string having a syntax that hides comments between a # char and a newline (or EOF), it effectively does so by infecting the substitution variables handler with knowledge about the syntax of one of the things it parses.

We may be better served to run the substitution variables on a key-by-key basis for settings, allowing settings like config.string to opt out of preemptive replacements because we know that they will be replaced later after the value has been syntactically parsed. This will ensure that the replacements run on config.string happen in a single pass, and not piecemeal in two passes.

Ha.. what a fun! Your point totally makes sense (which I didn't know), thank you.
Now, things are clear that we do not necessarily need to apply var substitution when loading the YAML. Later on, we properly load the configs based on the given options (config.string, config.path). Then, imperative pipeline compile operation executes the LSCL which wipes out the comments based on the rule. Let me please know if I am still missing something.

andsel

Globally seems good, but left a doubt on the effectiveness of removing the debug line that prints the full pipeline string passed down to the compiler.

andsel · 2024-04-10T10:03:26Z

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

@@ -36,7 +38,6 @@ def deep_replace(value)
      end
    else
      if value.is_a?(Array)
-        value_array_index = 0


:+1 it's a leftover from #13603

andsel · 2024-04-10T10:20:41Z

logstash-core/src/main/java/org/logstash/config/ir/PipelineConfig.java

@@ -172,7 +172,6 @@ public void displayDebugInformation() {
            logger.debug("\n\n{}", configPart.getText());
        }
        logger.debug("Merged config");
-        logger.debug("\n\n{}", this.configString());


It's duplicated only when the config in composed by just one SourceWithMetadata, but when the config is loaded from multiple files, like when glob operator is used, it logs, every single section, and then also the effective pipeline definition as it's passed down to the compiler.
So I'm little bit doubtful it remove it, especially in cases of strange compilation errors.

Example single file:

[2024-04-10T12:22:10,615][DEBUG][org.logstash.config.ir.PipelineConfig] -------- Logstash Config --------- [2024-04-10T12:22:10,615][DEBUG][org.logstash.config.ir.PipelineConfig] Config from source, source: LogStash::Config::Source::MultiLocal, pipeline_id:: test [2024-04-10T12:22:10,615][DEBUG][org.logstash.config.ir.PipelineConfig] Config string, protocol: string, id: config_string [2024-04-10T12:22:10,615][DEBUG][org.logstash.config.ir.PipelineConfig] input { generator {} } #${I_AM_NOT_HERE} filter { sleep { time => 100 } } output { #${I_AM_NOT_HERE} stdout { codec => rubydebug } # ${I_AM_NOT_HERE} } [2024-04-10T12:22:10,615][DEBUG][org.logstash.config.ir.PipelineConfig] Merged config [2024-04-10T12:22:10,615][DEBUG][org.logstash.config.ir.PipelineConfig] input { generator {} } #${I_AM_NOT_HERE} filter { sleep { time => 100 } } output { #${I_AM_NOT_HERE} stdout { codec => rubydebug } # ${I_AM_NOT_HERE} } [2024-04-10T12:22:10,616][DEBUG][logstash.agent ] Converging pipelines state {:actions_count=>1}

Example multiple files:

[2024-04-10T12:29:45,178][DEBUG][org.logstash.config.ir.PipelineConfig] -------- Logstash Config --------- [2024-04-10T12:29:45,178][DEBUG][org.logstash.config.ir.PipelineConfig] Config from source, source: LogStash::Config::Source::MultiLocal, pipeline_id:: test [2024-04-10T12:29:45,178][DEBUG][org.logstash.config.ir.PipelineConfig] Config string, protocol: file, id: /tmp/logstash_1.config [2024-04-10T12:29:45,179][DEBUG][org.logstash.config.ir.PipelineConfig] input { generator {} } #${I_AM_NOT_HERE} filter { sleep { time => 100 } } [2024-04-10T12:29:45,179][DEBUG][org.logstash.config.ir.PipelineConfig] Config string, protocol: file, id: /tmp/logstash_2.config [2024-04-10T12:29:45,179][DEBUG][org.logstash.config.ir.PipelineConfig] output { #${I_AM_NOT_HERE} stdout { codec => rubydebug } # ${I_AM_NOT_HERE} } [2024-04-10T12:29:45,179][DEBUG][org.logstash.config.ir.PipelineConfig] Merged config [2024-04-10T12:29:45,179][DEBUG][org.logstash.config.ir.PipelineConfig] input { generator {} } #${I_AM_NOT_HERE} filter { sleep { time => 100 } } output { #${I_AM_NOT_HERE} stdout { codec => rubydebug } # ${I_AM_NOT_HERE} } [2024-04-10T12:29:45,179][DEBUG][logstash.agent ] Converging pipelines state {:actions_count=>1}

logstash-core/src/main/java/org/logstash/config/ir/PipelineConfig.java

…fig.java Put the logging config back as it is being used with composed configs.

elastic-sonarqube · 2024-04-10T17:16:29Z

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

elasticmachine · 2024-04-10T17:18:14Z

💚 Build Succeeded

Buildkite Build
Commit: 03381c2

History

💚 Build #991 succeeded b0948a7
💚 Build #986 succeeded 4678c4b

cc @mashhurs

andsel

LGTM

…es () inside config.string in the pipelines.yml file, but that created a regression, becuase env vars is expected to be replaced in all the other yaml keys

The PR was created to skip resolving environment variable references in comments present in the “config.string” pipelines defined in the pipelines.yml file. However it introduced a bug that no longer resolves env var references in values of settings like pipeline.batch.size or queue.max_bytes. For now we’ll revert this PR and create a fix that handles both problems.

The PR was created to skip resolving environment variable references in comments present in the “config.string” pipelines defined in the pipelines.yml file. However it introduced a bug that no longer resolves env var references in values of settings like pipeline.batch.size or queue.max_bytes. For now we’ll revert this PR and create a fix that handles both problems. (cherry picked from commit efa8378)

The PR was created to skip resolving environment variable references in comments present in the “config.string” pipelines defined in the pipelines.yml file. However it introduced a bug that no longer resolves env var references in values of settings like pipeline.batch.size or queue.max_bytes. For now we’ll revert this PR and create a fix that handles both problems. (cherry picked from commit efa8378) Co-authored-by: Andrea Selva <[email protected]>

Wipe out comment lines if config comment contains .

7dde084

mashhurs added the bug label Apr 1, 2024

mashhurs self-assigned this Apr 1, 2024

mashhurs marked this pull request as ready for review April 1, 2024 20:19

mashhurs commented Apr 1, 2024

View reviewed changes

roaksoax added the status:needs-review label Apr 1, 2024

yaauie reviewed Apr 2, 2024

View reviewed changes

Remove substitution var process when loading the YAML, instead align …

b0948a7

…on the generic approach which LSCL happens during the pipeline compile.

mashhurs force-pushed the fix-config-with-commented-vars branch from 4678c4b to b0948a7 Compare April 3, 2024 06:15

mashhurs commented Apr 3, 2024

View reviewed changes

mashhurs requested a review from yaauie April 3, 2024 06:43

mashhurs changed the title ~~Wipe out comment lines if config comment contains .~~ Fix the exception behavior when config.string contains ${VAR} in the comments. Apr 3, 2024

andsel requested changes Apr 10, 2024

View reviewed changes

roaksoax added status:changes-requested and removed status:needs-review labels Apr 10, 2024

mashhurs commented Apr 10, 2024

View reviewed changes

logstash-core/src/main/java/org/logstash/config/ir/PipelineConfig.java Outdated Show resolved Hide resolved

Update logstash-core/src/main/java/org/logstash/config/ir/PipelineCon…

03381c2

…fig.java Put the logging config back as it is being used with composed configs.

mashhurs requested a review from andsel April 10, 2024 16:54

andsel approved these changes Apr 11, 2024

View reviewed changes

roaksoax added status:approved and removed status:changes-requested labels Apr 11, 2024

mashhurs merged commit 9483ee0 into elastic:main Apr 11, 2024
6 checks passed

mashhurs deleted the fix-config-with-commented-vars branch April 11, 2024 14:32

roaksoax removed the status:approved label Apr 11, 2024

andsel mentioned this pull request Jun 6, 2024

Revert pr #16050 #16201

Merged

5 tasks

github-actions bot mentioned this pull request Jun 6, 2024

Backport PR #16201 to 8.14: Revert pr #16050 #16203

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the exception behavior when `config.string` contains ${VAR} in the comments. #16050

Fix the exception behavior when `config.string` contains ${VAR} in the comments. #16050

mashhurs commented Apr 1, 2024 •

edited

Loading

mashhurs Apr 1, 2024

andsel Apr 10, 2024

yaauie left a comment •

edited

Loading

mashhurs Apr 3, 2024

andsel Apr 10, 2024

mashhurs Apr 10, 2024

andsel Apr 11, 2024

mashhurs Apr 3, 2024

mashhurs commented Apr 3, 2024 •

edited

Loading

andsel left a comment

andsel Apr 10, 2024

andsel Apr 10, 2024

elastic-sonarqube bot commented Apr 10, 2024

elasticmachine commented Apr 10, 2024

andsel left a comment

Fix the exception behavior when config.string contains ${VAR} in the comments. #16050

Fix the exception behavior when config.string contains ${VAR} in the comments. #16050

Conversation

mashhurs commented Apr 1, 2024 • edited Loading

Release notes

What does this PR do?

Why is it important/What is the impact to the user?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaauie left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mashhurs commented Apr 3, 2024 • edited Loading

andsel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elastic-sonarqube bot commented Apr 10, 2024

Quality Gate passed

elasticmachine commented Apr 10, 2024

💚 Build Succeeded

History

andsel left a comment

Choose a reason for hiding this comment

Fix the exception behavior when `config.string` contains ${VAR} in the comments. #16050

Fix the exception behavior when `config.string` contains ${VAR} in the comments. #16050

mashhurs commented Apr 1, 2024 •

edited

Loading

yaauie left a comment •

edited

Loading

mashhurs commented Apr 3, 2024 •

edited

Loading