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

Fail saved object migrations when encountering embeddable state from an unknown embeddable factory #117656

Closed
rudolf opened this issue Nov 5, 2021 · 5 comments
Labels
Feature:Embedding Embedding content via iFrame impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:medium Medium Level of Effort Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas

Comments

@rudolf
Copy link
Contributor

rudolf commented Nov 5, 2021

When a saved object contains persistable state from an unknown embeddable, the persisted state won't be migrated when Kibana gets upgraded.

This is because embeddables.getEmbeddableFactory returns an id: 'unknown' factor with migrations: {} when an embeddable isn't known https://github.com/elastic/kibana/blob/main/src/plugins/embeddable/server/plugin.ts#L139-L147

Even if the embeddable is later registered again, the migration might not run because the saved objects have already been marked as being up to date. I think the embeddable state might be migrated once it's loaded on the browser, but this means that extractReferences was potentially skipped and that because of _id regeneration the references in the embeddable might contain the old _id's.

In #107678 we decided that starting from 8.0 Kibana will fail to upgrade if there are saved objects of an unknown type to prevent silent data loss with _id regeneration.

It feels like the same reasoning could be applied to persistable state too.

If implemented this would enable a use case where a customer uses Kibana Cloud (because it's required for APM/Fleet), but has a on-prem Kibana with a custom plugin that registers an embeddable with extractReferences or migrations.

@Dosant
Copy link
Contributor

Dosant commented Nov 9, 2021

@elastic/kibana-app-services

@Dosant Dosant added the Feature:Embedding Embedding content via iFrame label Nov 9, 2021
@exalate-issue-sync exalate-issue-sync bot added impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. loe:small Small Level of Effort labels Nov 18, 2021
@petrklapka petrklapka added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas and removed Team:AppServicesSv labels Dec 12, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@ThomThomson ThomThomson added triage_needed impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. and removed impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. triage_needed labels Dec 12, 2022
@ThomThomson
Copy link
Contributor

@rudolf, do you know of any clients that this applies to? I haven't heard of any cases where clients have built a custom plugin that registers a new type of embeddable which has been around long enough / is stable enough to require migrations.

While it does seem like the right thing to do to throw an error in this case - after all, unknown embeddable types are potentially a risk - but knowing that a client is actually bound to encounter this issue will increase the estimated impact.

@ThomThomson ThomThomson added loe:medium Medium Level of Effort and removed loe:small Small Level of Effort labels Dec 15, 2022
@rudolf
Copy link
Contributor Author

rudolf commented Dec 19, 2022

We know of customers running custom plugins in an on-prem Kibana that connects to Cloud. But we don't know that any of these plugins use embeddables.

We're also planning to do #147445 which would mean embeddables are lazily migrated and would likely require us to separate the saved objects migrationVersion and the embeddable's migrationVersion. I think this work might solve this problem.

Overall though, without data that custom plugins registers embeddables I agree this doesn't feel like a high priority.

@ThomThomson
Copy link
Contributor

Closing this for the time being. We will be moving towards a client-side embeddable migration system that will make this obsolete.

@ThomThomson ThomThomson closed this as not planned Won't fix, can't repro, duplicate, stale Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Embedding Embedding content via iFrame impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:medium Medium Level of Effort Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas
Projects
None yet
Development

No branches or pull requests

5 participants