Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
2c3f76e
Add dynamic templates as parameter
mrodm Dec 12, 2024
41d7b88
Add validation for each dynamic template depending on the parameters
mrodm Dec 16, 2024
ff5298e
Fixes for dynamic templates
mrodm Dec 16, 2024
af04d66
Compare fields with dynamic templates
mrodm Dec 17, 2024
f21c712
Remove multi_fields from flattened fields
mrodm Dec 17, 2024
aa28db6
Test without filtering dynamic templates
mrodm Dec 17, 2024
3767111
Ensure properties subfields are validated accordingly
mrodm Dec 18, 2024
9fb80b4
Restore filtering and add tests
mrodm Dec 18, 2024
2ad28ac
Disable unmatch_mapping_type and match_mapping_type and continue look…
mrodm Dec 19, 2024
8bdad65
Merge upstream/main into validate-dynamic-mappings
mrodm Dec 19, 2024
151e59e
Refactors and remove log statements
mrodm Dec 19, 2024
d0ff6b9
Fix function naming
mrodm Dec 19, 2024
b474b4b
Add test with match_pattern regex
mrodm Dec 19, 2024
066b5a0
Add comment in tests
mrodm Dec 19, 2024
a7eb191
Remove loading schema from create validator for mappings method
mrodm Jan 7, 2025
7666ac2
Separate parsing and validation of dynamic templates
mrodm Jan 7, 2025
a718796
Support match_pattern for the other settings
mrodm Jan 7, 2025
0a5e775
fix test
mrodm Jan 8, 2025
131cc9c
Update tests for multi-fields
mrodm Jan 8, 2025
38db8df
Review multi-fields logic
mrodm Jan 8, 2025
9ff3d0c
Revisit multi-fields logic in ECS
mrodm Jan 8, 2025
5165612
Report all errors related to multi-fields comparing with ECS
mrodm Jan 8, 2025
ce557c9
Ignored validation of multi-fields with ECS
mrodm Jan 8, 2025
ebda859
Fix multi-field test
mrodm Jan 8, 2025
019ffd4
Rephrase errors
mrodm Jan 9, 2025
8f36c18
Validate fully dynamic objects (preview) with dynamic templates
mrodm Jan 9, 2025
d96ffbf
Rephrase debug message
mrodm Jan 9, 2025
dbca4fe
Add logging - to be removed
mrodm Jan 9, 2025
8fce0ec
Merge remote-tracking branch 'upstream/main' into validate-dynamic-ma…
mrodm Jan 14, 2025
c333707
Revisited log messages
mrodm Jan 20, 2025
0569cfb
Remove comments
mrodm Jan 20, 2025
1aea303
Remove more debug statements
mrodm Jan 20, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Remove multi_fields from flattened fields
multi_fields are now compared directly with dynamic templates or ECS
fields. It was required to know whether or not that field was a
multi_field or not to apply some specific validations.
  • Loading branch information
mrodm committed Dec 17, 2024
commit f21c7124ef9cd768d4c37245d5f11216334cccd3
168 changes: 117 additions & 51 deletions internal/fields/mappings.go
Original file line number Diff line number Diff line change
Expand Up @@ -387,42 +387,82 @@ func (v *MappingValidator) validateMappingInECSSchema(currentPath string, defini
return fmt.Errorf("missing definition for path (not in ECS)")
}

actualType := mappingParameter("type", definition)
if found.Type == actualType {
return nil
err := compareFieldDefinitionWithECS(currentPath, found, definition)
if err != nil {
return err
}

// Compare multifields
var ecsMultiFields []FieldDefinition
// Filter multi-fields added by appendECSMappingMultifields
for _, f := range found.MultiFields {
if f.External != externalFieldAppendedTag {
ecsMultiFields = append(ecsMultiFields, f)
}
}

if isMultiFields(definition) != (len(ecsMultiFields) > 0) {
return fmt.Errorf("not matched definitions for multifields for %q: actual multi_fields in mappings %t - ECS multi_fields length %d", currentPath, isMultiFields(definition), len(ecsMultiFields))
}
// TODO: Compare all parameters and multifieds

// exceptions related to numbers
if isNumberTypeField(found.Type, actualType) {
logger.Debugf("Allowed number fields with different types (ECS %s - actual %s)", string(found.Type), string(actualType))
// if there are no multifieds in ECS, nothing to compare
if len(ecsMultiFields) == 0 {
return nil
}
// any other field to validate here?
return fmt.Errorf("actual mapping type (%s) does not match with ECS definition type: %s", actualType, found.Type)

actualMultiFields, err := getMappingDefinitionsField("fields", definition)
if err != nil {
return fmt.Errorf("invalid multi_field mapping %q: %w", currentPath, err)
}

for _, ecsMultiField := range ecsMultiFields {
multiFieldPath := currentMappingPath(currentPath, ecsMultiField.Name)
actualMultiField, ok := actualMultiFields[ecsMultiField.Name]
if !ok {
return fmt.Errorf("missing multi_field definition for %q", multiFieldPath)
}

def, ok := actualMultiField.(map[string]any)
if !ok {
return fmt.Errorf("invalid multi_field mapping type: %q", multiFieldPath)
}

err := compareFieldDefinitionWithECS(multiFieldPath, &ecsMultiField, def)
if err != nil {
return err
}
}

return nil
}

func compareFieldDefinitionWithECS(currentPath string, ecs *FieldDefinition, actual map[string]any) error {
actualType := mappingParameter("type", actual)
if ecs.Type != actualType {
// exceptions related to numbers
if !isNumberTypeField(ecs.Type, actualType) {
return fmt.Errorf("actual mapping type (%s) does not match with ECS definition type: %s", actualType, ecs.Type)
} else {
logger.Debugf("Allowed number fields with different types (ECS %s - actual %s)", string(ecs.Type), string(actualType))
}
}

// Compare other parameters
metricType := mappingParameter("time_series_metric", actual)
if ecs.MetricType != metricType {
return fmt.Errorf("actual mapping \"time_series_metric\" (%s) does not match with ECS definition value: %s", metricType, ecs.MetricType)
}
return nil
}

// flattenMappings returns all the mapping definitions found at "path" flattened including
// specific entries for multi fields too.
func flattenMappings(path string, definition map[string]any) (map[string]any, error) {
newDefs := map[string]any{}
if isMultiFields(definition) {
multifields, err := getMappingDefinitionsField("fields", definition)
if err != nil {
return nil, multierror.Error{fmt.Errorf("invalid multi_field mapping %q: %w", path, err)}
}

// Include also the definition itself
newDefs[path] = definition

for key, object := range multifields {
currentPath := currentMappingPath(path, key)
def, ok := object.(map[string]any)
if !ok {
return nil, multierror.Error{fmt.Errorf("invalid multi_field mapping type: %q", path)}
}
newDefs[currentPath] = def
}
// multi_fields are going to be validated directly with the dynamic templates
// or with ECS fields
return newDefs, nil
}

Expand Down Expand Up @@ -646,6 +686,8 @@ func (v *MappingValidator) validateMappingsNotInPreview(currentPath string, chil
return errs.Unique()
}

// matchingWithDynamicTemplates validates a given definition (currentPath) with a set of dynamic templates.
// The dynamic templates parameters are based on https://www.elastic.co/guide/en/elasticsearch/reference/8.17/dynamic-templates.html
func (v *MappingValidator) matchingWithDynamicTemplates(currentPath string, definition map[string]any, dynamicTemplates []map[string]any) error {

parseSetting := func(value any) ([]string, error) {
Expand Down Expand Up @@ -681,27 +723,17 @@ func (v *MappingValidator) matchingWithDynamicTemplates(currentPath string, defi
return elems[len(elems)-1]
}

stringMatchesPatterns := func(regexes []string, elem string, fullRegex bool) (bool, error) {
stringMatchesPatterns := func(regexes []string, elem string) (bool, error) {
applies := false
for _, v := range regexes {
if !strings.Contains(v, "*") {
// not a regex
continue
}

var r string
if fullRegex {
r = v
} else {
r = strings.ReplaceAll(v, ".", "\\.")
r = strings.ReplaceAll(r, "*", ".*")
// Force to match beginning and ending
r = fmt.Sprintf("^%s$", r)
}

match, err := regexp.MatchString(r, elem)
match, err := regexp.MatchString(v, elem)
if err != nil {
return false, fmt.Errorf("failed to build regex %s: %w", r, err)
return false, fmt.Errorf("failed to build regex %s: %w", v, err)
}
if match {
applies = true
Expand All @@ -711,6 +743,18 @@ func (v *MappingValidator) matchingWithDynamicTemplates(currentPath string, defi
return applies, nil
}

stringMatchesWildcards := func(regexes []string, elem string) (bool, error) {
for i, v := range regexes {
r := strings.ReplaceAll(v, ".", "\\.")
r = strings.ReplaceAll(r, "*", ".*")
// Force to match beginning and ending
r = fmt.Sprintf("^%s$", r)

regexes[i] = r
}
return stringMatchesPatterns(regexes, elem)
}

fieldType := mappingParameter("type", definition)
if fieldType == "" {
return fmt.Errorf("missing type parameter for field: %q", currentPath)
Expand All @@ -729,15 +773,17 @@ func (v *MappingValidator) matchingWithDynamicTemplates(currentPath string, defi
rawContents = value
}

// if strings.HasPrefix(templateName, "_embedded_ecs-") {
// continue
// }
// if strings.HasPrefix(templateName, "ecs_") {
// continue
// }
// if slices.Contains([]string{"all_strings_to_keywords", "strings_as_keyword"}, templateName) {
// continue
// }
// Filter out dynamic templates created by elastic-package (import_mappings)
// or added automatically by ecs@mappings component template
if strings.HasPrefix(templateName, "_embedded_ecs-") {
continue
}
if strings.HasPrefix(templateName, "ecs_") {
continue
}
if slices.Contains([]string{"all_strings_to_keywords", "strings_as_keyword"}, templateName) {
continue
}

logger.Debugf("Checking dynamic template for %q: %q", currentPath, templateName)

Expand Down Expand Up @@ -777,10 +823,17 @@ func (v *MappingValidator) matchingWithDynamicTemplates(currentPath string, defi
matched = false
break
}
matches, err := stringMatchesPatterns(values, name, fullRegex)

var matches bool
if fullRegex {
matches, err = stringMatchesPatterns(values, name)
} else {
matches, err = stringMatchesWildcards(values, name)
}
if err != nil {
return fmt.Errorf("failed to parse dynamic template %s: %w", templateName, err)
}

if !matches {
logger.Debugf(">> Issue: not matches")
matched = false
Expand All @@ -796,10 +849,17 @@ func (v *MappingValidator) matchingWithDynamicTemplates(currentPath string, defi
matched = false
break
}
matches, err := stringMatchesPatterns(values, name, fullRegex)

var matches bool
if fullRegex {
matches, err = stringMatchesPatterns(values, name)
} else {
matches, err = stringMatchesWildcards(values, name)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does match_pattern applies also for unmatch ?

According to this, maybe it just applies for match.
https://www.elastic.co/guide/en/elasticsearch/reference/current/dynamic-templates.html#match-unmatch

WDYT ?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the docs seem to indicate that it only applies to match, but it would be weird that it is not possible to set this option for unmatch. Maybe match_pattern applies to both?

In any case, do we generate templates with these options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the docs seem to indicate that it only applies to match, but it would be weird that it is not possible to set this option for unmatch. Maybe match_pattern applies to both?

According to the tests it looks like it applies to both parameters
https://github.com/elastic/elasticsearch/blob/fe3a3cb70e3dd18b3e5ddaf6a63389b4cb16b776/server/src/test/java/org/elasticsearch/index/mapper/DynamicTemplatesTests.java#L2391-L2397

And also it looks like it could apply to path_match and path_unmatch, lookign at this test
https://github.com/elastic/elasticsearch/blob/75bf27c8b69b7c32fb200afd5a244e7b1f8aea32/x-pack/plugin/core/template-resources/src/main/resources/watch-history.json#L23

Currently, I think that math_pattern parameter is not used. Fleet does not generate that and currently the ECS component template does not use it neither.

if err != nil {
return fmt.Errorf("failed to parse dynamic template %s: %w", templateName, err)
}

if matches {
logger.Debugf(">> Issue: matches")
matched = false
Expand All @@ -810,7 +870,7 @@ func (v *MappingValidator) matchingWithDynamicTemplates(currentPath string, defi
if err != nil {
return fmt.Errorf("failed to check path_match setting: %w", err)
}
matches, err := stringMatchesPatterns(values, currentPath, false)
matches, err := stringMatchesWildcards(values, currentPath)
if err != nil {
return fmt.Errorf("failed to parse dynamic template %s: %w", templateName, err)
}
Expand All @@ -824,7 +884,7 @@ func (v *MappingValidator) matchingWithDynamicTemplates(currentPath string, defi
if err != nil {
return fmt.Errorf("failed to check path_unmatch setting: %w", err)
}
matches, err := stringMatchesPatterns(values, currentPath, false)
matches, err := stringMatchesWildcards(values, currentPath)
if err != nil {
return fmt.Errorf("failed to parse dynamic template %s: %w", templateName, err)
}
Expand Down Expand Up @@ -888,7 +948,7 @@ func (v *MappingValidator) matchingWithDynamicTemplates(currentPath string, defi
}

logger.Debugf(">>> No template matching")
return fmt.Errorf("no template matching for %q", currentPath)
return fmt.Errorf("no template matching for path: %q", currentPath)
}

// validateObjectMappingAndParameters validates the current object or field parameter (currentPath) comparing the values
Expand All @@ -906,8 +966,14 @@ func (v *MappingValidator) validateObjectMappingAndParameters(previewValue, actu
if !ok {
errs = append(errs, fmt.Errorf("unexpected type in actual mappings for path: %q", currentPath))
}
if len(dynamicTemplates) == 0 {
logger.Debugf(">>> Compare mappings map[string]any %s", currentPath)
}
errs = append(errs, v.compareMappings(currentPath, previewField, actualField, dynamicTemplates)...)
case any:
if len(dynamicTemplates) == 0 {
logger.Debugf(">>> Compare mappings any %s", currentPath)
}
// Validate each setting/parameter of the mapping
// If a mapping exist in both preview and actual, they should be the same. But forcing to compare each parameter just in case
if previewValue == actualValue {
Expand Down
4 changes: 3 additions & 1 deletion internal/fields/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ import (
"github.com/elastic/elastic-package/internal/packages/buildmanifest"
)

const externalFieldAppendedTag = "ecs_component"

var (
semver2_0_0 = semver.MustParse("2.0.0")
semver2_3_0 = semver.MustParse("2.3.0")
Expand Down Expand Up @@ -448,7 +450,7 @@ func appendECSMappingMultifields(schema []FieldDefinition, prefix string) []Fiel
{
Name: "text",
Type: "match_only_text",
External: "ecs",
External: externalFieldAppendedTag,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Workaround to be able to filter multifieds generated here during the validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be a better approach for this ?

Copy link
Member

Choose a reason for hiding this comment

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

Do we have many failures if we don't do this? Maybe we can leave this for a separate change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As talked offline, we decided to ignore the validation of these multi-fields.
Added a comment about the reasoning behind this in https://github.com/elastic/elastic-package/pull/2285/files#diff-67b718cebdeb840d99361c07166f75df079827a75df20850ef6cada861304521R318-R322

},
},
},
Expand Down