-
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] Rename ingestManager plugin ID fleet #83200
[Fleet] Rename ingestManager plugin ID fleet #83200
Conversation
370adc9
to
fa9537c
Compare
fa9537c
to
a48b2a6
Compare
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.
ES UI code LGTM, didn't test locally.
Pinging @elastic/apm-ui (Team:apm) |
Pinging @elastic/ingest-management (Team:Ingest Management) |
@@ -186,15 +180,15 @@ export class IngestManagerPlugin | |||
if (deps.features) { | |||
deps.features.registerKibanaFeature({ | |||
id: PLUGIN_ID, |
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.
question: @legrego IIRC changing feature ID is a breaking change (we changed the PLUGIN_ID
) and we shouldn't make it in a minor upgrade, but I'm not sure what is our policy regarding features that belong to plugins that are in a beta?
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.
Heya Fleet team:
tl;dr I think we can rename in this specific case, please take the time to read this entire comment to understand the implications.
IIRC changing feature ID is a breaking change (we changed the PLUGIN_ID) and we shouldn't make it in a minor upgrade
That's exactly right. Changing a feature id will change the associated privilege ids that are registered with Elasticsearch on startup. For example, prior to this change, a user may have been assigned a custom role which grants access to Ingest Manager. The role would look something like this:
{
"name": "some-custom-role",
"cluster": [],
"index": [],
"run_as": [],
"applications": [
{
"application": "kibana-.kibana",
"privileges": [
"feature_ingestManager.all"
],
"resources": [
"space:default"
]
}
]
}
The privilege that gets assigned to the role (feature_ingestManager.all
) is derived in part from the feature id. Renaming your feature means that any role which previously granted access to your feature will no longer grant access, because Kibana doesn't have a feature called ingestManager
anymore. The role should instead look like:
{
"name": "some-custom-role",
"cluster": [],
"index": [],
"run_as": [],
"applications": [
{
"application": "kibana-.kibana",
"privileges": [
"feature_fleet.all"
],
"resources": [
"space:default"
]
}
]
}
The problem is that Kibana can't just go and rewrite all of these role definitions. We don't have any sort of migration system in place yet (#68814)
I'm not sure what is our policy regarding features that belong to plugins that are in a beta?
This might be our saving grace. I think we can allow the rename in this specific case, but only because this is a beta feature, and we explicitly tell users not to run this in production yet.
However, this is something that you need to document in the release notes. We have to at least give administrators a fighting chance of restoring access to their users, and not surprise them.
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 will add a release note 👍 ,
I do not think is so problematic here as an user is not able to use the Fleet
plugin if he do not have the superuser
role
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.
Sounds like y'all got all of the angles covered here, but I just wanted to mention that from various sources it looks like people are very positive about Fleet and Elastic Agent, and that Beta testers go out on a limb to try things out and give us early feedback. So as a general rule, I think it's important to have empathy for them and avoid introducing breaking changes that will essentially surprise and punish people who are trying to help us, if at all possible.
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 added the release note
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 tested this locally and didn't manage to break it. 👍
…e-ingest-manager-fleet-plugin
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.
apm changes lgtm
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.
Looks good Nicolas. Thanks for the changes to the Security Solution code 👍
title: i18n.translate('xpack.fleet.oldAppTitle', { defaultMessage: 'Ingest Manager' }), | ||
async mount(params: AppMountParameters) { | ||
const [coreStart] = await core.getStartServices(); | ||
coreStart.application.navigateToApp('fleet', { |
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.
Nice.
So this still show a left-nav (kibana) nav item, correct?
should there be a flag (in settings) to possibly remove it? (I'm thinking of non-existing fleet users that may download/install >= 7.11)
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.
the navlink is hidden with
navLinkStatus: AppNavLinkStatus.hidden,
@elastic/siem I would love to have a review here :) |
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.
LGTM on SIEM changes, thank you!
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsasync chunk count
History
To update your PR or re-run it, just comment with: |
* master: skip "Dashboards linked by a drilldown are both copied to a space" (elastic#83824) [alerts] adds action group and date to mustache template variables for actions (elastic#83195) skip flaky suite (elastic#79389) [DOCS] Reallocates limitations to point-of-use (elastic#79582) [Enterprise Search] Engine overview layout stub (elastic#83756) Disable exporting/importing of templates. Optimize pitch images a bit (elastic#83098) [DOCS] Consolidates plugins (elastic#83712) [ML] Space management UI (elastic#83320) test just part of the message to avoid updates (elastic#83703) [Data Table] Remove extra column in split mode (elastic#83193) Improve snapshot error messages (elastic#83785) skip flaky suite (elastic#83773) skip flaky suite (elastic#83771) skip flaky suite (elastic#65278) skip flaky suite (elastic#83793) [Task Manager] Ensures retries are inferred from the schedule of recurring tasks (elastic#83682) [index patterns] improve index pattern cache (elastic#83368) [Fleet] Rename ingestManager plugin ID fleet (elastic#83200) fixed pagination in connectors list (elastic#83638)
Summary
Last PR on the effort of renaming ingestManager Fleet
Rename
ingestManager
plugin IDfleet
This change the app path from
/app/ingestManager
to/app/fleet
I changed the following plugins accordingly:
Also I renamed all the
IngestManagerPlugin
IngestManagerSetup
, ..., toFleet*
Release note breaking
The
ingestManager
plugin as been renamedfleet
, this mean:/app/ingestManager
to/app/fleet
feature_ingestManager.*
is not valid anymore and should be replaced byfeature_fleet.*