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.

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

[regression] Syntax error with "if" on multiple lines #2850

Closed
ochedru opened this issue Mar 18, 2015 · 10 comments
Closed

[regression] Syntax error with "if" on multiple lines #2850

ochedru opened this issue Mar 18, 2015 · 10 comments

Comments

@ochedru
Copy link

ochedru commented Mar 18, 2015

Just tried 1.5.0 RC2 and got a syntax error with my configuration file (worked fine in 1.4.2):

SyntaxError: (eval):160: syntax error, unexpected kOR
      or [tag] == "catalina.out"
       ^
        eval at org/jruby/RubyKernel.java:1107
  initialize at /opt/logstash/lib/logstash/pipeline.rb:32
     execute at /opt/logstash/lib/logstash/agent.rb:114
         run at /opt/logstash/lib/logstash/runner.rb:124
        call at org/jruby/RubyProc.java:271
         run at /opt/logstash/lib/logstash/runner.rb:129
        call at org/jruby/RubyProc.java:271
  initialize at /opt/logstash/vendor/bundle/jruby/1.9/gems/stud-0.0.19/lib/stud/task.rb:12

Here is the culprit block from the configuration file filter section:

  if [tag] == "foo"
    or [tag] == "catalina.out"
    or [tag] == "bar" {

Workaround: put all "or" conditions on the same line.

@untergeek
Copy link
Member

Thanks for submitting this. We'll look at the config parser on this.

Here's a shorter potential workaround:

if [tag] in ['foo', 'catalina.out', 'bar'] {

@ggrossetie
Copy link

@untergeek Is this issue will be fixed before the 1.5.0 release ?

@timbunce
Copy link

timbunce commented May 8, 2015

I just hit this with 1.5.0 RC4.

We've lots of conditionals spread over multiple lines interspersed with comment lines explaining the condition expression that follows. Untangling those is a pain and ends up with a block of comments followed by one very very long condition expression that's hard to read.

@jordansissel
Copy link
Contributor

@timbunce for clarity, this is a bug and a regression. :)

@jordansissel jordansissel added this to the v1.5.1 milestone May 19, 2015
@jordansissel jordansissel changed the title Syntax error with "if" on multiple lines [regression] Syntax error with "if" on multiple lines May 19, 2015
@jordansissel
Copy link
Contributor

Testing this:

[14] pry(main)> a = LogStashConfigParser.new; config = a.parse("filter { if [foo]\n or [bar]\n or [baz] { test { } } }");; puts config.compile
...
          def cond_func_1(input_events)
            result = []
            input_events.each do |event|
              events = [event]
              if ((event["[foo]"]) or (event["[bar]"]) or (event["[baz]"])) # if [foo]
 or [bar]
 or [baz]
            events = @filter_test_3.multi_filter(events)


              end
              result += events
            end
            result
          end

@jordansissel
Copy link
Contributor

The bug in the compiled code is this generation:

              if ((event["[foo]"]) or (event["[bar]"]) or (event["[baz]"])) # if [foo]
 or [bar]
 or [baz]

It's including the 'or [bar]' for some reason. I'll take a look at the config AST to figure out what's causing this.

@jordansissel
Copy link
Contributor

Haha, this bug...

[6] pry(#<LogStash::Config::AST::If>)> puts compile
=> "if ((event[\"[test]\"]) or (event[\"[whatever]\"]) or (event[\"[whatever]\"])) # if [test]\n   or [whatever]\n   or [whatever]\n            events = @filter_fancy_1.multi_filter(events)\n  \n"

The bug is that it takes your logstash config and tries to embed it in a comment in the compiled code (for easier debugging), but there are newlines, which make it invalid ruby code - when compiled. This should be an easy fix.

The line of code that causes it was introduced in 4211522 (April 2012), so I'm not yet sure how this only affects 1.5.0 and not 1.4.2.

@jordansissel
Copy link
Contributor

I can confirm that 4211522 isn't included in 1.4.2

jordansissel added a commit to jordansissel/logstash that referenced this issue May 19, 2015
Logstash's config compiler adds a comment to the compiled code, like

    if ..... # if [your] and [conditional]

The idea is to to help aid in reading the compiled logstash config.
However, if a conditional has newlines in it, the `#text_value` of
that conditional will have newlines, and we'll accidentally create
invalid ruby code which will fail with SyntaxError.

Prior to this change, the following Logstash config, under 1.5.0,
would cause a crash on startup:

    if [some]
      or [condition] {
      ...
    }

The cause was that Logstash would compile this to:

    if event("[some]"]) || event("[condition]") # if [some]
    or [condition]
      ...
    end

The 2nd line there `or [condition]` was intended to be on the line
above.

This change strips the line terminators \r and \n, just in case, and
provides a test case to cover.

I verified that this test case _fails_ without the config_ast.rb patch
and _succeeds_ with the patch.

Fixes elastic#2850
@jordansissel
Copy link
Contributor

#3281 has a fix for this.

jordansissel added a commit that referenced this issue May 19, 2015
Logstash's config compiler adds a comment to the compiled code, like

    if ..... # if [your] and [conditional]

The idea is to to help aid in reading the compiled logstash config.
However, if a conditional has newlines in it, the `#text_value` of
that conditional will have newlines, and we'll accidentally create
invalid ruby code which will fail with SyntaxError.

Prior to this change, the following Logstash config, under 1.5.0,
would cause a crash on startup:

    if [some]
      or [condition] {
      ...
    }

The cause was that Logstash would compile this to:

    if event("[some]"]) || event("[condition]") # if [some]
    or [condition]
      ...
    end

The 2nd line there `or [condition]` was intended to be on the line
above.

This change strips the line terminators \r and \n, just in case, and
provides a test case to cover.

I verified that this test case _fails_ without the config_ast.rb patch
and _succeeds_ with the patch.

Fixes #2850

Fixes #3281
@ochedru
Copy link
Author

ochedru commented May 19, 2015

Thank you Jordan!
I should learn Ruby some day to contribute.

On May 19, 2015, at 10:01 PM, Jordan Sissel notifications@github.com wrote:

Closed #2850 via 99741e5.


Reply to this email directly or view it on GitHub.

ph added a commit to ph/logstash that referenced this issue Jun 5, 2015
This is a followup of the issues elastic#2850 and elastic#3281.

The following configuration:

    if [condition] {

    } else if [condition1]
      or [condition2] {
        ..
    }

Was compiled to ruby like this:
    elsif condition or condition2 # else if [condition1]
      or [condition2]

and making the intepreter fails.
ph added a commit to ph/logstash that referenced this issue Jun 5, 2015
This is a followup of the issues elastic#2850 and elastic#3281.

The following configuration:

    if [condition] {

    } else if [condition1]
      or [condition2] {
        ..
    }

Was compiled to ruby like this:
    elsif condition or condition2 # else if [condition1]
      or [condition2]

and making the intepreter fails.
ph added a commit that referenced this issue Jun 5, 2015
This is a followup of the issues #2850 and #3281.

The following configuration:

    if [condition] {

    } else if [condition1]
      or [condition2] {
        ..
    }

Was compiled to ruby like this:
    elsif condition or condition2 # else if [condition1]
      or [condition2]

and making the intepreter fails.

Fixes #3386
ph added a commit that referenced this issue Jun 5, 2015
This is a followup of the issues #2850 and #3281.

The following configuration:

    if [condition] {

    } else if [condition1]
      or [condition2] {
        ..
    }

Was compiled to ruby like this:
    elsif condition or condition2 # else if [condition1]
      or [condition2]

and making the intepreter fails.

Fixes #3386
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants