🌊 Make client check for hierarchy conflicts before creating streams by miltonhultgren · Pull Request #208914 · elastic/kibana

miltonhultgren · 2025-01-30T11:31:17Z

Summary

If you enable streams (which creates logs) and then try to create logs.child.grandchild but logs.child already exists as either an index or an unwired (Classic) stream, then we end up in a weird state where logs.child.grandchild gets created as a wired child but then the request fails as it tries to turn the unwired stream into a wired stream.

This PR adds a step that asserts that there are no such conflicts in the hierarchy before proceeding.
It also adds a check to ensure Streams are enabled before allowing the creation of any streams, as well as blocking the creation of a root stream that isn't logs.
Finally, there is some minor improvements to error handling for when a data stream isn't found and error messages.

…treams

miltonhultgren · 2025-01-30T11:33:16Z

x-pack/solutions/observability/plugins/streams/server/lib/streams/client.ts

+  private async assertNoHierarchicalConflicts(definitionName: string) {
+    const streamNames = [...getAncestors(definitionName), definitionName];
+    const hasConflict = await Promise.all(
+      streamNames.map((streamName) => this.isStreamNameTaken(streamName))


Small note: If we used arrow functions to define our methods we could write this simply as streamNames.map(this.isStreamNameTaken) since the context would already be bound.

I'm always in favour of referencing functions vs inline callbacks, I wouldn't mind if you change the method declaration to an arrow function.

miltonhultgren · 2025-01-30T11:33:59Z

x-pack/solutions/observability/plugins/streams/server/lib/streams/client.ts

+      if (!isDefinitionNotFoundError(error)) {
+        throw error;
+      }
+    }
+
+    try {
+      await this.dependencies.scopedClusterClient.asCurrentUser.indices.get({
+        index: streamName,
+      });


I'm not super happy with the side effect driven flow here but flipping it just feels so verbose.

miltonhultgren · 2025-01-30T11:35:24Z

x-pack/solutions/observability/plugins/streams/server/lib/streams/client.ts

    return this.dependencies.scopedClusterClient.asCurrentUser.indices
      .getDataStream({ name })
      .then((response) => {
+        if (response.data_streams.length === 0) {


This is where the original error came from, if there is no data stream this API does not throw any error, it simply returns an empty list.

Which caused the flow to continue to try to translate this undefined object into a Classic stream definition hence causing the undefined.name error.

miltonhultgren · 2025-01-30T11:36:19Z

x-pack/solutions/observability/plugins/streams/server/routes/streams/crud/route.ts

    const { streamsClient } = await getScopedClients({ request });

+    if (!(await streamsClient.isStreamsEnabled())) {
+      throw badData('Streams are not enabled');


badData results in a HTTP 422, since the request is syntactically correct and would pass our validation but there is a semantic error of streams not being enabled yet.

…hierarchy-conflicts

x-pack/solutions/observability/plugins/streams/server/routes/streams/enablement/route.ts

x-pack/solutions/observability/plugins/streams/server/routes/streams/crud/route.ts

tonyghiani

Overall looks good, you got some test failures, I'll give another quick review once it's solved but it should be good to go 👌

tonyghiani

Overall looks good, you got some test failures, I'll give another quick review once it's solved but it should be good to go 👌

…hierarchy-conflicts

tonyghiani

LGTM, thanks for the changes 👌

x-pack/solutions/observability/plugins/streams/server/lib/streams/root_stream_definition.ts

x-pack/solutions/observability/plugins/streams/server/lib/streams/client.ts

flash1293

see comment above

…hierarchy-conflicts

elasticmachine · 2025-02-05T13:25:37Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: bdac5e2

Failed CI Steps

Jest Tests #15

Test Failures

[job] [logs] Jest Tests #15 / Configure renders correctly

Metrics [docs]

✅ unchanged

History

💚 Build #273220 succeeded 5c7bd81
💔 Build #272661 failed c747fed
💔 Build #271939 failed b09d831

flash1293

LGTM

kibanamachine · 2025-02-05T14:02:10Z

Starting backport for target branches: 9.0

https://github.com/elastic/kibana/actions/runs/13159058514

kibanamachine · 2025-02-05T14:07:40Z

💔 All backports failed

Status	Branch	Result
❌	9.0	Backport failed because of merge conflicts You might need to backport the following PRs to 9.0: - [Streams 🌊] Introduce GroupStreams (#208126)

Manual backport

To create the backport manually run:

node scripts/backport --pr 208914

Questions ?

Please refer to the Backport tool documentation

kibanamachine · 2025-02-06T09:52:56Z

Starting backport for target branches: 8.16, 8.17, 8.18, 8.x

https://github.com/elastic/kibana/actions/runs/13176143714

kibanamachine · 2025-02-06T09:59:38Z

💔 All backports failed

Status	Branch	Result
❌	8.16	Backport failed because of merge conflicts You might need to backport the following PRs to 8.16: - [OBX-UX-MGMT][BUG] Fix chart in Custom Threshold rule when the field name has slashes (#209263)
❌	8.17	Backport failed because of merge conflicts You might need to backport the following PRs to 8.17: - [Detection Engine][Docs] Updating examples to meet old ascii docs (#207558) - [OBX-UX-MGMT][BUG] Fix chart in Custom Threshold rule when the field name has slashes (#209263)
❌	8.18	Backport failed because of merge conflicts
❌	8.x	Backport failed because of merge conflicts You might need to backport the following PRs to 8.x: - [Streams 🌊] Introduce GroupStreams (#208126)

Manual backport

To create the backport manually run:

node scripts/backport --pr 208914

Questions ?

Please refer to the Backport tool documentation

kibanamachine · 2025-02-06T10:23:30Z

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/13176673344

kibanamachine · 2025-02-06T10:28:51Z

💔 All backports failed

Status	Branch	Result
❌	8.x	Backport failed because of merge conflicts You might need to backport the following PRs to 8.x: - [Streams 🌊] Introduce GroupStreams (#208126)

Manual backport

To create the backport manually run:

node scripts/backport --pr 208914

Questions ?

Please refer to the Backport tool documentation

kibanamachine · 2025-02-06T13:10:59Z

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/13176673344

…lastic#208914) ## Summary If you enable streams (which creates `logs`) and then try to create `logs.child.grandchild` but `logs.child` already exists as either an index or an unwired (Classic) stream, then we end up in a weird state where `logs.child.grandchild` gets created as a wired child but then the request fails as it tries to turn the unwired stream into a wired stream. This PR adds a step that asserts that there are no such conflicts in the hierarchy before proceeding. It also adds a check to ensure Streams are enabled before allowing the creation of any streams, as well as blocking the creation of a root stream that isn't `logs`. Finally, there is some minor improvements to error handling for when a data stream isn't found and error messages. (cherry picked from commit 3c4694e)

kibanamachine · 2025-02-06T13:16:43Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…eams (#208914) (#210003) # Backport This will backport the following commits from `main` to `8.x`: - [🌊 Make client check for hierarchy conflicts before creating streams (#208914)](#208914)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Milton Hultgren <milton.hultgren@elastic.co>

…lastic#208914) ## Summary If you enable streams (which creates `logs`) and then try to create `logs.child.grandchild` but `logs.child` already exists as either an index or an unwired (Classic) stream, then we end up in a weird state where `logs.child.grandchild` gets created as a wired child but then the request fails as it tries to turn the unwired stream into a wired stream. This PR adds a step that asserts that there are no such conflicts in the hierarchy before proceeding. It also adds a check to ensure Streams are enabled before allowing the creation of any streams, as well as blocking the creation of a root stream that isn't `logs`. Finally, there is some minor improvements to error handling for when a data stream isn't found and error messages.

[Streams] Make client check for hierarchy conflicts before creating s…

b6d0ecb

…treams

miltonhultgren requested review from a team as code owners January 30, 2025 11:31

miltonhultgren added release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Feature:Streams This is the label for the Streams Project labels Jan 30, 2025

miltonhultgren commented Jan 30, 2025

View reviewed changes

Merge branch 'main' of github.com:elastic/kibana into streams-assert-…

b09d831

…hierarchy-conflicts

tonyghiani reviewed Jan 30, 2025

View reviewed changes

x-pack/solutions/observability/plugins/streams/server/routes/streams/enablement/route.ts Outdated Show resolved Hide resolved

tonyghiani reviewed Jan 30, 2025

View reviewed changes

x-pack/solutions/observability/plugins/streams/server/routes/streams/crud/route.ts Outdated Show resolved Hide resolved

tonyghiani reviewed Jan 30, 2025

View reviewed changes

miltonhultgren added 2 commits February 3, 2025 12:38

Merge branch 'main' of github.com:elastic/kibana into streams-assert-…

4e0dd7d

…hierarchy-conflicts

Fix failing test, reword error messages

c747fed

tonyghiani approved these changes Feb 3, 2025

View reviewed changes

flash1293 reviewed Feb 4, 2025

View reviewed changes

x-pack/solutions/observability/plugins/streams/server/lib/streams/root_stream_definition.ts Outdated Show resolved Hide resolved

x-pack/solutions/observability/plugins/streams/server/lib/streams/client.ts Outdated Show resolved Hide resolved

flash1293 requested changes Feb 4, 2025

View reviewed changes

miltonhultgren added 3 commits February 4, 2025 16:51

Merge branch 'main' of github.com:elastic/kibana into streams-assert-…

4999d39

…hierarchy-conflicts

Use getSegments

b8ac05d

Only run for wired definitions

5c7bd81

miltonhultgren requested a review from flash1293 February 4, 2025 19:06

Merge branch 'main' into streams-assert-hierarchy-conflicts

bdac5e2

flash1293 approved these changes Feb 5, 2025

View reviewed changes

miltonhultgren merged commit 3c4694e into elastic:main Feb 5, 2025
1 check passed

kibanamachine added the v9.1.0 label Feb 5, 2025

miltonhultgren added backport:prev-major and removed backport:prev-minor labels Feb 6, 2025

miltonhultgren added backport:version Backport to applied version labels v8.19.0 and removed backport:prev-major labels Feb 6, 2025

kibanamachine mentioned this pull request Feb 6, 2025

[8.x] 🌊 Make client check for hierarchy conflicts before creating streams (#208914) #210003

Merged

Conversation

miltonhultgren commented Jan 30, 2025 • edited by kibanamachine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

miltonhultgren Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

tonyghiani Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

miltonhultgren Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

miltonhultgren Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

miltonhultgren Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tonyghiani left a comment

Choose a reason for hiding this comment

Uh oh!

tonyghiani left a comment

Choose a reason for hiding this comment

Uh oh!

tonyghiani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

flash1293 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Feb 5, 2025

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

History

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kibanamachine commented Feb 5, 2025

Uh oh!

kibanamachine commented Feb 5, 2025

💔 All backports failed

Manual backport

Questions ?

Uh oh!

kibanamachine commented Feb 6, 2025

Uh oh!

kibanamachine commented Feb 6, 2025

💔 All backports failed

Manual backport

Questions ?

Uh oh!

kibanamachine commented Feb 6, 2025

Uh oh!

kibanamachine commented Feb 6, 2025

💔 All backports failed

Manual backport

Questions ?

Uh oh!

kibanamachine commented Feb 6, 2025

Uh oh!

kibanamachine commented Feb 6, 2025

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

miltonhultgren commented Jan 30, 2025 •

edited by kibanamachine

Loading

flash1293 left a comment •

edited

Loading