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.

Sign up for GitHub

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

Jump to bottom

[Ingest Manager] Guard against empty stream.datasource and namespace #18769

Merged
merged 5 commits into from
May 27, 2020
Merged

[Ingest Manager] Guard against empty stream.datasource and namespace #18769

merged 5 commits into from
May 27, 2020

Conversation

michalpristas
Copy link
Contributor

@michalpristas michalpristas commented May 27, 2020 •
edited
Loading

What does this PR do?

This PR fallbacks to default values if namespace or datasource is an empty string

Why is it important?

Empty string is not a valid value

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

@michalpristas michalpristas added bug review needs_backport PR is waiting to be backported to other branches. Team:Ingest Management Ingest Management:alpha1 Group issues for ingest management alpha1 labels May 27, 2020
@michalpristas michalpristas self-assigned this May 27, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ingest-management (Team:Ingest Management)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels May 27, 2020
@michalpristas michalpristas changed the title [Ingest Manager] Guard agains empty stream.datasource and namespace [Ingest Manager] Guard against empty stream.datasource and namespace May 27, 2020
@elasticmachine
Copy link
Collaborator

elasticmachine commented May 27, 2020 •
edited
Loading

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #18769 updated]

  • Start Time: 2020-05-27T15:13:41.593+0000

  • Duration: 31 min 9 sec

Test stats 🧪

Test Results
Failed 0
Passed 504
Skipped 127
Total 631

ruflin
ruflin reviewed May 27, 2020
View reviewed changes
x-pack/elastic-agent/pkg/agent/transpiler/rules_test.go
@@ -80,6 +87,14 @@ datasources:
- paths: /var/log/mysql/access.log
dataset: dsds
index: mytype-dsds-nsns
- name: All specified with empty strings
namespace: ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the expected now be default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is test for injecting index, it does not change existing values.
we either generate index based on these values or we're adding add_fields processor with correct values

Copy link
Contributor

Choose a reason for hiding this comment

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

we need both. values and index must match.

In a follow up we can discuss, if we should even error if it is "".

Copy link
Contributor

Choose a reason for hiding this comment

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

@michalpristas I also thought the result should be default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think we can fix it but outside of injectIndex rule, what this should do is injecting index and not modifying anything on top of that.

ph
ph reviewed May 27, 2020
View reviewed changes
x-pack/elastic-agent/CHANGELOG.asciidoc
@@ -39,7 +39,8 @@
- Remove fleet admin from setup script {pull}18611[18611]
- Correctly report platform and family. {issue}18665[18665]
- Clean action store after enrolling to new configuration {pull}18656[18656]
[Ingest Manager] Avoid watching monitor logs {pull}18723[18723]
- Avoid watching monitor logs {pull}18723[18723]
Copy link
Contributor

Choose a reason for hiding this comment

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

double entry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no i'm just fixing format of previous one

@ph ph mentioned this pull request May 27, 2020
Closed
@ph ph requested a review from ruflin May 27, 2020 15:58
ph
ph approved these changes May 27, 2020
View reviewed changes
Copy link
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

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

LGTM, test look complete and easy to remove.

@michalpristas michalpristas merged commit d37b598 into elastic:master May 27, 2020
michalpristas added a commit to michalpristas/beats that referenced this pull request May 28, 2020
@michalpristas
[Ingest Manager] Guard against empty stream.datasource and namespace (e…
13b7ab8 
…lastic#18769)

[Ingest Manager] Guard against empty stream.datasource and namespace (elastic#18769)
michalpristas added a commit to michalpristas/beats that referenced this pull request May 28, 2020
@michalpristas
[Ingest Manager] Guard against empty stream.datasource and namespace (e…
ff5aded 
…lastic#18769)

[Ingest Manager] Guard against empty stream.datasource and namespace (elastic#18769)
This was referenced May 28, 2020
Merged
Merged
michalpristas added a commit that referenced this pull request May 28, 2020
@michalpristas
Cherry-pick #18769 to 7.8: Guard against empty stream.datasource and …
b17a3cf 
…namespace (#18806)

* [Ingest Manager] Guard against empty stream.datasource and namespace (#18769)
michalpristas added a commit that referenced this pull request May 28, 2020
@michalpristas
Cherry-pick #18769 to 7.x: Guard against empty stream.datasource and …
90e9630 
…namespace (#18805)

Cherry-pick #18769 to 7.x:  Guard against empty stream.datasource and namespace (#18805)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ph ph ph approved these changes

@ruflin ruflin Awaiting requested review from ruflin

Assignees

@michalpristas michalpristas

Labels
bug Ingest Management:alpha1 Group issues for ingest management alpha1 needs_backport PR is waiting to be backported to other branches. review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@michalpristas @elasticmachine @ph @ruflin