Add support to define runtime fields #505
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
🌐 Coverage report
|
Create another test package to check kibana.version used when there are runtime fields defined. Removed warning message from the kibana version errors for input packages.
| "bad_runtime_kibana_version": []string{ | ||
| "conditions.kibana.version must be ^8.9.0 or greater to include runtime fields", | ||
| }, | ||
| } |
There was a problem hiding this comment.
Created a new test function to check minimum kibana versions.
| return nil | ||
| } | ||
|
|
||
| return errors.New("Warning: conditions.kibana.version must be ^8.8.0 or greater for non experimental input packages (version > 1.0.0)") | ||
| return errors.New("conditions.kibana.version must be ^8.8.0 or greater for non experimental input packages (version > 1.0.0)") |
There was a problem hiding this comment.
Removed Warning: prefix since this error is always raised and fails the validation.
| return nil | ||
| } | ||
|
|
||
| if kibanaVersionConditionIsGreaterThanOrEqualTo(kibanaVersionCondition, "8.9.0") { |
There was a problem hiding this comment.
As there is no failure if a package that contains a runtime field is installed in kibana versions that do not support those kind of fields, this validation rule ensures that the packages must have at least a given kibana version.
What version should I set here @kpollich ? I was thinking that maybe it is 8.9 ?
There was a problem hiding this comment.
Talked offline with @kpollich , Elastic stack / Kibana 8.9.0 version is going to be set as minimum version for runtime fields.
| case string: | ||
| r.enabled = true | ||
| r.script = v | ||
| default: | ||
| // JSONSchema already checks about the type of this field (e.g. int or float) | ||
| r.enabled = true | ||
| r.script = string(data) |
There was a problem hiding this comment.
I think we should be on the safe side and return an error here for unexpected types, without assuming that this has always passed first through the json schema validator.
If we want to support numbers, I would add a case for them.
| case string: | |
| r.enabled = true | |
| r.script = v | |
| default: | |
| // JSONSchema already checks about the type of this field (e.g. int or float) | |
| r.enabled = true | |
| r.script = string(data) | |
| case string: | |
| r.enabled = true | |
| r.script = v | |
| case float64: | |
| r.enabled = true | |
| r.script = string(data) | |
| default: | |
| return fmt.Errorf("unexpected type.... |
(json package always uses float64 for numbers, except when UseNumber() is used).
There was a problem hiding this comment.
TBH not sure if we need to support numbers (e.g. float64), just added as a test case for completeness.
Returning that error in the default case makes the two implementations for unmarshaling YAML and JSON different, at least as they are now. UnmarshalYAML should be able to differentiate between strings and float64, int... etc, but it only receives a value.Value that is a string.
It could be added to the UnmarshalYAML something like (or viceversa allowing it):
if _, err := strconv.ParseFloat(text, 8); err == nil {
return fmt.Errorf("unexpected type found float64")
}
And other should also be checked I think.
A side-effect of introducing that error, it also adds an error for each JSON schema error too:
validator_test.go:173:
Error Trace: /home/mariorodriguez/Coding/work/package-spec/code/go/pkg/validator/validator_test.go:173
Error: "found 10 validation errors:
1. file "../../../../test/packages/bad_runtime_fields/data_stream/foo/fields/fields.yml" is invalid: field 0: Must not be present
2. file "../../../../test/packages/bad_runtime_fields/data_stream/foo/fields/fields.yml" is invalid: field 1.runtime: Invalid type. Expected: string, given: integer
3. file "../../../../test/packages/bad_runtime_fields/data_stream/foo/fields/fields.yml" is invalid: field 2: Must not be present
4. file "../../../../test/packages/bad_runtime_fields/data_stream/foo/fields/fields.yml" is invalid: field 3: Must not be present
5. file "../../../../test/packages/bad_runtime_fields/data_stream/foo/fields/fields.yml" is invalid: can't unmarshal fields: yaml.Unmarshal failed (path: data_stream/foo/fields/fields.yml): unexpected type found float64
6. file "../../../../test/packages/bad_runtime_fields/data_stream/foo/fields/fields.yml" is invalid: can't unmarshal fields: yaml.Unmarshal failed (path: data_stream/foo/fields/fields.yml): unexpected type found float64
7. file "../../../../test/packages/bad_runtime_fields/data_stream/foo/fields/fields.yml" is invalid: can't unmarshal fields: yaml.Unmarshal failed (path: data_stream/foo/fields/fields.yml): unexpected type found float64
8. file "../../../../test/packages/bad_runtime_fields/data_stream/foo/fields/fields.yml" is invalid: can't unmarshal fields: yaml.Unmarshal failed (path: data_stream/foo/fields/fields.yml): unexpected type found float64
9. file "../../../../test/packages/bad_runtime_fields/data_stream/foo/fields/fields.yml" is invalid: can't unmarshal fields: yaml.Unmarshal failed (path: data_stream/foo/fields/fields.yml): unexpected type found float64
10. file "../../../../test/packages/bad_runtime_fields/data_stream/foo/fields/fields.yml" is invalid: can't unmarshal fields: yaml.Unmarshal failed (path: data_stream/foo/fields/fields.yml): unexpected type found float64
" should have 4 item(s), but has 10
Test: TestValidateFile/bad_runtime_fields
It looks that it would be too many errors to be shown to the user/developer for the same field wrong type:
- There are 4 wrong fields in the
bad_runtime_fieldspackage:data_stream/foo/fields/fields.yml. - The error is shown for every time that YAML file is parsed.
An option for that would be check for duplicated errors and just show once that error:
--- code/go/internal/validator/spec.go
+++ code/go/internal/validator/spec.go
@@ -93,16 +93,24 @@ func processErrors(errs ve.ValidationErrors) ve.ValidationErrors {
"Must validate all the schemas (allOf)",
"Must validate at least one schema (anyOf)",
}
+
+ duplicated := make(map[string]struct{})
for _, e := range errs {
for _, msg := range msgTransforms {
+ if _, ok := duplicated[e.Error()]; ok {
+ continue
+ }
if strings.Contains(e.Error(), msg.original) {
- processedErrs = append(processedErrs, errors.New(strings.Replace(e.Error(), msg.original, msg.new, 1)))
+ transformedErr := errors.New(strings.Replace(e.Error(), msg.original, msg.new, 1))
+ processedErrs = append(processedErrs, transformedErr)
+ duplicated[transformedErr.Error()] = struct{}{}
continue
}
if substringInSlice(e.Error(), redundant) {
continue
}
processedErrs = append(processedErrs, e)
+ duplicated[e.Error()] = struct{}{}
}
}Given that, I have some doubts @jsoriano :
- should we just accept strings for now?
- if other types are not allowed, in
unmarshalYAMLshould we check for int, unsigned, ... too ? - If that error is raised, WDYT about removing duplicate error messages?
There was a problem hiding this comment.
Ok, I wouldn't add complexity here to handle special cases. I would be fine with accepting only strings and bools. There would be any valid cases for numbers here?
There was a problem hiding this comment.
I think that the scripts defined in runtime fields usually won't set just a number. Adding just a number for a field it could be also done in the ingest pipeline too.
Just tested adding runtime field with just a number in the script field using this index_template:
elasticsearch:
index_template:
mappings:
runtime:
foo:
type: keyword
script:
source: "42"And there is an error when trying to install the package:
2023/04/25 16:35:49 DEBUG POST https://127.0.0.1:5601/api/fleet/epm/packages
Error: can't install the package: can't install the package: could not zip-install package; API status code = 500; response body = {"statusCode":500,"error":"Internal Server Error","message":"mapper_parsing_exception\n\tCaused by:\n\t\tscript_exception: compile error\n\tRoot causes:\n\t\tscript_exception: compile error"}
Tested with the Elastic stack versions 8.8.0-SNAPSHOT and 8.0.0
As there will be compile errors, I guess we could just check bools or everything else being a string, couldn't we?
There was a problem hiding this comment.
Yes, let's consider that this is a bool or a script, and rely on script compilation for the rest.
| - boolean | ||
| - date | ||
| - double | ||
| - geo_point | ||
| - ip | ||
| - keyword | ||
| - long |
There was a problem hiding this comment.
This list is not the complete list available.
According to the docs, the complete list is:
- boolean
- composite
- date
- double
- geo_point
- ip
- keyword
- long
- lookup
In this condition, just the types that we support in the type field are added. Missing types:
- composite
- lookup
code/go/internal/validator/semantic/validate_minimum_kibana_version.go
Outdated
Show resolved
Hide resolved
What does this PR do?
Add support to define runtime fields.
In Kibana versions that do not support runtime fields, it is created a mapping with the given type. Because of that, as there is no failure when installing a package that contains a runtime field in those kibana versions, a new validation rule has been introduced to ensure that the packages must have at least a given kibana version.
Here it is the issue to add this support in Kibana elastic/kibana#155255
Checklist
test/packagesthat prove my change is effective.spec/changelog.yml.runtimekey to be used from kibana.version 8.9?Related issues