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

[Fleet] Add a is_managed field to package policy #136183

Merged
merged 18 commits into from
Jul 13, 2022

Conversation

shahzad31
Copy link
Contributor

Summary

This requirement came out of elastic/uptime#475 and it's to support #135782

i separated out in a separate PR to make code review easier.

In synthetics we want to control to synthetics integration policies from Synthetics plugin. When user adds a monitor, we create a respective integration policy in synthetics package. Since that is entirely managed by synthetics plugin, we don't want users to edit those in view. Since source of truth in this case will be uptime monitor saved object.

This PR adds is_externally_managed a flag in policies, which will indicate if this is externally managed integration.

When this flag is set to true, we will disable actions
image

And also when user is editing, we disable editing and adds a link to uptime

image

If user has existing synthetics monitors, they will keep working same or if they add them via fleet ui, they will continue working same.

@shahzad31 shahzad31 marked this pull request as ready for review July 12, 2022 12:36
@shahzad31 shahzad31 requested review from a team as code owners July 12, 2022 12:36
@shahzad31 shahzad31 added release_note:skip Skip the PR/issue when compiling release notes v8.4.0 labels Jul 12, 2022
@joshdover
Copy link
Contributor

@shahzad31 How is this functionally different than the is_frozen attribute we already have support for?

@joshdover
Copy link
Contributor

cc @nchaulet I think you would be best to guide this

@joshdover joshdover added the Team:Fleet Team label for Observability Data Collection Fleet team label Jul 12, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@shahzad31
Copy link
Contributor Author

@shahzad31 How is this functionally different than the is_frozen attribute we already have support for?

i wasn't aware of is_frozen, let me check that out.

@shahzad31
Copy link
Contributor Author

@shahzad31 How is this functionally different than the is_frozen attribute we already have support for?

i wasn't aware of is_frozen, let me check that out.

i can't find is_frozen option.

@nchaulet
Copy link
Member

Hi @shahzad31, I am not sure the is_frozen will totally solve your use case I have a couple of questions (just discovering that issue so maybe these questions can sound dumb :) )

  • What block this managed package policy to be edited from the Fleet UI? is there no way to sync Fleet package policy with your saved object using Fleet provided hooks?
  • Can the package policy be deleted from Fleet?
  • Does some synthetics package policy will be edited from the Fleet UI and some from your plugin?

@shahzad31
Copy link
Contributor Author

Hi @shahzad31, I am not sure the is_frozen will totally solve your use case I have a couple of questions (just discovering that issue so maybe these questions can sound dumb :) )

Those are totally valid question, and thank you so much for taking a quick look

  • What block this managed package policy to be edited from the Fleet UI? is there no way to sync Fleet package policy with your saved object using Fleet provided hooks?

I think it would be complex, potentially it can be done since we control the synthetics fleet ui by custom controllers we have in uptime app. But i don't think we see any value in that. Since we want 100% of our users to interact from monitor management.

  • Can the package policy be deleted from Fleet?

No it shouldn't be. User shouldn't be able to delete these externally managed policies from FLeet UI. I think this PR does prevent that. Is there any other way to delete these from fleet UI?

  • Does some synthetics package policy will be edited from the Fleet UI and some from your plugin?

So for now yes, since we want our changes to be backward compatible. And user may have existing synthetics policies or may want to ad new from fleet ui. So that means they will be able to edit/delete those from fleet UI.

@botelastic botelastic bot added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Jul 12, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@shahzad31 shahzad31 requested a review from a team as a code owner July 12, 2022 18:03
Copy link
Contributor

@kfirpeled kfirpeled left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved changes to cloud_security_posture plugin

@nchaulet
Copy link
Member

nchaulet commented Jul 12, 2022

I saw potentially a bunch of issues with this that are currently not addressed and we probably need to discuss:

  • Managed package policy should probably not be editable in Fleet HTTP API
  • How package policy upgrade will work?

I am still trying to understand your saved object and the link to package policy, can you give more info on what do you save in saved object and how|when you transform this in package policy?

@nchaulet
Copy link
Member

@shahzad31 as we discussed offline I tried to play with frozen input but it seems too limited for what you want to achieve this allow to have some variable non editable but it does not allow to block package policy deletion.

I think it will make sense to allow the is_externally_managed flag, we already have a similar flag at the agent policy level call is_managed do you mind renaming is_externally_managed => is_managed to be consistant, also I think we need to make a little more changes for managed package policies:

@nchaulet nchaulet self-requested a review July 13, 2022 13:26
@shahzad31
Copy link
Contributor Author

@shahzad31 as we discussed offline I tried to play with frozen input but it seems too limited for what you want to achieve this allow to have some variable non editable but it does not allow to block package policy deletion.

I think it will make sense to allow the is_externally_managed flag, we already have a similar flag at the agent policy level call is_managed do you mind renaming is_externally_managed => is_managed to be consistant, also I think we need to make a little more changes for managed package policies:

  • a managed package policy should not be editable or updatable unless you pass a force flag similar to the agent policy behavior (see here for example nchaulet/kibana@67683c9/x-pack/plugins/fleet/server/services/agent_policy.ts#L655-L657)

  • In the UI:

    • we should probably disable upgrade and delete in package policy action
    Screen Shot 2022-07-12 at 4 14 15 PM
    • when we edit|view a package policy we should probably disable all the fields and we should show a callout to explain that it's managed

I have added a callout
image

@shahzad31
Copy link
Contributor Author

also renamed it to is_managed

@shahzad31 shahzad31 requested a review from nchaulet July 13, 2022 15:23
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
fleet 1390 1393 +3

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
fleet 848.9KB 849.5KB +589.0B
synthetics 882.1KB 882.6KB +492.0B
total +1.1KB

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/development-plugin-saved-objects.html#_mappings

id before after diff
ingest-package-policies 38 39 +1
Unknown metric groups

API count

id before after diff
fleet 1522 1525 +3

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@nchaulet nchaulet changed the title [Fleet] Add an externally managed option to policies [Fleet] Add a is_managed field to package policy Jul 13, 2022
Copy link
Member

@nchaulet nchaulet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 🚀
It will be nice if you can add an API integration test for a managed package policy here (or create a follow up issue so we keep track of that)
I created a follow up issue to improve the Fleet UI for these managed spec #136322

Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uptime changes LGTM

@shahzad31
Copy link
Contributor Author

Looks good to me 🚀 It will be nice if you can add an API integration test for a managed package policy here (or create a follow up issue so we keep track of that) I created a follow up issue to improve the Fleet UI for these managed spec #136322

Thanks, i will do a follow up PR or an issue to cover that. Though i will add tests from synthetics perspective in follow up PR as well.

@shahzad31 shahzad31 merged commit b3011ba into elastic:main Jul 13, 2022
@shahzad31 shahzad31 deleted the externally-managed-integerations branch July 13, 2022 19:57
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jul 13, 2022
props.isManaged ? (
<EuiToolTip
title={i18n.translate('xpack.fleet.externallyManagedLabel', {
defaultMessage: 'This is externally managed integration policy.',
Copy link
Contributor

@hbharding hbharding Jul 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message should go in the content prop. Only use the title prop if content is being used. The title prop adds a horizontal divider, and looks bad when no context exists below.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants