-
Notifications
You must be signed in to change notification settings - Fork 119
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.
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
Fix validations based on fields imported from ECS #1452
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ import ( | |
"fmt" | ||
"io" | ||
"net/http" | ||
"net/url" | ||
"os" | ||
"path/filepath" | ||
"strings" | ||
|
@@ -24,6 +25,7 @@ import ( | |
const ( | ||
ecsSchemaName = "ecs" | ||
gitReferencePrefix = "git@" | ||
localFilePrefix = "file://" | ||
|
||
ecsSchemaFile = "ecs_nested.yml" | ||
ecsSchemaURL = "https://raw.githubusercontent.com/elastic/ecs/%s/generated/ecs/%s" | ||
|
@@ -70,6 +72,15 @@ func loadECSFieldsSchema(dep buildmanifest.ECSDependency) ([]FieldDefinition, er | |
} | ||
|
||
func readECSFieldsSchemaFile(dep buildmanifest.ECSDependency) ([]byte, error) { | ||
if strings.HasPrefix(dep.Reference, localFilePrefix) { | ||
uri, err := url.Parse(dep.Reference) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
return os.ReadFile(uri.Path) | ||
} | ||
|
||
gitReference, err := asGitReference(dep.Reference) | ||
if err != nil { | ||
return nil, fmt.Errorf("can't process the value as Git reference: %w", err) | ||
|
@@ -152,6 +163,10 @@ type InjectFieldsOptions struct { | |
// ECS fields at the top level, when they cannot be reused there. | ||
DisallowReusableECSFieldsAtTopLevel bool | ||
|
||
// IncludeValidationSettings can be set to enable the injection of settings of imported | ||
// fields that are only used for validation of documents, but are not needed on built packages. | ||
IncludeValidationSettings bool | ||
|
||
root string | ||
} | ||
|
||
|
@@ -182,7 +197,7 @@ func (dm *DependencyManager) injectFieldsWithOptions(defs []common.MapStr, optio | |
return nil, false, fmt.Errorf("field %s cannot be reused at top level", fieldPath) | ||
} | ||
|
||
transformed := transformImportedField(imported) | ||
transformed := transformImportedField(imported, options) | ||
|
||
// Allow overrides of everything, except the imported type, for consistency. | ||
transformed.DeepUpdate(def) | ||
|
@@ -295,7 +310,7 @@ func buildFieldPath(root string, field common.MapStr) string { | |
return path | ||
} | ||
|
||
func transformImportedField(fd FieldDefinition) common.MapStr { | ||
func transformImportedField(fd FieldDefinition, options InjectFieldsOptions) common.MapStr { | ||
m := common.MapStr{ | ||
"name": fd.Name, | ||
"type": fd.Type, | ||
|
@@ -318,17 +333,28 @@ func transformImportedField(fd FieldDefinition) common.MapStr { | |
m["doc_values"] = *fd.DocValues | ||
} | ||
|
||
if len(fd.Normalize) > 0 { | ||
m["normalize"] = fd.Normalize | ||
} | ||
|
||
if len(fd.MultiFields) > 0 { | ||
var t []common.MapStr | ||
for _, f := range fd.MultiFields { | ||
i := transformImportedField(f) | ||
i := transformImportedField(f, options) | ||
t = append(t, i) | ||
} | ||
m.Put("multi_fields", t) | ||
} | ||
|
||
if options.IncludeValidationSettings { | ||
if len(fd.Normalize) > 0 { | ||
m["normalize"] = fd.Normalize | ||
} | ||
|
||
if len(fd.AllowedValues) > 0 { | ||
m["allowed_values"] = fd.AllowedValues | ||
} | ||
|
||
if len(fd.ExpectedValues) > 0 { | ||
m["expected_values"] = fd.ExpectedValues | ||
} | ||
} | ||
Comment on lines
+340
to
+352
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This means that these fields (normalize, allowed_values, expected_values) won't be added to each field definition when the package are built, is that right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is not used in Fleet or Elasticsearch. |
||
|
||
return m | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,11 +5,15 @@ | |
package fields | ||
|
||
import ( | ||
"encoding/json" | ||
"path/filepath" | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
"github.com/stretchr/testify/require" | ||
|
||
"github.com/elastic/elastic-package/internal/common" | ||
"github.com/elastic/elastic-package/internal/packages/buildmanifest" | ||
) | ||
|
||
func TestDependencyManagerInjectExternalFields(t *testing.T) { | ||
|
@@ -230,6 +234,9 @@ func TestDependencyManagerInjectExternalFields(t *testing.T) { | |
"external": "test", | ||
}, | ||
}, | ||
options: InjectFieldsOptions{ | ||
IncludeValidationSettings: true, | ||
}, | ||
result: []common.MapStr{ | ||
{ | ||
"name": "host.ip", | ||
|
@@ -607,3 +614,117 @@ func TestDependencyManagerInjectExternalFields(t *testing.T) { | |
}) | ||
} | ||
} | ||
|
||
func TestDependencyManagerWithECS(t *testing.T) { | ||
const ecsNestedPath8_10_0 = "./testdata/ecs_nested_v8.10.0.yml" | ||
ecsPath, _ := filepath.Abs(ecsNestedPath8_10_0) | ||
deps := buildmanifest.Dependencies{ | ||
ECS: buildmanifest.ECSDependency{ | ||
Reference: "file://" + ecsPath, | ||
}, | ||
} | ||
dm, err := CreateFieldDependencyManager(deps) | ||
require.NoError(t, err) | ||
|
||
cases := []struct { | ||
title string | ||
defs []common.MapStr | ||
result []common.MapStr | ||
options InjectFieldsOptions | ||
checkFn func(*testing.T, []common.MapStr) | ||
valid bool | ||
}{ | ||
{ | ||
title: "disallowed reusable field at lop level", | ||
defs: []common.MapStr{ | ||
{ | ||
"name": "geo.city_name", | ||
"external": "ecs", | ||
}, | ||
}, | ||
options: InjectFieldsOptions{ | ||
DisallowReusableECSFieldsAtTopLevel: true, | ||
}, | ||
valid: false, | ||
}, | ||
{ | ||
title: "legacy support to reuse field at lop level", | ||
defs: []common.MapStr{ | ||
{ | ||
"name": "geo.city_name", | ||
"external": "ecs", | ||
}, | ||
}, | ||
options: InjectFieldsOptions{ | ||
DisallowReusableECSFieldsAtTopLevel: false, | ||
}, | ||
result: []common.MapStr{ | ||
{ | ||
"name": "geo.city_name", | ||
"description": "City name.", | ||
"type": "keyword", | ||
}, | ||
}, | ||
valid: true, | ||
}, | ||
{ | ||
title: "allowed values are injected for validation", | ||
defs: []common.MapStr{ | ||
{ | ||
"name": "event.type", | ||
"external": "ecs", | ||
}, | ||
}, | ||
options: InjectFieldsOptions{ | ||
IncludeValidationSettings: true, | ||
}, | ||
valid: true, | ||
checkFn: func(t *testing.T, result []common.MapStr) { | ||
require.Len(t, result, 1) | ||
_, ok := result[0]["allowed_values"] | ||
if !assert.True(t, ok) { | ||
d, _ := json.MarshalIndent(result[0], "", " ") | ||
t.Logf("expected to find allowed_values in %s", string(d)) | ||
} | ||
}, | ||
Comment on lines
+680
to
+687
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
}, | ||
{ | ||
title: "allowed values are not injected when not intended for validation", | ||
defs: []common.MapStr{ | ||
{ | ||
"name": "event.type", | ||
"external": "ecs", | ||
}, | ||
}, | ||
options: InjectFieldsOptions{ | ||
IncludeValidationSettings: false, | ||
}, | ||
valid: true, | ||
checkFn: func(t *testing.T, result []common.MapStr) { | ||
require.Len(t, result, 1) | ||
_, ok := result[0]["allowed_values"] | ||
assert.False(t, ok) | ||
}, | ||
}, | ||
} | ||
|
||
for _, c := range cases { | ||
t.Run(c.title, func(t *testing.T) { | ||
result, _, err := dm.InjectFieldsWithOptions(c.defs, c.options) | ||
if !c.valid { | ||
assert.Error(t, err) | ||
return | ||
} | ||
|
||
assert.NoError(t, err) | ||
if len(c.result) > 0 { | ||
assert.EqualValues(t, c.result, result) | ||
} | ||
if c.checkFn != nil { | ||
t.Run("checkFn", func(t *testing.T) { | ||
c.checkFn(t, result) | ||
}) | ||
} | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving this to the if below will effectively remove
normalize
from the built packages. But I think this is not needed there? 🤔I could revert this change if we consider it risky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this field is not used afterwards by Elasticsearch, I think it could be moved to that new
if
block.And IIRC , this function is just executed while doing comparing documents (testing) with external fields. In built packages, I guess it's not needed.