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

Disable legacy url preflights logic when not required #112492

Closed
LeeDr opened this issue Sep 16, 2021 · 12 comments
Closed

Disable legacy url preflights logic when not required #112492

LeeDr opened this issue Sep 16, 2021 · 12 comments
Labels
discuss Feature:Saved Objects Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!

Comments

@LeeDr
Copy link
Contributor

LeeDr commented Sep 16, 2021

Describe the feature: There's ongoing work related to "Prevent creating saved object legacy URL conflicts". Without going into the details of that work, it seems like it might be very useful if we kept track of the "initial version" of a Kibana instance somehow. My first thought is that it would be added to the config doc in the .kibana index, but there could be better alternatives.

Describe a specific use case for the feature: The use case would be that if a new Kibana instance was created on version 8.+, the code could know that there weren't any legacy URL alias conflicts and could potentially save an Elasticsearch query every time a saved object was opened. Reference: https://www.elastic.co/guide/en/kibana/master/sharing-saved-objects.html#sharing-saved-objects-faq-resolve-outcomes

Slight complication if I create a 8+ Kibana instance and then import saved objects from a previous version. Since we never tracked this before, I think we would just have to remove the initialVersion from the config doc and fall back to the behavior when we don't know what version we started with.

We currently have some fields in the config docs like this (from 7.15.0);

coreMigrationVersion: 7.15.0
migrationVersion.config 7.13.0

We actually have a config doc per space. But there's always a default space config. Maybe that's all we need.

//cc @jportner

@botelastic botelastic bot added the needs-team Issues missing a team label label Sep 16, 2021
@jportner
Copy link
Contributor

Describe a specific use case for the feature: The use case would be that if a new Kibana instance was created on version 8.+, the code could know that there weren't any legacy URL alias conflicts and could potentially save an Elasticsearch query every time a saved object was opened.

In this situation, we could also skip querying for conflict scenarios before create / bulkCreate (planned in a new RFC but not implemented yet).

Slight complication if I create a 8+ Kibana instance and then import saved objects from a previous version.

Actually that's not an issue, saved objects are only "converted" (with new IDs and aliases) when Kibana is upgraded. Importing old saved objects won't have that effect.


There is a risk that if we store this info in the config saved object, if that object gets deleted for any reason, then Kibana will recreate it and start behaving differently. Right now the only use case for different behavior is legacy URL aliases, but if we add other use cases this could potentially wind up having a lot of impact on a user. I wonder if it would be better if we wanted to do this to use a different hidden, space-agnostic saved object type to store this information. I'm curious to see what @elastic/kibana-core thinks.

Another option, if we didn't want to store this info in a saved object: Kibana could query Elasticsearch for any legacy URL aliases on startup, and if it doesn't find any, it could flip a switch to skip the aforementioned queries. Just spitballing 😄

@jportner jportner added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Sep 17, 2021
@botelastic botelastic bot removed the needs-team Issues missing a team label label Sep 17, 2021
@rudolf
Copy link
Contributor

rudolf commented Sep 20, 2021

One risk I see with this is that in development we have a different performance profile from what most of our 7.x to 8.x users would experience. But we could always switch on conflict detection in development mode.

I like that this gives us a narrative to encourage users to remove aliases "To improve the performance of Kibana, consider deleting the following legacy url aliases which haven't been used in the last 90 days."

Since a legacy url alias "targets" an object in a specific namespace, I think we might be able to make this behaviour more granular and only apply conflict detection to namespaces which have legacy url aliases pointing to them?

This shouldn't be something a user can configure, and ideally it would automatically be switched on once a cluster (or namespace?) no longer has any legacy url aliases. So using a startup query sounds like the best way to achieve that, if a user deletes all url aliases and restarts Kibana conflict detection will automatically be switched off.

The only downside is that it's not so easy to inspect whether a given Kibana is using conflict detection or not, so we should probably have an info log message on startup to help us should be ever have to debug an issue with a user.

@pgayvallet
Copy link
Contributor

the code could know that there weren't any legacy URL alias conflicts and could potentially save an Elasticsearch query every time a saved object was opened

Which exact checks do we plan to disable here if the (TBD) criteria match? I'm guessing all legacy alias related checks? so the ones performed during resolve +bulkResolve, but also (not implemented yet) for create/bulkCreate?

Kibana could query Elasticsearch for any legacy URL aliases on startup, and if it doesn't find any, it could flip a switch to skip the aforementioned queries

++, Seems more solid to me than relying on a static value stored on some doc.

Since a legacy url alias "targets" an object in a specific namespace, I think we might be able to make this behaviour more granular and only apply conflict detection to namespaces which have legacy url aliases pointing to them?

We got APIs potentially targeting multiple spaces (e.g you can bulkCreate objects with distinct initialNamespaces). If this would make the preflight check implementation too complex, I would probably avoid doing this per-namespace.

@pgayvallet
Copy link
Contributor

As a side note, we should probably rename the issue to reflect the real problem. Disable legacy url preflights logic when not required?

@LeeDr
Copy link
Contributor Author

LeeDr commented Sep 21, 2021

Kibana could query Elasticsearch for any legacy URL aliases on startup, and if it doesn't find any, it could flip a switch to skip the aforementioned queries

This has the added advantage of disabling the alias checks in Kibana instances which did start on an earlier version but didn't use spaces and therefor didn't create any URL aliases.

@LeeDr LeeDr changed the title Add initialVersion to config saved object Disable legacy url preflights logic when not required Sep 21, 2021
@pgayvallet
Copy link
Contributor

@rudolf @jportner Do you think the perf improvement is significant enough that we want to prioritize this for early 8.x?

@rudolf
Copy link
Contributor

rudolf commented Sep 29, 2021

At this point it's just an uninformed guess, but my guess is that most plugins and most users won't see a significant performance benefit. However, heavy users of performance critical plugins like alerting/task manager might and having an escape hatch for them would reduce the potential impact.

So I think we should start by trying to quantify if it would introduce a performance regression for alerting as a kind of case study to understand how much of a difference it could make to other areas.

@jportner
Copy link
Contributor

At this point it's just an uninformed guess, but my guess is that most plugins and most users won't see a significant performance benefit. However, heavy users of performance critical plugins like alerting/task manager might and having an escape hatch for them would reduce the potential impact.

So I think we should start by trying to quantify if it would introduce a performance regression for alerting as a kind of case study to understand how much of a difference it could make to other areas.

Agreed.

@pgayvallet pgayvallet added the Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! label Jul 5, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

@pgayvallet
Copy link
Contributor

cc-ing to security, do you think this is something that would still make sense today, or shall we just close this issue?

@azasypkin
Copy link
Member

cc-ing to security, do you think this is something that would still make sense today, or shall we just close this issue?

It looks like we never actually tried to quantify the performance benefits of these improvements, which means they were never prioritized due to a lack of demand. On the security side, I think it's fine to close this issue and reopen it if it ever becomes a problem worth solving.

@rayafratkina
Copy link
Contributor

We have not seen cases where this improvement would be needed. Closing for now since we are not planning to address.

@rayafratkina rayafratkina closed this as not planned Won't fix, can't repro, duplicate, stale Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Feature:Saved Objects Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more!
Projects
None yet
Development

No branches or pull requests

7 participants