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.

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

Do not require an object type for objects with enabled: false #675

Merged
merged 2 commits into from
Dec 4, 2023

Conversation

jsoriano
Copy link
Member

@jsoriano jsoriano commented Dec 4, 2023

What does this PR do?

Removes requirement of having an object_type for objects with enabled: false.

Why is it important?

Objects with enabled: false disable any parsing of their children elements, they are not indexed and only available on the _source. For these objects it should be possible to don't provide any mapping for subobjects.

For objects with enabled: false and object_type, Fleet will still create a mapping for the matching children objects, and Elasticsearch will still enforce it.

Checklist

Related issues

Objects with `enabled: false` disable any parsing of their children
elements, they are not indexed and only available on the `_source`.
For these objects it should be possible to don't provide any mapping for
subobjects.
@jsoriano jsoriano requested a review from maxcold December 4, 2023 13:26
@jsoriano jsoriano self-assigned this Dec 4, 2023
@jsoriano jsoriano requested a review from a team as a code owner December 4, 2023 13:26
Comment on lines 537 to 544
oneOf:
- required:
- object_type
- properties:
enabled:
const: false
required:
- enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

As it is, it would still be allowed this kind of field definitions:

         - name: user_provided_metadata
           type: object
           object_type: keyword
           enabled: false

Probably it is required to add these other conditions ?
Not tested with any other examples

Suggested change
oneOf:
- required:
- object_type
- properties:
enabled:
const: false
required:
- enabled
oneOf:
- required:
- object_type
properties:
enabled:
const: true
- properties:
enabled:
const: false
required:
- enabled
not:
required:
- object_type

With the example above, it would raise this error:

  1. file "../../../../test/packages/good_v3/elasticsearch/transform/metadata_united/fields/fields.yml" is invalid: field 1.fields.1.fields.24.enabled: 1.fields.1.fields.24.enabled does not match: true

And for this mapping

    - name: user_provided_metadata
      type: object
      enabled: true

it would fail with this error:

  1. file "../../../../test/packages/good_v3/elasticsearch/transform/metadata_united/fields/fields.yml" is invalid: field 1.fields.1.fields.24.enabled: 1.fields.1.fields.24.enabled does not match: false

Copy link
Member Author

Choose a reason for hiding this comment

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

As it is, it would still be allowed this kind of field definitions:

         - name: user_provided_metadata
           type: object
           object_type: keyword
           enabled: false

We have to permit this, at least for 3.0.0-3.0.2, to avoid making a breaking change. There are some packages, at least cloud_security_posture using this now. I opted with the option of being more permissive.

If we consider that is problematic to allow this (it is as we have seen), I could try to add logic to disallow this starting on 3.0.3.

  • name: user_provided_metadata
    type: object
    enabled: true

This mapping should not be allowed in any case and indeed it is with the current code, I will take a look and add a test for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @mrodm, I think your proposal works well. I have added a JSON patch to keep things as they are before 3.0.2, so we only disallow having an object_type with enabled: false starting on 3.0.3, and we don't break current cloud_security posture package.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have to permit this, at least for 3.0.0-3.0.2, to avoid making a breaking change. There are some packages, at least cloud_security_posture using this now. I opted with the option of being more permissive.

Ok, right we need to avoid breaking changes 👍

This mapping should not be allowed in any case and indeed it is with the current code, I will take a look and add a test for this.

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

We have to permit this, at least for 3.0.0-3.0.2, to avoid making a breaking change. There are some packages, at least cloud_security_posture using this now. I opted with the option of being more permissive.

Ok, right we need to avoid breaking changes 👍

This mapping should not be allowed in any case and indeed it is with the current code, I will take a look and add a test for this.

Thanks!

@elasticmachine
Copy link

💚 Build Succeeded

History

cc @jsoriano

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.

LGTM!

@jsoriano jsoriano merged commit ce92645 into elastic:main Dec 4, 2023
3 checks passed
@jsoriano jsoriano deleted the object-type-enabled-false branch December 4, 2023 16:46
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