LIR: merge hash attributes of same name to support legacy configurations

[yaauie]

A build failure of the grok plugin when run through LIR/lscl indicates that
there is an expectation for multiple Attributes of the same name to be merged
together.

This commit ports the failing spec in the plugin to an abstraction that can be
tested within logstash-core, and adds a caveat to the LSCL LIR-builder to
ensure that we merge the hashes in a way that is compatible with legacy
behaviour.

NOTE: when multiple Attributes of the same name are used in a single config,
it's possible to create configurations that circumvent AST::Hash's ability to
report duplicate keys.

Discovered while working on logstash-plugins/logstash-filter-grok#124

[original-brownbear]

@andrewvc @jordansissel can you guys take a look at this?

I don't know if we actually want to support this kind of settings merge?

I'm not sure why the specs in Grok failed against LS master, currently, they are passing just fine for me locally.

[jordansissel]

I don't know if we actually want to support this kind of settings merge?

Historically (years), Logstash has allowed these two things to be equivalent configurations:

# Example A
foo => "bar"
foo => "baz"

# Example B
foo => [ "bar", "baz" ]

The above (array merging) has been supported (on purpose) since at least Logstash 1.0.0 (6+ years)

And similar for hashes. Hash syntax was supported long before, but hash rocket { x => y } was added in Logstash 1.2.0

# Example A
foo => { "bar" => "hello" }
foo => { "baz" => "goodbye" }

# Example B
foo => {
  "bar" => "hello"
  "baz" => "goodbye"
}

Hash merging happens outside the grammar and in the config ast/compiler. My memory is old, but I think this is the code that does it in Logstash 1.2.0

[yaauie]

Historically (years), Logstash has allowed these two things to be equivalent configurations:

# Example A
foo => "bar"
foo => "baz"

# Example B
foo => [ "bar", "baz" ]

The current LSCL/LIR doesn't implement this either; it falls back to +, which would concatenate the two strings:

foo => "barbaz"

[original-brownbear]

@jordansissel alright thanks a lot, with that information I'm good to review this one :)

[original-brownbear]

now the issue does reproduce for me on Travis: https://travis-ci.org/logstash-plugins/logstash-filter-grok/jobs/298174280#L1019 but not locally ... will investigate this a little

[original-brownbear]

@yaauie patch LGTM :) but can we improve this comment a little? Reading this in isolation I wouldn't understand what's going on here tbh.
Maybe we can just spell it out explicitly instead of that long explanation and say:

# For legacy reasons, a config can contain multiple `AST::Attribute`s with the same name 
# and a hash type value (e.g., "match" in the grok filter)
# which are merged into a single hash value; 
# E.g. `{"match" => {"baz" => "bar"}, "match" => {"foo" => "bulb"}}` is interpreted as
#  `{"match" => {"baz" => "bar", "foo" => "blub"}}`.   

IMO that makes it a lot clearer, wdyt?

[original-brownbear]

@yaauie @jordansissel

quote @jordansissel from Slack on this:

I am open to simplifying the grammar if we think it'll make it easier for users

I think this is more a case of making things easier for us than for users. As far as I can see this is broken in all versions that have the LIR Pipeline logic in them (all 5.6.x right?) and we didn't get a single complaint about this being broken. Also, at least to me personally merging two hashes like that feels very unnatural.

---

Unless BwC is a concert here, I'd probably just stop supporting this (especially since it's not even documented, making it super unlikely many people use it).

[andrewvc]

This is tricky, 6.0.0 GA is right upon us.

We have two logical paths in front of us:

1. Simplify hash handling and stop merging duplicate keys
2. Maintain old behavior

Some thoughts:

• Ideally, for 1, we would have made that decision earlier in the 6.x timeline.
• At this juncture we are post RC2. Only critical bugfixes should be going in.
• As @original-brownbear points out no one has complained yet! But then again, betas don't see a huge amount of usage

I'm leaning toward preserving this behavior. Since the change was unintentional I have a hard time arguing that this is not a bug.

I'm +1 on simplifying this behavior in the 7.x timeframe, and committing that to master.

@jordansissel WDYT?

[jordansissel]

@andrewvc +1 simplifying if we can for 7.0; gonna be an interesting path towards a deprecation-and-upgrade scenario for users (telling users this feature is changing, helping them migrate, etc).

I talked with @yaauie today, and he has enough information to move forward. We can add more tests as desired for the supported-but-not-well-documented merging behavior to ensure we keep it until we intend to remove it.

[yaauie]

@original-brownbear I've re-worded the comment; thanks for the critique.

@andrewvc: thoughts on merge targets for this? I know 6.0 is pretty much frozen due to the pending whole-stack release, but would the risk be worth including that in the targets so that we don't introduce this bug to a released version?

[andrewvc]

This is a bug so it is valid to be merged into 6.0

…

On Nov 7, 2017 3:44 PM, "Ry Biesemeyer" ***@***.***> wrote: @original-brownbear <https://github.com/original-brownbear> I've re-worded the comment; thanks for the critique. @andrewvc <https://github.com/andrewvc>: thoughts on merge targets for this? I know 6.0 is pretty much frozen due to the pending whole-stack release, but would the risk be worth including that in the targets so that we don't introduce this bug to a released version? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8597 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIBY0f838cl_g2rfD7VIRznp_DW_jJXks5s0M8pgaJpZM4QTwX8> .

[original-brownbear]

@yaauie thanks, LGTM :)

[elasticsearch-bot]

Ry Biesemeyer merged this into the following branches!

Branch	Commits
master	148ffd3

[elasticsearch-bot]

Ry Biesemeyer merged this into the following branches!

Branch	Commits
6.x	db50857

[andrewvc]

Did we want to merge this into master? IIRC we discussed removing the extra functionality here and just making the hash work like a plain hash no? In this case I assume we'd raise an error for keys declared twice.

[original-brownbear]

@andrewvc actually, it's not so bad keeping the GIT history clean that we have this in master and then adjust it there :)

Added #8605 for this.

[jakelandis]

@yaauie to make the 6.0 release, you will need to merge to the 6.0 branch. (6.x is currently targeting 6.1)

[yaauie]

@jakelandis this was separately backported to 6.0 in #8602