feat: Add validator for pipeline tags #1010
Conversation
taylor-swanson
force-pushed
the
feat/validate-unique-tags
branch
from
b2d626a to
53b2e9a
Compare
taylor-swanson
force-pushed
the
feat/validate-unique-tags
branch
from
8e07ee6 to
babead4
Compare
- Add validation rule SVR00006 to verify all ingest pipeline processors have a valid tag - Add validation rule SVR00007 to verify all ingest pipeline processor tags are unique
taylor-swanson
force-pushed
the
feat/validate-unique-tags
branch
from
0e3ba16 to
d1c725c
Compare
|
Hey @elastic/ecosystem @jsoriano, would you be able to give this a review when you have time? |
jsoriano
left a comment
jsoriano
left a comment
There was a problem hiding this comment.
Nice change, thanks! Added some small suggestions.
| errors = append(errors, subErrors...) | ||
| } | ||
|
|
||
| raw, ok := proc.Attributes["tag"] |
There was a problem hiding this comment.
looking into the pipeline spec
i don't see there the property "tag" defined...am i right assuming the definition should be here? if so, could we control the required validation version from the json patch rules?
There was a problem hiding this comment.
Yeah I noticed that tag wasn't defined there as well. We're also missing all of the required fields for each processor (i.e., the 'rename' processor requires the 'field' property).
In this case, though, tag isn't a required field by elasticsearch, rather, we're requiring it on certain processors in certain situations (only in the processors list, but no the global on_failure handler). For that reason, I wouldn't want to apply the tag enforcement globally.
There was a problem hiding this comment.
could we control the required validation version from the json patch rules?
Yes, you are right, it would be better to handle this from the json schema if possible. Though as implemented here it is easier to make it skippable, and to provide better error messages. If brought to the schema we'd need to add rules to code/go/internal/validator/spec.go. Also not sure about how to prevent duplications from the spec. So I am on the fence on this.
Maybe an option would be to use both, validate that there are tags in the spec, and check with a semantic rule that tags are not duplicated.
There was a problem hiding this comment.
Also not sure about how to prevent duplications from the spec.
Well, this seems to be easy too: https://json-schema.org/understanding-json-schema/reference/array#uniqueItems
@taylor-swanson wdyt about rewriting this with the spec?
There was a problem hiding this comment.
It's certainly an interesting idea... How would that work though since tag is nested a bit deeper in the tree compared to the processor itself?
processors:
- set:
tag: tag_1
- set:
tag: tag_2
It ends of being array.object.object to get down to tag.
There was a problem hiding this comment.
You are right, it would be complex to check unique items in different objects... I guess this part would still need to be in code.
- Change pointers to values in processor slices - Remove validation bypass code for duplicate tags - Mention that global on_failure processors are excluded from tag check
|
Hey @jsoriano, @teresaromero, just checking to see if there are any other issues with this PR. Thanks! |
💚 Build Succeeded
History
|
|
Hey @jsoriano, @teresaromero, I had to resolve merge conflicts and looks like it cleared the review, so will need another approval, thanks! |
4 participants
What does this PR do?
Why is it important?
Checklist
test/packagesthat prove my change is effective.spec/changelog.yml.Related issues