[Security Solution] Edit host isolation exception IP UI

[academo]

Summary

Continuation of #111253 #113541 and #113762

This PR introduces the functionality to edit a host isolation exception item.

All the validations remain the same as with the add an item.

Note to reviewers: I had to re-introduce ts-ignore in several parts where the LoadingResourceState is being used.

I will work on fixing all the related types problems with it in the upcoming weeks.

Checklist

Delete any items that are not applicable to this PR.

• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Unit or functional tests were updated or added to match the most common scenarios
• Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
• Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
• This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
• This was checked for cross-browser compatibility

[elasticmachine]

Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt)

[academo]

@elasticmachine merge upstream

[paul-tavares]

Provided some feedback, nothing major. I am a bit confused by some of the Unit tests where actions/setup seems to be done, but there are no checks (expect()) done following that setup. At the very least you should include a expect() at the end of them that check for "something" that indicates (to the future us 😁 ) what the intent of the test/setup is

[paul-tavares]

Not really sure why there is code after the expect() here (and maybe in other places). I know its not part of this PR, but I might have missed it in the prior one where this was introduced.

[academo]

It is a multi-step test. In this case:

• Check the submit button is enabled
• Press the button
• Check the expected action was triggered

The waiter variable, is a promise to waitForAction which is a check on itself. If that action would not trigger, this test will fail and the component would not be working as expected.

I create the waiter variable before the click to avoid race conditions on the spy and the render.

There's nothing that technically limit you to put expect only at the end of a test function. In this particular test, there's no further things to check in the UI.

[paul-tavares]

same here. Feels like maybe you are missing an expect() following this set of additional actions.

[academo]

It is complete. waiter is a promise that waits for an action and is a check on itself, it is just not called expect. If this action would not trigger when you click the button, this component would be broken.

[academo]

Provided some feedback, nothing major. I am a bit confused by some of the Unit tests where actions/setup seems to be done, but there are no checks (expect()) done following that setup. At the very least you should include a expect() at the end of them that check for "something" that indicates (to the future us grin ) what the intent of the test/setup is

@paul-tavares

actions/setup seems to be done, but there are no checks

waitForAction is a check on itself. It will trigger an error if such action was not triggered. So even though those tests look empty, they are validating that the actions are triggered correctly.

The reason why the setup seems strange like this:

const waiter = waitForAction(...);
render();
await waiter;

is because I faced many race conditions where the render will trigger the actions too fast for the middlewareSpy to register the action to spy and trigger. So I started following the pattern of first registering the spy, rendering, then waiting for the check.

I will check those tests once again to see if there's some UI check I should do, like a disabled button, but they are complete as they are.

[paul-tavares]

I still left some comments around the tests that have no expect, but that should not hold up this PR from my perspective.

👍

[kibanamachine]

💛 Build succeeded, but was flaky

• continuous-integration/kibana-ci/pull-request
• Commit: 8bc78ab
• Storybooks Preview
• Documentation Changes
• Flaky suites:
  • xpack-kibana-ciGroup6

---

Test Failures

Kibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/search/session·ts.apis search search session touched time updates when you poll on an search

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches

[00:00:00]     │
[00:00:00]       └-: apis
[00:00:00]         └-> "before all" hook in "apis"
[00:00:00]         └-: search
[00:00:00]           └-> "before all" hook in "search"
[00:00:03]           └-: search session
[00:00:03]             └-> "before all" hook for "should fail to extend a nonexistent session"
[00:00:03]             └-> should fail to extend a nonexistent session
[00:00:03]               └-> "before each" hook: global before each for "should fail to extend a nonexistent session"
[00:00:03]               │ proc [kibana] [2021-10-13T13:59:40.339+00:00][ERROR][plugins.dataEnhanced.data_enhanced] [object Object]
[00:00:03]               └- ✓ pass  (76ms) "apis search search session should fail to extend a nonexistent session"
[00:00:03]             └-> should sync search ids into not persisted session
[00:00:03]               └-> "before each" hook: global before each for "should sync search ids into not persisted session"
[00:00:03]               │ debg Waiting up to 5000ms for searches persisted into session...
[00:00:03]               │ proc [kibana] [2021-10-13T13:59:40.502+00:00][ERROR][plugins.dataEnhanced.data_enhanced] [object Object]
[00:00:03]               │ debg --- retry.waitForWithTimeout error: expected 200 "OK", got 404 "Not Found"
[00:00:04]               │ proc [kibana] [2021-10-13T13:59:41.071+00:00][ERROR][plugins.dataEnhanced.data_enhanced] [object Object]
[00:00:04]               │ debg --- retry.waitForWithTimeout failed again with the same message...
[00:00:04]               │ info [o.e.c.m.MetadataMappingService] [node-01] [.kibana_8.0.0_001/bf__YRUVRMq3CTTB3PsKjg] update_mapping [_doc]
[00:00:04]               └- ✓ pass  (1.3s) "apis search search session should sync search ids into not persisted session"
[00:00:04]             └-> should complete session when searches complete
[00:00:04]               └-> "before each" hook: global before each for "should complete session when searches complete"
[00:00:04]               │ debg Waiting up to 5000ms for searches persisted into session...
[00:00:04]               │ debg --- retry.waitForWithTimeout error: expected [] to contain 'FldfR295TDY2U2ZXbzFsbnZrQWZDSFEbMFR6b01pWHdRbWF2Y3lLbHlvbGNXZzoyMjUw'
[00:00:05]               │ debg --- retry.waitForWithTimeout failed again with the same message...
[00:00:08]               │ info [o.e.c.m.MetadataMappingService] [node-01] [.kibana_8.0.0_001/bf__YRUVRMq3CTTB3PsKjg] update_mapping [_doc]
[00:00:15]               │ debg Waiting up to 5000ms for searches eventually complete and session gets into the complete state...
[00:00:15]               └- ✓ pass  (11.3s) "apis search search session should complete session when searches complete"
[00:00:15]             └-> touched time updates when you poll on an search
[00:00:15]               └-> "before each" hook: global before each for "touched time updates when you poll on an search"
[00:00:15]               │ debg Waiting up to 20000ms for search session created...
[00:00:15]               │ proc [kibana] [2021-10-13T13:59:53.013+00:00][ERROR][plugins.dataEnhanced.data_enhanced] [object Object]
[00:00:16]               │ proc [kibana] [2021-10-13T13:59:53.577+00:00][ERROR][plugins.dataEnhanced.data_enhanced] [object Object]
[00:00:19]               └- ✖ fail: apis search search session touched time updates when you poll on an search
[00:00:19]               │      Error: expected '2021-10-13T13:59:53.996Z' to be below 2021-10-13T13:59:53.996Z
[00:00:19]               │       at Assertion.assert (/dev/shm/workspace/parallel/6/kibana/node_modules/@kbn/expect/expect.js:100:11)
[00:00:19]               │       at Assertion.lessThan.Assertion.below (/dev/shm/workspace/parallel/6/kibana/node_modules/@kbn/expect/expect.js:336:8)
[00:00:19]               │       at Function.lessThan (/dev/shm/workspace/parallel/6/kibana/node_modules/@kbn/expect/expect.js:531:15)
[00:00:19]               │       at Context.<anonymous> (test/api_integration/apis/search/session.ts:438:65)
[00:00:19]               │       at runMicrotasks (<anonymous>)
[00:00:19]               │       at processTicksAndRejections (internal/process/task_queues.js:95:5)
[00:00:19]               │       at Object.apply (/dev/shm/workspace/parallel/6/kibana/node_modules/@kbn/test/target_node/functional_test_runner/lib/mocha/wrap_function.js:87:16)
[00:00:19]               │ 
[00:00:19]               │ 

Stack Trace

Error: expected '2021-10-13T13:59:53.996Z' to be below 2021-10-13T13:59:53.996Z
    at Assertion.assert (/dev/shm/workspace/parallel/6/kibana/node_modules/@kbn/expect/expect.js:100:11)
    at Assertion.lessThan.Assertion.below (/dev/shm/workspace/parallel/6/kibana/node_modules/@kbn/expect/expect.js:336:8)
    at Function.lessThan (/dev/shm/workspace/parallel/6/kibana/node_modules/@kbn/expect/expect.js:531:15)
    at Context.<anonymous> (test/api_integration/apis/search/session.ts:438:65)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:95:5)
    at Object.apply (/dev/shm/workspace/parallel/6/kibana/node_modules/@kbn/test/target_node/functional_test_runner/lib/mocha/wrap_function.js:87:16)

---

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
securitySolution	4.6MB	4.6MB	+3.3KB

History

• 💚 Build #160437 succeeded 9d541ce
• 💚 Build #160330 succeeded f9c67ee
• 💚 Build #160115 succeeded b492695
• 💔 Build #159928 failed b08089b
• 💚 Build #159963 succeeded 3b2a41c

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

[ashokaditya]

I checked it out and took it for a spin. Works as expected. I have minor suggestions but not a blocker for rolling it out.
🚀 🐑

Note: I still believe the error messages could be better. Showing the failed to fetch or other such API-specific messages to the user is not useful in my opinion. Perhaps it's how it has been done so far, but I think this can be done better.

For instance, as a user I don't know what I should do with this info:

[ashokaditya]

nitpick: I think the One is not necessary for the name, Item already says it's a singular thing. Unless getHostIsolationExceptionItem name collides with another 😅

[ashokaditya]

suggestion: You can also use <HostIsolationExceptionsPageState['form']['entry']> here instead.

[kibanamachine]

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.