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

Jump to bottom

Remove comments of config.string. #16625

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Remove comments of config.string. #16625

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8730069
If config.string contains env in its comments, pipeline may not star…
mashhurs Nov 3, 2024
1615cb7
Consider if config contains comment like symbol ' #' in its real fiel…
mashhurs Nov 16, 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
59 changes: 58 additions & 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 @@ -23,14 +23,18 @@ class MultiLocal < Local
include LogStash::Util::SubstitutionVariables
include LogStash::Util::Loggable

REMOVE_COMMENTS_CONFIG_KEYS = %w(config.string)
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: currently only config.string may contain the comments. YAML::safe_load removes the comments from other keys such as pipeline.id.

CONFIG_LINE_COMMENT = " #"

def initialize(settings)
@original_settings = settings
super(settings)
@match_warning_done = false
end

def pipeline_configs
pipelines = deep_replace(retrieve_yaml_pipelines)
yaml_config_without_comments = remove_comments(retrieve_yaml_pipelines)
pipelines = deep_replace(yaml_config_without_comments)
pipelines_settings = pipelines.map do |pipeline_settings|
clone = @original_settings.clone
clone.merge_pipeline_settings(pipeline_settings)
Expand Down Expand Up @@ -134,5 +138,58 @@ def do_warning?
end
!done
end

ESCAPE_CHAR = "\\"

def remove_comment(config_line = "")
refined_line = config_line
comment_indices = []
comment_idx = -1
while (comment_idx = config_line.index(CONFIG_LINE_COMMENT, comment_idx + 1)) != nil
comment_indices << comment_idx
end

double_quotes_count = 0
single_quotes_count = 0
config_line.chars.each_with_index do |char, i|
if char == '"' && (config_line[i - ESCAPE_CHAR.length] != ESCAPE_CHAR || (config_line[i - ESCAPE_CHAR.length] == ESCAPE_CHAR && config_line[i - 2 * ESCAPE_CHAR.length] == ESCAPE_CHAR)) && double_quotes_count == 0 && single_quotes_count == 0
double_quotes_count += 1 unless comment_indices.include?(i)
elsif char == '"' && (config_line[i - ESCAPE_CHAR.length] != ESCAPE_CHAR || (config_line[i - ESCAPE_CHAR.length] == ESCAPE_CHAR && config_line[i - 2 * ESCAPE_CHAR.length] == ESCAPE_CHAR)) && double_quotes_count == 1 && single_quotes_count == 0
double_quotes_count -= 1 unless comment_indices.include?(i)
end

if char == "'" && (config_line[i - ESCAPE_CHAR.length] != ESCAPE_CHAR || (config_line[i - ESCAPE_CHAR.length] == ESCAPE_CHAR && config_line[i - 2 * ESCAPE_CHAR.length] == ESCAPE_CHAR)) && double_quotes_count == 0 && single_quotes_count == 0
single_quotes_count += 1 unless comment_indices.include?(i)
elsif char == "'" && (config_line[i - ESCAPE_CHAR.length] != ESCAPE_CHAR || (config_line[i - ESCAPE_CHAR.length] == ESCAPE_CHAR && config_line[i - 2 * ESCAPE_CHAR.length] == ESCAPE_CHAR)) && double_quotes_count == 0 && single_quotes_count == 1
single_quotes_count -= 1 unless comment_indices.include?(i)
end

if comment_indices.include?(i) && double_quotes_count == 0 && single_quotes_count == 0
refined_line = config_line[0...i]
break
end
end
refined_line
end

# @param [Object] `yaml_config` the input object
# @return [Object] Updated object where its key etries which match the `CONFIG_KEYS_TO_WIPE_OUT_COMMENTS` do not contain comments
def remove_comments(yaml_config)
case yaml_config
when Hash
yaml_config.each do |key, val|
yaml_config[key.to_s] = remove_comments(val) if REMOVE_COMMENTS_CONFIG_KEYS.include?(key)
end
when Array
yaml_config.map { |val| remove_comments(val) }
when String
yaml_config.lines.map do |yaml_config_line |
refined_config_line = yaml_config_line.strip.start_with?("#") ? "\n" : yaml_config_line
refined_config_line.include?(CONFIG_LINE_COMMENT) ? remove_comment(refined_config_line) : refined_config_line
end.join
else
yaml_config
end
end
end
end end end
53 changes: 41 additions & 12 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,18 +145,39 @@
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(:config_string) { "input {} output {}" }
ENV["ENV_TAG"] = "foo_tag"
ENV["ENV_TAG_ANOTHER"] = "bar_tag"
let(:config_string) {
"input {
beats {
port => 5555 # intentional comment contains \"${BEATS_DEV_PORT}\" variable, shouldn't break functionalities
host => \"127.0.0.1\"
}
# another intentional comment contains \"${BEATS_PROD_HOST}\" variable, shouldn't break functionalities
}
filter {
mutate { add_field => { \"oopsy\" => \"This is real data # not a ' comment, oopsy\" } }
mutate {
add_field => {
'doopsy' => \"This is real data # not a ' comment, doopsy\" # pipeline.workers: \"${PIPELINE_WORKERS}\"
}
add_tag => [ \"doopsy_tag\", \"${ENV_TAG}\" ] # \"${ENV_TAG_ANOTHER}
}
mutate {
add_field => {
\"hoopsy\" =>
'This is real data # not a \" comment, hoopsy'
}
}
mutate { add_field => { \"loopsy\" => \"This is real data # not a ' escape comment, \\\" loopsy\" } }
mutate { add_field => { \"woopsy\" => 'This is real data # not a \" comment, ${ENV_TAG_ANOTHER} whoopsy' } } # \"${ENV_TAG_ANOTHER} } }
}
output {
stdout { } # output
} # pipeline.workers: \"${PIPELINE_WORKERS}\"
# pipeline.workers: \"${PIPELINE_WORKERS}\"
"
}
let(:retrieved_pipelines) do
[
{ "pipeline.id" => "main", "config.string" => config_string },
Expand All @@ -175,6 +196,14 @@
expect(configs.last).to be_a(::LogStash::Config::PipelineConfig)
end

it "removes comments from `config.string`" do
expectation = "input {\n beats {\n port => 5555 host => \"127.0.0.1\"\n }\n\n }\n filter {\n mutate { add_field => { \"oopsy\" => \"This is real data # not a ' comment, oopsy\" } }\n mutate {\n add_field => {\n 'doopsy' => \"This is real data # not a ' comment, doopsy\" }\n add_tag => [ \"doopsy_tag\", \"${ENV_TAG}\" ] }\n mutate {\n add_field => {\n \"hoopsy\" =>\n 'This is real data # not a \" comment, hoopsy'\n }\n }\n mutate { add_field => { \"loopsy\" => \"This is real data # not a ' escape comment, \\\" loopsy\" } }\n mutate { add_field => { \"woopsy\" => 'This is real data # not a \" comment, ${ENV_TAG_ANOTHER} whoopsy' } } }\n output {\n stdout { } }\n "
config_without_comments = subject.send(:remove_comments, retrieved_pipelines)
config_without_comments.each do |config_line|
expect(config_line['config.string']).to match expectation
end
end

context "using non pipeline related settings" do
let(:retrieved_pipelines) do [
{ "pipeline.id" => "main", "config.string" => "", "api.http.port" => 22222 },
Expand Down