[Enterprise Search] Create HttpLogic Kea store, add http interceptors, and manage error connecting at top app-level #75790
Conversation
| @@ -67,6 +68,7 @@ export const renderApp = ( | |||
| > | |||
| <LicenseProvider license$={plugins.licensing.license$}> | |||
| <Provider store={store}> | |||
| <HttpProvider http={core.http} errorConnecting={errorConnecting} /> | |||
There was a problem hiding this comment.
Just wanted to make a quick note here: I left http in KibanaContext for now:
although I'm 50/50 on whether to remove it and have the only way for us to access http be from const { http } = useValues(HttpLogic) (vs also being able to do const { http } = useContext(KibanaContext)).
They both reference the same global Kibana http instance so it's not a question of accidentally using the wrong var or anything, it's just 2 different ways of calling http which may end up being confusing.
I will say that IMO useContext(KibanaContext) is significantly easier to mock and spy on in Jest tests (you just listen for mockKibanaContext.http changes) - whereas I ran into a super huge headache trying to pass a mockContext into a HttpLogic mock for some baffling reason.
Either way we decide, I definitely don't plan on removing all instances of const { http } = useContext(KibanaContext) in this PR, to try to keep it smaller for now. @scottybollinger / @JasonStoltz would definitely appreciate your thoughts if we should eventually remove them or if it's not a huge deal to have 2 ways of accessing http.
There was a problem hiding this comment.
I personally don't mind leaving both instance types since they reference the same thing and especially if it makes testing easier
There was a problem hiding this comment.
Yeah I'm 👍 for just leaving it as is for now and seeing how it goes. We could always change it later.
cee-chen
added
v7.10.0
release_note:skip
- For clearer error handling
- This allows us to:
- connect() directly to HttpLogic in other Kea logic files that need to make http calls, instead of passing in http manually via args
- Set http interceptors & remove them interceptors on unmount within Kea
- Share state derived from http (e.g. errorConnecting, readOnlyMode) between both AS & WS (but allow each app to handle that state differently if needed)
+ Refactors necessary for these changes:
- Kea types - add events key, clarify that mount returns an unmount function, fix reducer state type
- ReactDOM unmount - remove resetContext({}), was preventing logic from unmounting properly
- Since main app is now handling errorConnecting - http can now be connected directly from HttpLogic Kea store, so no need to pass it + minor cleanup in logic_overview.test.ts - remove unneeded unmount(), act(), switch to HttpLogic mock
- delete old ErrorState component - move LoadingState, EmptyState, and EngineOverviewHeader into subfolders in engine_overview
| @@ -52,7 +52,7 @@ export function registerEnginesRoute({ router, config, log }: IRouteDependencies | |||
| log.error(`Cannot connect to App Search: ${e.toString()}`); | |||
| if (e instanceof Error) log.debug(e.stack as string); | |||
|
|
|||
| return response.notFound({ body: 'cannot-connect' }); | |||
| return response.customError({ statusCode: 502, body: 'cannot-connect' }); | |||
There was a problem hiding this comment.
Should we distinguish authentication errors separately?
There was a problem hiding this comment.
Ahhh hm, great call. I can definitely try in a separate PR - I think I'd probably build off what Jason is doing in #75487 and check for the /login redirect there (since eventually all API endpoints should be using that helper).
We have to be careful about what statusCode we use for authentication though. If we try 401 or 403 I believe Kibana automatically responds to that by logging the Kibana user out (which is not what we want lmao). I'm tempted to keep the status code the same (502) and simply modify the body to specify a "Cannot authenticate this user" error vs a generic "Cannot connect to Enterprise Search" fallback.
What do you think?
There was a problem hiding this comment.
Definitely, separate PR. Was just thinking out loud.
If we try 401 or 403 I believe Kibana automatically responds to that by logging the Kibana user out (which is not what we want lmao).
Oy, definitely not. Does Kibana install some invasive, global hook to catch all 401/403's? I am not close enough to the code, but returning a 401/403 wrapped inside a 502 feels broken. Anyway, we can discuss separately. I don't intend to hold up progress on this PR on a tangential issue. Incremental progress FTW.
There was a problem hiding this comment.
I haven't dug through their code super deeply but yeah I believe every http request/response gets run through their global http lib (both on the client-side and server-side). I definitely see why and we get plenty of advantages of doing so, it's just in this one scenario that we don't particularly want that side-effect. I remember spending a real confused half hour trying to figure out why I was getting logged out during my initial MVP work though haha.
There was a problem hiding this comment.
Also there's some nuance to the status codes here - while it's true that the user is unauthorized in Enterprise Search, they're not necessarily unauthorized in Kibana (which is generating the status code), and as such it's not necessarily a client error / a 4xx doesn't necessarily apply.
I do think the 502 status code is the most generic while still applying to our use case:
This error response means that the server, while working as a gateway to get a response needed to handle the request, got an invalid response
In this case, a /login redirect (unauthenticated in Enterprise Search) qualifies as an "invalid response" just IMO.
There was a problem hiding this comment.
I agree with you there Constance 👍
cee-chen
changed the title
x-pack/plugins/enterprise_search/server/routes/app_search/engines.test.ts
Show resolved
Hide resolved
x-pack/plugins/enterprise_search/server/routes/workplace_search/overview.test.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/enterprise_search/server/routes/workplace_search/overview.test.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/enterprise_search/server/routes/app_search/engines.test.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/enterprise_search/server/routes/app_search/engines.test.ts
Outdated
Show resolved
Hide resolved
💚 Build SucceededBuild metrics@kbn/optimizer bundle module count
async chunks size
page load bundle size
History
To update your PR or re-run it, just comment with: |
|
Great changes Constance, thank you. |
…, and manage error connecting at top app-level (#75790) (#75821) * [Setup] Change error connecting status code to 502 - For clearer error handling * Set up new HttpProvider/Logic Kea store & listeners - This allows us to: - connect() directly to HttpLogic in other Kea logic files that need to make http calls, instead of passing in http manually via args - Set http interceptors & remove them interceptors on unmount within Kea - Share state derived from http (e.g. errorConnecting, readOnlyMode) between both AS & WS (but allow each app to handle that state differently if needed) + Refactors necessary for these changes: - Kea types - add events key, clarify that mount returns an unmount function, fix reducer state type - ReactDOM unmount - remove resetContext({}), was preventing logic from unmounting properly * Update AS & WS to show error connecting component at app level * [WS] Remove errorConnecting logic & http arg from Overview - Since main app is now handling errorConnecting - http can now be connected directly from HttpLogic Kea store, so no need to pass it + minor cleanup in logic_overview.test.ts - remove unneeded unmount(), act(), switch to HttpLogic mock * [AS] Add top-level ErrorConnecting component & remove error logic from EngineOverview * [AS] Clean up/move EngineOverview child components into subfolder - delete old ErrorState component - move LoadingState, EmptyState, and EngineOverviewHeader into subfolders in engine_overview * PR feedback: Update test assertions 404 copy
Summary
I strongly recommend following along by commit! 🔍
This PR:
connect()directly to HttpLogic in other Kea logic files that need to make http calls (instead of passing in http manually via args - cc @scottybollinger)errorConnecting,readOnlyMode) between both AS & WS (but allow each app to handle that state differently if needed)QA
When Enterprise Search is unavailable on load
/api/enterprise_search/config_data(which returns a 502 Bad Gateway)When Enterprise Search becomes unavailable mid-session
Regression testing
Checklist