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

Add setting to manage policy templates behavior in Kibana #825

Merged
merged 7 commits into from
Oct 28, 2024

Conversation

mrodm
Copy link
Contributor

@mrodm mrodm commented Oct 24, 2024

What does this PR do?

Adds a new setting policy_templates_behavior to manage how the policy templates / tiles are shown to the user in Kibana.

Values that admit this new field:

  • combined_policy
  • individual_policies
  • all

Why is it important?

There are some integrations that do not require to show a tile for the integration itself, or vice-versa, there could be integrations that it is not desired to show each of the policy templates (child policy templates) defined as tiles, just the one referring to the integration itself (parent tile)

Checklist

Related issues

@mrodm mrodm self-assigned this Oct 24, 2024
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this new setting just for integrations.

Tried to look into kibana's code here: https://github.com/elastic/kibana/blob/e5b0b8108b392dae070b87175cf2d43522576c4d/x-pack/plugins/fleet/public/applications/integrations/sections/epm/screens/home/hooks/use_available_packages.tsx#L82

IIUC Kibana just shows different tiles in case of integrations packages. Input packages would just show one tile. Is my understanding correct?

Or, should it be added also for input packages ?

@jen-huang @kpollich

Copy link
Member

Choose a reason for hiding this comment

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

I wonder why we support multiple policy templates in input packages 🤔 What could be a use case? I think we don't have any input package with more than one. Maybe we should change the spec to be more explicit about this.

Some previous discussion about this here: https://github.com/elastic/package-spec/pull/325/files#r860573848

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least, currently all input packages defined in the integrations repository have just one policy template.

Forcing input packages to have just one policy template could be done by adding maxItems setting (it could be done in this PR or in a separate PR):

--- spec/input/manifest.spec.yml
+++ spec/input/manifest.spec.yml
@@ -38,6 +38,7 @@ spec:
     policy_templates:
       description: List of policy templates offered by this package.
       type: array
+      maxItems: 1
       items:
         type: object
         additionalProperties: false

What could be a use case?

I was thinking for instance in the "Custom logs" input package, would that make sense to add a new policy template there to read logs from another source? or would it be better to create a new input package?

Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC Kibana just shows different tiles in case of integrations packages. Input packages would just show one tile. Is my understanding correct?

Kibana does not enforce the distinction between integration and input type packages when it comes to displaying the tiles, it only reads the number of policy_templates

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking for instance in the "Custom logs" input package, would that make sense to add a new policy template there to read logs from another source? or would it be better to create a new input package?

Yes, in this case it would be better to create another input package.

Maybe a use case could be to have an input that collects from the same source, but with a different method. But in this case maybe it would be better to do #744.

I would be more leaned towards limiting input packages to a single policy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it is not clear whether or not input packages should have a limitation in the number of policy templates, I would suggest not doing that for now.

Comment on lines 365 to 378
# requires a conditional JSON schema to update the value depending
# on the policy_templates length
policy_templates_behavior:
description: >
Behavior to manage the policy templates (policy_templates field) defined that must be available in Kibana as tiles.
This is just applicable for those packages that define more than one policy template. By default for these packages, Kibana
shows all the policy templates defined in the manifest separately plus another one referring to the
whole integration (that contains all the policy templates definitions) available to the user.

If the `integration_package_only` option is selected, then just one policy template for the whole integration containing all policy
templates will be available in Kibana. If the `policy_templates_only` option is selected, then just the policy templates defined
in the manifest will be available in Kibana without the specific policy template for the whole integration. If the `default` option
is selected, then Kibana keeps the same behavior described above.
type: string
Copy link
Contributor Author

@mrodm mrodm Oct 24, 2024

Choose a reason for hiding this comment

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

Not sure about the wording or even the naming for the options here.
WDYT ? Any other suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

Proposed rewording in the comment above.

@mrodm mrodm marked this pull request as ready for review October 24, 2024 15:07
@mrodm mrodm requested a review from a team as a code owner October 24, 2024 15:07
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Codewise LGTM, added some suggestions about wording and the possible values for the setting.

Comment on lines 369 to 377
Behavior to manage the policy templates (policy_templates field) defined that must be available in Kibana as tiles.
This is just applicable for those packages that define more than one policy template. By default for these packages, Kibana
shows all the policy templates defined in the manifest separately plus another one referring to the
whole integration (that contains all the policy templates definitions) available to the user.

If the `integration_package_only` option is selected, then just one policy template for the whole integration containing all policy
templates will be available in Kibana. If the `policy_templates_only` option is selected, then just the policy templates defined
in the manifest will be available in Kibana without the specific policy template for the whole integration. If the `default` option
is selected, then Kibana keeps the same behavior described above.
Copy link
Member

Choose a reason for hiding this comment

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

Simplified wording, removing references to the visual representation. Also proposing these three values:

  • combined_policy: to indicate that there is a single policy template that combines them all.
  • individual_policies: to indicate that policies are available individually, without a combined policy.
  • all: the default value, where both the combined policy and the individual policies are available.
Suggested change
Behavior to manage the policy templates (policy_templates field) defined that must be available in Kibana as tiles.
This is just applicable for those packages that define more than one policy template. By default for these packages, Kibana
shows all the policy templates defined in the manifest separately plus another one referring to the
whole integration (that contains all the policy templates definitions) available to the user.
If the `integration_package_only` option is selected, then just one policy template for the whole integration containing all policy
templates will be available in Kibana. If the `policy_templates_only` option is selected, then just the policy templates defined
in the manifest will be available in Kibana without the specific policy template for the whole integration. If the `default` option
is selected, then Kibana keeps the same behavior described above.
Expected behavior when there are more than one policy template defined.
When set to `combined_policy`, a single policy template is available, that
combines all the defined templates. When set to `individual_policies`, all
policies are individually available, but there is no combined policy.
The default value is `all`, where the combined policy template is available
along with the individual policies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this proposal that does not mention any reference to tiles in Kibana.
And it also looks easier to understand what each option is aimed for.

Thanks!

I'll do the required changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in b2ff189

Comment on lines 365 to 378
# requires a conditional JSON schema to update the value depending
# on the policy_templates length
policy_templates_behavior:
description: >
Behavior to manage the policy templates (policy_templates field) defined that must be available in Kibana as tiles.
This is just applicable for those packages that define more than one policy template. By default for these packages, Kibana
shows all the policy templates defined in the manifest separately plus another one referring to the
whole integration (that contains all the policy templates definitions) available to the user.

If the `integration_package_only` option is selected, then just one policy template for the whole integration containing all policy
templates will be available in Kibana. If the `policy_templates_only` option is selected, then just the policy templates defined
in the manifest will be available in Kibana without the specific policy template for the whole integration. If the `default` option
is selected, then Kibana keeps the same behavior described above.
type: string
Copy link
Member

Choose a reason for hiding this comment

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

Proposed rewording in the comment above.

spec/integration/manifest.spec.yml Outdated Show resolved Hide resolved
Comment on lines 518 to 520
- integration_package_only
- policy_templates_only
- default
Copy link
Member

Choose a reason for hiding this comment

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

Options proposed above.

Suggested change
- integration_package_only
- policy_templates_only
- default
- combined_policy
- individual_policies
- all

Copy link
Member

Choose a reason for hiding this comment

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

I wonder why we support multiple policy templates in input packages 🤔 What could be a use case? I think we don't have any input package with more than one. Maybe we should change the spec to be more explicit about this.

Some previous discussion about this here: https://github.com/elastic/package-spec/pull/325/files#r860573848

@elasticmachine
Copy link

💚 Build Succeeded

History

cc @mrodm

@mrodm mrodm changed the title Add setting to manage policy templates behaviour in Kibana Add setting to manage policy templates behavior in Kibana Oct 28, 2024
@mrodm mrodm merged commit cc52823 into elastic:main Oct 28, 2024
3 checks passed
@mrodm mrodm deleted the add_policy_tiles_behaviour branch October 28, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants