-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Fleet] Make default integration install explicit #121628
[Fleet] Make default integration install explicit #121628
Conversation
@@ -1382,7 +1384,9 @@ export async function incrementPackageName( | |||
? packagePolicyData.items | |||
.filter((ds) => Boolean(ds.name.match(pkgPoliciesNamePattern))) | |||
.map((ds) => parseInt(ds.name.match(pkgPoliciesNamePattern)![1], 10)) | |||
.sort() | |||
.sort((a: number, b: number) => { | |||
return a - b; |
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.
this fixes a bug, default sorting doesn't work correctly on numbers, e.g. "1, 10, 9"
- this was giving an error when reaching system-10
name in integrations, and the logic returned system-10
again as incremented name
EDIT: changed this to find max number, no need to sort.
try { | ||
if (withSysMonitoring) { |
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.
potential problem:
installing these packages might take long (about 2 mins) which causes network timeout. this results in an error, and fails next agent policy creation request. Could this be improved by bundled default packages?
EDIT: during latest testing, the install took much less time (25s), will keep an eye on this
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.
Bundled default packages aren't going to make much of a difference here based on my investigations in #110500. We'll likely need elastic/elasticsearch#77505 to fix this.
defaultMessage="Collecting monitoring logs and metrics will also create an {agent} integration. Monitoring data will be written to the default namespace specified above." | ||
values={{ | ||
agent: ( | ||
<AgentPolicyPackageBadge pkgName={'elastic_agent'} pkgTitle={'Elastic Agent'} /> |
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.
currently displaying a generic badge. in order to display the package's real badge, I would need to know the latest version number, which seems overkill
Pinging @elastic/fleet (Team:Fleet) |
@@ -546,6 +545,7 @@ export async function createInstallation(options: { | |||
? true | |||
: undefined; | |||
|
|||
// TODO cleanup removable flag and isUnremovablePackage function |
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.
@mostlyjason what about the endpoint package, can it be removable as well?
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.
I don't see why not. Adding @kevinlog for confirmation. For context, we are trying to remove the concept of unremovable packages since they will no longer be installed by default. This would also allow users to reinstall the package if there is a problem. Can you think of any reason users shouldn't be able to remove endpoint, provided they removed all integration policies first?
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.
@mostlyjason @juliaElastic I think it's OK to make the endpoint package removable especially for reasons you stated such as allowing users to uninstall and reinstall if there is a problem.
If the user removed all integration policies first then all of their Endpoints would be uninstalled. I would just like to test what would happen with their data if they uninstalled the package. This would imply that we remove mappings, etc. If we're going to make the Endpoint package removable, I'd like to open up a separate ticket/PR if possible where we can test a few different scenarios before merging/releasing.
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.
@kevinlog thanks for your response, are you comfortable with testing this soon as we are planning to merge this feature for 8.1? If not, should we keep endpoint package unremovable for now?
cc @joshdover
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.
When we uninstall packages, I don't believe any existing data should be affected, though the next rollover of the data stream would cause the write index to pickup the generic mappings for logs-*-*
and metrics-*-*
rather than the endpoint-specific ones. This would only affect new data that is ingested which shouldn't be happening once a package is uninstalled anyways.
// NOTE: we ignore failures in attempting to create package policy, since agent policy might have been created | ||
// successfully | ||
withSysMonitoring | ||
// TODO case when both isDefaultFleetServer and withSysMonitoring is true |
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.
@joshdover is it a valid use case to add system monitoring to default fleet server policy? currently default fleet server policy only includes fleet server integration
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.
I think that is probably a valid use case for preconfiguration / kibana.yml, but isn't possible via the UI today.
Is there a major hurdle in supporting this case?
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.
I thought we are removing the default fleet server policy? I didn't read the code, so am I missing context behind this question?
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.
After reading the comments here and in design doc, it seems that we can completely get rid of the flag to mark a policy default (is_default
and is_default_fleet_server
flags in code). I wasn't aware of this before, probably because on the designs the policies were still called "Default policy" and "Default Fleet Server policy".
@joshdover I think we can support system integration with fleet server policy, I didn't want to start implementing it until confirmed.
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.
Makes sense to me. If we're going to drop the concept of default completely, we should remove that field from our Saved Object mappings and add a Saved Object migration that deletes this field.
@elasticmachine merge upstream |
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.
One last comment otherwise 🚀
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Summary
Making default integration install explicit
Fixes #108456
Manual test cases:
Initial kibana startup:
Add integration flow:
Fleet Server policy flow on-prem:
Add agent flow (supposing Fleet Server is installed):
Cloud (depends on change in cloud stackpack)
Checklist
Delete any items that are not applicable to this PR.
For maintainers