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

Allow integration developers to edit managed content #170517

Closed
2 tasks
drewdaemon opened this issue Nov 3, 2023 · 22 comments
Closed
2 tasks

Allow integration developers to edit managed content #170517

drewdaemon opened this issue Nov 3, 2023 · 22 comments
Assignees
Labels
impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Team:Fleet Team label for Observability Data Collection Fleet team

Comments

@drewdaemon
Copy link
Contributor

drewdaemon commented Nov 3, 2023

In #70461 Fleet asked for managed content to be readonly to stop users from losing changes on package upgrades.

In #166204 dashboards were made read-only. However, this prevents integration authors from making any changes to managed dashboards in 8.11. This prevents them from adopting any features introduced from 8.11 on.

We must provide integration developers with the ability to edit these assets.

Tasks

@drewdaemon drewdaemon added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. labels Nov 3, 2023
@elasticmachine
Copy link
Contributor

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

@ThomThomson
Copy link
Contributor

@P1llus @drewdaemon, I assume the usual workflow is to:

  1. edit the managed Dashboard directly
  2. save the edits
  3. export the saved object

Just a guess, but I wonder if a workaround could be:

  1. clone the managed Dashboard
  2. edit the cloned Dashboard
  3. save the edits
  4. export the saved object
  5. Manually edit the saved object id in the export to match the original managed dashboard.

This wouldn't be ideal long term, but it would at least be a workaround for 8.11 if any changes need to be made.

Additionally, I don't really think this workaround should be built on the Dashboard side. Maybe we could add a remove managed state option to the saved objects management page for admins?

@P1llus
Copy link
Member

P1llus commented Nov 3, 2023

@ThomThomson thats correct, its fine for 8.11.0, its not blocking us yet, but would be good to have a fix for the next patch release.

The implementation I do not have much comments on, whichever is fine by us 😀

@ThomThomson
Copy link
Contributor

Tagging the @elastic/kibana-core team here as well for ideas on the implementation.

@drewdaemon
Copy link
Contributor Author

drewdaemon commented Nov 3, 2023

@ThomThomson I tagged your team because this needs a team and Dashboards are the only content that is currently read-only. I agree that the solution needs to fit all content types.

Maybe we could add a remove managed state option to the saved objects management page for admins?

I don't think this needs to be a user-facing feature. Integration developers make changes to these dashboards using a local stack that gets created by https://github.com/elastic/elastic-package. That tool could flip a Kibana configuration switch.

I defer to Core on the specifics of how best to make this happen!

@ThomThomson
Copy link
Contributor

@drewdaemon yes the Presentation team can certainly drive this.

using a local stack ...

Great news! This opens up many more avenues to tackle this problem.

@TinaHeiligers
Copy link
Contributor

Manually edit the saved object id in the export to match the original managed dashboard.

@ThomThomson this is never a good idea! Manually editing a saved object is prone to errors. We've seen many a kibana migration fail from corrupt documents, often the case when objects are manually edited.

Integration developers make changes to these dashboards using a local stack that gets created by elastic/elastic-package. That tool could flip a Kibana configuration switch.

++
Having an external tool make changes to managed objects and then importing those as completely new content, was the process we agreed on with Fleet a few months back.

@rudolf
Copy link
Contributor

rudolf commented Nov 6, 2023

I agree with @drewdaemon that this sounds more like a internal elastic developer focussed request than something we expect users would ever want/need to use.

I don't have strong opinions about the implementation, but my suggestion would be to push these kinds of development-focussed hacks as close as possible to the team that depends on them. I would only change this in the dashboard plugin if there's no other way.

In my opinion this is hack that's only relevant to fleet package / integration developers. So I wonder if fleet shouldn't add a development only configuration option that would set managed: false on all dashboards installed from a package. Conversely somewhere in the package build toolchain dashboards would have managed: true set before being commited to git.

cc @elastic/fleet

@kpollich
Copy link
Member

kpollich commented Nov 6, 2023

So I wonder if fleet shouldn't add a development only configuration option that would set managed: false on all dashboards installed from a package. Conversely somewhere in the package build toolchain dashboards would have managed: true set before being commited to git.

I think this is a good solution, but I don't know that we even need to track the managed property in Git. We can add a setting (e.g. xpack.fleet.installKibanaAssetsAsUnmanaged) to kibana.yml that can either be the default when env.mode.dev is true, or can be explicitly enabled by elastic-package. I'd lean towards making this an explicit opt-in that's set in the Kibana config used by elastic-package stack up.

Fleet already sets managed: true regardless of whether managed is explicitly set on the asset source or not. So, the fact that the managed property exists in the actual JSON blob stored with the package archive is irrelevant at this point - the asset will always have managed: true. Since the logic around this property is entirely contained to Fleet, I think adding the setting in front of it and having elastic-package explicitly provide it is the best path forward.

@kpollich kpollich added the Team:Fleet Team label for Observability Data Collection Fleet team label Nov 6, 2023
@drewdaemon
Copy link
Contributor Author

I really like where this is going. I feel that this is making the change in the right place and prevents us from complicating the logic around how we treat managed content in Kibana 👍

@ThomThomson
Copy link
Contributor

ThomThomson commented Nov 6, 2023

I really like where this is going.

++

@rudolf, agreed this code should stay out of the Dashboard plugin if at all possible. I'm removing the Presentation team label from this issue because the solution that @kpollich suggested seems to be the best way forward.

@ThomThomson ThomThomson removed the Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas label Nov 6, 2023
@kpollich
Copy link
Member

kpollich commented Nov 8, 2023

We'll want to make sure we don't box ourselves into a corner with this behavior. Using the kibana setting is fine for now, and will allow us to move quickly. However, we may want to consider making this a runtime settings configurable somewhere in Kibana for the future. Changing this behavior in serverless, for example, won't be possible if the setting is exposed only through kibana.yml.

I think in the interest of moving quickly, we should seek to solve this problem for today's ecosystem of integration development by using the kibana.yml settings and elastic-package, but I don't want to eliminate the possibility of allowing serverless/cloud users to control this via a runtime setting somewhere in Kibana.

@TinaHeiligers
Copy link
Contributor

Changing this behavior in serverless, for example, won't be possible if the setting is exposed only through kibana.yml.

That's a very good point that we have tp be proactive on and open an issue now for it.

@drewdaemon
Copy link
Contributor Author

I don't want to eliminate the possibility of allowing serverless/cloud users to control this via a runtime setting somewhere in Kibana.

Makes sense.

Changing this behavior in serverless, for example, won't be possible if the setting is exposed only through kibana.yml.

That's a very good point that we have tp be proactive on and open an issue now for it.

A bit of context on this request (correct me if wrong @ruflin ):

There's an idea that, in the future, we may give users the capabilities to build their own integrations directly in Kibana. The proposed runtime flag could be part of that.

However, giving users these capabilities feels like a project in and of itself. Until we have someone defining and driving it, I'm not sure we need to go further than making sure we haven't ruled it out technically.

@jsoriano
Copy link
Member

I only recently discovered this discussion, the proposed approach looks generally good to me, at least by now, but I have some concerns:

  • Serverless. It is already discussed in the issue, but I would like to add that we included a Serverless provider in elastic-package v.0.87.0, effectively allowing to develop and test packages in Serverless. Development experience will be limited if we don't have an option for this environment.
  • Making Fleet to stop including managed: true will change how the package behaves in development and in production. We may mitigate this by adding a toggle to elastic-package so developers can optionally test the package with managed: true, but having to reconfigure, restart, reinstall and so on can be cumbersome.
  • Contributions may be more complicated. A user who wants to contribute a change in a dashboard would have to set-up a different stack with this flag, and do the changes there. I have to admit that we don't use to receive contributions in dashboards, but this will make it more unlikely.

So I think it is still worth considering to add some way to make dashboards editable.

I don't have strong opinions about the implementation, but my suggestion would be to push these kinds of development-focussed hacks as close as possible to the team that depends on them. I would only change this in the dashboard plugin if there's no other way.

Would it be an option to add the runtime toggle only in the EPM UI? So for example in the assets view, an "Edit", or "Make editable", button is added, and the Fleet plugin, that is the one that manages these assets, removes "managed": "true".
This way this would only work for Fleet-managed assets and we wouldn't need to add anything in the dashboards plugin or for all saved objects. It would still be a bit hidden, and it would work the same in local or in serverless, in development or in production.

assets-edit

@drewdaemon
Copy link
Contributor Author

drewdaemon commented Nov 22, 2023

@jsoriano thanks for the comments. We know this isn't a perfect solution but we're aiming to unblock the integration developers quickly. Our tactical plan doesn't rule out future enhancements to the process of integration development.

A few questions for you:

we included a Serverless provider in elastic-package v.0.87.0, effectively allowing to develop and test packages in Serverless. Development experience will be limited if we don't have an option for this environment.

elastic-package turning off the managed property will mitigate this, right?

Making Fleet to stop including managed: true will change how the package behaves in development and in production.

Could I hear more details on this? The only difference I know of is that we will keep users from editing or deleting managed content. Does this difference introduce risk into integration development?

Contributions may be more complicated. A user who wants to contribute a change in a dashboard would have to set-up a different stack with this flag, and do the changes there. I have to admit that we don't use to receive contributions in dashboards, but this will make it more unlikely.

Are users able to contribute changes today without using elastic-package?

@jsoriano
Copy link
Member

We know this isn't a perfect solution but we're aiming to unblock the integration developers quickly. Our tactical plan doesn't rule out future enhancements to the process of integration development.

+1 to unblock this at least for local development environments.

we included a Serverless provider in elastic-package v.0.87.0, effectively allowing to develop and test packages in Serverless. Development experience will be limited if we don't have an option for this environment.

elastic-package turning off the managed property will mitigate this, right?

Would it be an option to turn off the managed property using the APIs? This could be a way to support edition and we would not need any change in Kibana.

We could add something like an elastic-package edit dashboard subcommand that removes the managed property. This would be aligned with the current workflow as users also need to use elastic-package export dashboard to export the dashboards.

Making Fleet to stop including managed: true will change how the package behaves in development and in production.

Could I hear more details on this? The only difference I know of is that we will keep users from editing or deleting managed content. Does this difference introduce risk into integration development?

Yes, there is an only difference now, but it is a difference, and I guess there can be more in the future.

There is also a risk that we may have code that rely on these properties (now or in the future), and the only place where we won't have them is in packages development environments. For example I had to double-check if we use managed in elastic-package dump (we don't, but we use managed_by), and I would have to check if Fleet is using it somewhere.

Maybe it is too nitpicky 🙂 but I see certain risk on removing something that uses to be there.

Contributions may be more complicated. A user who wants to contribute a change in a dashboard would have to set-up a different stack with this flag, and do the changes there. I have to admit that we don't use to receive contributions in dashboards, but this will make it more unlikely.

Are users able to contribute changes today without using elastic-package?

To work on packages you need elastic-package and a stack. This stack can be provided by elastic-package itself, locally or in serverless, but at the moment it can be any stack. With this change we would be limiting this to only the local stack provided by elastic-package. This is the most common development environment, so not such a problem, but is a limitation we are introducing.

@kpollich
Copy link
Member

I'm definitely okay with just unblocking the most common use case to make sure package developers can move forward with updating dashboards ASAP. I think we can confidently say that iterating on integration assets by using elastic-package and the local stack environment it provides is our officially supported means of package development. I don't think we've ever been explicit about this, but we really just implicitly support other environments today.

We haven't taken a stance one way or the other, but based on how the majority integration developers work and how this issue was raised by folks who work off of elastic-package stack I think we can start by fixing it for that use case.

So, I think we should move forward with the implementation proposed here even if it is only limited to our officially support package development environment. This doesn't preclude us from improving support here in the future. For example, @jsoriano - I really like the idea of having a button + API call somewhere in Kibana to "unlock" a given asset for editing. But then we run into a similar problem set as we've discussed in this issue: in what environments/contexts do we allow users to actually take that action? Should we show the button and make the API accessible such that any user on any cluster could "unmanage" an asset at any time? That's probably not a great idea as it defeats the purpose of managed content (which is now read-only) in the first place. Maybe there could be a separate user permission/role that allows users to do this. Then we'd want all integration developers to give themselves this role in their stack of choice, and we'd probably wind up configuring elastic-package stack to auto-assign the role to its default Kibana user and that feels like we're basically back in this same discussion 😅

@jsoriano
Copy link
Member

jsoriano commented Nov 22, 2023

That's probably not a great idea as it defeats the purpose of managed content (which is now read-only) in the first place.

Is managed content expected to be read-only also at the API level?

we included a Serverless provider in elastic-package v.0.87.0, effectively allowing to develop and test packages in Serverless. Development experience will be limited if we don't have an option for this environment.

elastic-package turning off the managed property will mitigate this, right?

Would it be an option to turn off the managed property using the APIs? This could be a way to support edition and we would not need any change in Kibana.

We could add something like an elastic-package edit dashboard subcommand that removes the managed property. This would be aligned with the current workflow as users also need to use elastic-package export dashboard to export the dashboards.

Jill and I have discussed about this approach, and we think that it would be worth to explore this option. It adds an additional step when editing dashboard, but it would fit well with current development workflow, and we can deliver it faster as it would be a change only in elastic-package.

We would add something like a new elastic-package edit dashboard command, that for the chosen dashboard/s it would:

  • Export the dashboard and set "managed": false in the exported objects.
  • Delete the exported dashboards.
  • Import the updated exported objects.

@drewdaemon @kpollich do we expect to have something preventing to delete managed saved objects? I tried with the Delete saved objects API and it worked, but this API is deprecated. Is there going to be another API replacing this one?

To confirm, these are the API operations we would need for this approach:

  • Export API, in preview (or deprecated GET API).
  • Delete API (deprecated).
  • Import API, in preview (or deprecated PUT API).

@TinaHeiligers
Copy link
Contributor

I tried with the Delete saved objects API and it worked, but this API is deprecated. Is there going to be another API replacing this one

@jsoriano @rudolf would need to confirm but AFAIK, we don't have plans to delete these deprecated SO API's any time soon. We're' well aware that they're used extensively!

@drewdaemon
Copy link
Contributor Author

drewdaemon commented Nov 22, 2023

@jsoriano I am currently driving the Kibana discussion on managed content. I have seen managed content as a UX, not a security feature. In other words, I have understood the request to be "stop users from accidentally getting into bad states" as opposed to "securely prevent motivated actors from circumventing controls."

In that spirit, we are planning to have the UI prevent deleting and editing managed content. However, there are no current plans to lock edits or deletions to this content at the API level. I haven't yet been told that this is a problem, but feel free to chime in @kpollich .

FWIW, there is an open discussion about possibly

  • making exported managed content unmanaged
  • preventing managed content from being overwritten from the saved objects import API

I do not personally see a conflict between those decisions and your idea for elastic-package edit dashboard.

@jillguyonnet
Copy link
Contributor

Hi all, v0.94.0 of elastic-package has been released and includes the new elastic-package edit dashboards command. This new command allows dashboards to be edited in Kibana: instructions can be found at https://github.com/elastic/elastic-package/blob/main/docs/howto/make_dashboards_editable.md.

Thank you everyone for your contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Team:Fleet Team label for Observability Data Collection Fleet team
Projects
None yet
Development

No branches or pull requests

9 participants