Skip to content

Stricter validation of data stream settings#587

Merged
jsoriano merged 9 commits intoelastic:mainfrom
jsoriano:validate-data-stream-settings
Sep 11, 2023
Merged

Stricter validation of data stream settings#587
jsoriano merged 9 commits intoelastic:mainfrom
jsoriano:validate-data-stream-settings

Conversation

@jsoriano
Copy link
Member

@jsoriano jsoriano commented Sep 7, 2023

What does this PR do?

Limit the index settings that can be defined in data stream manifests.

Why is it important?

Package compatibility with the stack is going to be controlled by the package spec version. Each version of Kibana will support a range of versions of the spec, so the spec needs to be specific about the features each package can use.

Checklist

  • I have added test packages to test/packages that prove my change is effective.
  • I have added an entry in spec/changelog.yml.
  • I have added a patch so this only applies to v3.

Related issues

@jsoriano
Copy link
Member Author

jsoriano commented Sep 7, 2023

test integrations

@jsoriano
Copy link
Member Author

jsoriano commented Sep 7, 2023

test integrations

@jsoriano jsoriano marked this pull request as ready for review September 7, 2023 16:24
@jsoriano jsoriano requested a review from a team as a code owner September 7, 2023 16:24
Comment on lines +288 to +297
type:
$ref: "./fields/fields.spec.yml#/items/properties/type"
scaling_factor:
$ref: "./fields/fields.spec.yml#/items/properties/scaling_factor"
metrics:
$ref: "./fields/fields.spec.yml#/items/properties/metrics"
default_metric:
$ref: "./fields/fields.spec.yml#/items/properties/default_metric"
ignore_above:
$ref: "./fields/fields.spec.yml#/items/properties/ignore_above"
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at this mapping definition, when import_mappings is true in _dev/build/build.yml in a package, all the mappings defined in this file are added in the built package:
https://github.com/elastic/elastic-package/blob/9d80c6cc282d04f521cd763abd42d529f9679cce/internal/builder/_static/ecs_mappings.yaml#L19

Checking those mappings, at least match, path_match and fields fields are defined under mapping key too. Some examples:

These definitions would be in the built package, and this built package is going to be checked by our publishing CI jobs to validate them before publishing them.

So I guess more fields would be needed here, wouldn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added exception for these fields in d946169.

type: array
items:
type: object
# maxProperties: 1 # No idea why this doesn't work. But this will be also checked by Elasticsearch.
Copy link
Contributor

Choose a reason for hiding this comment

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

Tried to use this value, but I get this error:

could not read root folder spec file: could not load spec for "integration/data_stream/spec.yml": could not load schema for "integration/data_stream": failed to load schema for "integration/data_stream/manifest.spec.yml": maxProperties must be of an integer

I didn't find why it could be detected as a different type than integer :/

Copy link
Member Author

Choose a reason for hiding this comment

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

Workaround added in 2d31fd8

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding this!

@jsoriano jsoriano requested a review from mrodm September 11, 2023 12:06
@elasticmachine
Copy link

💚 Build Succeeded

History

Copy link
Contributor

@mrodm mrodm left a comment

Choose a reason for hiding this comment

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

Thanks!

🚀

@jsoriano jsoriano merged commit 089517f into elastic:main Sep 11, 2023
@jsoriano jsoriano deleted the validate-data-stream-settings branch September 11, 2023 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants