[js-yaml to yaml migration] @elastic/fleet#252345
Conversation
| `"{\\"id\\":\\"1234\\",\\"outputs\\":{\\"default\\":{\\"type\\":\\"elasticsearch\\",\\"hosts\\":[\\"http://localhost:9200\\"]}},\\"inputs\\":[{\\"id\\":\\"test_input-secrets-abcd1234\\",\\"revision\\":1,\\"name\\":\\"secrets-1\\",\\"type\\":\\"test_input\\",\\"data_stream\\":{\\"namespace\\":\\"default\\"},\\"use_output\\":\\"default\\",\\"package_policy_id\\":\\"abcd1234\\",\\"package_var_secret\\":\\"\${SECRET_0}\\",\\"input_var_secret\\":\\"\${SECRET_1}\\",\\"streams\\":[{\\"id\\":\\"test_input-secrets.log-abcd1234\\",\\"data_stream\\":{\\"type\\":\\"logs\\",\\"dataset\\":\\"secrets.log\\"},\\"package_var_secret\\":\\"\${SECRET_0}\\",\\"input_var_secret\\":\\"\${SECRET_1}\\",\\"stream_var_secret\\":\\"\${SECRET_2}\\"}],\\"meta\\":{\\"package\\":{\\"name\\":\\"secrets\\",\\"version\\":\\"1.0.0\\"}}}],\\"secret_references\\":[{\\"id\\":\\"secret-id-1\\"},{\\"id\\":\\"secret-id-2\\"},{\\"id\\":\\"secret-id-3\\"}],\\"revision\\":2,\\"agent\\":{},\\"signed\\":{},\\"output_permissions\\":{},\\"fleet\\":{}}"` | ||
| ); | ||
| expect(yaml).toMatchInlineSnapshot(` | ||
| "id: \\"1234\\" |
There was a problem hiding this comment.
This snapshot now reflects an actual yaml dump of the object. I tested it against the output from js-yaml and it is identical. I could not find a need to pass the stringify/dump function to fullAgentPolicyToYaml, as every call site just passed in the imported dump function as-is from js-yaml. That said if, if there is a reason to keep this modular, then we can put that back in.
There was a problem hiding this comment.
If I remember the why it was modular it was to avoid the fleet bundle to be too big and to not include the yaml dependencies, but if we are confortable with increasing the limit it should be fine
There was a problem hiding this comment.
Thanks for the context @nchaulet! I don't have a strong opinion on bundle size. Maybe @elastic/kibana-operations has insight here?
There was a problem hiding this comment.
Preference is async in most cases, unless it's needed immediately on app load. The less we load synchronously the quicker the page will appear.
There was a problem hiding this comment.
Worth a try. It depends on how many packages end up using it. If it's only fleet we're just shifting the sync load to another bundle, but if there's a few plugins using the package the tradeoff may be worth it.
jeramysoucy
added
release_note:skip
|
Pinging @elastic/kibana-security (Team:Security) |
botelastic
Bot
added
the
Team:Fleet
nchaulet
left a comment
nchaulet
left a comment
There was a problem hiding this comment.
Pulled this locally and test a few of the Fleet flows and it seems to work as expected, just one small suggestion to use error code instead of looking at message but otherwise LGTM 🚀
| filesManagement: 5208 | ||
| fileUpload: 22957 | ||
| fleet: 209495 | ||
| fleet: 322115 |
There was a problem hiding this comment.
Do we know what's happening with this? We may want to move yaml to ui-shared-deps-npm.
There was a problem hiding this comment.
We used to have dynamic import for js-yaml that code path changed #252345 (comment)
There was a problem hiding this comment.
Missed it, thanks. Replied in that thread.
jbudz
left a comment
jbudz
left a comment
There was a problem hiding this comment.
limits.yml related changes LGTM.
💔 Build Failed
Failed CI Steps
Test Failures
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsAPI count
async chunk count
History
|
| // Fallback to the first model value if the requested index is out of range | ||
| return values[0]!; | ||
| }, nthIndex); | ||
| }).toPass({ timeout: 30_000 }); |
There was a problem hiding this comment.
This might be overkill
|
@nchaulet & @jbudz I finally got a clean CI run here. The yaml package is now loaded asynchronously on the browser side, which as intended, eliminates the need to increase the Fleet package size limit. Could you both re-review, as there have been quite a few changes to this PR recently? What we're doing here sets the foundation for the rest of js-yaml -> yaml migration throughout the codebase. Manual testing of the UI is a good idea since that is affected the most with the later changes. |
|
While testing locally, I found an issue, though I'm not sure it's caused by this pr. When adding yaml text to In cloud: |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| } | ||
| ); | ||
| } | ||
| return; |
There was a problem hiding this comment.
🟢 Low form_settings/form_settings.ts:45
When setting.type === 'yaml', the early return on line 45 bypasses the Zod schema validation on lines 47-49. This means YAML settings are only validated for syntax via parse(val) — any constraints in setting.schema (type checks, required fields, min/max values) are never enforced, allowing invalid data through.
| return; | |
| return; |
🤖 Copy this AI Prompt to have your agent fix this:
In file x-pack/platform/plugins/shared/fleet/server/services/form_settings/form_settings.ts around line 45:
When `setting.type === 'yaml'`, the early `return` on line 45 bypasses the Zod schema validation on lines 47-49. This means YAML settings are only validated for syntax via `parse(val)` — any constraints in `setting.schema` (type checks, required fields, min/max values) are never enforced, allowing invalid data through.
Evidence trail:
x-pack/platform/plugins/shared/fleet/server/services/form_settings/form_settings.ts lines 33-50 (the validate function with early return for YAML type); x-pack/platform/plugins/shared/fleet/common/settings/types.ts lines 24-35 (SettingsConfig interface showing schema is required); x-pack/platform/plugins/shared/fleet/common/settings/agent_policy_settings.tsx lines 29-42 (zodStringWithYamlValidation definition) and lines 286-291 (YAML setting with type: 'yaml' and schema property); x-pack/platform/plugins/shared/fleet/server/services/form_settings/form_settings.test.ts lines 35-43 (test showing YAML setting with schema: z.string())
ApprovabilityVerdict: Needs human review 1 blocking correctness issue found. CODEOWNERS file was modified by a non-owner — requires human review You can customize Macroscope's approvability policy. Learn more. |
| ); | ||
| const yaml = useYaml(); | ||
| // Showing streams toggle state (initialized once when yaml loads so we can validate) | ||
| const [isShowingStreams, setIsShowingStreams] = useState<boolean>(false); |
There was a problem hiding this comment.
🟢 Low components/package_policy_input_panel.tsx:152
If a user clicks the "Change defaults" button to toggle the streams section before the yaml module finishes loading, the useEffect at lines 154–178 overwrites their choice when yaml finally resolves. This happens because isShowingStreamsInitialized.current is still false during the click, so the effect recomputes and sets isShowingStreams to the default value, collapsing a section the user just expanded.
🤖 Copy this AI Prompt to have your agent fix this:
In file x-pack/platform/plugins/shared/fleet/public/applications/fleet/sections/agent_policy/create_package_policy_page/components/steps/components/package_policy_input_panel.tsx around line 152:
If a user clicks the "Change defaults" button to toggle the streams section before the `yaml` module finishes loading, the `useEffect` at lines 154–178 overwrites their choice when `yaml` finally resolves. This happens because `isShowingStreamsInitialized.current` is still `false` during the click, so the effect recomputes and sets `isShowingStreams` to the default value, collapsing a section the user just expanded.
Evidence trail:
x-pack/platform/plugins/shared/fleet/public/applications/fleet/sections/agent_policy/create_package_policy_page/components/steps/components/package_policy_input_panel.tsx lines 150-178 (useYaml() call, state/ref initialization, useEffect with yaml dependency), line 447 (onClick handler that only calls setIsShowingStreams without setting isShowingStreamsInitialized.current), x-pack/platform/plugins/shared/fleet/public/services/use_yaml.ts lines 22-43 (useYaml hook returns null while loading)
| useEffect(() => { | ||
| getYamlFormatters().then(setFormatters); | ||
| }, []); |
There was a problem hiding this comment.
🟢 Low components/agent_policy_yaml_flyout.tsx:53
If getYamlFormatters() rejects (e.g., the yaml module fails to load), the error is unhandled and formatters stays null, leaving the UI stuck on <Loading /> forever with no error shown to the user. Consider adding .catch() to handle the error.
- useEffect(() => {
- getYamlFormatters().then(setFormatters);
- }, []);
+ useEffect(() => {
+ getYamlFormatters().then(setFormatters).catch((err) => {
+ // Handle error appropriately
+ });
+ }, []);🤖 Copy this AI Prompt to have your agent fix this:
In file x-pack/platform/plugins/shared/fleet/public/applications/fleet/components/agent_policy_yaml_flyout.tsx around lines 53-55:
If `getYamlFormatters()` rejects (e.g., the yaml module fails to load), the error is unhandled and `formatters` stays `null`, leaving the UI stuck on `<Loading />` forever with no error shown to the user. Consider adding `.catch()` to handle the error.
Evidence trail:
x-pack/platform/plugins/shared/fleet/public/applications/fleet/components/agent_policy_yaml_flyout.tsx at REVIEWED_COMMIT:
- Line 50: `const [formatters, setFormatters] = useState<YamlFormatters | null>(null);`
- Lines 52-54: `useEffect(() => { getYamlFormatters().then(setFormatters); }, []);` - no .catch() handler
- Lines 65-67: Rendering logic shows `<Loading />` when `!formatters` is true
- Line 60: `error` variable only captures errors from `useGetOneAgentPolicyFull`, not from formatters loading
When the OTel exporter PR was merged into main and then merged into this branch, otelExporterConfigInput was left using the removed validateYamlConfig import. Use validateYamlConfigFn consistently, matching additionalYamlConfigInput above it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Medium
When yaml is null (still loading asynchronously), validateYamlConfigFn is set to () => undefined on line 236. This validator is passed to useInput for both additionalYamlConfigInput and otelExporterConfigInput. Since returning undefined means "no validation errors", if a user submits the form before the yaml module finishes loading, invalid YAML configuration will pass validation and be submitted. The submit button has no guard for !yaml (see isDisabled on line 1187), making this race condition exploitable.
🤖 Copy this AI Prompt to have your agent fix this:
In file x-pack/platform/plugins/shared/fleet/public/applications/fleet/sections/settings/components/edit_output_flyout/use_output_form.tsx around line 1187:
When `yaml` is `null` (still loading asynchronously), `validateYamlConfigFn` is set to `() => undefined` on line 236. This validator is passed to `useInput` for both `additionalYamlConfigInput` and `otelExporterConfigInput`. Since returning `undefined` means "no validation errors", if a user submits the form before the yaml module finishes loading, invalid YAML configuration will pass validation and be submitted. The submit button has no guard for `!yaml` (see `isDisabled` on line 1187), making this race condition exploitable.
Evidence trail:
1. x-pack/platform/plugins/shared/fleet/public/applications/fleet/sections/settings/components/edit_output_flyout/use_output_form.tsx line 236: `const validateYamlConfigFn = yaml ? createValidateYamlConfig(yaml.parse) : () => undefined;`
2. Same file lines 243-252: `additionalYamlConfigInput` and `otelExporterConfigInput` use `validateYamlConfigFn`
3. Same file lines 1187-1188: `isDisabled` return value doesn't check for `!yaml`
4. x-pack/platform/plugins/shared/fleet/public/services/use_yaml.ts lines 22-24: `useYaml` returns `null` initially while loading async
5. x-pack/platform/plugins/shared/fleet/public/hooks/use_input.ts lines 62-71: `validate()` returns `true` when validator returns `undefined`
|
@juliaElastic Sorry to ask again but could you give this another manual test? I had to make several changes after your last review due to merge conflicts and other issues. |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled line counts
Total ESLint disabled count
History
|
|
The date behaviour in |
|
@efd6 Thanks for identifying this. I am looking into a fix now, and have asked Fleet to assess test coverage. |
13 participants
Migration: js-yaml → yaml (with async loading)
Part of #233198
This PR migrates the Fleet plugin from
js-yamlto theyamlpackage, with asynchronous loading on the client side to avoid increasing the plugin's bundle size.Summary
js-yamlimports with theyamlpackage across ~160 Fleet files (server + client + common + tests)@kbn/yaml-loaderpackage that provides async loading of theyamlmodule vialoadYaml()(dynamicimport('yaml'))yamlasynchronously through auseYaml()React hook andgetYamlFormatters()utility, keeping theyamlmodule out of the synchronous bundleyamldirectly (no async needed)fullAgentPolicyToYaml,fullAgentConfigMapToYaml,createYamlKeysSorter, etc.) use dependency injection — callers pass theyamlmodule (or specific functions likeparse/stringify) so these utilities remain environment-agnosticKey changes
New package:
@kbn/yaml-loadersrc/platform/packages/shared/kbn-yaml-loader/— providesloadYaml()which returnsPromise<typeof import('yaml')>tsconfig.base.json,config-paths.json, and rootpackage.jsonAsync loading infrastructure (client-side)
public/services/use_yaml.ts—useYaml()React hook that loads theyamlmodule asynchronously and caches it globally so subsequent hook calls resolve synchronouslypublic/services/yaml_formatters.ts—getYamlFormatters()returns a cached promise of formatter functions (fullAgentPolicyToYaml,fullAgentConfigMapToYaml)Dependency injection in common services
common/services/yaml_utils.ts—YamlModuleinterface +createYamlKeysSorter()andtoYaml()now accept ayamlmodule parametercommon/services/agent_cm_to_yaml.ts—fullAgentConfigMapToYaml()acceptsyaml: YamlModulecommon/services/full_agent_policy_to_yaml.ts—fullAgentPolicyToYaml()acceptsyaml: YamlModulecommon/settings/agent_policy_settings.tsx— Zod YAML validation uses asyncloadYaml()in.refine()callbacks viasafeParseAsync()Client-side component updates
Components that previously imported
js-yamlsynchronously now useuseYaml()orgetYamlFormatters():edit_output_flyout/index.tsx+use_output_form.tsx— output form YAML config validation and preset detectionedit_fleet_proxy_flyout/use_fleet_proxy_form.tsx— proxy headers YAML validation/parsingagent_policy_yaml_flyout.tsx— agent policy YAML display/downloadagent_effective_config_flyout.tsx— agent effective config displaypackage_policy_input_panel.tsx— stream visibility based on YAML validationagent_enrollment_flyout/hooks.tsx— enrollment YAML generationuse_change_log.ts— changelog YAML parsinghas_invalid_but_required_var.ts,output_form_validators.tsx— acceptparsefunction parameterform.tsx,use_package_policy.tsx,add_integration.tsx) — passyamltovalidatePackagePolicyPreset sync fix for async race condition
edit_output_flyout/index.tsx— added auseEffectthat syncs the output preset to"custom"when theyamlmodule loads and the YAML config already contains reserved performance keys. This prevents a race condition where typing reserved keys beforeyamlloads would leave the preset incorrectly set to"balanced".Server-side changes
yamldirectly (import yaml from 'yaml'orimport { parse, stringify } from 'yaml')yamlmodule to common utilities via dependency injectionAPI mapping (js-yaml → yaml)
load()parse()dump()stringify()noRefs: truealiasDuplicateObjects: falseskipInvalid: truestrict: falsesortKeyssortMapEntriesJSON_SCHEMAschema: 'core'Test updates
jest.mockforuseYamlin test files where async loading would cause issues (single_page_layout/index.test.tsx,edit_package_policy_page/index.test.tsx, etc.)yamlpackage output formattingparsefunction or mockYamlModulewhere dependency injection was addedsafeParseAsync()for async YAML validationTesting
Note: This is part of a larger
js-yaml→yamlmigration effort across Kibana. Other teams' files are handled in separate PRs. These changes were generated using Cursor.