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

Jump to bottom

[Fleet] Fix datastream settings on package upgrade and package policy creation #147368

Merged
merged 14 commits into from
Dec 14, 2022
Merged

[Fleet] Fix datastream settings on package upgrade and package policy creation #147368

merged 14 commits into from
Dec 14, 2022

Conversation

nchaulet
Copy link
Member

Description

Related to #147034

That PR fix a few bugs with the synthetic source experimental datastream feature:

  • we are not preserving the already set experimental features when upgrading a package => this is now fixed in the prepareTemplate function where we pass the existing datastream features
  • we were not loading correctly the already set experimental feature when creating a new package policy => this is now fixed in the packageToPackagePolicy function
  • we were mutating the original package policy object when editing experimental feature, the "save integration" button was
    disabled because of this as the original package policy and the updated one are not different

testing

There is no package using the source_mode: synthetic feature for now but you can edit locally a package or use this test package https://github.com/elastic/kibana/tree/main/x-pack/test/fleet_api_integration/apis/fixtures/test_packages/non_epr_fields/1.0.0

All the changes are covered by unit tests

@nchaulet nchaulet added bug Fixes for quality problems that affect the customer experience Team:Fleet Team label for Observability Data Collection Fleet team v8.7.0 labels Dec 12, 2022
@nchaulet nchaulet requested a review from a team as a code owner December 12, 2022 16:26
@nchaulet nchaulet self-assigned this Dec 12, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@nchaulet nchaulet added the release_note:skip Skip the PR/issue when compiling release notes label Dec 12, 2022
@nchaulet
Copy link
Member Author

@elasticmachine merge upstream

@nchaulet
Copy link
Member Author

@elasticmachine merge upstream

@nchaulet
Copy link
Member Author

@elasticmachine merge upstream

hop-dev
hop-dev approved these changes Dec 13, 2022
View reviewed changes
Copy link
Contributor

@hop-dev hop-dev left a comment

Choose a reason for hiding this comment

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

Great, thanks for all the tests 👍

@nchaulet
Copy link
Member Author

@shahzad31 this test is always failing but it does seem to be related to any of my change here, and I was not able to get it failing locally apis synthetics CRUD routes DeleteProjectMonitors deletes integration policies when project monitors are deleted

@dominiqueclarke dominiqueclarke self-requested a review December 13, 2022 15:04
@nchaulet
Copy link
Member Author

@elasticmachine merge upstream

@elastic elastic deleted a comment from kibana-ci Dec 13, 2022
@nchaulet nchaulet force-pushed the fix-datastream-settings branch from 8eb7595 to ef2484e Compare December 13, 2022 16:08
@nchaulet nchaulet closed this Dec 13, 2022
@nchaulet nchaulet reopened this Dec 13, 2022
@nchaulet
Copy link
Member Author

@elasticmachine merge upstream

@elastic elastic deleted a comment from kibana-ci Dec 13, 2022
@nchaulet
Copy link
Member Author

@elasticmachine merge upstream

@nchaulet
Copy link
Member Author

@elasticmachine merge upstream

@nchaulet nchaulet force-pushed the fix-datastream-settings branch from 652a0f6 to 2531462 Compare December 13, 2022 23:01
@nchaulet nchaulet force-pushed the fix-datastream-settings branch from 2531462 to 08195b3 Compare December 13, 2022 23:11
@nchaulet nchaulet enabled auto-merge (squash) December 14, 2022 01:49
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
fleet 897.6KB 897.6KB +52.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
fleet 120.5KB 120.7KB +198.0B
Unknown metric groups

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 60 66 +6
osquery 109 115 +6
securitySolution 445 451 +6
total +20

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 69 75 +6
osquery 110 117 +7
securitySolution 521 527 +6
total +21

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @nchaulet

@nchaulet nchaulet merged commit 5a9c04b into elastic:main Dec 14, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Dec 14, 2022
@nchaulet nchaulet mentioned this pull request Dec 14, 2022
Merged
@ruflin
Copy link
Contributor

ruflin commented Dec 14, 2022

There is no package using the source_mode: synthetic feature for now

@nchaulet As you likely understand I'm slightly concern by this. We should have tests in place that the issues found in #147034 (comment) are fixed.

@nchaulet
Copy link
Member Author

We should have tests in place that the issues found in #147034 (comment) are fixed.

@ruflin I added unit tests that cover the changes here

@ruflin
Copy link
Contributor

ruflin commented Dec 15, 2022

In #147034 (comment) I left a question around the following scenario:

  • Synthetic source is enabled in the package
  • Data starts to ship to it and synthetic source is actively disabled
  • User creates a second policy with the same integration. What does the toggle in the policy show now? It should be disable because that is the setting of the data stream that already exists and must overwrite the package default.
  • Same scenario with a different namespace, what happens now?

Can you comment on what the expected behaviour after this PR is? Especially interested in your thoughts around namespaces.

nreese pushed a commit to nreese/kibana that referenced this pull request Dec 16, 2022
@nchaulet @nreese
[Fleet] Fix datastream settings on package upgrade and package policy…
4dc3f94 
… creation (elastic#147368)
@ruflin
Copy link
Contributor

ruflin commented Dec 16, 2022

I did test this again on the most recent -SNAPSHOT builds from today. The setting on the data_stream itself is correctly set including the template. But when I go to the UI, I see the following:

Screenshot 2022-12-16 at 16 06 28

@nchaulet
Copy link
Member Author

I did test this again on the most recent -SNAPSHOT builds from today. The setting on the data_stream itself is correctly set including the template. But when I go to the UI, I see the following:

@ruflin Does your package policy version is up to date with the system package installed? I think I may have found an issue here

@ruflin
Copy link
Contributor

ruflin commented Dec 19, 2022

Not sure I fully understand. For testing I use this PR: elastic/integrations#4749 Then the elastic-package build and run elastic-package stack up --version=8.7.0-SNAPSHOT from the integrations directory. I made some changes on my end to the yaml to ensure the package installed (or at least the policy I see) is the one from the package itself.

@ruflin ruflin mentioned this pull request Dec 23, 2022
Closed
@nchaulet nchaulet deleted the fix-datastream-settings branch January 3, 2023 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hop-dev hop-dev hop-dev approved these changes

@dominiqueclarke dominiqueclarke Awaiting requested review from dominiqueclarke

Assignees

@nchaulet nchaulet

Labels
backport:skip This commit does not require backporting bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v8.7.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@nchaulet @elasticmachine @kibana-ci @ruflin @hop-dev @kibanamachine @dominiqueclarke