Skip to content
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.

Sign up for GitHub

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

Validate fields using ECS JSON schema when all stack versions support ecs@mappings #1711

Merged
merged 17 commits into from
Mar 18, 2024
Merged

Validate fields using ECS JSON schema when all stack versions support ecs@mappings #1711

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
4395eb3
Load JSON schema for ECS when package-spec >= 3.1.3
zmoog Mar 5, 2024
91c9085
Continue support import mappings (deprecated only)
zmoog Mar 6, 2024
02c8e9c
Cleanup
zmoog Mar 6, 2024
96f3418
Trying to infer if all stack versions support ECS
zmoog Mar 7, 2024
edc0bf9
Use all ES releases < 8.13.0 to infer ECS support
zmoog Mar 7, 2024
1eb3df1
Fix linter objections (sorry pal, you're right!)
zmoog Mar 7, 2024
bb16985
Update internal/fields/validate.go
zmoog Mar 8, 2024
bb05a75
Remove stack versions that predate Fleet GA
zmoog Mar 8, 2024
cf82a7d
Pre-parse all stack versions and move them out fn
zmoog Mar 8, 2024
e279d80
Reverse stack version order to improve efficiency
zmoog Mar 8, 2024
cb9f689
Update internal/fields/validate_test.go
zmoog Mar 8, 2024
e8f0bae
Move ECS support checking in dedicated function
zmoog Mar 8, 2024
151c527
Add upcoming 7.17.x patch releases
zmoog Mar 8, 2024
f15fd18
Add package to test ecs@mappings support
zmoog Mar 8, 2024
f0a1ee1
Apply suggestions from code review
zmoog Mar 13, 2024
15f2529
Log on importing JSON schema from external source
zmoog Mar 13, 2024
9e333ac
Move log message to the right place
zmoog Mar 13, 2024
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
2 changes: 1 addition & 1 deletion internal/builder/dynamic_mappings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ func addDynamicMappings(packageRoot, destinationDir string) error {
if err != nil {
return err
}
os.WriteFile(packageManifest, contents, 0664)
err = os.WriteFile(packageManifest, contents, 0664)
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion internal/fields/dependency_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ func (dm *DependencyManager) importField(schemaName, fieldPath string) (FieldDef
return *imported, nil
}

// ImportAllFields method resolves all fields avaialble in the default ECS schema.
// ImportAllFields method resolves all fields available in the default ECS schema.
func (dm *DependencyManager) ImportAllFields(schemaName string) ([]FieldDefinition, error) {
if dm == nil {
return []FieldDefinition{}, fmt.Errorf(`importing all external fields: external fields not allowed because dependencies file "_dev/build/build.yml" is missing`)
Expand Down
211 changes: 210 additions & 1 deletion internal/fields/validate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,8 +215,38 @@ func initDependencyManagement(packageRoot string, specVersion semver.Version, im
return nil, nil, fmt.Errorf("can't create field dependency manager: %w", err)
}

//
// Check if the package embeds ECS mappings
//
zmoog marked this conversation as resolved.
Show resolved Hide resolved
packageEmbedsEcsMappings := buildManifest.ImportMappings() && !specVersion.LessThan(semver2_3_0)
if !packageEmbedsEcsMappings {
logger.Debugf("Package does not embed ECS mappings")
}

//
// Check if the stack version includes ECS mappings
//
stackIncludesEcsMapping := false

packageManifest, err := packages.ReadPackageManifestFromPackageRoot(packageRoot)
if err != nil {
return nil, nil, fmt.Errorf("can't read package manifest: %w", err)
}

if len(packageManifest.Conditions.Kibana.Version) > 0 {
kibanaConstraints, err := semver.NewConstraint(packageManifest.Conditions.Kibana.Version)
if err != nil {
return nil, nil, fmt.Errorf("invalid constraint for Kibana: %w", err)
}
stackIncludesEcsMapping = allVersionsIncludeECS(kibanaConstraints)
}
zmoog marked this conversation as resolved.
Show resolved Hide resolved

// If the package embeds ECS mappings, or the stack version includes ECS mappings, then
// we should import the ECS schema to validate the package fields against it.
var schema []FieldDefinition
if buildManifest.ImportMappings() && !specVersion.LessThan(semver2_3_0) && importECSSchema {
if (packageEmbedsEcsMappings || stackIncludesEcsMapping) && importECSSchema {
Copy link
Member

Choose a reason for hiding this comment

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

Please, add or modify a test package so we have a test case with a package that uses ECS fields, but doesn't embeds the mappings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want to test the scenario where the package uses the ECS in Elasticsearch, we need a stack version 8.13.0.

@mrodm, which version of the stack the tests run on? Can we have stack version 8.13.0 (for all tests or just one)?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mrodm, which version of the stack the tests run on? Can we have stack version 8.13.0 (for all tests or just one)?

Currently, the steps in the CI to test packages run with the default version. IIRC that version now should be 8.12.0.

If this needs to be changed, it needs to be updated scripts/test-check-packages.sh to allow define --version flag in the elastic-package stack up command through an environment variable or so. This will require also update

In this implementation, the ECS fields are imported before validation by the test runners (e.g. kibana version condition is ^8.13.0), so there should not be errors related to field definitions. Is it required to start an Elastic stack ^8.13.0 too? @zmoog

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this implementation, the ECS fields are imported before validation by the test runners (e.g. kibana version condition is ^8.13.0), so there should not be errors related to field definitions. Is it required to start an Elastic stack ^8.13.0 too?

Correct!

elastic-package validates the ECS fields using the JSON schema during the static checks, so it does not need to run on Elastic stack ^8.13.0.

// Import all fields from external schema (most likely ECS) to
// validate the package fields against it.
ecsSchema, err := fdm.ImportAllFields(defaultExternal)
if err != nil {
return nil, nil, err
Expand All @@ -227,6 +257,185 @@ func initDependencyManagement(packageRoot string, specVersion semver.Version, im
return fdm, schema, nil
}

// allVersionsIncludeECS Check if all the stack versions in the constraints include ECS mappings. Only the stack
// versions 8.13.0 and above include ECS mappings.
//
// Returns true if all the stack versions in the constraints include ECS mappings, otherwise returns false.
func allVersionsIncludeECS(kibanaConstraints *semver.Constraints) bool {
// List of Elasticsearch releases that do not include ECS mappings (all versions before 8.13.0).
versionsStr := []string{
zmoog marked this conversation as resolved.
Show resolved Hide resolved
"6.8.1",
"6.8.10",
zmoog marked this conversation as resolved.
Show resolved Hide resolved
"6.8.11",
"6.8.12",
"6.8.13",
"6.8.14",
"6.8.15",
"6.8.16",
"6.8.17",
"6.8.18",
"6.8.19",
"6.8.2",
"6.8.20",
"6.8.21",
"6.8.22",
"6.8.23",
"6.8.3",
"6.8.4",
"6.8.5",
"6.8.6",
"6.8.7",
"6.8.8",
"6.8.9",
"7.10.0",
"7.10.1",
"7.10.2",
"7.11.0",
"7.11.1",
"7.11.2",
"7.12.0",
"7.12.1",
"7.13.0",
"7.13.1",
"7.13.2",
"7.13.3",
"7.13.4",
"7.14.0",
zmoog marked this conversation as resolved.
Show resolved Hide resolved
"7.14.1",
"7.14.2",
"7.15.0",
"7.15.1",
"7.15.2",
"7.16.0",
"7.16.1",
"7.16.2",
"7.16.3",
"7.17.0",
"7.17.1",
"7.17.10",
"7.17.11",
"7.17.12",
"7.17.13",
"7.17.14",
"7.17.15",
"7.17.16",
"7.17.17",
"7.17.18",
"7.17.2",
"7.17.3",
"7.17.4",
"7.17.5",
"7.17.6",
"7.17.7",
"7.17.8",
"7.17.9",
"7.2.0",
"7.2.1",
"7.3.0",
"7.3.1",
"7.3.2",
"7.4.0",
"7.4.1",
"7.4.2",
"7.5.0",
"7.5.1",
"7.5.2",
"7.6.0",
"7.6.1",
"7.6.2",
"7.7.0",
"7.7.1",
"7.8.0",
"7.8.1",
"7.9.0",
"7.9.1",
"7.9.2",
"7.9.3",
"8.0.0",
"8.0.1",
"8.1.0",
"8.1.1",
"8.1.2",
"8.1.3",
"8.10.0",
"8.10.1",
"8.10.2",
"8.10.3",
"8.10.4",
"8.11.0",
"8.11.1",
"8.11.2",
"8.11.3",
"8.11.4",
"8.12.0",
"8.12.1",
"8.12.2",
"8.2.0",
zmoog marked this conversation as resolved.
Show resolved Hide resolved
"8.2.1",
"8.2.2",
"8.2.3",
"8.3.0",
"8.3.1",
"8.3.2",
"8.3.3",
"8.4.0",
"8.4.1",
"8.4.2",
"8.4.3",
"8.5.0",
"8.5.1",
"8.5.2",
"8.5.3",
"8.6.0",
"8.6.1",
"8.6.2",
"8.7.0",
"8.7.1",
"8.8.0",
"8.8.1",
"8.8.2",
"8.9.0",
"8.9.1",
"8.9.2",
}

// Looking for a version that satisfies the package constraints.
for _, vStr := range versionsStr {
v, err := semver.NewVersion(vStr)
if err != nil {
logger.Errorf("invalid stack version %q: %v", vStr, err)
continue
}

if kibanaConstraints.Check(v) {
// Found a version that satisfies the constraints,
// so at least this version does not include
// ECS mappings.
return false
}
}

// If no version satisfies the constraints, then all versions
// include ECS mappings.
return true

// This check works under the assumption the constraints are not limited
// upwards.
//
// For example, if the constraint is `>= 8.12.0` and the stack version is
// `8.12.999`, the constraint will be satisfied.
//
// However, if the constraint is `>= 8.0.0, < 8.10.0` the check will not
// return the right result.
//
// To support this, we would need to check the constraint against a larger
// set of versions, and check if the constraint is satisfied for all
// of them, like in the commented out example above.
//
// lastStackVersionWithoutEcsMappings := semver.MustParse("8.12.999")
// return !kibanaConstraints.Check(lastStackVersionWithoutEcsMappings)
Comment on lines +372 to +386
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a simpler implementation that checks the Kibana constraints agains the latest known stack version that does NOT support ecs@mappings.

This check works under the assumption the constraints are not limited upwards (for example, ^8.10.0 or ^7.14.0 || ^8.0.0).

I plan to remove at the end of the review process, but I want to mention it as an option.

Copy link
Member

Choose a reason for hiding this comment

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

This option is actually not bad, but yeah, let's go with the other one that looks safer.

}

//go:embed _static/allowed_geo_ips.txt
var allowedGeoIPs string

Expand Down
50 changes: 50 additions & 0 deletions internal/fields/validate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -961,6 +961,56 @@ func TestValidateExternalMultiField(t *testing.T) {
require.Empty(t, errs)
}

func TestValidateStackVersionsWithEcsMappings(t *testing.T) {
// List of unique stack constraints extracted from the
// package manifest files in the elastic/integrations
// repository.
constraints := []struct {
Constraints string
SupportEcs bool
}{
zmoog marked this conversation as resolved.
Show resolved Hide resolved
{"^7.14.0 || ^8.0.0", false},
{"^7.14.1 || ^8.0.0", false},
{"^7.14.1 || ^8.8.0", false},
{"^7.16.0 || ^8.0.0", false},
{"^7.17.0 || ^8.0.0", false},
{"^8.0.0", false},
{"^8.10.1", false},
{"^8.10.2", false},
{"^8.11.0", false},
{"^8.11.2", false},
{"^8.12.0", false},
{"^8.12.1", false},
{"^8.12.2", false},
{"^8.13.0", true},
{"^8.14.0", true},
{"^8.2.0", false},
{"^8.2.1", false},
{"^8.3.0", false},
{"^8.4.0", false},
{"^8.5.0", false},
{"^8.5.1", false},
{"^8.6.0", false},
{"^8.6.1", false},
{"^8.7.0", false},
{"^8.7.1", false},
{"^8.8.0", false},
{"^8.8.1", false},
{"^8.8.2", false},
{"^8.9.0", false},
{">= 8.0.0, < 8.10.0", false},
{">= 8.0.0, < 8.0.1", false},
}

for _, c := range constraints {
constraint, err := semver.NewConstraint(c.Constraints)
if err != nil {
require.NoError(t, err)
}
require.Equal(t, c.SupportEcs, allVersionsIncludeECS(constraint), "constraint: %s", c.Constraints)
zmoog marked this conversation as resolved.
Show resolved Hide resolved
}
}

func readTestResults(t *testing.T, path string) (f results) {
c, err := os.ReadFile(path)
require.NoError(t, err)
Expand Down
2 changes: 1 addition & 1 deletion internal/packages/buildmanifest/build_manifest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func (bm *BuildManifest) HasDependencies() bool {
return bm.Dependencies.ECS.Reference != ""
}

// HasDependencies function checks if there are any dependencies defined.
// ImportMappings function checks if there are any dependencies defined.
func (bm *BuildManifest) ImportMappings() bool {
return bm.Dependencies.ECS.ImportMappings
}
Expand Down