[Infrastructure UI] Add strict payload validation to inventory_views endpoint by crespocarlos · Pull Request #160852 · elastic/kibana

crespocarlos · 2023-06-29T09:26:36Z

Summary

This PR adds strict payload validation to inventory_views endpoint. I tried to make the types consistent between sever and frontend code and because of that more changes had to be made. I also refactored the toolbar_control component, decoupling them from Inventory View and Metrics Explorer View types

How to test

Call the endpoint below trying to use invalid values. see here.

[POST|PUT] kbn:/api/infra/inventory_views
{
  "attributes": {
    "metric": {
        "type": [TYPE]
    },
    "sort": {
        "by": "name"
        "direction": "desc"
    },
    "groupBy": [],
    "nodeType": "host",
    "view": "map",
    "customOptions": [],
    "customMetrics": [],
    "boundsOverride": {
        "max": 1,
        "min": 0
    },
    "autoBounds": true,
    "accountId": "",
    "region": "",
    "autoReload": false,
    "filterQuery": {
        "expression": "",
        "kind": "kuery"
    },
    "legend": {
        "palette": "cool",
        "steps": 10,
        "reverseColors": false
    },
    "timelineOpen": false,
    "name": "test-uptime"
  }
}

Set up a local Kibana instance
Navigate to Infrastructure
In the UI, use the Saved View feature and try different field combinations

ghost · 2023-06-29T09:26:49Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

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

crespocarlos · 2023-06-30T10:16:57Z

@elasticmachine merge upstream

…y-view-strict-input-validation

crespocarlos · 2023-06-30T11:48:24Z

x-pack/plugins/infra/public/pages/metrics/metrics_explorer/components/saved_views.tsx


  return (
-    <SavedViewsToolbarControls<MetricExplorerViewState>
+    <SavedViewsToolbarControls<any, MetricExplorerViewState>


This will be fixed in this PR #160982

elasticmachine · 2023-06-30T11:48:43Z

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

tonyghiani · 2023-06-30T11:59:30Z

x-pack/plugins/infra/server/saved_objects/inventory_view/types.ts

  rt.type({
    id: rt.string,
-    attributes: inventoryViewSavedObjectAttributesRT,
+    attributes: inventoryViewAttributesRT,


note: I think coupling the attributes to the type defined and used also for the API responses revert the changes made for the migrations to versioned APIs, where is states the persistence schema shouldn't be directly exposed. In terms of SO attributes definition, I think we should approach it as done for the logView SO on this PR. Wdyt?

Good catch! thanks

tonyghiani · 2023-06-30T12:01:33Z

x-pack/plugins/infra/common/inventory_views/types.ts

+export const inventoryColorPaletteRT = rt.union([
+  rt.literal('status'),
+  rt.literal('temperature'),
+  rt.literal('cool'),
+  rt.literal('warm'),
+  rt.literal('positive'),
+  rt.literal('negative'),
+]);


nit: we could use the rt.keyof function here too

tonyghiani · 2023-06-30T12:07:19Z

x-pack/plugins/infra/common/http_api/inventory_views/v1/find_inventory_view.ts

-import { nonEmptyStringRt } from '@kbn/io-ts-utils';
 import * as rt from 'io-ts';
-
-export const findInventoryViewAttributesResponseRT = rt.strict({


question: I remembered this was a strict type to strip out the non-essential properties for this endpoint when the response is encoded in the server, I think doing so it will now leak all SO attributes on this response for every saved view, could you verify that please?

I didn't see any weird attributes. I'll check it again.

Fixed it. thanks!

tonyghiani · 2023-06-30T12:12:50Z

x-pack/plugins/infra/public/pages/metrics/inventory_view/components/waffle/palette_preview.tsx

 import React from 'react';
 import { euiStyled } from '@kbn/kibana-react-plugin/common';
-import { InventoryColorPalette } from '../../../../../lib/lib';
+import type { InventoryColorPalette } from '../../../../../lib/lib';


nit: just cause I noticed it in some other files and I see here the change is pretty specific, the import type syntax is useful to tell the bundler that if an import has runtime code but we just want to use it for typing (e.g. an enum or class have runtime code), we can use that syntax then to exclude from the bundling.
In this case InventoryColorPalette is a simple type with no runtime code, so the import can be the normal one as import type won't bring any benefit in this case.

It doesn't hurt to keep anyway, it's non-blocking at all :)

tonyghiani

LGTM, thanks for the changes 👏

crespocarlos · 2023-06-30T19:51:50Z

@elasticmachine merge upstream

neptunian · 2023-06-30T20:15:07Z

x-pack/plugins/infra/server/saved_objects/inventory_view/types.ts

  rt.type({
    id: rt.string,
-    attributes: inventoryViewSavedObjectAttributesRT,
+    attributes: inventoryViewsSavedObjectAttributesRT,


perhaps we should keep this as not plural, inventoryViewSavedObjectAttributesRT instead of inventoryViewsSavedObjectAttributesRT for consistency's sake with InventoryViewSavedObject below.

crespocarlos · 2023-07-03T07:34:15Z

@elasticmachine merge upstream

crespocarlos · 2023-07-03T08:35:52Z

@elasticmachine merge upstream

crespocarlos · 2023-07-04T18:12:19Z

@elasticmachine merge upstream

kibana-ci · 2023-07-04T19:14:12Z

💚 Build Succeeded

Buildkite Build
Commit: c00ae1b

Metrics [docs]

Async chunks

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

id	before	after	diff
`infra`	2.0MB	2.0MB	-1.0KB

Page load bundle

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

id	before	after	diff
`infra`	119.7KB	120.4KB	+770.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	14	16	+2
`securitySolution`	410	414	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	15	17	+2
`securitySolution`	489	493	+4
total			+6

History

💚 Build #139717 succeeded 2d24158
💔 Build #139702 failed 5eac15f
💚 Build #139595 succeeded 4f9933a
💛 Build #139556 was flaky 3cc8ba2
💔 Build #139508 failed 1fe79b5
💔 Build #139462 failed 4e0e533

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

* main: (354 commits) [Synthetics] Overview page fix last refresh value display (elastic#161086) [Synthetics] Remove TLS alert option for ICMP monitor (elastic#161173) fixing the path of manifets for hints autodiscover (elastic#161075) [Fleet] Fix permissions in integrations Assets page (elastic#161233) Update publicBaseUrl warning id (elastic#161204) [ML] Fix Anomaly Explorer URL for alerting context with non-default space (elastic#160899) [Enterprise Search]Add 404 error handling for mappings and documents endpoints (elastic#161203) [Logs Shared] Move LogStream and LogView into new shared plugin (elastic#161151) [Security Solutions] Fix CellActions component should hide ShowTopN action for nested fields (elastic#159645) [SecuritySolutions] Remove filter actions from Cases alerts table and fix show_top_n action (elastic#161150) [Infrastructure UI] Add strict payload validation to inventory_views endpoint (elastic#160852) [api-docs] 2023-07-05 Daily api_docs build (elastic#161225) Fix errors in custom metric payload in SLO dev docs (elastic#161141) [data views] Fix overwrite param for create (elastic#160953) [Synthetics] Perform params API HTTP migration (elastic#160575) [Cloud Security][FTR]Refactor API FTR to use .to.eql instead of .to.be (elastic#160694) Have SLO routes return a 403 instead of a 400 when user has an insufficient license (elastic#161193) [Discover] Fix shared links flaky test (elastic#161172) [ftr] Improve FTR error handling for NoSuchSessionError (elastic#161025) skip flaky suite (elastic#151981) ...

…_views endpoint (#160982) closes [#157520](#157520) ## Summary This PR adds strict payload validation to `metrics_explorer_views` endpoint. This PR depends on this to be merged #160852 ### How to test - Call the endpoint below trying to use invalid values. see [here](https://github.com/elastic/kibana/pull/160982/files#diff-4573683b3b62cdf5f6426ec345b7ad6c7d6e6328237b213ca7519f686d8fa951R125-R131). ```bash [POST|PUT] kbn:/api/infra/metrics_explorer_views { "attributes": { "name": "Ad-hoc", "options": { "aggregation": "avg", "metrics": [ { "aggregation": "avg", "field": "system.cpu.total.norm.pct", "color": "color0" }, ], "source": "default", "groupBy": [ "host.name" ] }, "chartOptions": { "type": "line", "yAxisMode": "fromZero", "stack": false }, "currentTimerange": { "from": "now-1h", "to": "now", "interval": ">=10s" } } } ``` - Set up a local Kibana instance - Navigate to `Infrastructure > Metrics Explorer` - In the UI, use the Saved View feature and try different field combinations --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

## Summary Fixes #187254 Strict checks on the attribute `legend.steps` was added to the inventory view API in v8.10 (#160852), but they were not defined in the saved object type for that data, resulting in older versions migrating bad data, and being unable to use the API. This PR adds versioning to the Inventory view saved object type that constrains legend.steps to between 2 and 18, per the strict typing added in #160852 #### Open questions Should we add notes to all versions of kibana between v8.10 and oldest version available for backport describing this bug? ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

## Summary Fixes elastic#187254 Strict checks on the attribute `legend.steps` was added to the inventory view API in v8.10 (elastic#160852), but they were not defined in the saved object type for that data, resulting in older versions migrating bad data, and being unable to use the API. This PR adds versioning to the Inventory view saved object type that constrains legend.steps to between 2 and 18, per the strict typing added in elastic#160852 #### Open questions Should we add notes to all versions of kibana between v8.10 and oldest version available for backport describing this bug? ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit a108c63) # Conflicts: # x-pack/solutions/observability/plugins/infra/tsconfig.json

## Summary Fixes elastic#187254 Strict checks on the attribute `legend.steps` was added to the inventory view API in v8.10 (elastic#160852), but they were not defined in the saved object type for that data, resulting in older versions migrating bad data, and being unable to use the API. This PR adds versioning to the Inventory view saved object type that constrains legend.steps to between 2 and 18, per the strict typing added in elastic#160852 #### Open questions Should we add notes to all versions of kibana between v8.10 and oldest version available for backport describing this bug? ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit a108c63) # Conflicts: # x-pack/plugins/observability_solution/infra/server/saved_objects/inventory_view/inventory_view_saved_object.test.ts # x-pack/plugins/observability_solution/infra/tsconfig.json

) # Backport This will backport the following commits from `main` to `8.x`: - [Added versioning to inventory_view_saved_object (#207007)](#207007)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)

…1632) # Backport This will backport the following commits from `main` to `8.18`: - [Added versioning to inventory_view_saved_object (#207007)](#207007)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)

…1634) # Backport This will backport the following commits from `main` to `8.17`: - [Added versioning to inventory_view_saved_object (#207007)](#207007)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)

Add strict payload validation to invntory_views endpoint

a141531

Update README.md

bf4a41c

crespocarlos changed the title ~~[Infrastructure UI] Add strict payload validation to invntory_views endpoint~~ [Infrastructure UI] Add strict payload validation to inventory_views endpoint Jun 29, 2023

crespocarlos added 3 commits June 29, 2023 12:39

Fix update endpoint payload type

6e1fe5e

Refactor saved view types

d621f6b

Revert some type changes and adjust toolbar_control component

58b71bb

crespocarlos force-pushed the 157505-inventory-view-strict-input-validation branch from 467885d to 58b71bb Compare June 30, 2023 09:14

kibanamachine and others added 2 commits June 30, 2023 06:16

Merge branch 'main' into 157505-inventory-view-strict-input-validation

c70fb9b

Merge branch 'main' of github.com:elastic/kibana into 157505-inventor…

10c042d

…y-view-strict-input-validation

crespocarlos commented Jun 30, 2023

View reviewed changes

crespocarlos marked this pull request as ready for review June 30, 2023 11:48

crespocarlos requested a review from a team as a code owner June 30, 2023 11:48

crespocarlos requested a review from a team June 30, 2023 11:48

tonyghiani reviewed Jun 30, 2023

View reviewed changes

crespocarlos mentioned this pull request Jun 30, 2023

[Infrastructure UI] Add strict payload validation to metrics_explorer_views endpoint #160982

Merged

tonyghiani reviewed Jun 30, 2023

View reviewed changes

Decouple Saved View schema

ed95718

tonyghiani approved these changes Jun 30, 2023

View reviewed changes

crespocarlos added 4 commits June 30, 2023 17:14

Clean up

fc0256f

More clean up

1fe79b5

Fix missing attributes

f343285

Improve mapper

3cc8ba2

Merge branch 'main' into 157505-inventory-view-strict-input-validation

4f9933a

neptunian reviewed Jun 30, 2023

View reviewed changes

Typo

43cc290

Merge branch 'main' into 157505-inventory-view-strict-input-validation

5eac15f

Merge branch 'main' into 157505-inventory-view-strict-input-validation

2d24158

dgieselaar approved these changes Jul 4, 2023

View reviewed changes

Merge branch 'main' into 157505-inventory-view-strict-input-validation

c00ae1b

crespocarlos merged commit d79e69a into elastic:main Jul 5, 2023

crespocarlos deleted the 157505-inventory-view-strict-input-validation branch July 5, 2023 06:52

crespocarlos mentioned this pull request Nov 10, 2023

Failing test: Firefox XPack UI Functional Tests.x-pack/test/functional/apps/infra/home_page·ts - InfraOps App Metrics UI Home page Saved Views should create a new saved view and load it #157713

Closed

jguay mentioned this pull request Jul 1, 2024

500 when loading Observability Infrastructure Inventory view after upgrading to 8.10.0 or higher #187254

Closed

bryce-b mentioned this pull request Jan 16, 2025

Added versioning to inventory_view_saved_object #207007

Merged

2 tasks

Conversation

crespocarlos commented Jun 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

How to test

Uh oh!

ghost commented Jun 29, 2023

🤖 GitHub comments

Uh oh!

crespocarlos commented Jun 30, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jun 30, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonyghiani left a comment

Choose a reason for hiding this comment

Uh oh!

crespocarlos commented Jun 30, 2023

Uh oh!

neptunian Jun 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crespocarlos commented Jul 3, 2023

Uh oh!

crespocarlos commented Jul 3, 2023

Uh oh!

crespocarlos commented Jul 4, 2023

Uh oh!

kibana-ci commented Jul 4, 2023

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

ESLint disabled line counts

Total ESLint disabled count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

crespocarlos commented Jun 29, 2023 •

edited

Loading

neptunian Jun 30, 2023 •

edited

Loading