Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Apr 11, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7dde084
Wipe out comment lines if config comment contains .
mashhurs Apr 1, 2024
b0948a7
Remove substitution var process when loading the YAML, instead align …
mashhurs Apr 3, 2024
03381c2
Update logstash-core/src/main/java/org/logstash/config/ir/PipelineCon…
mashhurs Apr 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion logstash-core/lib/logstash/config/source/multi_local.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ def initialize(settings)
end

def pipeline_configs
pipelines = deep_replace(retrieve_yaml_pipelines())
pipelines = retrieve_yaml_pipelines
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pipelines_settings = pipelines.map do |pipeline_settings|
clone = @original_settings.clone
clone.merge_pipeline_settings(pipeline_settings)
Expand Down
1 change: 0 additions & 1 deletion logstash-core/lib/logstash/util/substitution_variables.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ def deep_replace(value)
end
else
if value.is_a?(Array)
value_array_index = 0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review note: this isn't used anywhere.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:+1 it's a leftover from #13603

value.each_with_index do |single_value, i|
value[i] = deep_replace(single_value)
end
Expand Down
20 changes: 16 additions & 4 deletions logstash-core/spec/logstash/config/source/multi_local_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,21 +145,33 @@
end

describe "#pipeline_configs" do

let(:config_string) {
"input {
udp {
port => 5555 # intentional comment contains \"${UDP_DEV_PORT}\" variable, shouldn't break functionalities
host => \"127.0.0.1\"
}
# another intentional comment contains \"${UDP_PROD_HOST}\" variable, shouldn't break functionalities
}
output {}"
}
let(:retrieved_pipelines) do
[
{ "pipeline.id" => "main", "config.string" => "input {} output {}" },
{ "pipeline.id" => "backup", "config.string" => "input {} output {}" }
{ "pipeline.id" => "main", "config.string" => config_string },
{ "pipeline.id" => "backup", "config.string" => config_string }
]
end

before(:each) do
allow(subject).to receive(:retrieve_yaml_pipelines).and_return(retrieved_pipelines)
end

it "should return instances of PipelineConfig" do
configs = subject.pipeline_configs
expect(configs).to be_a(Array)
expect(subject.pipeline_configs.first).to be_a(::LogStash::Config::PipelineConfig)
expect(subject.pipeline_configs.last).to be_a(::LogStash::Config::PipelineConfig)
expect(configs.first).to be_a(::LogStash::Config::PipelineConfig)
expect(configs.last).to be_a(::LogStash::Config::PipelineConfig)
end

context "using non pipeline related settings" do
Expand Down
1 change: 0 additions & 1 deletion logstash-core/src/main/java/org/logstash/config/ir/PipelineConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,6 @@ public void displayDebugInformation() {
logger.debug("\n\n{}", configPart.getText());
}
logger.debug("Merged config");
mashhurs marked this conversation as resolved.
Show resolved Hide resolved
logger.debug("\n\n{}", this.configString());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 👍

}

public SourceWithMetadata lookupSource(int globalLineNumber, int sourceColumn)
Expand Down