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] Settings Framework API and UI #179795

Merged
merged 32 commits into from
Apr 9, 2024

Conversation

nchaulet
Copy link
Member

@nchaulet nchaulet commented Apr 2, 2024

Summary

Follow up on #170539
Related to https://github.com/elastic/ingest-dev/issues/2471 (Phase 1)

Dynamically creating settings fields from configuration.
These settings are saved in the agent policy SO's advanced_settings field.
In the current pr the agent policy read/create/update works including the UI.
It still has to be extended to support a few more type of settings: e.g. dropdown values, settings consisting of multiple fields.

image

These settings are added to the full agent policy agents section:
image

Old description:

Add support for saved object mapping and api field name,

Example API calls and full policy generation

Screenshot 2024-04-02 at 3 54 35 PM Screenshot 2024-04-02 at 4 13 42 PM Screenshot 2024-04-02 at 4 13 54 PM Screenshot 2024-04-02 at 5 42 27 PM

Open questions/Issues

Saved objects

*I think we will still have to do some work to add a new model version when adding a new saved object field, I do not see an easy way to programatically generate that. In a first time it probably could be a manual action to add those migration

API

Open api generation, I think as a first iteration it could be a manual operation to update openAPI spec, but we should be able to programatically generate that with a script in the future

@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • /oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@@ -0,0 +1,81 @@
/*
Copy link
Member Author

Choose a reason for hiding this comment

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

Created that service that return mappings, api schema validation, and full policy values for a given section (right now we only have AGENT_POLICY_ADVANCED_SETTINGS )

@nchaulet nchaulet force-pushed the feature-settings-configurable branch from 78a4355 to 64d3598 Compare April 2, 2024 09:40
import type { SavedObjectsFieldMapping } from '@kbn/core-saved-objects-server';
import type { z } from 'zod';

export interface SettingsConfig {
Copy link
Member Author

Choose a reason for hiding this comment

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

I added the saved_object_field and api_field definition here

@nchaulet nchaulet force-pushed the feature-settings-configurable branch from 64d3598 to 459ed22 Compare April 2, 2024 09:47
@nchaulet
Copy link
Member Author

nchaulet commented Apr 2, 2024

/ci

@nchaulet nchaulet added release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team labels Apr 3, 2024
name: string;
mapping: SavedObjectsFieldMapping;
};
api_field: {
Copy link
Member Author

@nchaulet nchaulet Apr 3, 2024

Choose a reason for hiding this comment

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

used an object here so it easily extendable in the future,

@nchaulet
Copy link
Member Author

nchaulet commented Apr 3, 2024

/ci

@@ -156,6 +157,7 @@ export const getSavedObjectTypes = (): { [key: string]: SavedObjectsType } => ({
is_protected: { type: 'boolean' },
overrides: { type: 'flattened', index: false },
keep_monitoring_alive: { type: 'boolean' },
...getSettingsSavedObjectMappings('AGENT_POLICY_ADVANCED_SETTINGS'),
Copy link
Contributor

@juliaElastic juliaElastic Apr 3, 2024

Choose a reason for hiding this comment

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

Saved objects
*I think we will still have to do some work to add a new model version when adding a new saved object field, I do not see an easy way to programatically generate that. In a first time it probably could be a manual action to add those migration

I see what you mean, without adding model versions, I am getting an error when trying to set the new field

{
    "statusCode": 400,
    "error": "Bad Request",
    "message": "[1:349] mapping set to strict, dynamic introduction of [agent_limits_go_max_procs] within [ingest-agent-policies] is not allowed: strict_dynamic_mapping_exception\n\tRoot causes:\n\t\tstrict_dynamic_mapping_exception: [1:349] mapping set to strict, dynamic introduction of [agent_limits_go_max_procs] within [ingest-agent-policies] is not allowed"
}

Added this modelVersions locally to get it working:

    modelVersions: {
      '1': {
        changes: [
          {
            type: 'mappings_addition',
            addedMappings: {
              ...getSettingsSavedObjectMappings('AGENT_POLICY_ADVANCED_SETTINGS'),
            },
          },
        ],
      },
    },

I am wondering if we could add a generic mapping here, since these fields are usually not indexed anyway, something like:

advanced_settings: { type: 'flattened', index: false }

One caveat with this would be that we can't set a field type mapping on specific fields e.g. go_max_procs to be an integer. But I think it doesn't matter since the fields are not indexed.

Copy link
Contributor

@juliaElastic juliaElastic Apr 4, 2024

Choose a reason for hiding this comment

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

I made this work locally, to save these settings under a generic advanced_settings mapping in the SO. The full agent policy hasn't changed.

PUT kbn:/api/fleet/agent_policies/b180cbb0-b276-4f96-97a7-e2acb97c6a36
{
  "name": "Agent policy 2",
  "namespace": "default",
  "advanced_settings": {
    "agent_limits_go_max_procs": 6
  }
}

This simplifies the settings config that we don't need saved_object_field.

@juliaElastic
Copy link
Contributor

/ci

@juliaElastic
Copy link
Contributor

/ci

@juliaElastic
Copy link
Contributor

/ci

}
>
<EuiFormRow fullWidth key={fieldKey} error={error} isInvalid={!!error}>
<EuiFieldText
Copy link
Contributor

Choose a reason for hiding this comment

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

there is a lot of duplication between number and text type, I wonder if there could be a common component as a wrapper

Copy link
Contributor

Choose a reason for hiding this comment

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

refactored this component to use a common SettingsFieldWrapper

@juliaElastic
Copy link
Contributor

/ci

@juliaElastic
Copy link
Contributor

/ci

@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@juliaElastic
Copy link
Contributor

/ci

name: string;
};
// form error state is passed up to the form
updateAdvancedSettingsHasErrors?: (hasErrors: boolean) => void;
Copy link
Contributor

@juliaElastic juliaElastic Apr 8, 2024

Choose a reason for hiding this comment

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

I don't really like this to add a function to the SettingsConfig, haven't found a better way to pass down this function to the dynamic components.
It is needed to make the submit button disabled if one of the advanced fields are invalid.

EDIT: I found a better solution to put this function in AgentPolicyFormContext

image

@juliaElastic juliaElastic force-pushed the feature-settings-configurable branch from 547f9ed to 6879094 Compare April 8, 2024 12:17
@juliaElastic juliaElastic force-pushed the feature-settings-configurable branch from 6935271 to 779acf5 Compare April 8, 2024 12:52
@juliaElastic juliaElastic changed the title [Fleet] Settings Framework API and SO [Fleet] Settings Framework API and UI Apr 9, 2024
@juliaElastic juliaElastic requested a review from a team April 9, 2024 11:00
@juliaElastic juliaElastic added release_note:enhancement and removed release_note:skip Skip the PR/issue when compiling release notes labels Apr 9, 2024
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
fleet 971 984 +13

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 1116 1117 +1

Async chunks

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

id before after diff
fleet 1.2MB 1.3MB +69.3KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
fleet 158.6KB 158.7KB +26.0B
Unknown metric groups

API count

id before after diff
fleet 1236 1237 +1

History

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

Copy link
Member

@kpollich kpollich left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 - thanks for picking up where Nicolas and I left off 🙂

@@ -69,6 +69,10 @@ properties:
type: object
description: Override settings that are defined in the agent policy. Input settings cannot be overridden. The override option should be used only in unusual circumstances and not as a routine procedure.
nullable: true
advanced_settings:
Copy link
Contributor

Choose a reason for hiding this comment

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

We decided to go with a generic advanced_settings in openapi for now. We will look at generating the openapi spec from the settings config later.

Copy link
Contributor

@kilfoyle kilfoyle left a comment

Choose a reason for hiding this comment

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

LGTM for the descriptions! 👍

@juliaElastic juliaElastic merged commit c88b3bd into elastic:main Apr 9, 2024
22 checks passed
@kibanamachine kibanamachine added v8.14.0 backport:skip This commit does not require backporting labels Apr 9, 2024
@amitkanfer
Copy link

🚀

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:enhancement Team:Fleet Team label for Observability Data Collection Fleet team v8.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.