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

Allow the usage of ' ' and '/' in field names #20142

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

janheise
Copy link
Contributor

@janheise janheise commented Aug 9, 2024

Description

This PR adds support for and / in field names. This was forbidden in earlier versions due to restrictions in ES/lucene.

TODO: improve this description & changelog

Motivation and Context

The definition of Whitespace: https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/Character.html#isWhitespace(char)

for reference, see https://github.com/Graylog2/graylog-plugin-enterprise/issues/8169

How Has This Been Tested?

The unit tests for Messages have been modified to include whitespace & / in tests.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.

@janheise janheise requested a review from kroepke August 13, 2024 14:45
@janheise janheise requested review from systemboogie and a team November 6, 2024 11:22
@janheise janheise marked this pull request as ready for review November 6, 2024 11:22
@janheise janheise removed the request for review from kroepke November 6, 2024 11:26
@kroepke
Copy link
Member

kroepke commented Nov 6, 2024

Before merging this, we need to ensure that all places that deal with field names properly escape them, especially URL construction and notification templates.

Copy link
Member

@kroepke kroepke left a comment

Choose a reason for hiding this comment

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

Please address the exhaustive testing before merging this.

@janheise janheise force-pushed the fix/allow-whitespace-and-slash-in-field-names branch 2 times, most recently from 6dd636e to c5b88ab Compare November 6, 2024 13:37
@systemboogie
Copy link

systemboogie commented Nov 15, 2024

@janheise I noticed a problem with search query auto-completion. When searching for a field name with a slash (e.g. abc/slash), auto-completion correctly suggests the field, but does not escape the slash in the suggestion...

Screenshot 2024-11-15 at 15 30 06

... which then leads to an error when performing the search:

Screenshot 2024-11-15 at 15 41 26

The same happens when searching for a field name that contains a space character (e.g. abc space)

@systemboogie
Copy link

systemboogie commented Nov 15, 2024

@janheise Another observation related to the _exists_ operator.

Works fine for slash, e.g. _exists_:abc\/slash.

Space in a field name needs to be escaped as well (e.g. _exists_:abc\ space) and the search yields the correct results. However, the UI shows warnings.

Screenshot 2024-11-15 at 16 25 51

@systemboogie
Copy link

systemboogie commented Nov 21, 2024

Some more findings around auto-completion: All query input fields with auto-completion suggest a field name with slash or space correctly, but in the suggested field names, slash and space characters are un-escaped. Hopefully this is a thing that can be fixed in a central place?

  • Search page > Query input field
  • Search page > Search result list (message table) > Add to query
  • Search page > Search filter > Query input field
  • Enterprise > My search filters

Then I found an issue in areas where we pre-populate a query input field. Slash and space in field names are un-escaped there as well. We should escape those chars in the generated query. Some areas where I observed that until now, I guess there are more:

  • Search page > Create an event definition from a value > Leads to the event definition config with query input field populated
  • Alert page > Replay search

While testing alerts, I stumbled across an inconsistency. Consider one key-value pair abc/slash:"with slash" and another one abc/slash/2:"with slash"

  • An event definition that looks for abc/slash:"with slash" does not fire an alert. Pretty expected due to the un-escaped slash
  • An event definition that looks for abc\/slash:"with slash" does fire an alert. Also expected
  • An event definition that looks for abc/slash/2:"with slash does fire an alert as well, despite the un-escaped slashes. And the alert correctly lists only messages with the abc/slash/2 field
  • That made me check the behavior of the search page... and the displayed results are different there. A search query like abc/slash/2:"with slash" does not show a validation error (only a warning), so a user is able to submit the query. However, the search does not only show messages that contain abc/slash/2, but also unexpected ones, e.g. abc/slash and also /. Seems like the parser does not account for more than one slash in a field name or we use different search endpoints in different places or maybe even both applies

When replaying a search from a fired alert, the slash and space characters are un-escaped in the search query input field, but nonetheless the search results in the widgets below are correct

@systemboogie
Copy link

systemboogie commented Nov 26, 2024

In the Events & Alerts section, we saw that a slash in the field name must be escaped in order to make the event/alert work. In the Pipeline Rules section, this is the other way around - i.e. when escaping a slash in the field name input, the rule does not catch the respective message:

  • Consider a message with a field name abc/slash
    • If you use abc/slash in the rule builder (see example with has_field, the rule catches the expected log message
    • If you use abc\/slash in the rule builder, the rule does not catch the expected log message
  • The same applies for a field name abc space
  • Confusingly, for a field name abc/slash/2, the rule never catches the expected log message, regardless of whether you escape the slashes or not
    • abc/slash/2
    • abc\/slash\/2
rule "Add new field if abc/slash is present in message"
when
  has_field(
    field : "abc/slash"
  )
then
  set_field(
    field : "abcNewSlash",
    value : "This field was created by a pipeline rule that checks for the presence of a field named \"abc/slash\"."
  );
end

@@ -518,6 +518,9 @@ inputbuffer_wait_strategy = blocking
# Enable the message journal.
message_journal_enabled = true

# since we allow space and slash as valid characters in field names, the cleanKey function in pipelines needs to have a switch to be backwards compatible for users that depend on it
clean_key_keeps_space_and_slash = true

Choose a reason for hiding this comment

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

Nit: Maybe prefix it with pipeline_ or even pipeline_rules_?

Suggested change
clean_key_keeps_space_and_slash = true
pipeline_clean_key_keeps_space_and_slash = true

@systemboogie
Copy link

systemboogie commented Nov 27, 2024

Event definitions, custom fields: Dynamically reading the value from a field does not work when the original field name contains a space character.

Given you add a custom field to an event and you want it to dynamically assign the value from another (original) field. That works fine if the original field’s name contains a slash character, but not if it contains a space character. None of the below-mentioned JMTE template strings works. The resulting fields object in the event is just empty (which is strange, because I think the usual behavior is to create the custom field with an empty value in case the original field does not exist).

  • ${source.abc space}
  • ${source."abc space"}
  • ${source."abc\ space"}

If the original field contains a slash, it works - regardless of whether the template is ${source.abc/slash} or ${source."abc/slash"}.

@janheise janheise force-pushed the fix/allow-whitespace-and-slash-in-field-names branch from 3ca4b6e to bdb54df Compare November 27, 2024 13:30
@systemboogie
Copy link

systemboogie commented Nov 27, 2024

Another thing related to my previous comment, which is dealing with JMTE strings:

In Notification JMTE templates, it is not possible to print the value of a field name that contains a space character. Field names with a slash work fine.

Given you have a field name abc space. It is not possible to print its value in a notification - doing so results in an error and blocks the notification from being sent.

Tried several variations:

  • ${message.fields.abc space}
  • ${message.fields."abc space"}
  • ${message.fields."abc\ space"}

Error message example:

graylog-1   | 2024-11-27 15:30:03,719 ERROR: org.graylog.integrations.notifications.types.SlackEventNotification - Invalid Custom Message template.[com.floreysoft.jmte.message.ParseException: Error while parsing 'message.fields."abc space"' at location (24:3): Invalid expression!]
graylog-1   | 2024-11-27 15:30:03,719 ERROR: org.graylog.scheduler.JobExecutionEngine - Job execution error - trigger=67473afbbcc7616941437f90 job=674728ecbcc76169414346d1
graylog-1   | org.graylog.scheduler.JobExecutionException: Failed permanently to execute notification, giving up - <674728ecbcc76169414346d0/Slack notification/slack-notification-v1>
graylog-1   | 	at org.graylog.events.notifications.EventNotificationExecutionJob.execute(EventNotificationExecutionJob.java:158) ~[graylog.jar:?]

@janheise
Copy link
Contributor Author

  • Search page > Query input field
  • Search page > Search result list (message table) > Add to query
  • Search page > Search filter > Query input field
  • Enterprise > My search filters

=> have been addressed

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

Successfully merging this pull request may close these issues.

3 participants