[Ingest Manager] Refuse invalid stream values in configuration#19587
[Ingest Manager] Refuse invalid stream values in configuration#19587michalpristas merged 13 commits intoelastic:masterfrom
Conversation
|
Pinging @elastic/ingest-management (Team:Ingest Management) |
💚 Build Succeeded
Expand to view the summary
Build stats
|
blakerouse
left a comment
There was a problem hiding this comment.
Overall I think it looks good, just a couple of changes.
| - type: system/metrics | ||
|
|
||
| # The only two requirement are that it has only characters allowed in an Elasticsearch index name | ||
| # and does NOT contain a `-`. |
There was a problem hiding this comment.
I think it would be better to be explicit on what is allowed even if it is conservative in nature. That way its less likely for them to make a mistake and its clear directly from the comment.
How about:
Must be all lowercase, no spaces, only characters a-z or numbers 0-9 (no special characters), and no more than 255 characters.
I think the character length might be an issue being its concat-ed to make the index name.
| use_output: default | ||
| streams: | ||
| - metricset: cpu | ||
| # Same requirements as for the namespace apply. |
There was a problem hiding this comment.
I think it would be better to be explicit here as well, what ever is picked above, should just repeat verbatim here.
|
|
||
| streamNodes, ok := streamsList.Value().([]transpiler.Node) | ||
| if !ok { | ||
| return errors.New("streams is not a list", errors.TypeConfig) |
There was a problem hiding this comment.
Have you tested with Endpoint Security enabled in Fleet? I think this will cause an issue, because it does come with streams: [] and something similar failed in another rule because of it.
https://github.com/elastic/beats/pull/19248/files#diff-68f0d6baed417771ae64bf9e5a7587a7R267
There was a problem hiding this comment.
good point here
| return false | ||
| } | ||
|
|
||
| return true |
There was a problem hiding this comment.
Why duplicate the same function and just give it the same name? Could remove the second one and rename it to isValid. Or could do:
func isDatasetValid(dataset string) bool {
return isNamespaceValid(dataset)
}
That way in the future if the validation of dataset does differ from namespace, then only that function body needs to be changed.
|
added a bit different strategy for checks and i'm also checking composed result of index to match the criteria, if resulting index would not match the criteria configuration fails immediately |
blakerouse
left a comment
There was a problem hiding this comment.
Looks good. Thanks for updating the config to be explicit.
…ic#19587) * added stream check * changelog * fmt * invalid names * updated config files * updated config files * changes in all config files * config typo * small changes * Update go.sum
* upstream/master: (25 commits) [Elastic Agent] Send checkin payload to Fleet (elastic#19857) [Ingest Manager] Fixed tests across agent elastic#19877 [Ingest Manager] Fix serialization test elastic#19876 Fix service start type mapping in windows/service metricset (elastic#19551) ci: Change comment trigger detection method (elastic#19827) Add 21 autogenerated filesets from rsa2elk devices (elastic#19713) [Ingest Manager] Agent config cleanup (elastic#19848) libbeat/publisher/pipeline: fix data races (elastic#19821) Update monitoring-internal-collection.asciidoc (elastic#19422) (elastic#19697) [Elastic Agent] Trust exchange endpoint must bind to 127.0.0.1 (elastic#19861) Specify an ECS version in Auditbeat/Packetbeat/Winlogbeat (elastic#19159) Add azure billing metricset (elastic#19207) Add support for appinsights in the metricbeat azure module (elastic#18940) Add MySQL query metricset with lightweight module and SQL helper (elastic#18955) [Ingest Manager] Refuse invalid stream values in configuration (elastic#19587) Do not use vendor during integration tests (elastic#19839) LIBBEAT: Enhancement Convert dissected values from String to other basic data types and IP (elastic#18683) [Elastic Agent] Remove support for "logs" and only support logfile (elastic#19761) [CI] support windows-2012 (elastic#19773) Do not update go.mod during packaging and testing (elastic#19823) ...
…ic#19587) * added stream check * changelog * fmt * invalid names * updated config files * updated config files * changes in all config files * config typo * small changes * Update go.sum
What does this PR do?
Adds a filter which errors out when dataset.name or namespace is empty string.
Why is it important?
Described in #18772
Checklist
CHANGELOG.next.asciidocorCHANGELOG-developer.next.asciidoc.Fixes: #18772
Fixes: #17730