Render Mustache expressions in routing rules datasets values #1393

zmoog · 2023-08-14T23:03:25Z

Motivation

Datasets values in the routing_rules.yml file can contain mustache expressions like:

{{kubernetes.labels.elastic_co/dataset}}

However, the current test implementation in elastic-package use these values as static strings, so tests fail on integrations using datasets with mustache expressions like the Kubernetes integration.

Change description

This PR add the rendering mustache expressions in datasets values using the github.com/cbroglie/mustache library.

Additional Notes

Closes #1388
Unblocks elastic/integrations#7118

Reviewer checklist

PR address a single concern.
PR title and description are appropriately filled.
Changes will be merged in main.
Changes are covered by tests.
Logging is meaningful in case of troubleshooting.
~~Docs are updated (at least the README.md, if needed).~~
History is clean, commit messages are meaningful (see CONTRIBUTING.md) and are well-formatted.

This is a quick PoC to test using the github.com/hoisie/mustache library to render Mustache expressions in datasets values. Datasets values containing mustache expressions like: {{kubernetes.labels.elastic_co/dataset}} are rendered using document values. Currently, the principal limit is the validator is using processed documents (docs returned by the ingest pipeline), but for this purpose of using the original documents instead (docs read from the test case file).

internal/fields/validate.go

I just noticed the original github.com/hoisie/mustache seems unmaintained (last commit happened on Aug 5, 2016). So I switched to github.com/cbroglie/mustache, a fork of the same repo that looks active. They added essentials improvements, like returning an error on the `Render()` API.

I didn't run `elastic-package check` in the kubernetes test package. Without a proper format, the test detects a dirty git working directory at the end of the test run.

kaiyan-sheng · 2023-08-22T16:43:59Z

@zmoog Thanks for making the fix 😬 @jsoriano Do you mind having a separate test package kubernetes here or you prefer we add this test case into the existing aws test package?

zmoog · 2023-08-22T17:18:24Z

Thanks for making the fix 😬 @jsoriano Do you mind having a separate test package kubernetes here or you prefer we add this test case into the existing aws test package?

I was also wondering about adding a Kubernetes test vs. using the existing Firehose one.

I added a Kubernetes one mainly because I had clear real-world examples for this scenario and less on Firehose. But I am of course willing to consolidate in one if you think it's a better option.

jsoriano · 2023-08-23T08:42:07Z

There is already a kubernetes test package, under test/packages/with-kind.

I am fine with adding this case to the firehose or the existing kubernetes package, but please don't add another kubernetes test package.

zmoog · 2023-08-23T09:11:08Z

There is already a kubernetes test package, under test/packages/with-kind.

Oh, thanks! I missed this one, my bad.

I am fine with adding this case to the firehose or the existing kubernetes package, but please don't add another kubernetes test package.

Got it! I'm moving my test case into one of the existing packages.

We already have one at: test/packages/with-kind/kubernetes So this one is redundant. Dropping it!

To test that routing rules work with datasets and namespaces, we add another rule that uses the `{{cloud.account.id}}` for both if the `cloud.region` is `eu-west-1`.

zmoog · 2023-08-23T13:16:49Z

@jsoriano, I added the simple case possible to the firehose integration.

kaiyan-sheng

LGTM! The test case for Cloudtrail is not real but enough for testing.

zmoog · 2023-08-23T21:49:06Z

LGTM! The test case for Cloudtrail is not real but enough for testing.

Yeah, it's functional for testing, but it could carry better semantics. Let me see if I can find a better test case.

Changing the expression test to make it more realistic. The main challenge here is to find a source event field value I can use for a dataset or namespace: it can't contain `-`.

zmoog · 2023-08-23T22:42:03Z

Pushed a more realistic test case, but still not great.

The main problem is the firehose event does not contain a lot of field values I can use for a dataset or namespace, due to data stream fields name restrictions.

jsoriano · 2023-08-24T12:54:48Z

test integrations

elasticmachine · 2023-08-24T13:00:01Z

Created or updated PR in integrations repostiory to test this vesrion. Check elastic/integrations#7531

jsoriano

Looks good, please wait for CI in elastic/integrations#7531.

internal/fields/validate.go

jsoriano · 2023-08-24T12:53:36Z

test/packages/parallel/awsfirehose/data_stream/log/routing_rules.yml

@@ -1,6 +1,11 @@
 - source_dataset: awsfirehose.log
  rules:
+    - target_dataset:
+        - "{{cloud.account.id}}"


For a test this looks enough, but isn't this intended to be used with fields that contain datasets? Like {{data_stream.dataset}} or similar?

In general, expressions in the routing rules enable the rule to pick a dataset or namespace value for a field in the log event.

For example, the Kubernetes integration picks the dataset value from the {{kubernetes.labels.elastic_co/dataset}} field.

Unfortunately, the Firehose event does not offer a lot of fields with values compatible with the data stream naming restrictions.

So, I imagined a scenario where users want to partition the log events into multiple AWS account-based data streams using {{cloud.account.id}}. But it's a bit confusing.

The other option is to add an extra field in the log event, for example, {{labels.elastic_co/dataset}}, and use this field in the rule. It does not exist in the Firehose realm but is less confusing overall. WDYT?

Co-authored-by: Jaime Soriano Pastor <[email protected]>

Even if Firehose currently does not provide an `aws.labels` field, using it in the routing rules tests is probably less confusing than using the field `{{cloud.account.id}}` as target dataset or namespace.

elasticmachine · 2023-08-27T22:25:25Z

💚 Build Succeeded

Buildkite Build
Commit: 4db10c6

History

💔 Build #1355 failed 1718682
💚 Build #1346 succeeded ac18517
💔 Build #1345 failed 5c251cf
💚 Build #1334 succeeded 9b9136c
💚 Build #1331 succeeded edd774a

cc @zmoog

zmoog force-pushed the zmoog/add-support-expressions-in-routing-rules branch from b825f33 to 681e688 Compare August 14, 2023 23:23

jsoriano reviewed Aug 15, 2023

View reviewed changes

internal/fields/validate.go Outdated Show resolved Hide resolved

zmoog self-assigned this Aug 16, 2023

zmoog added 2 commits August 16, 2023 16:38

Add tests for dataset expressions rendering

b191cd0

zmoog changed the title ~~[WIP] Evaluate expressions in routing rules datasets values~~ Render Mustache expressions in routing rules datasets values Aug 16, 2023

Fix routing_rules.yml format

cfea2c4

I didn't run `elastic-package check` in the kubernetes test package. Without a proper format, the test detects a dirty git working directory at the end of the test run.

zmoog marked this pull request as ready for review August 16, 2023 17:01

zmoog requested review from kaiyan-sheng and jsoriano August 16, 2023 17:01

jlind23 requested a review from mrodm August 22, 2023 17:12

zmoog added 2 commits August 23, 2023 11:38

Remove the kubernetes test package

4a678ae

We already have one at: test/packages/with-kind/kubernetes So this one is redundant. Dropping it!

Add a test rule with expression

edd774a

To test that routing rules work with datasets and namespaces, we add another rule that uses the `{{cloud.account.id}}` for both if the `cloud.region` is `eu-west-1`.

zmoog force-pushed the zmoog/add-support-expressions-in-routing-rules branch from c201406 to edd774a Compare August 23, 2023 11:47

kaiyan-sheng approved these changes Aug 23, 2023

View reviewed changes

Update expression tests

9b9136c

Changing the expression test to make it more realistic. The main challenge here is to find a source event field value I can use for a dataset or namespace: it can't contain `-`.

elasticmachine mentioned this pull request Aug 24, 2023

Test elastic-package#1393 - DO NOT MERGE elastic/integrations#7531

Closed

jsoriano approved these changes Aug 24, 2023

View reviewed changes

zmoog and others added 2 commits August 25, 2023 06:16

Update internal/fields/validate.go

5c251cf

Co-authored-by: Jaime Soriano Pastor <[email protected]>

Make static checker happy

ac18517

Make more realistic tests adding a labels field

4db10c6

Even if Firehose currently does not provide an `aws.labels` field, using it in the routing rules tests is probably less confusing than using the field `{{cloud.account.id}}` as target dataset or namespace.

zmoog force-pushed the zmoog/add-support-expressions-in-routing-rules branch from 1718682 to 4db10c6 Compare August 27, 2023 21:47

zmoog merged commit 8fe73a1 into elastic:main Aug 28, 2023

zmoog deleted the zmoog/add-support-expressions-in-routing-rules branch August 28, 2023 00:21

zmoog mentioned this pull request Aug 30, 2023

Use an unreleased version of elastic-package zmoog/public-notes#48

Open

zmoog mentioned this pull request Sep 4, 2023

[Kubernetes] Reroute container logs based on pod annotations elastic/integrations#7118

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Render Mustache expressions in routing rules datasets values #1393

Render Mustache expressions in routing rules datasets values #1393

zmoog commented Aug 14, 2023 •

edited

Loading

kaiyan-sheng commented Aug 22, 2023

zmoog commented Aug 22, 2023 •

edited

Loading

jsoriano commented Aug 23, 2023

zmoog commented Aug 23, 2023 •

edited

Loading

zmoog commented Aug 23, 2023

kaiyan-sheng left a comment

zmoog commented Aug 23, 2023

zmoog commented Aug 23, 2023

jsoriano commented Aug 24, 2023

elasticmachine commented Aug 24, 2023

jsoriano left a comment

jsoriano Aug 24, 2023

zmoog Aug 25, 2023

elasticmachine commented Aug 27, 2023

Render Mustache expressions in routing rules datasets values #1393

Render Mustache expressions in routing rules datasets values #1393

Conversation

zmoog commented Aug 14, 2023 • edited Loading

Motivation

Change description

Additional Notes

Reviewer checklist

kaiyan-sheng commented Aug 22, 2023

zmoog commented Aug 22, 2023 • edited Loading

jsoriano commented Aug 23, 2023

zmoog commented Aug 23, 2023 • edited Loading

zmoog commented Aug 23, 2023

kaiyan-sheng left a comment

Choose a reason for hiding this comment

zmoog commented Aug 23, 2023

zmoog commented Aug 23, 2023

jsoriano commented Aug 24, 2023

elasticmachine commented Aug 24, 2023

jsoriano left a comment

Choose a reason for hiding this comment

jsoriano Aug 24, 2023

Choose a reason for hiding this comment

zmoog Aug 25, 2023

Choose a reason for hiding this comment

elasticmachine commented Aug 27, 2023

💚 Build Succeeded

History

zmoog commented Aug 14, 2023 •

edited

Loading

zmoog commented Aug 22, 2023 •

edited

Loading

zmoog commented Aug 23, 2023 •

edited

Loading